Add opt-in knob to enable EnvoyPatchPolicy#1632
Conversation
Alice-Lilith
left a comment
There was a problem hiding this comment.
Yeah I think opting-in explicitly to "experimental" features is fine
Fixes envoyproxy#1468 Signed-off-by: Arko Dasgupta <arko@tetrate.io>
c2159f1 to
41c2080
Compare
Codecov Report
@@ Coverage Diff @@
## main #1632 +/- ##
==========================================
- Coverage 60.89% 60.69% -0.20%
==========================================
Files 83 84 +1
Lines 12519 12592 +73
==========================================
+ Hits 7623 7643 +20
- Misses 4408 4455 +47
- Partials 488 494 +6
|
|
can EG reuse |
| // +optional | ||
| ControllerName string `json:"controllerName,omitempty"` | ||
|
|
||
| // ExtensionAPIs defines the settings related to specific Gateway API Extensions |
There was a problem hiding this comment.
any suggestions for improving it ?
There was a problem hiding this comment.
The comment (and the name ExtensionAPIs as well) looks vague to me. Did you intentionally make it this way because this API will also serve other purposes other just than disabling/enabling specific features?
There was a problem hiding this comment.
yeah sorta, trying to use a high level noun that can encapsulate an entire class of settings, so chose ExtensionAPIs to signify all settings pertaining to it
There was a problem hiding this comment.
EnvoyPatchPolicy is not part of the Gateway API. That's why I feel confused when I saw the location of the ExtensionAPIs(it's part of the Gateway Structure) and the comment “ ExtensionAPIs defines the settings related to specific Gateway API Extensions implemented by Envoy Gateway”.
There was a problem hiding this comment.
we will need to fix this before v0.5.0, since we want EnvoyPatchPolicy to be opt in
There was a problem hiding this comment.
@kflynn @AliceProxy and I discussed this issue in the community meeting today, and favor 3.
Although ExtentionAPIs is not the best name, we cannot come up with a better name at this point so prefer sticking to it for now,
There was a problem hiding this comment.
chatted with @zhaohuabing a little more, and he shared his hesitation with
gateway:
extensionAPIs:
...
where extensionAPIs are not part of the Gateway API, which is valid.
another option is to reuse the top level extension field, that is used by the extension service
extension:
apis:
enableEnvoyPatchPolicy: true
controlPlane:
hooks:
service:
There was a problem hiding this comment.
It would be more confusing if there were two fields prefixed with "extension" at the top level.
Gateway can be the container of the settings of the whole EG, not just Gateway API. I suggest a minor change to the comment to address this:
Change
"Gateway defines the desired Gateway API configuration of Envoy Gateway."
To
"Gateway defines the desired API configuration of Envoy Gateway."
@zirain can you elaborate on this |
There was a problem hiding this comment.
LGTM.
I would feel more comfortable if the word "Gateway" is removed from the comment of the Gateway struct :-)
"Gateway defines the desired API configuration of Envoy Gateway."
Because it's a container of the settings of the whole Envoy Gateway API, not only limited to Gateway API.
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
b2de913 to
464eec9
Compare
|
hey @AliceProxy @kflynn @haq204 , in the latest commit I moved the |
zhaohuabing
left a comment
There was a problem hiding this comment.
LGTM
I'm ok with either way, slightly prefer placing the extensionApis at the top level.
| // | ||
| // +optional | ||
| Extension *Extension `json:"extension,omitempty"` | ||
| ExtensionManager *ExtensionManager `json:"extensionManager,omitempty"` |
There was a problem hiding this comment.
lgtm even though this is a breaking change for Envoy Gateway extensions. If possible we should draw extra attention to this in the release notes with a
There was a problem hiding this comment.
will do, adding the release notes label to ensure we handle ^
Fixes #1468