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

docs: fix user/tls-cert-manager Issuer/ClusterIssuer #1473

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

tommie
Copy link
Contributor

@tommie tommie commented Jun 1, 2023

Based on feedback from @irbekrm of cert-manager.

Based on feedback from @irbekrm of cert-manager.

Signed-off-by: Tommie Gannert <tommie@gannert.se>
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #1473 (689d34a) into main (35e6166) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1473      +/-   ##
==========================================
+ Coverage   61.65%   61.67%   +0.01%     
==========================================
  Files          79       79              
  Lines       11490    11490              
==========================================
+ Hits         7084     7086       +2     
+ Misses       3947     3945       -2     
  Partials      459      459              

see 2 files with indirect coverage changes

Copy link

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks for writing the awesome tutorial and thanks for making the changes @tommie !

I've added a nit.

Apart from that I have two optional suggestions:

  • we really don't recommend using self-signed issuer for any other case except for quick testing of things and for bootstrapping a root CA cert. In our own example setups we typically use a selfsigned issuer to issue a CA cert for a CA issuer that can then be used to issue certs for applications like so. I understand that you may not want to make the setup too complex here, but perhaps would be nice to add an additional warning that the self signed issuer should not be used beyond a quick test of things

  • In the garbage collection section it mentions that Certificates will be garbage collected when Secrets are removed - this is not quite the case, for gateway-shim/ingress-shim they will be deleted because if the relevant TLS block has been removed from Gateway or Ingress. If a Secret was deleted (i.e by a user) that is referenced by a Certificate, cert-manager would actually re-create the Secret

docs/latest/user/tls-cert-manager.md Outdated Show resolved Hide resolved
Incorporate additional feedback from @irbekrm.

Signed-off-by: Tommie Gannert <tommie@gannert.se>
@tommie
Copy link
Contributor Author

tommie commented Jun 1, 2023

  • we really don't recommend using self-signed issuer for any other case except for quick testing of things and for bootstrapping a root CA cert. In our own example setups we typically use a selfsigned issuer to issue a CA cert for a CA issuer that can then be used to issue certs for applications like so. I understand that you may not want to make the setup too complex here, but perhaps would be nice to add an additional warning that the self signed issuer should not be used beyond a quick test of things

Yes, I want something that is simple to start with. selfSigned has no configuration, and doesn't require access to external services, which makes it an ideal thing to start with. Difficult to screw up. I've added a note.

  • In the garbage collection section it mentions that Certificates will be garbage collected when Secrets are removed - this is not quite the case, for gateway-shim/ingress-shim they will be deleted because if the relevant TLS block has been removed from Gateway or Ingress. If a Secret was deleted (i.e by a user) that is referenced by a Certificate, cert-manager would actually re-create the Secret

Nice catch. Updated.

@irbekrm
Copy link

irbekrm commented Jun 2, 2023

Thanks for making the changes! Looks great from cert-manager perspective 😄

/lgtm

@tommie tommie marked this pull request as ready for review June 2, 2023 14:18
@tommie tommie requested a review from a team as a code owner June 2, 2023 14:18
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

thanks for the updates @tommie and @irbekrm for the review

@arkodg arkodg merged commit 572c495 into envoyproxy:main Jun 2, 2023
18 checks passed
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.

None yet

3 participants