Skip to content

router/jwt_authn: remove support for v2 deprecated regex and cors.#16517

Merged
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
htuch:remove-v2-regex
Jun 1, 2021
Merged

router/jwt_authn: remove support for v2 deprecated regex and cors.#16517
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
htuch:remove-v2-regex

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented May 16, 2021

Risk level: Low
Testing: Unit tests modified.

Signed-off-by: Harvey Tuch htuch@google.com

Risk level: Low
Testing: Unit tests modified.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Copy Markdown
Member Author

htuch commented May 18, 2021

/assign-from @envoyproxy/maintainers

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/maintainers assignee is @mattklein123

🐱

Caused by: a #16517 (comment) was created by @htuch.

see: more, trace.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented May 18, 2021

/assign-from @envoyproxy/maintainers

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/maintainers assignee is @junr03

🐱

Caused by: a #16517 (comment) was created by @htuch.

see: more, trace.

alyssawilk
alyssawilk previously approved these changes May 20, 2021
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

YAY!!!!

LGTM modulo that one question

header_match_type_ = HeaderMatchType::Value;
value_ = config.exact_match();
break;
case envoy::config::route::v3::HeaderMatcher::HeaderMatchSpecifierCase::
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.

I don't love that there's a default in this switch statement. lazy check: we'll fail to load before we can hit tihs case yeah?

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.

We could add LOG_EVERY_N error when the default is hit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is intentional behavior it seems, see

@alyssawilk
Copy link
Copy Markdown
Contributor

oh, and needs merge :-/

@yanavlasov yanavlasov self-assigned this May 21, 2021
@yanavlasov
Copy link
Copy Markdown
Contributor

/wait

@htuch
Copy link
Copy Markdown
Member Author

htuch commented May 31, 2021

Conflict fixed, PTAL and please merge if looks good as I'm OOO. Thanks.

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

LGTM!!!!

@alyssawilk alyssawilk merged commit 4bb8236 into envoyproxy:main Jun 1, 2021
@alyssawilk
Copy link
Copy Markdown
Contributor

cc @phlax just to make sure this one was on your radar

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
…nvoyproxy#16517)

Risk level: Low
Testing: Unit tests modified.

Signed-off-by: Harvey Tuch <htuch@google.com>
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.

5 participants