Skip to content

Skip alpn rewrite if alpnOverride filter metadata is false#4783

Merged
istio-testing merged 1 commit intoistio:masterfrom
ksubrmnn:alpnOverride
Aug 1, 2023
Merged

Skip alpn rewrite if alpnOverride filter metadata is false#4783
istio-testing merged 1 commit intoistio:masterfrom
ksubrmnn:alpnOverride

Conversation

@ksubrmnn
Copy link
Copy Markdown
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
Copy Markdown

😊 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
Copy Markdown
Contributor Author

ksubrmnn commented Jul 6, 2023

cc @howardjohn @keithmattix

Copy link
Copy Markdown
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

}

Http::FilterHeadersStatus AlpnFilter::decodeHeaders(Http::RequestHeaderMap&, bool) {
auto upstream_info = decoder_callbacks_->streamInfo().upstreamInfo();
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

jewertow commented Aug 1, 2023

/test test-asan

1 similar comment
@jewertow
Copy link
Copy Markdown
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