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.

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

Supporting reading TLS data from places other than Kubernetes Secrets #1972

Open
munnerz opened this issue Dec 17, 2020 · 8 comments
Open

Supporting reading TLS data from places other than Kubernetes Secrets #1972

munnerz opened this issue Dec 17, 2020 · 8 comments
Labels
kind/feature lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@munnerz
Copy link

munnerz commented Dec 17, 2020

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

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
Copy link
Member

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

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.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 23, 2021
@dprotaso
Copy link
Member

dprotaso commented May 3, 2021

/remove-lifecycle stale

@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 3, 2021
@markusthoemmes
Copy link
Contributor

Imma going to take a whack at this one.

/assign

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 24, 2021
@dprotaso
Copy link
Member

/lifecycle frozen

@dprotaso dprotaso reopened this Sep 23, 2021
@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 23, 2021
@knative-prow-robot knative-prow-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Sep 23, 2021
@markusthoemmes
Copy link
Contributor

/unassign

not doing this rn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests

5 participants