Skip to content
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

Removed all parts related to runtime guard - https://github.com/envoy… #34199

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

danieldradware
Copy link
Contributor

…proxy/envoy/issues/33627

Commit Message:
Removed all parts related to runtime guard

Additional Description:
As Envoy team asked from me to remove all relevant parts of runtime guard (#33627)
In this PR I removed it.

Risk Level:
Testing:
Removed tests

Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #34199 was opened by danieldradware.

see: more, trace.

@adisuissa
Copy link
Contributor

/wait

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/wait-any

@@ -330,10 +329,6 @@ absl::string_view ExtractorImpl::extractJWT(absl::string_view value_str,
}

// There should be two dots (periods; 0x2e) inside the string, but we don't verify that here
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.token_passed_entirely")) {
return value_str.substr(starting);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't get something fundamental in this PR.
In #28678 you introduced a feature, and added a runtime-guard around it, so that if one wants to temporarily disable it, they can.
This PR, IIUC, is removing the implementation that was added (reverting the previous PR). Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! you're right I missed it that runtime guard is by default enabled.
Now I fixed it and now it will always run my change.
Till now code is cutting the valid token and from now always pass it as is.
Thanks for your important comment!

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM. Please add a release note stating that the runtime guard is removed.
Assigning @lizan as codeowner and senior-maintainer reviwer.
/assign @lizan

Signed-off-by: Daniel Danan <danield@radware.com>
…work with my changes (without runtime guard) + returned a comment to extractor.cc

Signed-off-by: Daniel Danan <danield@radware.com>
Signed-off-by: Daniel Danan <danield@radware.com>
Signed-off-by: Daniel Danan <danield@radware.com>
@danieldradware
Copy link
Contributor Author

Added to release notes

changelogs/current.yaml Outdated Show resolved Hide resolved
Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: danieldradware <117576776+danieldradware@users.noreply.github.com>
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@adisuissa
Copy link
Contributor

Apologies, the assignment bot seems to be broken.

Assigning @lizan as codeowner and senior-maintainer reviewer.
/assign @lizan

@jmarantz
Copy link
Contributor

@zuercher for senior stamp

@adisuissa adisuissa merged commit fa6b696 into envoyproxy:main Jun 3, 2024
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants