OSSM 3376 Deprecate fields pilotSecretName and rootCAConfigMapName#1137
OSSM 3376 Deprecate fields pilotSecretName and rootCAConfigMapName#1137mayleighnmyers wants to merge 7 commits intomaistra:maistra-2.4from
Conversation
…ger configuration
…ger configuration
|
Skipping CI for Draft Pull Request. |
|
Skipping CI for Draft Pull Request. |
| case *v1.ServiceMeshControlPlane: | ||
| return false | ||
| case *v2.ServiceMeshControlPlane: | ||
| return s.Spec.IsPilotSecretNameEnabled() |
There was a problem hiding this comment.
I think it would be better if the code in this function was inlined here (i.e. remove the function and move the code here).
| case *v1.ServiceMeshControlPlane: | ||
| return false | ||
| case *v2.ServiceMeshControlPlane: | ||
| return s.Spec.IsRootCAConfigMapNameEnabled() |
|
|
||
| hasPilotSecretName := ContainsPilotSecretNameField(smcp) | ||
| if hasPilotSecretName { | ||
| return admission.ValidationResponse(false, "SMCP does not allow PilotSecretName field") |
There was a problem hiding this comment.
Try to put yourself in the user's shoes. When they get this error message, will they know what's wrong? I think the user might then ask "why doesn't the SMCP allow this field?". It's best if the message itself explains why.
There was a problem hiding this comment.
I am having a hard time coming up with a more intellect response in this field. Any idea on what to say?
There was a problem hiding this comment.
Oh, I re-read the Jira issue and it looks like we shouldn't return an error but a deprecation warning. So, the message could read something like spec.foo.bar.baz.pilotSecretName is deprecated; will be removed in 2.5.0; use foo.bar.baz.xyz instead.
We need to figure out how to emit a warning.
There was a problem hiding this comment.
Oh, I re-read the Jira issue and it looks like we shouldn't return an error but a deprecation warning.
I can see in the JIRA issue "...if the deprecated fields exist, a warning should be returned."
There was a problem hiding this comment.
Yes, as I said: we must return a warning not an error. And we can't do that until we upgrade k8s.io/api to v0.19+.
There was a problem hiding this comment.
Ah, ok, nevermind. I misunderstood your comment and I thought you mean I specified wrong requirements.
| hasRootCAConfigMapName := ContainsRootCAConfigMapNameField(smcp) | ||
| if hasRootCAConfigMapName { | ||
| return admission.ValidationResponse(false, "SMCP does not allow RootCAConfigMapName field") | ||
| } |
There was a problem hiding this comment.
I noticed you add this validation code to the ValidateRequest function, which isn't the best place for it. Instead, I think you should create a validateSecurity function and call it in ValidateV2(). The ValidateRequest function exists for cases where you need to actually check the admission.Request object.
|
@mayleighnmyers: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
https://issues.redhat.com/browse/OSSM-3376