-
Notifications
You must be signed in to change notification settings - Fork 263
feat: pipeline tests for Cilium eBPF host routing + dualstack #4154
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
base: master
Are you sure you want to change the base?
Conversation
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 adds comprehensive end-to-end pipeline tests for Cilium eBPF with host routing enabled on dualstack AKS clusters. The implementation mirrors the existing Cilium dualstack overlay tests while introducing eBPF-specific configurations and deployment workflows.
Key Changes:
- New Cilium eBPF dualstack configuration with native routing mode and proper CIDR settings for both IPv4 and IPv6
- Pipeline integration with new cluster creation, deployment, and E2E test stages for the
cilium_ebpf_ds_e2etest suite - Container specification fixes to explicitly target the
cilium-agentcontainer in multi-container Cilium pods
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/manifests/cilium/v1.17/ebpf/dualstack/static/cilium-config.yaml | New Cilium ConfigMap for eBPF dualstack with native routing mode and routing CIDRs |
| test/integration/manifests/cilium/v1.17/ebpf/dualstack/static/azure-ip-masq-agent-config-reconciled.yaml | IP masquerade agent configuration for non-masquerade CIDRs |
| test/integration/manifests/cilium/v1.17/ebpf/dualstack/cilium.yaml | Complete Cilium DaemonSet definition with eBPF configurations, init containers, and volume mounts |
| hack/aks/deploy.mk | Added deploy-ebpf-dualstack-cilium target with required variable exports and envsubst template processing |
| hack/aks/Makefile | Made POD_CIDR overridable and added POD_CIDRS support for dualstack cluster creation |
| .pipelines/pipeline.yaml | Integrated new cilium_ebpf_ds_e2e test suite into the pipeline with proper dependencies and cleanup |
| .pipelines/singletenancy/cilium-dualstack-ebpf/cilium-dualstack-e2e-job-template.yaml | Job template defining cluster creation and E2E test stages with required environment variables |
| .pipelines/singletenancy/cilium-dualstack-ebpf/cilium-dualstack-e2e-step-template.yaml | Step template with complete test workflow including Cilium installation, connectivity tests, and validation |
| .pipelines/templates/create-cluster.yaml | Added POD_CIDRS export for dualstack cluster configuration |
| .pipelines/templates/log.steps.yaml | Added container name specification for Cilium kubectl exec commands |
| .pipelines/templates/log-template.yaml | Added container name specification for Cilium kubectl exec commands |
| test/validate/linux_validate.go | Added containerName field to cilium state file validation check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...tion/manifests/cilium/v1.17/ebpf/dualstack/static/azure-ip-masq-agent-config-reconciled.yaml
Show resolved
Hide resolved
test/integration/manifests/cilium/v1.17/ebpf/dualstack/static/cilium-config.yaml
Outdated
Show resolved
Hide resolved
| podLabelSelector: ciliumLabelSelector, | ||
| podNamespace: privilegedNamespace, | ||
| cmd: ciliumStateFileCmd, | ||
| containerName: "cilium-agent", |
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.
was it selecting the sidecar?
QxBytes
left a comment
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.
are we planning on running with 1.18 as well? In the future the default version will likely be bumped up and there won't be files for 1.18
| name: cilium-netns | ||
| - configMap: | ||
| defaultMode: 420 | ||
| name: allowed-iptables-patterns |
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.
planning on adding the allowed-ip6tables-patterns or enabling the ipv6 flag on the azure iptables monitor binary?
| AZURE_IP_MASQ_MERGER_TAG ?= v0.0.1-0 | ||
| # so we can use in envsubst | ||
| export IPV6_HP_BPF_VERSION | ||
| export IPV6_IMAGE_REGISTRY |
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.
do we want to add this to the defaults (ex: after line 5) as well? My initial intention for the exports was so that we would export the makefile variables if they weren't set as environment variables prior
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.
would we add enable-remote-node-masquerade here? I guess it doesn't matter too much since we aren't examining the src ips in the tests I believe
| ipam-cilium-node-update-rate: 15s | ||
| ipam: delegated-plugin | ||
| ipv4-native-routing-cidr: 10.244.0.0/16 | ||
| ipv6-native-routing-cidr: fdd5:a27a:b4bc:99d6::/64 |
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.
maybe comment either in pr desc or here that it needs to match the vnet
| echo "Install az cli extension preview" | ||
| az extension add --name aks-preview | ||
| az extension update --name aks-preview | ||
| export POD_CIDRS="10.244.0.0/16,fdd5:a27a:b4bc:99d6::/64" |
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.
can this be passed in as a parameter?
| name: "CiliumStatus" | ||
| displayName: "Cilium Status" | ||
| - task: AzureCLI@2 |
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.
is it possible to push this into a template for dualstack in general, or at least cilium dualstack?
Reason for Change:
This PR adds tests for dualstack clusters with eBPF host routing enabled. Key changes:
Issue Fixed:
Requirements:
Notes: