Supporting reading TLS data from places other than Kubernetes Secrets

[munnerz]

When deploying a webhook, it may be desirable to not make use of the certificates controller (for generating a TLS certificate for the webhook), nor reading the TLS data from a Kubernetes secret (e.g. in the case where some form of TLS certificate/identity document is already available within the webhook pod).

It is already possible to not instantiate the certificate controller (by excluding it from the call to sharedmain.WebhookMainWithConfig, however the webhook.Run method explicitly sets the GetCertificate field on the http.Server to one that requires a Secret lister to function:

pkg/webhook/webhook.go

Lines 187 to 206 in b0c121f

	GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
	secret, err := wh.secretlister.Secrets(system.Namespace()).Get(wh.Options.SecretName)
	if err != nil {
	return nil, err
	}
	serverKey, ok := secret.Data[certresources.ServerKey]
	if !ok {
	return nil, errors.New("server key missing")
	}
	serverCert, ok := secret.Data[certresources.ServerCert]
	if !ok {
	return nil, errors.New("server cert missing")
	}
	cert, err := tls.X509KeyPair(serverCert, serverKey)
	if err != nil {
	return nil, err
	}
	return &cert, nil
	},

Ideally, this would be one of a number of optional mechanisms for loading TLS certificate data. This would increase the flexibility of the knative.dev/pkg package.

I'm considering whether we should simply allow setting GetCertificate directly, however changes are needed to webhook.New which currently fetches the SecretInformer from the SharedInformerFactory (thereby causing the informer factory to require read permission on secrets to start):

pkg/webhook/webhook.go

Lines 110 to 115 in b0c121f

	// Injection is too aggressive for this case because by simply linking this
	// library we force consumers to have secret access. If we require that one
	// of the admission controllers' informers *also* require the secret
	// informer, then we can fetch the shared informer factory here and produce
	// a new secret informer from it.
	secretInformer := kubeinformerfactory.Get(ctx).Core().V1().Secrets()

I think we could introduce a new NewWithCertificateSource or something, which would allow a user to override this behaviour (and the existing behaviour of the New function could be retained without breaking Go API compatibility).

/kind feature

[markusthoemmes]

Ack! I need the very same thing, where my cluster has already mechanisms to come up with certificates that I'd like to mount for the webhook to "just" pick up.

I'd love to see those as commandline options for webhooks generically too, so existing webhooks (like in Knative Serving) can be altered to do things differently.

[mattmoor]

I think defining an interface to encapsulate the webhook's needs and having something like the certificate controller implement that as one option would be a welcome refactor (I was talking about this a while back with someone 🤔 ). With that, it should be straightforward to add new strategies.

cc @dprotaso for thoughts.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[dprotaso]

/remove-lifecycle stale

[markusthoemmes]

Imma going to take a whack at this one.

/assign

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[dprotaso]

/lifecycle frozen

[markusthoemmes]

/unassign

not doing this rn.