Skip to content

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

Merged
merged 1 commit into from
May 28, 2025

Conversation

pratikjagrut
Copy link
Contributor

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 type provisioning.cattle.io/cloud-credential (e.g., to add the field.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 adding matchConditions. 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 type provisioning.cattle.io/cloud-credential.

The specific CEL expression implemented is:

object != null && object.type == "provisioning.cattle.io/cloud-credential"

This ensures that:

  1. The webhook is only invoked for secrets of the intended type.
  2. The condition correctly handles DELETE operations where object would be null, preventing evaluation errors.

@pratikjagrut pratikjagrut requested a review from a team as a code owner May 12, 2025 06:03
@prachidamle prachidamle requested review from crobby and tomleb May 12, 2025 19:31
Copy link
Contributor

@crobby crobby left a 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?

@pratikjagrut pratikjagrut force-pushed the gh#49750-webhook-secret branch 2 times, most recently from dc8c848 to f635697 Compare May 14, 2025 06:27
@pratikjagrut pratikjagrut changed the title fix: Invoke webhook for cloud-credential secrets only fix: add matchCondition to invoke mutation webhook only for cloud-credential secret May 14, 2025
@pratikjagrut pratikjagrut requested a review from crobby May 14, 2025 07:12
@pratikjagrut
Copy link
Contributor Author

@crobby I added the tests.

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tomleb tomleb left a 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.

Comment on lines 80 to 85
if request.DryRun != nil && *request.DryRun {
return &admissionv1.AdmissionResponse{
Allowed: true,
}, nil
}

Copy link
Contributor

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{
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@pratikjagrut pratikjagrut force-pushed the gh#49750-webhook-secret branch 2 times, most recently from e207505 to 4b22d75 Compare May 19, 2025 05:49
@pratikjagrut pratikjagrut force-pushed the gh#49750-webhook-secret branch from 4b22d75 to f35f8b2 Compare May 19, 2025 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants