-
Notifications
You must be signed in to change notification settings - Fork 74
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
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. |
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
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?
@@ -72,17 +72,18 @@ func (m *Mutator) MutatingWebhook(clientConfig admissionregistrationv1.WebhookCl | |||
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
MutatingWebhookConfiguration
for 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/creatorId
annotation).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-extension
secret required by the remotedialer-proxy (RDP), leading to Rancher startup failures or other operational issues.Solution
This PR modifies the
MutatingWebhookConfiguration
for 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:
DELETE
operations whereobject
would benull
, preventing evaluation errors.