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.

Sign up for GitHub

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

Jump to bottom

Websocket: Fixed not ignoring of disabled filter for Websocket #35292

Merged
merged 5 commits into from
Jul 29, 2024
Merged

Websocket: Fixed not ignoring of disabled filter for Websocket #35292

merged 5 commits into from
Jul 29, 2024

Conversation

DvoryankinEvgeny
Copy link
Contributor

Signed-off-by: Dvoriankin Evgenii evgenijdv@gmail.com

Commit Message: Websocket: Fixed not ignoring of disabled filter for Websocket
Additional Description:
Risk Level: Low
Testing: Integration test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Fixes: #35191
(https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

@repokitteh-read-only RepoKitteh - Read Only
Copy link

Hi @DvoryankinEvgeny, 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.

🐱

Caused by: #35292 was opened by DvoryankinEvgeny.

see: more, trace.

@repokitteh-read-only RepoKitteh - Read Only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #35292 was opened by DvoryankinEvgeny.

see: more, trace.

@DvoryankinEvgeny DvoryankinEvgeny mentioned this pull request Jul 20, 2024
Closed
@DvoryankinEvgeny DvoryankinEvgeny marked this pull request as ready for review July 22, 2024 13:50
@DvoryankinEvgeny
Copy link
Contributor Author

@alyssawilk @mattklein123 could you please take a look on my changes? Detailed description of what is going on there is in #35191 ticket.
Thank you in advance

@yanavlasov
Copy link
Contributor

Assigning @botengyao for the first pass review

botengyao
botengyao reviewed Jul 25, 2024
View reviewed changes
Copy link
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Overall LGTM.

Could you also add a change log to current.yaml?

/wait

test/integration/websocket_integration_test.cc Outdated Show resolved Hide resolved
test/integration/websocket_integration_test.cc Outdated Show resolved Hide resolved
@DvoryankinEvgeny
Fixed not ignoring disabled filters for websockets
caccfe1 
Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
@DvoryankinEvgeny
Fixed tests after modifying createUpgradeFilterChain
df946ce 
Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
@DvoryankinEvgeny
Added integration test for websocket upgrade with disabled per route
5701829 
filter

Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
@DvoryankinEvgeny
Fixed review issues
15ac33b 
Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
@DvoryankinEvgeny
Fixed a few misspells in current changelog
e9a7143 
Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
@DvoryankinEvgeny
Copy link
Contributor Author

@botengyao thank you for the review. I moved my configs into the test body, got rid of my custom headers and reused existing helpers to generate them, add a change log into current.yaml and fixed a few misspells in that file.

botengyao
botengyao approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

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

This looks neat. LGTM, thanks!

And defer to @KBaichoo for another pass.

@DvoryankinEvgeny
Copy link
Contributor Author

Great! Thank you @botengyao

@KBaichoo KBaichoo self-assigned this Jul 25, 2024
KBaichoo
KBaichoo approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Really awesome work @DvoryankinEvgeny ! Thank you for digging into this.

envoy/http/filter_factory.h Show resolved Hide resolved
@DvoryankinEvgeny
Copy link
Contributor Author

Really awesome work @DvoryankinEvgeny ! Thank you for digging into this.

Thank you for your review and your kind words, glad to read it.

@KBaichoo KBaichoo removed the waiting label Jul 29, 2024
@yanavlasov yanavlasov merged commit cfedcdb into envoyproxy:main Jul 29, 2024
51 checks passed
@DvoryankinEvgeny DvoryankinEvgeny deleted the 35191 branch July 30, 2024 07:39
@DvoryankinEvgeny DvoryankinEvgeny mentioned this pull request Jul 30, 2024
Merged
martinduke pushed a commit to martinduke/envoy that referenced this pull request Aug 8, 2024
@DvoryankinEvgeny @martinduke
Websocket: Fixed not ignoring of disabled filter for Websocket (envoy…
828f768 
…proxy#35292)

---------

Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
Signed-off-by: Martin Duke <martin.h.duke@gmail.com>
asingh-g pushed a commit to asingh-g/envoy that referenced this pull request Aug 20, 2024
@DvoryankinEvgeny @asingh-g
Websocket: Fixed not ignoring of disabled filter for Websocket (envoy…
363d872 
…proxy#35292)

---------

Signed-off-by: Dvoriankin Evgenii <evgenijdv@gmail.com>
Signed-off-by: asingh-g <abhisinghx@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KBaichoo KBaichoo KBaichoo approved these changes

@botengyao botengyao botengyao approved these changes

@yanavlasov yanavlasov yanavlasov approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk alyssawilk is a code owner

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@KBaichoo KBaichoo

@yanavlasov yanavlasov

@botengyao botengyao

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to Websocket doesn't ignore disabled 'typed_per_filter_config' for this route
4 participants
@DvoryankinEvgeny @yanavlasov @KBaichoo @botengyao