kong icon indicating copy to clipboard operation
kong copied to clipboard

Published 20 hours ago • verified publisher icon Kong

feat(ip-restriction): Add TCP Support

Open scrudge opened this issue 2 years ago • 22 comments

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

  • [x] The Pull Request has tests
  • [x] There's an entry in the CHANGELOG
  • [x] Link to docs pr: https://github.com/Kong/docs.konghq.com/pull/5736

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

scrudge avatar Feb 06 '23 16:02 scrudge

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

chronolaw avatar Feb 07 '23 02:02 chronolaw

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

scrudge avatar Feb 07 '23 03:02 scrudge

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

chronolaw avatar Feb 14 '23 09:02 chronolaw

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.

scrudge avatar Feb 15 '23 01:02 scrudge

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.

chronolaw avatar Feb 15 '23 01:02 chronolaw

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.

scrudge avatar Feb 15 '23 01:02 scrudge

@scrudge Did you mean to add a test for the new functionality? I only see changes to the existing test. Please rebase to the master branch as well. Thanks!

hanshuebner avatar Feb 24 '23 08:02 hanshuebner

@scrudge Could you please address the latest feedback?

hbagdi avatar Mar 08 '23 00:03 hbagdi

Missing test for new functionality.

As I mentioned early, I have know idea how to test TCP. Is there some example you can suggest. Thanks.

scrudge avatar Mar 08 '23 04:03 scrudge

@chronolaw How do we test this feature? Could you please share some thoughts on this here?

hbagdi avatar Mar 10 '23 20:03 hbagdi

I think we could find some example code in spec/02-integration/05-proxy, such as 28-stream_plugins_triggering_spec.lua and 19-grpc_proxy_spec.lua.

chronolaw avatar Mar 14 '23 01:03 chronolaw

@scrudge Mind digging in those files and getting back?

hbagdi avatar Mar 14 '23 20:03 hbagdi

Yes, thanks.

On Tue, Mar 14, 2023, 3:01 PM Harry @.***> wrote:

@scrudge https://github.com/scrudge Mind digging in those files and getting back?

— Reply to this email directly, view it on GitHub https://github.com/Kong/kong/pull/10245#issuecomment-1468745254, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS4RZQXMOATSMPC7MMKJZ3W4DFB3ANCNFSM6AAAAAAUS4K6SQ . You are receiving this because you were mentioned.Message ID: @.***>

scrudge avatar Mar 24 '23 11:03 scrudge

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar May 12 '23 20:05 stale[bot]

Who do I need to yell at to reopen this

jjchambl avatar Jun 12 '23 19:06 jjchambl

@hbagdi can we re-open this? We are about to update with some tests.

scrudge avatar Jun 14 '23 01:06 scrudge

Reopening. Tests to be added soon

jeffrey-arndt avatar Jun 14 '23 21:06 jeffrey-arndt

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 14 '23 21:06 CLAassistant

Sorry, I tried applying a last minute suggestion and it broke the build. I've rolled it back and added tests, which are all passing. I believe this is ready for review.

scrudge avatar Jun 16 '23 13:06 scrudge

@hanshuebner @bungle

There's one remaining comment. I think I've resolved the issue, just waiting for feedback. https://github.com/Kong/kong/pull/10245#discussion_r1123420637

scrudge avatar Jun 27 '23 13:06 scrudge

Please add an entry to CHANGELOG.md.

hbagdi avatar Jun 27 '23 18:06 hbagdi

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 avatar Jul 03 '23 16:07 scrudge

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.

scrudge avatar Jul 07 '23 11:07 scrudge

@scrudge Thank your for your contribution!

hanshuebner avatar Jul 08 '23 05:07 hanshuebner

Thank you for all your help.

On Sat, Jul 8, 2023, 8:54 AM Hans Hübner @.***> wrote:

@scrudge https://github.com/scrudge Thank your for your contribution!

— Reply to this email directly, view it on GitHub https://github.com/Kong/kong/pull/10245#issuecomment-1626877380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS4RZRV36J36FL3EPKEGUTXPDYXZANCNFSM6AAAAAAUS4K6SQ . You are receiving this because you were mentioned.Message ID: @.***>

scrudge avatar Jul 08 '23 06:07 scrudge