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

adding a new workflow to validate the FlexibleIPAM feature #6879

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KMAnju-2021
Copy link
Contributor

@KMAnju-2021 KMAnju-2021 commented Dec 19, 2024

@KMAnju-2021 KMAnju-2021 marked this pull request as draft December 19, 2024 06:33
@rajnkamr rajnkamr added this to the Antrea v2.3 release milestone Dec 26, 2024
@KMAnju-2021 KMAnju-2021 force-pushed the add-cncf-runners branch 22 times, most recently from 33a2b50 to 9486da3 Compare January 2, 2025 04:30
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

The title of the PR is misleading: we are adding a new workflow to validate the FlexibleIPAM feature. The fact that we use a non-default runner is an "implementation detail".

.github/workflows/kind.yml Outdated Show resolved Hide resolved
ci/kind/install-ipset.sh Outdated Show resolved Hide resolved
ci/kind/kind-setup.sh Outdated Show resolved Hide resolved
ci/kind/kind-setup.sh Outdated Show resolved Hide resolved
@KMAnju-2021 KMAnju-2021 changed the title [WIP] adding self-hosted runners [WIP] adding a new workflow to validate the FlexibleIPAM feature Jan 3, 2025
@KMAnju-2021 KMAnju-2021 force-pushed the add-cncf-runners branch 7 times, most recently from f9401e7 to d1ba24e Compare January 24, 2025 13:03
mkdir log
mkdir test-ipam-e2e-coverage
ANTREA_LOG_DIR=$PWD/log ANTREA_COV_DIR=$PWD/test-ipam-e2e-coverage ./ci/kind/test-e2e-kind.sh \
--encap-mode noEncap \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid adding this, as this will anyway be the case when providing --flexible-ipam

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we will not provide --encap-mode noEncap then it will take default mode encap and in generate-manifest.sh script it will override the value after flexible-ipam check.

.github/workflows/kind.yml Show resolved Hide resolved
ci/kind/kind-setup.sh Outdated Show resolved Hide resolved
@@ -300,6 +300,18 @@ function configure_vlan_subnets {
docker_run_with_host_net iptables -t filter -A FORWARD -i ${vlan_interfaces[j]} -o ${vlan_interfaces[i]} -j ACCEPT
done
done

# Adding ipset rules to prevent SNAT based on flexibleIPAM e2e tests
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment and add a more verbose and accurate one for each of the 2 iptables rules below.
In particular, this comment is not accurate for the second iptables rules.

docker_run_with_host_net ipset list excluded_subnets

docker_run_with_host_net iptables -t nat -I POSTROUTING 1 ! -o $bridge_interface -s 192.168.240.0/24 -m set --match-set excluded_subnets dst -j RETURN
docker_run_with_host_net iptables -t nat -A POSTROUTING ! -o $bridge_interface -s 10.244.0.0/16 -m set ! --match-set excluded_subnets dst -j MASQUERADE
Copy link
Contributor

Choose a reason for hiding this comment

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

when using FlexibleIPAM, noSNAT is set to true and Antrea will not perform SNAT for Pod traffic
this rule seems to counteract that behavior, by reestablishing SNAT for regular Pods (which use the default Pod CIDR). Can you explain why it is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what I observe that when we create kind network then docker will add corresponding default NAT rule in the iptables:
`Chain POSTROUTING (policy ACCEPT 887 packets, 63828 bytes)

pkts bytes target prot opt in out source destination

0     0 MASQUERADE  0    --  *      !br-053bd7f613f8  192.168.240.0/24     0.0.0.0/0           

0     0 MASQUERADE  0    --  *      !docker0  172.17.0.0/16        0.0.0.0/0           

0     0 MASQUERADE  6    --  *      *       192.168.240.2        192.168.240.2        tcp dpt:6443`

and because of that it's performing SNAT for the case:#6237, and failing tests, to prevent that i added two NAT rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's answering my question.
The purpose of the first rule is to prevent SNAT between VLANs and I understand that. The second rule on the other hand is forcing SNAT for "regular" (NodeIPAM) Pods, so I am asking you to clarify why it's needed with a comment. I'm assuming something like this is correct:

With FlexibleIPAM, Antrea SNAT is disabled (noSNAT: true) so Pods don't have access to the external network by default (including regular / NodeIPAM Pods). With our configuration, Docker will already perform SNAT for FlexibleIPAM Pods. However, we need a custom SNAT rule for regular Pods.

I am not sure that the Pods really need access to the external network, but maybe some tests break without this?

ci/kind/kind-setup.sh Outdated Show resolved Hide resolved
ci/kind/kind-setup.sh Outdated Show resolved Hide resolved
ci/kind/kind-setup.sh Outdated Show resolved Hide resolved
ci/kind/test-e2e-kind.sh Outdated Show resolved Hide resolved
ci/kind/test-e2e-kind.sh Outdated Show resolved Hide resolved
@KMAnju-2021 KMAnju-2021 force-pushed the add-cncf-runners branch 19 times, most recently from 934cf91 to 66bcec4 Compare January 29, 2025 10:36
Signed-off-by: KMAnju-2021 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants