Refactor Connect Inject Webhook to use webhook-cert-manager #861
Conversation
|
@thisisnotashwin lmk when you want a full review, this approach looks good to me. |
|
@lkysow will do. I initially hoped this PR would be reasonable isolated from the other changes getting made in the refactor but it is starting to appear a little trickier. Ill probably use this as a reference PR for a future PR that covers more of the changes. Will remove you and Iryna as reviewers to reduce some of the noise from the notifications. |
ishustava
changed the title
ishustava
changed the base branch from
feature/connect-refactor
to
feature-tproxy
ishustava
force-pushed
the
webhook-refactor
branch
from
760fb94
to
f90fefb
Compare
ishustava
force-pushed
the
webhook-refactor
branch
from
f90fefb
to
a48e8e9
Compare
| livenessProbe: | ||
| httpGet: | ||
| path: /health/ready | ||
| port: 8080 | ||
| scheme: HTTPS | ||
| failureThreshold: 2 | ||
| initialDelaySeconds: 1 | ||
| periodSeconds: 2 | ||
| successThreshold: 1 | ||
| timeoutSeconds: 5 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /health/ready | ||
| port: 8080 | ||
| scheme: HTTPS | ||
| failureThreshold: 2 | ||
| initialDelaySeconds: 2 | ||
| periodSeconds: 2 | ||
| successThreshold: 1 | ||
| timeoutSeconds: 5 | ||
| startupProbe: | ||
| httpGet: | ||
| path: /health/ready | ||
| port: 8080 | ||
| scheme: HTTPS | ||
| failureThreshold: 15 | ||
| periodSeconds: 2 | ||
| timeoutSeconds: 5 |
There was a problem hiding this comment.
we're removing these for the time being as we'd need to address them differently now that webhook runs through the operator-sdk. It'll be addressed in a separate PR.
ishustava
requested review from
a team,
lkysow and
ndhanushkodi
and removed request for
a team
ishustava
force-pushed
the
webhook-refactor
branch
from
edab293
to
1075741
Compare
ishustava
force-pushed
the
webhook-refactor
branch
from
1075741
to
5672e7a
Compare
| @@ -26,7 +26,7 @@ webhooks: | |||
| name: {{ template "consul.fullname" . }}-connect-injector-svc | |||
| namespace: {{ .Release.Namespace }} | |||
| path: "/mutate" | |||
| caBundle: {{ .Values.connectInject.certs.caBundle | quote }} | |||
| caBundle: Cg== | |||
There was a problem hiding this comment.
I think this has to do with base64 but what are the contents it encodes? Can we add a comment here?
There was a problem hiding this comment.
yeah it's a base64 encoded newline. Will add!
There was a problem hiding this comment.
Actually, I realized that this property is not required, so I removed it instead to avoid confusion.
* Enable webhook-cert-manager whenever either controller or connectInject is enabled * Remove connectInject.certs values. This behavior was already broken, and we don't want to support it going forward with webhook-cert-manager. Co-authored-by: Iryna Shustava <[email protected]>
* Enable webhook-cert-manager whenever either controller or connectInject is enabled * Remove connectInject.certs values. This behavior was already broken, and we don't want to support it going forward with webhook-cert-manager. Co-authored-by: Iryna Shustava <[email protected]>
This PR is a partner to hashicorp/consul-k8s#454 and proposes the following changes:
The draft was created by @thisisnotashwin and cleanup and tests added by @ishustava.
[original description]
This is a rough draft but is at a good place to see the changes made and the impact on Helm. This will eventually be a PR against a
feature-tproxybranch