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

feat(ip-restriction): Add TCP Support #10245

Merged
merged 55 commits into from Jul 7, 2023
Merged

feat(ip-restriction): Add TCP Support #10245

merged 55 commits into from Jul 7, 2023

Conversation

scrudge
Copy link
Contributor

@scrudge scrudge commented Feb 6, 2023

Summary

This change adds TCP support to the ip-restriction plugin by implementing the Stream module's preread function.

When a TCP connection is rejected due to IP restriction rules, a JSON error response is written to the stream and the connection is closed.

Checklist

Full changelog

  • Add protocols to schema: "tcp", "tls", "grpc", "grpcs"
  • Implement preread function
  • Make handler logic generic to be shared by HTTP and TCP

Issue reference

Fix #6679

KAG-722

kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

chronolaw commented Feb 7, 2023

This PR is almost the same with #10229, What is the relation?

@scrudge
Copy link
Contributor Author

scrudge commented Feb 7, 2023

We work on the same team. We need this feature in 2.8.x because we can't upgrade to v3 yet.

kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
@chronolaw chronolaw added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Feb 14, 2023
@chronolaw
Copy link
Contributor

Could you add some test cases for this PR? Thanks.

@scrudge
Copy link
Contributor Author

scrudge commented Feb 15, 2023

Could you add some test cases for this PR? Thanks.

Sure, but I'm completely lost on how testing works here. is this the test for this plugin :
https://github.com/Kong/kong/blob/master/t/01-pdk/03-ip/01-is_trusted.t

Also, is there some docs on how testing works? Thanks.

@chronolaw chronolaw removed the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Feb 15, 2023
@chronolaw
Copy link
Contributor

This plugin's test should be spec/03-plugins/17-ip-restriction.
Perhaps we do not have a proper doc for the test, but don't worry, we can implement the feature first.

@scrudge
Copy link
Contributor Author

scrudge commented Feb 15, 2023

Yeah, I found the tests right after I asked :-)

I'm actually working on the other suggested changed ATM.
I'll also try to fix the test that are broken from the change in response message.

kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
kong/plugins/ip-restriction/handler.lua Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 20, 2023
@scrudge scrudge requested review from chronolaw and a user and removed request for chronolaw February 20, 2023 15:42
@scrudge scrudge requested a review from hanshuebner June 21, 2023 18:11
@scrudge
Copy link
Contributor Author

scrudge commented Jun 27, 2023

@hanshuebner @bungle

There's one remaining comment. I think I've resolved the issue, just waiting for feedback.
#10245 (comment)

@hbagdi
Copy link
Member

hbagdi commented Jun 27, 2023

Please add an entry to CHANGELOG.md.

@scrudge scrudge requested a review from hbagdi June 30, 2023 20:49
@scrudge
Copy link
Contributor Author

scrudge commented Jul 3, 2023

Could you please add some return statements (it makes code cleaner about its intent).

@bungle can you please approve changes you requested? Thank you.

@scrudge
Copy link
Contributor Author

scrudge commented Jul 7, 2023

Could you please add some return statements (it makes code cleaner about its intent).

@bungle can you please approve changes you requested? Thank you.

@hanshuebner @hbagdi
I believe this PR is held waiting for a response from @bungle but I haven't heard from him since his original comment. Is there anyway to get this completed?
Thanks for all the help.

@hanshuebner hanshuebner dismissed bungle’s stale review July 7, 2023 13:40

Feedback has been addressed, ngx.exit is no longer used.

@hanshuebner hanshuebner merged commit c6987b8 into Kong:master Jul 7, 2023
32 checks passed
@hanshuebner
Copy link
Contributor

@scrudge Thank your for your contribution!

@scrudge
Copy link
Contributor Author

scrudge commented Jul 8, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author/community PRs from the open-source community (not Kong Inc) pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... plugins/ip-restriction schema-change-noteworthy size/L version-compatibility Incompatibility with older data plane versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IP Restriction at TCP Level.
9 participants