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

Vault: document the new field "serviceAccountRef" #1081

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 30, 2022
@netlify
Copy link

netlify bot commented Sep 30, 2022

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 32a3e8b
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/640704f952a90e0008e13c09
😎 Deploy Preview https://deploy-preview-1081--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.

@maelvls maelvls marked this pull request as draft October 3, 2022 13:42
@jetstack-bot jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2022
@maelvls maelvls force-pushed the document-vault-bound-serviceaccount branch 2 times, most recently from f48cf84 to 072fbe5 Compare February 9, 2023 14:10
@maelvls maelvls marked this pull request as ready for review February 9, 2023 14:17
@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 Feb 9, 2023
Copy link
Member

@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.

Nice start to the documentation for this feature, but please add some narrative and some links to Hashicorp Vault documentation explaining how this style of authentication works.

Shouldn't we also explain how to configure vault for this authentication (both for in-cluster vault and out-of-cluster vault installations) or is that explained elsewhere in this document?

Also please rebase this onto the release-next branch, adding a release note to go with it.

content/docs/configuration/vault.md Outdated Show resolved Hide resolved
Comment on lines 265 to 266
Using the field `serviceAccountRef` instead of `secretRef`, you can let
cert-manager request ephemeral tokens.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should elaborate more on what it means to "request ephemeral tokens".
For example: https://cert-manager.io/docs/configuration/acme/dns01/azuredns/#managed-identity-using-aad-workload-identity

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 added a paragraph on "request ephemeral tokens". Hopefully it makes more sense now.

content/docs/configuration/vault.md Outdated Show resolved Hide resolved
content/docs/configuration/vault.md Outdated Show resolved Hide resolved
content/docs/configuration/vault.md Outdated Show resolved Hide resolved
mountPath: /v1/auth/kubernetes
serviceAccountRef:
name: vault-issuer
```
Copy link
Member

Choose a reason for hiding this comment

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

Add a note about Issuer vs ClusterIssuer and how Issuer can only refer to a ServiceAccount in the same namespace while ClusterIssuer must refer to a ServiceAccount in the --cluster-scoped namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I added the following:

Issuer vs. ClusterIssuer: With an Issuer resource, you can only refer to a
service account located in the same namespace as the Issuer. With a
ClusterIssuer, the service account must be located in the namespace that is
configured by the flag --cluster-resource-namespace.

@maelvls maelvls changed the base branch from master to release-next February 20, 2023 17:39
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Feb 20, 2023
Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls maelvls force-pushed the document-vault-bound-serviceaccount branch from f405ed8 to 56fd5f0 Compare February 21, 2023 11:36
@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. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 21, 2023
@maelvls maelvls force-pushed the document-vault-bound-serviceaccount branch from 9b482ae to c2748dd Compare February 21, 2023 17:48
@maelvls
Copy link
Member Author

maelvls commented Feb 21, 2023

Shouldn't we also explain how to configure vault for this authentication (both for in-cluster vault and out-of-cluster vault installations)

The only documentation that exists in the page is the vault write auth... command that I added. It only focuses on in-cluster installations.

content/docs/configuration/vault.md Outdated Show resolved Hide resolved
content/docs/configuration/vault.md Outdated Show resolved Hide resolved
content/docs/configuration/vault.md Outdated Show resolved Hide resolved
content/docs/contributing/release-process.md Outdated Show resolved Hide resolved
Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls maelvls force-pushed the document-vault-bound-serviceaccount branch from c2748dd to ab117a7 Compare February 27, 2023 15:26
maelvls and others added 2 commits February 27, 2023 16:41
Co-authored-by: Richard Wall <wallrj@users.noreply.github.com>
Signed-off-by: Maël Valais <mael@vls.dev>
Co-authored-by: Richard Wall <wallrj@users.noreply.github.com>
Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls
Copy link
Member Author

maelvls commented Feb 27, 2023

I addressed the comments and have added a paragraph to the release-notes-1.12.md page. Please take another look.

Signed-off-by: Maël Valais <mael@vls.dev>
Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls maelvls force-pushed the document-vault-bound-serviceaccount branch from 5224b0e to c399aae Compare February 27, 2023 16:14
Signed-off-by: Maël Valais <mael@vls.dev>
package.json Outdated
@@ -22,7 +22,7 @@
"generate:sitemap": "next-sitemap",
"export": "next export",
"start": "next start",
"check": "concurrently --group --timings npm:check:* # Run all the npm check:* scripts in parallel",
"check": "npm exec concurrently -y -- --group --timings npm:check:* # Run all the npm check:* scripts in parallel",
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change?
All the npm installed executables are installed in .node_modules/.bin by npm ci and are in PATH when these npm run scripts are run, so why do you add the npm exec ... -y here?

Copy link
Member

Choose a reason for hiding this comment

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

If the documentation in https://github.com/cert-manager/website#website-development-tooling is wrong, we should fix that.

…ool"

This reverts commit c399aae.

I mistakenly forgot to run "npm i" before running ./scripts/verify.

Signed-off-by: Maël Valais <mael@vls.dev>
Copy link
Member

@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.

Thanks!

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls, 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

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. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants