-
Notifications
You must be signed in to change notification settings - Fork 632
Move BackendTLS configuration to GatewayTLSConfig #4009
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
Conversation
|
/assign @robscott @youngnick @shaneutt |
apis/v1/gateway_types.go
Outdated
| // +optional | ||
| // <gateway:experimental> | ||
| FrontendValidation FrontendTLSValidation `json:"frontendValidation"` | ||
| BackendValidation *GatewayBackendTLS `json:"backendValidation,omitempty"` |
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.
I’m not sure if BackendValidation is the best name for this field, since validation is actually part of the BackendTLSPolicy.
A structure like the following might be clearer:
tls:
frontend:
validation: {}
backend:
clientCertificateRef: {}
Alternatively, we could use the terms origination (for backend) and termination (for frontend), which are common in TLS contexts.
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.
yeah it makes sense. I updated PR PTAL.
I will update index.md and CRDs fields once we agree on final form.
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! I would also love to hear some other opinions on this.
\cc @shaneutt, @robscott, @youngnick
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.
We have a GEP that establishes some naming guidelines here: https://gateway-api.sigs.k8s.io/geps/gep-2907. I think an update to that GEP is the first thing to do here.
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.
I'm not sure what you mean - @snorwin suggested the change above, which @kl52752 implemented, which uses frontend and backend. Is that not in line with what we have in GEP-2907?
In another comment thread here, I suggested termination and origination, but @robscott also reminded me about GEP2907 and the agreed names there, and I agreed that it's better to stick with them.
robscott
left a comment
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.
This is great, thanks @kl52752!
31c2e3b to
76c650a
Compare
|
Thanks @robscott for the review. I updated API with your suggestion. I will update index.md files and examples once we have all review. |
robscott
left a comment
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 @kl52752! This helps make our top level Gateway configuration far more consistent.
|
@shaneutt I would like to request the extension for this GEP and include it in v1.4.0 release in experimental channel. We need to reach out agreement for naming convention and I think that we are very close and need 24h to finalize this. |
3b0e4ed to
6f76a70
Compare
|
/retest |
shaneutt
left a comment
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.
As per our conversation about this on the community call today:
This is not about changing behavior, but about changing wording and API spec prior to lock-in which would make it harder. There was a lot of support on the call that we should move forward. Since this seems like an overall improvement:
/lgtm
However it was also mentioned that the GEP and documentation is unclear about some of the details. @kl52752 are you open to providing more context in the GEP as per our discussion today, as part of the 1 week extension we're doing here?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kl52752, robscott, shaneutt, snorwin 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 |
I created a PR with updates to GEP-2907. Should I updated also https://gateway-api.sigs.k8s.io/guides/tls/? |
On our call yesterday, we pointed out that it's common that experimental stuff lags in documentation, so we don't want to hold you to some standard we've not held for others. At your option, if you feel inclined and can spare the time, that would be great, however it doesn't preclude the notion of us merging this. |
|
Thanks @kl52752! It looks like everything's been resolved here and we have multiple approvals now, removing the hold and we can cover docs + GEP update in the follow up PRs. /hold cancel |
* Move BackendTLS configuration to GatewayTLSConfig * review
What type of PR is this?
/kind gep
What this PR does / why we need it:
Merge Gateway TLS configuration for frontend and backend validation into top level GatewayTLSConfig.
Which issue(s) this PR fixes:
Related to: GEP-3155
Related to: #3979
Does this PR introduce a user-facing change?: