add support for external certs #1655
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1655 +/- ##
==========================================
+ Coverage 47.91% 48.89% +0.97%
==========================================
Files 162 188 +26
Lines 23491 19286 -4205
==========================================
- Hits 11256 9430 -1826
+ Misses 11014 8625 -2389
- Partials 1221 1231 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // Create or refresh the certs based on clientConfig | ||
| s.dnsName = dnsName | ||
| certs, err := s.read() | ||
| if err != nil { |
There was a problem hiding this comment.
is it possible the ca read from the secret is the same as before ? it necessary to update webhookconfiguration again ?
There was a problem hiding this comment.
add logic to check if the certs are same as before, however, the changed bool value is not used in webhook controller, and the webhook controller will use reflect.DeepEqual() to see if the webhook configuration need to be updated.
| } | ||
|
|
||
| // EnsureCert read and validate certs from k8s secret. | ||
| func (s *externalCertWriter) EnsureCert(dnsName string) (*generator.Artifacts, bool, error) { |
Signed-off-by: Kuromesi <blackfacepan@163.com>
Signed-off-by: Kuromesi <blackfacepan@163.com>
Signed-off-by: kuromesi <blackfacepan@163.com>
Signed-off-by: Kuromesi <blackfacepan@163.com>
| @@ -35,7 +35,7 @@ spec: | |||
| - --enable-leader-election | |||
| - --logtostderr=true | |||
| - --v=5 | |||
| - --feature-gates=AllAlpha=true | |||
| - --feature-gates=AllAlpha=true,EnableExternalCerts=false | |||
There was a problem hiding this comment.
it is not necessary to add this flag is EnableExternalCerts is false by defautl
| continue | ||
| } | ||
| if !bytes.Equal(wh.ClientConfig.CABundle, caBundle) { | ||
| return fmt.Errorf("caBundle of ValidatingWebhookConfiguration %s does not match the external caBundle", mutatingWebhookConfigurationName) | ||
| } |
There was a problem hiding this comment.
just return at the end of the if clause, and remove the else to make indentation smaller
| if !bytes.Equal(crd.Spec.Conversion.Webhook.ClientConfig.CABundle, caBundle) { | ||
| return fmt.Errorf("caBundle of CRD %s does not match external caBundle", crd.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
just return in the end of if clause and remove the else to make the indentation smaller
Ⅰ. Describe what this PR does
Add new feature to read and validate certs stored in secret in order to support external certs generated by cert manager and etc.
Ⅱ. Does this pull request fix one issue?
Fixes #509.