Skip to content

Conversation

ksubrmnn
Copy link
Contributor

@ksubrmnn ksubrmnn commented Jul 6, 2023

What this PR does / why we need it:
Skips alpn header rewrite if alpnOverride filter metadata is false. This metadata is set by Istio when TLS mode is simple.

Works with istio/istio#44918

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Partially solves istio/istio#40680

Special notes for your reviewer:
Please read the RFC for the design details

Can I get a pointer for where/how to add a test?

@ksubrmnn ksubrmnn requested a review from a team July 6, 2023 22:25
@istio-policy-bot
Copy link

😊 Welcome @ksubrmnn! This is either your first contribution to the Istio proxy repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 6, 2023
@ksubrmnn
Copy link
Contributor Author

ksubrmnn commented Jul 6, 2023

cc @howardjohn @keithmattix

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

conceptually lgtm but I am not an expert on this code so I won't/can't approve

cc @kyessenov

@@ -55,6 +55,24 @@ Http::Protocol AlpnFilterConfig::getHttpProtocol(
}

Http::FilterHeadersStatus AlpnFilter::decodeHeaders(Http::RequestHeaderMap&, bool) {
auto upstream_info = decoder_callbacks_->streamInfo().upstreamInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const?

Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Looks fine, can you add a test?

@kyessenov
Copy link
Contributor

asan is a flake, you can ignore stackdriver test failures
/retest

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 17, 2023
@ksubrmnn ksubrmnn added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 18, 2023
@keithmattix
Copy link
Contributor

Part of istio/istio#40680

@ksubrmnn ksubrmnn removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 31, 2023
Signed-off-by: Kalya Subramanian <kasubra@microsoft.com>
@jewertow
Copy link
Member

jewertow commented Aug 1, 2023

/test test-asan

1 similar comment
@jewertow
Copy link
Member

jewertow commented Aug 1, 2023

/test test-asan

@istio-testing istio-testing merged commit 58ac7be into istio:master Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ALPN filter incorrectly applies to non-Istio TLS traffic
9 participants