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

fix(kuma-cni): support port exclusion for UIDs #8319

Merged
merged 6 commits into from
Nov 13, 2023

Conversation

lobkovilya
Copy link
Contributor

@lobkovilya lobkovilya commented Nov 10, 2023

Kuma Init container supports the following pod annotations:

  • traffic.kuma.io/exclude-outbound-tcp-ports-for-uids
  • traffic.kuma.io/exclude-outbound-udp-ports-for-uids

but Kuma CNI doesn't. Current PR adds support for these annotations for Kuma CNI.

Ideally, we'd like to converge Kuma CNI and Transparent Proxy code bases to share parsing and config structures.

Checklist prior to review

  • Link to relevant issue as well as docs and UI issues --
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
    • Don't forget ci/ labels to run additional/fewer tests
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label) --

…s-for-uids' and 'traffic.kuma.io/exclude-outbound-udp-ports-for-uids' annotations

Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
@lobkovilya lobkovilya marked this pull request as ready for review November 13, 2023 07:56
@lobkovilya lobkovilya requested a review from a team as a code owner November 13, 2023 07:56
@lobkovilya lobkovilya requested review from jakubdyszkiewicz and bartsmykla and removed request for a team November 13, 2023 07:56
Copy link
Contributor

@slonka slonka left a comment

Choose a reason for hiding this comment

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

looks good, two things to consider:

  1. have you checked if there are other annotations that are missing parsing and only works in init-containers?
  2. should we add a comment in init-container and cni that until Add a more systematic way to add transparent-proxy config #8324 is resolved new annotations need to be added in both places?

test/e2e/cni/exclude_outbound_port.go Show resolved Hide resolved
@lobkovilya lobkovilya added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Nov 13, 2023
@lobkovilya
Copy link
Contributor Author

  1. I looked through the list of annotations, kuma.io/sidecar-uid is missing from CNI. But I assume this is by design since we always set UID to default.
  2. Good idea, let me add a comment

@slonka slonka removed the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Nov 13, 2023
@slonka
Copy link
Contributor

slonka commented Nov 13, 2023

I looked through the list of annotations, kuma.io/sidecar-uid is missing from CNI. But I assume this is by design since we always set UID to default.

@bartsmykla this should be configurable as well, right?

edit: https://github.com/kumahq/kuma/pull/2221/files#r659633047

@bartsmykla
Copy link
Contributor

I think it should be configurable as current approach of always setting the default UID is incorrect when we would change via kuma.io/sidecar-uid annotation on pod.

@slonka slonka added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Nov 13, 2023
@slonka slonka enabled auto-merge (squash) November 13, 2023 09:51
@slonka
Copy link
Contributor

slonka commented Nov 13, 2023

Only

1 test failed out of 144
[It] Advanced LocalityAwareness with MeshLoadBalancingStrategy and Enabled Egress should route based on defined strategy with egress enabled

failed so I'm merging this since improvement for it is already in the branch

@slonka slonka disabled auto-merge November 13, 2023 10:14
@slonka slonka merged commit c06cc52 into kumahq:release-2.5 Nov 13, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants