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

feat: use cert-controller rotator for server certs #93

Merged
merged 2 commits into from
Jul 19, 2021
Merged

feat: use cert-controller rotator for server certs #93

merged 2 commits into from
Jul 19, 2021

Conversation

aramase
Copy link
Member

@aramase aramase commented Jul 13, 2021

Signed-off-by: Anish Ramasekar [email protected]

Reason for Change:

Requirements

  • squashed commits
  • included documentation
  • added unit tests and e2e tests (if applicable).

Issue Fixed:

fixes #91

Please answer the following questions with yes/no:

Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

  • yes
  • no

The changes have been inspired by the gatekeeper project: https://github.com/open-policy-agent/gatekeeper

Notes for Reviewers:
Doc update to remove cert-manager requirement/install will be done at the time of release.

@aramase aramase marked this pull request as ready for review July 15, 2021 15:03
@aramase aramase requested a review from sozercan July 15, 2021 15:03
@@ -110,6 +112,8 @@ test_helm_chart() {
--set azureTenantID="${AZURE_TENANT_ID}" \
--namespace aad-pi-webhook-system \
--wait
# adding a sleep here because the cert mount is not ready immediately after the deploy
sleep 120
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to add this sleep because the mount timing isn't deterministic. @sozercan Is there anything in this change that's different from gatekeeper that you think could cause the delay?

I think the secret data isn't up-to-date during the initial mount republish by kubelet and it only succeeds in the next republish loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't repro this

@aramase aramase requested a review from chewong July 15, 2021 15:06
config/manager/manager.yaml Outdated Show resolved Hide resolved
cmd/webhook/main.go Outdated Show resolved Hide resolved
@aramase aramase requested a review from chewong July 15, 2021 20:32
chewong
chewong previously approved these changes Jul 15, 2021
Copy link
Contributor

@chewong chewong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending review from @sozercan

@chewong chewong dismissed their stale review July 16, 2021 23:40

new comments

Copy link
Contributor

@chewong chewong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the following volume mount in third_party/open-policy-agent/gatekeeper/helmify/kustomize-for-helm.yaml, which was merged to the helm chart template:

        - mountPath: /tmp/k8s-webhook-server/serving-certs
          name: cert
          readOnly: true

@aramase aramase requested review from sozercan and chewong July 19, 2021 20:55
@aramase aramase enabled auto-merge (squash) July 19, 2021 21:51
@aramase aramase merged commit 7845d70 into Azure:main Jul 19, 2021
@aramase aramase deleted the cert-controller branch July 19, 2021 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use cert-controller for generating and rotating certs
3 participants