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
Adds ingress annotation support for alt-names #6190
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Robert van Veelen <vanveele@users.noreply.github.com>
Signed-off-by: Robert van Veelen <vanveele@users.noreply.github.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vanveele The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @vanveele. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Robert van Veelen <vanveele@users.noreply.github.com>
Signed-off-by: Robert van Veelen <vanveele@users.noreply.github.com>
Looking forward to using this. |
@vanveele Normally, the DNS names should be based on what is configured in the ingress right? Could you explain why you cannot specify an additional DNS name in that list of hosts? |
@SgtCoDFish Can you help out to get someone be assigned to triage this? It's small but offers great value. Please and thank you :) |
I think I'd agree with Tim's question to be honest; why is this required? I'm not saying it's definitely not required, but at least on the face of it, I'd think that this seems redundant! I wouldn't personally want to use this - which again doesn't mean it's not useful to someone, but I think it would be worth hearing a bit more detail on this first! |
@SgtCoDFish Could you link me to Tim's question? I'm not seeing it. I think this comment sums it up well:
I'm in this exact same boat. We have a hot-spare cluster that we want to carry the same SAN as the other control plane, in case we need to cut over. AFAIK, the only way to do this is by creating a Certificate resource directly, which is a bit more onerous for some services. Is there an undocumented annotation that covers this use case already, or maybe something else I'm missing? Thanks as always for staying on top of this org! |
Apologies, I should have tagged @inteon rather than using his name! I meant this comment. Specifically:
Going from your example, I think I'm still missing why that's not possible (not saying it's impossible - I just want to understand!) My view of this architecture with (say) an AWS EKS primary and a GKE hot spare would be that both clusters would have an Question number 2: This PR currently overrides the DNS names which have been retrieved from the |
That might be a condition specific to those clouds. I can't really speak to those - I'm working on-premise. I assume managed clusters use a native concept of autoscaling and use an out-of-band load balancer to reach the control plane. And AWS/GKE LBs use their own CA so they can issue whatever certs they want, without some of the constraints that on-prem would be subject to with ACME HTTP01 services. Edit: I think I get where you are going here. Let's use the rancher chart's
Hmm good catch. I don't have a great answer for this off the cuff. Maybe I can dedicate some more review and testing time in November or December and validate this fully. |
I can see how that would be annoying yeah. I'd not considered I think where I'm landing with this for now is:
In summary, I don't think I'd merge this PR as-is with it replacing the DNSNames. I think there's a case to be made for this feature if it appended to the DNS names - I'd value the input of other maintainers and community members on this too. |
Behaviour to replace spec was picked to remain consistent with other existing annotations, for example the adjacent If the ingress annotation were appended to the existing dnsnames then the TLS secret annotation of the same name would differ in value. Changing the ingress annotation name from 'cert-manager.io/alt-names' -> 'cert-manager.io/additional-alt-names' to implement the new logic may avoid confusion. |
I definitely agree - we should rename to that. Or to more closely match the existing names, maybe |
Issues go stale after 90d of inactivity. |
/remove-lifecycle stale |
Issues go stale after 90d of inactivity. |
Pull Request Motivation
Add support for ingress annotation controlling Certificate SAN DNS names. Fixes #5897 .
Kind
/kind feature
Release Note