envoy
envoy copied to clipboard
envoyproxy
Disable shadow host suffix append
Commit Message: Add runtime feature envoy.reloadable_features.enable_shadow_header_suffix to toggle appending the -shadow suffix to mirrored clusters. Default is false (appends -shadow).
Additional Description: Issue: https://github.com/envoyproxy/envoy/issues/9094
Risk Level: Low
Testing: Unit
Docs Changes: Updated route mirror
Release Notes: Updated current.yaml
Platform Specific Features: n/a
Optional Runtime guard: envoy_reloadable_features_enable_shadow_header_suffix
Fixes #9094
Hi @jh125486, welcome and thank you for your contribution.
We will try to review your Pull Request as quickly as possible.
In the meantime, please take a look at the contribution guidelines if you have not done so already.
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/31930 was opened by jh125486.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/31930 was opened by jh125486.
/wait
Unless the plan is to remove the suffix entirely in all cases, I think the proto change is more in line with how we usually handle conditional features. Especially since this could useful on a cluster-by-cluster basis.
I’m working on the proto change/tests, a bit slow because building is broken on ARM, but I should have something tomorrow morning.
V/r Jacob Hochstetler
From: Stephan Zuercher @.> Sent: Monday, January 22, 2024 4:46:47 PM To: envoyproxy/envoy @.> Cc: Jacob Hochstetler @.>; Mention @.> Subject: Re: [envoyproxy/envoy] Disable shadow host suffix append (PR #31930)
/wait
Unless the plan is to remove the suffix entirely in all cases, I think the proto change is more in line with how we usually handle conditional features. Especially since this could useful on a cluster-by-cluster basis.
— Reply to this email directly, view it on GitHubhttps://github.com/envoyproxy/envoy/pull/31930#issuecomment-1904963230, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAN5QHKWYVM3CSWTZ5SL7DLYP3T5PAVCNFSM6AAAAABCFLSKOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBUHE3DGMRTGA. You are receiving this because you were mentioned.Message ID: @.***>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/31930 was synchronize by brianwarner.
/wait
Unless the plan is to remove the suffix entirely in all cases, I think the proto change is more in line with how we usually handle conditional features. Especially since this could useful on a cluster-by-cluster basis.
@botengyao / @zuercher Ready for review on the proto API changes.
Thank you for your contribution! Overall LGTM. In addition to the comments, please add an integration test that tests this feature and a release-note.
/wait
Yep, I can fix these things.
For release notes, I thought the section I added to current.yaml was where it went. Is there another place I need to put release notes?
For full transparency, I can't get the dev container to work through our proxy, bazel arm builds are broken (The current user has CAP_DAC_OVERRIDE set error) and consequently I'm sorta flying blind without an IDE.
I thought the section I added to current.yaml was where it went
all good - it is
For full transparency, I can't get the dev container to work through our proxy, bazel arm builds are broken (The current user has CAP_DAC_OVERRIDE set error) and consequently I'm sorta flying blind without an IDE.
looking at the .devcontainer caps - i think its overly permissive - not sure why it needs so much capability - iirc these can be used for privilege escalation
https://github.com/envoyproxy/envoy/blob/45ab9cfa30c7631fe325847dd291e2dcc80c1894/.devcontainer/devcontainer.json#L6-L9
i dont use vscode myself, so not sure if they are necessary - my guess is that these were just copied from the ci/run_envoy_docker.sh script which i think used to have similar
(EDIT: not 100% this is the issue - but probs above point stands)
windows failiure is unrelated https://github.com/envoyproxy/envoy/issues/32060
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Sorry it took so long... got the requests changes done. It's currently working through my side.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
Still currently working through my side.
@phlax PR has been updated, sorry for the delay. I have not updated the change log yet, as it didn't make sense to do so until the actual code was looked over.
/assign @htuch Harvey it looks like you've already taken a look at this PR. Mind reviewing it?
