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

An Azure AKS + Let's Encrypt Tutorial #1120

Merged
merged 18 commits into from
Jan 10, 2023

Conversation

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2022
@wallrj wallrj changed the title An Azure AKS + Let's Encrypt Tutorial WIP: An Azure AKS + Let's Encrypt Tutorial Nov 25, 2022
@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 25, 2022
@netlify
Copy link

netlify bot commented Nov 25, 2022

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 637e157
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/63bd7e3d02aca20008d1c6ba
😎 Deploy Preview https://deploy-preview-1120--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@pinkfloydx33
Copy link

AZWI can be installed as a first class feature of AKS using the --enable-workload-identity switch. It's behind a feature flag currently but will be the recommended way to install once the feature is GA. Not sure if it's worth including that information or not: here

@pinkfloydx33
Copy link

Also FWIW, I think most people may be used to specifying the managed identity's clientID using the ServiceAccount annotation. The value on the CRD is meant to act as an override for cases where you are dealing with multiple DNS zones that need different MSI's for access (though it can work perfectly fine this way).

I pushed for that feature to be added to the PR so that there was parity with pod-identity and so that cert-manager behaved similarly to other applications that have already added AZWI support. Personally, I only need interaction with one DNS zone per cluster, so specifying the clientId as a ServiceAccount annotation and omitting the managedIdentity object is what I would do. I'm not sure what the common case would look like though...

Maybe include an example of both ways? One using the annotation only and another example for "configuring cert-manager to use multiple DNS zones requiring different MSI's"?

--node-count 1 \
--node-vm-size "Standard_B2s" \
--load-balancer-sku basic \
--enable-oidc-issuer # ℹ️ This is required for workload identity federation and is only available when using the aks-preview extension.
Copy link

@weisdd weisdd Nov 26, 2022

Choose a reason for hiding this comment

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

Thanks for writing the detailed docs! I'll try to have a closer look at the document soon.

There's a tiny thing that I've just spotted:

  • OIDC issuer has been promoted to GA in AKS 2022-10-16, so I think the aks-preview extension is no longer a requirement for that.
Suggested change
--enable-oidc-issuer # ℹ️ This is required for workload identity federation and is only available when using the aks-preview extension.
--enable-oidc-issuer # ℹ️ This is required for workload identity federation.

aks-preview is definitely needed when Workload Identity Webhook itself is installed as a preview extension, not sure if it's mandatory for running az identity federated-credential create assuming the webhook is installed through a chart.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@wallrj
Copy link
Member Author

wallrj commented Nov 29, 2022

Thanks both of you for the feedback. I've updated the tutorial to use the --enable-workload-identity flag,
and I've added the service account label through a helm install --set-string flag,
and that simplifies things quite a lot.

@jetstack-bot jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 29, 2022
--node-vm-size "Standard_B2s" \
--load-balancer-sku basic \
--enable-oidc-issuer \
--enable-workload-identity # ℹ️ This is currently only available when using the aks-preview extension.
Copy link

Choose a reason for hiding this comment

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

I think it'd be useful to mention that if EnableWorkloadIdentityPreview feature flag is not yet enabled for the subscription, a user will have to follow these steps: https://learn.microsoft.com/en-us/azure/aks/workload-identity-deploy-cluster#register-the-enableworkloadidentitypreview-feature-flag

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@wallrj wallrj changed the title WIP: An Azure AKS + Let's Encrypt Tutorial An Azure AKS + Let's Encrypt Tutorial Nov 30, 2022
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2022
@wallrj

This comment was marked as outdated.

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2022
@wallrj
Copy link
Member Author

wallrj commented Nov 30, 2022

Maybe include an example of both

@pinkfloydx33 I didn't have the energy to document client-id both ways.
It seems to me that adding the client-id to the issuer spec is more appropriate because otherwise it requires a decision to be made about which managed identity to associate with the cert-manager service account by a cluster administrator at cert-manager install time.
This seems like a decision to be made by a cert-manager administrator who is responsible for configuring the clusterissuer....that's my excuse anyway :-)

@pinkfloydx33
Copy link

Fine by me, makes sense. Like I said, not sure what the common case will be.

