-
Notifications
You must be signed in to change notification settings - Fork 480
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
feat: support to configure multiple certificates on GatewayTLSConfig #852
feat: support to configure multiple certificates on GatewayTLSConfig #852
Conversation
Welcome @tokers! |
Hi @tokers. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define the behavior when there are multiple?
I think this one is likely sufficiently complex that we'll need a GEP first, I created #851 to track that, will try to get a GEP PR out later today. I can add some details on how multiple should be handled there. |
Adding a hold until GEP is approved. /hold |
Cool, look forward to it. |
Update: I've opened #856 to introduce a GEP for this. Once that merges in, we can remove the hold here. |
Are you still able to take this on @tokers? Would love to get this in in the next couple of days. If you're not able to, I'm happy to take this on. |
Yeah, I still focus on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @tokers! This mostly LGTM, just had a few nits. I think the k8s automation will complain about merge commits in this PR - please rebase and squash your commits when you get a chance.
/ok-to-test |
One more thing that's difficult to comment on in the diff - as part of this change you'll also need to update the TLS examples in the examples/v1alpha2 directory. |
ea55892
to
e2c6fd5
Compare
OK, I rebased all the commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tokers! I think you need to run make generate
again, I can add a LGTM after that.
/approve
apis/v1alpha2/gateway_types.go
Outdated
// Listener has a TLS configuration with a TLS CertificateRef | ||
// that is invalid or cannot be resolved. | ||
// Listener has a TLS configuration with at least one TLS CertificateRef | ||
// that are invalid or cannot be resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think this should still be "is"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, let me tweak it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott, tokers 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 |
b39dbf1
to
e7d51ed
Compare
/test pull-gateway-api-verify |
@tokers Can you run |
Sorry for the delay, I will fix the CI errors today. |
It seems that this is due to the version of go language (the |
What's the go version in the CI? Maybe I can try to switch to that one and re-execute the |
Signed-off-by: Chao Zhang <[email protected]>
e7d51ed
to
b93c20f
Compare
Thanks! /lgtm |
Hold was waiting for GEP to be approved, which happened ~weeks ago. /hold cancel |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR uses
CertificateRefs
to insteadCertificateRef
so that multiple certificates can be configured for a Listener. With this feature, we can support things like following items:Which issue(s) this PR fixes:
Fixes #851
Does this PR introduce a user-facing change?: