-
Notifications
You must be signed in to change notification settings - Fork 498
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
Add ParametersRef
to Gateway.spec.infrastructure
#2924
Conversation
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 @howardjohn! Definitely a stretch to get into v1.1 given the timeline, but since this is just a part of a GEP that was already promoted to experimental, it seems like there might be a small chance it could fit in if we don't run into any significant issues.
apis/v1/gateway_types.go
Outdated
// Support: Implementation-specific | ||
// | ||
// +optional | ||
ParametersRef *ParametersReference `json:"parametersRef,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.
This allows for cross-namespace references, I'm not sure we want to allow for that. The corresponding GEP doesn't make it obvious if those are needed, do you mind updating the GEP to clarify the goals 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 made a new LocalParametersReference
to force it to be same namespace
// parameters corresponding to the Gateway. This is optional if the | ||
// controller does not require any additional configuration. | ||
// | ||
// This follows the same semantics as GatewayClass's `parametersRef`, but on a per-Gateway basis |
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 think we need to describe how merging works if/when this is configured at both layers.
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.
Added
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.
Would like to see this get in!
For context, GEP-1867, which is currently in Experimental status, covers the background motivation and why GatewayClass parametersRef
is not sufficient, and #2911 is an example of a specific issue that arose from the tension between how GatewayClass parametersRef
was intended/suggested to be used (as a template), and the role that it's being stretched into filling in practice because of the gap that adding this Gateway-scoped option for implementation-specific dynamic configuration would help solve.
A summary of the feedback that blocked adding this during API review previously can be found at #1757 (comment) and additional supporting context for how Gateways are being used (and who is configuring them) to justify this need can be found in #1653 (comment).
@howardjohn Would this solve the core need in #567?
@mikemorris from my POV this is good enough to close #567 |
I'm still a little worried that this will drop the incentive for people to bring further changes to That said, /approve |
fwiw I don't think we have plans to implement paramsref in istio in the near term so we are incentivezed to keep pushing for first class fields once this lands :-) |
/assign @robscott |
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.
/approve
/lgtm
/unhold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: howardjohn, mikemorris, shaneutt, youngnick 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 |
c3f1996
to
d60bcf8
Compare
d60bcf8
to
ec6beaa
Compare
ec6beaa
to
cb6dd21
Compare
/lgtm |
What type of PR is this?
/kind gep
What this PR does / why we need it:
Follow up to #1868, which was down-scoped to only include labels and annotations to reduce friction on the initial GEP
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: