-
Notifications
You must be signed in to change notification settings - Fork 80
fix: add matchCondition to invoke mutation webhook only for cloud-credential secret #912
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.
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
Conversation
crobby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a pretty straightforward change, but can a test be added for this?
dc8c848 to
f635697
Compare
|
@crobby I added the tests. |
ericpromislow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tomleb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern with the DELETE case since it's not clear to me that it's meant to be skipped here.
| if request.DryRun != nil && *request.DryRun { | ||
| return &admissionv1.AdmissionResponse{ | ||
| Allowed: true, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this was removed?
| mutatingWebhook := admission.NewDefaultMutatingWebhook(m, clientConfig, admissionregistrationv1.NamespacedScope, m.Operations()) | ||
| mutatingWebhook.SideEffects = admission.Ptr(admissionregistrationv1.SideEffectClassNoneOnDryRun) | ||
| mutatingWebhook.TimeoutSeconds = admission.Ptr(int32(15)) | ||
| mutatingWebhook.MatchConditions = []admissionregistrationv1.MatchCondition{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code only checks for cloud-credential for the CREATE case:
func (m *Mutator) admitCreate(secret *corev1.Secret, request *admission.Request) (*admissionv1.AdmissionResponse, error) {
if secret.Type != "provisioning.cattle.io/cloud-credential" {
return &admissionv1.AdmissionResponse{
Allowed: true,
}, nil
}but there's no such check for the delete. However this match condition will be for CREATE and DELETE operations. Are we sure that the DELETE behavior was only for cloud credentials? The tests in mutator_test.go doesn't have a type field set so it feels like it could be for other secret types as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tom,
Thanks for pointing that out.
The delete operation could apply to any secret type.
So, I’ve updated the match expression and added a test to verify that the delete operation behaves as expected.
e207505 to
4b22d75
Compare
4b22d75 to
f35f8b2
Compare
Issue:
rancher/rancher#49750
Problem
The
MutatingWebhookConfigurationfor secrets was configured to invoke the Rancher webhook for all secret creation and update operations. However, the webhook's internal mutation logic for secrets is specifically designed to act only upon secrets of typeprovisioning.cattle.io/cloud-credential(e.g., to add thefield.cattle.io/creatorIdannotation).This broad invocation scope meant that if the webhook service was temporarily unavailable (for instance, during Rancher startup, upgrades, or pod restarts), it could inadvertently block the creation or update of any secret. This included critical internal secrets, such as the
api-extensionsecret required by the remotedialer-proxy (RDP), leading to Rancher startup failures or other operational issues.Solution
This PR modifies the
MutatingWebhookConfigurationfor secrets by addingmatchConditions. A CEL (Common Expression Language) expression is now used to instruct the Kubernetes API server to only forward admission requests to this webhook if the secret object being processed is of typeprovisioning.cattle.io/cloud-credential.The specific CEL expression implemented is:
This ensures that:
DELETEoperations whereobjectwould benull, preventing evaluation errors.