🌱 Handle TLSOpts.GetCertificate in webhook #2291
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
|
/assign @sbueringer |
pkg/webhook/server.go
Outdated
| }() | ||
|
|
||
| // load CA to verify client certificate | ||
| if s.ClientCAName != "" { |
There was a problem hiding this comment.
I think configuring ClientCAs / ClientAuth is an orthogonal feature to GetCertificate so it shouldn't be inside this if
There was a problem hiding this comment.
🤔 Problem is that we're using CertDir and CertCAName to get the certificate from the local file
There was a problem hiding this comment.
But that's only a matter of writing different godoc comments, right?
There was a problem hiding this comment.
We could also decouple by deprecating ClientCAName and introducing ClientCAFile
Then it's only a bit hacky until we remove ClientCAName
There was a problem hiding this comment.
Folks can configure CA validation through TLS options if they wish to do so outside the CertDir. The current comments seem fine to me in those cases, wdyt? cc @alvaroaleman
There was a problem hiding this comment.
I'll update the PR in a bit
There was a problem hiding this comment.
PTAL, opted to both return errors, and allow to use ClientCAName w/ GetCertificate if folks still want
There was a problem hiding this comment.
Looks good, mind adding a testcase for GetCertificate not getting overriden if present?
vincepri
force-pushed
the
tls-opts-getcertificate
branch
2 times, most recently
from
1e52202
to
abe23de
Compare
k8s-ci-robot
added
size/L
This change rewrites some of the webhook server logic to better handle the user setting a custom configuration for the TLS options by providing a custom GetCertificate function. When that's present, we won't start a certwatcher routine. The change also updates the documentation to better clarify when each of the options are effective. Signed-off-by: Vince Prignano <vincepri@redhat.com>
vincepri
force-pushed
the
tls-opts-getcertificate
branch
from
abe23de
to
bd12701
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
This change rewrites some of the webhook server logic to better handle the user setting a custom configuration for the TLS options by providing a custom GetCertificate function. When that's present, we won't start a certwatcher routine. The change also updates the documentation to better clarify when each of the options are effective.
Fixed #2269