feat(https): support https webhooks #98
Conversation
… hooks Signed-off-by: Matt Harris <mwharri@gmail.com> Signed-off-by: Matt Harris <matthew.w.harris@walmart.com>
|
|
||
| // SetWebhookCABundleFromSchemaOrDefault evaluates webhook timeout and sets the | ||
| // evaluated timeout against WebhookCaller instance | ||
| func SetWebhookCABundleFromSchemaOrDefault(schema *v1alpha1.Webhook) webhook.InvokerOption { |
There was a problem hiding this comment.
This does not set any default. IMO we can rename the function to SetWebhookCABundleFromSchema
| client := &http.Client{Timeout: i.Timeout} | ||
| if i.CABundle != "" { |
There was a problem hiding this comment.
This maybe the right place, where we explain how this CABundle will be made available to the binary.
We should also add PR message that explain the same.
|
Thanks @mattharr-is for the PR. I have provided some suggestions. Would like to get this reviewed by @prateekpandey14 who has been working on webhooks & certificate management of the late. |
| client := &http.Client{Timeout: i.Timeout} | ||
| if i.CABundle != "" { | ||
| caPool := x509.NewCertPool() |
There was a problem hiding this comment.
It will be really great if we can add Unit Tests for these changes. Current code lacks UT & your additions will be helpful in this regard.
There was a problem hiding this comment.
I am very much agree with @luisdavim, we should be using k8s secrets type instead of a string to pass the certs.
|
|
||
| // SetWebhookCABundleFromSchemaOrDefault evaluates webhook timeout and sets the | ||
| // evaluated timeout against WebhookCaller instance | ||
| func SetWebhookCABundleFromSchemaOrDefault(schema *v1alpha1.Webhook) webhook.InvokerOption { |
There was a problem hiding this comment.
Comment has to be updated here, func sets the secret.
|
@prateekpandey14 will it be possible to provide some sample repos/projects that has already done this. We might want to use it for reference. @mattharr-is Do let us know your thoughts? I would be more interested to know the changes required to deploy metac binary with this PR. |
AmitKumarDas
changed the title
|
I'd like to present an opposing view on using e.g. For MutatingWebhookConfiguration webhooks it has Shouldn't this project follow the same pattern, for a more consistent user experience? |
|
That's a good point @mikebryant |
|
closing this due to inactivity |
add optional caBundle attribute to composite and decorator controller hooks