Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSSM 3376 Deprecate fields pilotSecretName and rootCAConfigMapName #1137

Draft
wants to merge 7 commits into
base: maistra-2.4
Choose a base branch
from
Draft

OSSM 3376 Deprecate fields pilotSecretName and rootCAConfigMapName #1137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
653ab7b
Deprecate fields pilotSecretName and rootCAConfigMapName in cert-mana…
mayleighnmyers Mar 16, 2023
c006f75
Update one rootCAConfigMapName
mayleighnmyers Mar 16, 2023
c93dfcf
Deprecate fields pilotSecretName and rootCAConfigMapName in cert-mana…
mayleighnmyers Mar 21, 2023
fa8b613
Fixing the field I had commited eariler
mayleighnmyers Mar 21, 2023
e0f9c4c
Cleaning up spaces
mayleighnmyers Mar 21, 2023
a2adaa1
Made a few changes due to testing errors
mayleighnmyers Mar 22, 2023
c5b29c9
Made changes
mayleighnmyers Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/apis/maistra/v2/servicemeshcontrolplane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,12 @@ func (s ControlPlaneSpec) IsClusterScoped() (bool, error) {
}
return controlPlaneMode == ControlPlaneModeValueClusterScoped, nil
}

func (s ControlPlaneSpec) IsPilotSecretNameEnabled() bool {
return s.Security.CertificateAuthority.CertManager != nil && s.Security.CertificateAuthority.CertManager.PilotCertSecretName != ""
mayleighnmyers marked this conversation as resolved.
Show resolved Hide resolved
}

func (s ControlPlaneSpec) IsRootCAConfigMapNameEnabled() bool {
return s.Security.CertificateAuthority.CertManager != nil && s.Security.CertificateAuthority.CertManager.RootCAConfigMapName != ""

}
33 changes: 32 additions & 1 deletion pkg/controller/versions/strategy_v2_4.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,41 @@ func (v *versionStrategyV2_4) ValidateRequest(ctx context.Context, cl client.Cli
return admission.ValidationResponse(false, "a cluster-scoped SMCP may only be created by users with cluster-admin permissions")
}
}

hasPilotSecretName := ContainsPilotSecretNameField(smcp)
if hasPilotSecretName {
return admission.ValidationResponse(false, "SMCP does not allow PilotSecretName field")
Copy link
Contributor

@luksa luksa Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having a hard time coming up with a more intellect response in this field. Any idea on what to say?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return admission.ValidationResponse(true, "")
}

func ContainsPilotSecretNameField(smcp metav1.Object) bool {
switch s := smcp.(type) {
case *v1.ServiceMeshControlPlane:
return false
case *v2.ServiceMeshControlPlane:
return s.Spec.IsPilotSecretNameEnabled()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

default:
return false
}
}
func ContainsRootCAConfigMapNameField(smcp metav1.Object) bool {
switch s := smcp.(type) {
case *v1.ServiceMeshControlPlane:
return false
case *v2.ServiceMeshControlPlane:
return s.Spec.IsRootCAConfigMapNameEnabled()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

default:
return false
}
}




func (v *versionStrategyV2_4) isRequesterClusterAdmin(ctx context.Context, cl client.Client, req admission.Request) (bool, error) {
sar := &authorizationv1.SubjectAccessReview{
Spec: authorizationv1.SubjectAccessReviewSpec{
Expand Down