@wallrj wallrj changed the base branch from master to release-next December 2, 2022 12:17
@jetstack-bot jetstack-bot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Dec 2, 2022
--wait \
--namespace cert-manager \
--set installCRDs=true \
--set-string 'serviceAccount.labels.azure\.workload\.identity/use=true' # ❗ Label the cert-manager service account for the attention of workload-identity webhook
Copy link

Choose a reason for hiding this comment

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

There's upcoming change in workload identity webhook, which would require the label to rather be set on pods (Azure/azure-workload-identity#601). For the transition period, any of those would work (serviceAccount or pod label: https://github.com/Azure/azure-workload-identity/blob/df6362a239c5d3fc19f48ff8bc7ae2ced594f116/pkg/webhook/webhook.go#L136). Since we never know which version of the webhook is installed in a cluster, I guess we can make the example universal by simply setting the label in two places.

Suggested change
--set-string 'serviceAccount.labels.azure\.workload\.identity/use=true' # ❗ Label the cert-manager service account for the attention of workload-identity webhook
--set-string 'podLabels.azure\.workload\.identity/use=true' \
--set-string 'serviceAccount.labels.azure\.workload\.identity/use=true' # ❗ Label the cert-manager service account and pod for the attention of workload-identity webhook

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've moved these to a separate values.yaml file for easier readability.

@wallrj wallrj force-pushed the azure-aks-tutorial branch 2 times, most recently from 6e7ded7 to 89a41fd Compare January 5, 2023 16:41
@@ -0,0 +1,6 @@
# values.yaml
podLabels:
azure.workload.identity/use: "true"
Copy link

Choose a reason for hiding this comment

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

Suggested change
azure.workload.identity/use: "true"
azure.workload.identity/use: "true"

The indentation is a bit off, the rest looks great :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jetstack-bot jetstack-bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 6, 2023
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
…ndex page

Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
Copy link
Member Author

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

@SgtCoDFish I think I addressed all your code review comments.
PTAL

clusterissuer-selfsigned.yaml Outdated Show resolved Hide resolved
content/docs/manifest.json Outdated Show resolved Hide resolved
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

I've added a couple of suggestions - should've thought about the embedded file thing before. Happy to merge as-is, but added a hold in case you'd like to embed the files!

Add it to `next-docs`
Add it to `docs/` but branch from the [`release-next` branch](https://github.com/cert-manager/website/tree/release-next) and merge the PR into that branch.
Copy link
Member

Choose a reason for hiding this comment

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

Great drive-by improvement 👍

Comment on lines +208 to +229
```yaml
# certificate.yaml
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: www
spec:
secretName: www-tls
privateKey:
rotationPolicy: Always
commonName: www.$DOMAIN_NAME
dnsNames:
- www.$DOMAIN_NAME
usages:
- digital signature
- key encipherment
- server auth
issuerRef:
name: selfsigned
kind: ClusterIssuer
```
🔗 <a href="certificate.yaml">`certificate.yaml`</a>
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I don't think it's required for you to do this, but seeing this just triggered a memory for me of one of the benefits of the website migration - embedding YAML files.

See for example the old ACME tutorial (which maybe should be a redirect to your newer/better one, but that's a task for another day) here.

(Which produces this page)

That would ensure that the YAML in this document doesn't get out of sync with the YAML in certificate.yaml.

I like that you've got an explicit link to it as well as embedding it, in any case!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. In #1140

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know about that feature. Thanks.

Comment on lines +483 to +487
```
helm upgrade cert-manager jetstack/cert-manager \
--namespace cert-manager \
--reuse-values \
--values values.yaml
Copy link
Member

Choose a reason for hiding this comment

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

nit (non-blocking): missing a tag for the code block, but it doesn't really matter!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done #1140

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2023
@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish, wallrj

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SgtCoDFish
Copy link
Member

Should also say that I've not tested this (no Azure env) but I think it looks awesome!

@wallrj
Copy link
Member Author

wallrj commented Jan 10, 2023

/hold cancel

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2023
@jetstack-bot jetstack-bot merged commit aada753 into cert-manager:release-next Jan 10, 2023
@wallrj wallrj deleted the azure-aks-tutorial branch January 10, 2023 16:30
@jetstack-bot jetstack-bot mentioned this pull request Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants