search

bytedance / g3

Enterprise-oriented Generic Proxy Solutions
Apache License 2.0
472 stars 35 forks source link

turn ProtocolInspectPolicy into AclDstHostRuleSet #344

Closed GlenDC closed 1 month ago

GlenDC commented 1 month ago

Closes #341

tested it with web sockets on a local g3 proxy, and it seems to work as expected:

websocket_inspect_policy:
      exact:
        default: intercept
        block:
          - "websocketstest.com"

Should work just as well for h2_inspect_policy, smtp_inspect_policy and imap_inspect_policy. But these I haven't tested. Code is same however.

zh-jq commented 1 month ago

I will do a full review after back to work on 10.10.

GlenDC commented 1 month ago

I will do a full review after back to work on 10.10.

Meaning in a month or what timeline can I expect? This is not to demand anything, but more so I can manage my own expectations and reminders. Either answer is fine, it is just to know :)

zh-jq commented 1 month ago

I will do a full review after back to work on 10.10.

Meaning in a month or what timeline can I expect? This is not to demand anything, but more so I can manage my own expectations and reminders. Either answer is fine, it is just to know :)

the day after tomorrow, maybe I should write 10/10 in English? (⊙o⊙)

GlenDC commented 1 month ago

All good. Thank you for clarifying. Now I understand. Thanks!

GlenDC commented 1 month ago

Ok @zh-jq @zh-jq-b I cleaned up the history. It has now just two commits. My initial code and then also one to apply all feedback from you

GlenDC commented 1 month ago

Double checked the code in this PR, looks still fine even after all the rebasing. I also do not see immediately where there could be a bug. Gonna double check running this code in a demo env and see if I still have the same issue, because as far as I can see the code maps 1 to 1 with the old logic, with the only difference being that we support acl-like sets now. Assuming the check methods are correct.

GlenDC commented 1 month ago

Great news @zh-jq I pulled this branch into a demo env and all seems to work. E.g.

websocket_inspect_policy:
    child:
    default: intercept
    block:
        - websocketstest.com

Blocks that host, but all others are fine. Seems that I had a mistake in my demo env due to a wrong merge at some point. With a clean slate and this branch merged into there all is good.

So seems no buggy code after all. So I think you can merge this code with just those two small remarks that are probably easier for you to get right as I have a feeling you have a specific solution in mind that is probably faster to just do yourself than communicate to me.

But if you think it is useful for me to do it do tell me with more explanation and detail and I will do my best to contribute it as well. All good for me.