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.

Sign up for GitHub

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

Jump to bottom

Add clarification to selfsigned bootstrapping #1278

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Add clarification to selfsigned bootstrapping #1278

merged 4 commits into from
Sep 19, 2023

Conversation

Nickmman
Copy link
Contributor

@Nickmman Nickmman commented Sep 3, 2023

This adds further clarification when bootstrapping a cluster with ClusterIssuer and a Root CA Certificate. PR stemmed from cert-manager/cert-manager#6313

@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. labels Sep 3, 2023
@netlify
Copy link

netlify bot commented Sep 3, 2023 •
edited
Loading

✅ Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit f28e9aa
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/6508d3d6ebd501000811378f
😎 Deploy Preview https://deploy-preview-1278--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 configuration.

@erikgb
Copy link
Contributor

erikgb commented Sep 8, 2023

LGTM, but I think our PKI expert should take a look. 😄

/cc @SgtCoDFish

erikgb
erikgb reviewed Sep 8, 2023
View reviewed changes
Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

Would it be possible to explain your cluster issuer use-case without semi-duplicating the YAML? IMO this would be easier to understand by describing the differences in using an issuer vs cluster issuer.

content/docs/configuration/selfsigned.md Outdated Show resolved Hide resolved
content/docs/configuration/selfsigned.md Outdated
ca:
secretName: root-secret
```
The first `ClusterIssuer` is used to sign the Root CA, in this case. The second `ClusterIssuer` is used to sign certificates using said Root CA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better if we refer to the cluster issuers by name? WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I'd agree with this, it's a good suggestion!

SgtCoDFish
SgtCoDFish requested changes Sep 13, 2023
View reviewed changes
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.

First of all, thanks for contributing - really great to see!

I don't have anything to add beyond what Erik already said. I think if his comments are addressed I'd gladly merge!

content/docs/configuration/selfsigned.md Outdated
ca:
secretName: root-secret
```
The first `ClusterIssuer` is used to sign the Root CA, in this case. The second `ClusterIssuer` is used to sign certificates using said Root CA.
Copy link
Member

Choose a reason for hiding this comment

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

I'd agree with this, it's a good suggestion!

@Nickmman
Copy link
Contributor Author

@erikgb @SgtCoDFish Apologies for the delay. Thank you for taking the time to review and comment on this PR. I've accepted the suggestions and also pushed a new commit where I reword and clarify the excerpt regarding double ClusterIssuers.

SgtCoDFish
SgtCoDFish approved these changes Sep 15, 2023
View reviewed changes
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

Looks like a great improvement, thank you so much for contributing 😁

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2023
@SgtCoDFish
Copy link
Member

Ah, you might need to add "selfsigned-issuer" to .spelling!

@jetstack-bot jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed lgtm Indicates that a PR is ready to be merged. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 18, 2023
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Sep 18, 2023
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2023
Nickmman and others added 4 commits September 18, 2023 19:48
@Nickmman
Add clarification to selfsigned bootstrapping
a05856e 
Signed-off-by: Nicolas Pais <[email protected]>
@Nickmman @erikgb
Update namespace delcaration
81bf3ef 
Cluster issuer will look for the secret in the namespace where cert-manager is installed

Co-authored-by: Erik Godding Boye <[email protected]>
Signed-off-by: Nicolas Pais <[email protected]>
@Nickmman
Refer to ClusterIssuer by name, reword for clarity
b97f9c0 
Signed-off-by: Nicolas Pais <[email protected]>
@Nickmman
Add selfsigned-issuer to .spelling
f28e9aa 
Signed-off-by: Nicolas Pais <[email protected]>
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2023
@SgtCoDFish
Copy link
Member

/lgtm

Thanks again for this!

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2023
@jetstack-bot jetstack-bot merged commit a1acac7 into cert-manager:master Sep 19, 2023
1 check passed
@jetstack-bot jetstack-bot mentioned this pull request Dec 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgb erikgb erikgb left review comments

@SgtCoDFish SgtCoDFish Awaiting requested review from SgtCoDFish

Assignees
No one assigned
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Nickmman @erikgb @jetstack-bot @SgtCoDFish