api: add detectMisdirectedRequests to EnvoyProxy - #9675
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9675 +/- ##
==========================================
- Coverage 81.29% 81.27% -0.02%
==========================================
Files 263 263
Lines 40859 40859
==========================================
- Hits 33216 33210 -6
- Misses 7643 7649 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Default: false | ||
| // | ||
| // +optional | ||
| DetectMisdirectedRequests *bool `json:"detectMisdirectedRequests,omitempty"` |
There was a problem hiding this comment.
can we have another top level fields for features like this may happen in the future?
There was a problem hiding this comment.
Sounds good. How about a typed struct named features like following?
spec:
features:
detectMisdirectedRequests: trueThere was a problem hiding this comment.
@envoyproxy/gateway-maintainers wdyt?
There was a problem hiding this comment.
Updated. I moved detectMisdirectedRequests under a typed features field.
There was a problem hiding this comment.
I prefer OverlappingTLSHandling to this, and then we should define as enum to extend feature in the future.
// +kubebuilder:validation:Enum=DowngradeToHTTP1;Reject
// +optional
OverlappingTLSHandling *OverlappingTLSHandling
There was a problem hiding this comment.
Thanks, I’m fine with the OverlappingTLSHandling name too.
Do you expect this to support three or more handling modes in the future?
Since this is currently just two states, enabled or disabled, I think a bool is reasonable. If we expect more handling modes soon, an enum would make sense.
There was a problem hiding this comment.
Im not sure we'd like to add another option to this field in the future. But v1alpha1 must migrate to v1alpha2 if we'd like to add any mode since bool can only handle two states literally. Whereas if we goes with this as enum, we won't care about migration.
5646738 to
d851ccf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d851ccf213
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Features defines the features that are enabled for this EnvoyProxy. | ||
| // +optional | ||
| Features *EnvoyProxyFeatures `json:"features,omitempty"` |
There was a problem hiding this comment.
Add a release note for the new EnvoyProxy API
This introduces a user-visible EnvoyProxy API field, but the commit adds no fragment under release-notes/current/. When the next release notes are generated, this API addition will therefore be omitted; add a new_features/ fragment describing the new field and its currently disabled-by-default behavior.
Useful? React with 👍 / 👎.
d851ccf to
99eac78
Compare
|
Fixed Hugo Docs / docs-lint (pull_request) ci fails in #9684 |
|
|
||
| // Features defines the features that are enabled for this EnvoyProxy. | ||
| // +optional | ||
| Features *EnvoyProxyFeatures `json:"features,omitempty"` |
There was a problem hiding this comment.
This project is for envoy, so I think it's clearer.
| Features *EnvoyProxyFeatures `json:"features,omitempty"` | |
| ProxyFeatures *ProxyFeatures `json:"proxyFeatures,omitempty"` |
| // +optional | ||
| Features *EnvoyProxyFeatures `json:"features,omitempty"` |
There was a problem hiding this comment.
| // +optional | |
| Features *EnvoyProxyFeatures `json:"features,omitempty"` | |
| // +optional | |
| // +kubebuilder:validation:MinProperties=1 | |
| Features EnvoyProxyFeatures `json:"features,omitempty"` |
You can define this field without using pointer if you add MinProperties field = 1 since it doesn't admit zero value struct.
|
@arkodg @zhaohuabing |
|
hey @zirain can you elaborate on #5879 (comment) ? |
I'm not sure if it's reasonable to add a CTP in |
|
imo the focus should be UX, ALPN lives in CTP should it makes sense for this behavior knob to live close to it |
I have no strong opinion on UX part, since I believe it should be default behavior. |
|
To keep it next to alpnProtocols (as @arkodg suggested), I'll add I'd like to confirm a few things before implementing
|
Signed-off-by: Kise Ryota <kiseryota.contact@gmail.com>
Signed-off-by: Kise Ryota <kiseryota.contact@gmail.com>
e363de1 to
f6ae4f0
Compare
How about making it more explicit?
I'd prefer to keep the existing behavior to avoid breaking change. If adding a CTP to pass comformance tests is not feasible, then we can make the default on and add a breaking change note to warn users who have old clients in production that don't understand 421 Misdirected Request, and ask them to turn it off with CTP. |
vote for off to avoid a breaking change |
+1 to enum over bool |
|
@arkodg @zhaohuabing Agreed on moving from a bool to an enum and no breaking change. I’ll go with OverlappingTLSHandling as suggested: |
it should be default off it live in CTP. |
What this PR does / why we need it:
This is part 1 of 3 for #5879, adding API support for GEP-3567 misdirected request detection.
This PR adds the
detectMisdirectedRequestsfield toEnvoyProxyand updates the generated API artifacts.Follow-up PRs will add:
HTTPRouteHTTPSListenerDetectMisdirectedRequests.The field defaults to disabled, and this PR does not change runtime behavior. The implementation is intentionally left for a follow-up PR so this PR can focus on the API
shape: field name, type, placement, and documentation.
Which issue(s) this PR fixes:
Fixes #5879
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.