-
Notifications
You must be signed in to change notification settings - Fork 249
feat: add azure ip masq merger #3737
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
Conversation
merges ip masq agent configs into one configmap and outputs to a directory adds the non masquerade ips list together and ORs the other flags together if no configmap that starts with ip-masq exists, no merged configmap is output and any previously created merged configmap in the merged-config directory is deleted this binary is separate from the rest of the repo similar to azure-ipam
ip masq merger is only meant to work on linux since ip masq agent only runs on linux
rename ip masq merger to be consistent with other files add golangci yml for linting fix shadowing issues with err and use newer method to check file not found error fix linter issues use require for tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new binary called "azure ip masq merger" to merge multiple IP masquerade configurations for Azure Container Networking. Key changes include:
- New implementation of the merger logic in ip_masq_merger.go.
- Comprehensive unit tests in ip_masq_merger_test.go.
- Updates to Dockerfile, Makefile, and go.mod to support building and packaging the new binary.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
azure-ip-masq-merger/ip_masq_merger.go | New merger implementation and underlying config validation. |
azure-ip-masq-merger/ip_masq_merger_test.go | Added unit tests for merging and CIDR validation. |
azure-ip-masq-merger/go.mod | Module and dependency management updates. |
azure-ip-masq-merger/Dockerfile | Dockerfile updates to build multi-platform images. |
azure-ip-masq-merger/.golangci.yml | Linters configuration for code quality. |
Makefile | Build targets and packaging instructions for the new binary. |
Comments suppressed due to low confidence (1)
azure-ip-masq-merger/ip_masq_merger.go:250
- Consider sorting 'cidrsList' after merging to ensure deterministic ordering of CIDRs, which could aid in debugging and configuration comparisons.
for cidr := range cidrsSet { cidrsList = append(cidrsList, cidr) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few minor comments. Also, are you using ip-masq-agent-v2 basically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
Summarizing our discussion offline for context: ip masq merger will merge configs when ebpf host routing and bpf ip masq agent is enabled. It will be deployed to clusters either with sidecare or initcontainer in the cilium daemonset. Dockerfile images will be updated in later stages along with pipeline image builds.
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
Reason for Change:
This PR creates a new binary ip masq merger binary which merges two ip masq configs together and places them in an output directory for consumption. If no configmaps are found, it will also automatically delete the previously merged configmap. Adds makefile targets for building the binary and image with the appropriate dockerfile. Does not make pipeline changes. Tested on a cluster as a sidecar.
Issue Fixed:
Requirements:
Notes: