Implement FromCIDR ingress tests #1494

christarazi · 2023-04-08T01:00:44Z

Requires #1498 and I pulled out #1501 to a separate PR because it broke CI on cilium/cilium.

Review commit by commit.

connectivity/check: Deploy host-netns daemonset by default
internal/cli/cmd: Clarify --nodes-without-cilium
.github/workflows: Label nodes for --nodes-without-cilium
.github/workflows: Install static routes for nodes without Cilium
connectivity/check: Validate --external-from-cidrs
Rename current CIDR tests to to-cidr.go
connectivity: Refactor scenarios passed to tests
connectivity: Implement FromCIDR tests
.github: Prepare workflows for tests with a 3rd node

Providing commit msg of the most important commit for convenience:

connectivity: Implement FromCIDR tests

These should cover the tests from the Cilium (github.com/cilium/cilium)
Ginkgo suite for FromCIDRs, see 1.

michi-covalent

lgtm on 23a8dc8. extra credit if you add a unit test.

.github/workflows/kind.yaml

christarazi · 2023-04-14T00:14:44Z

internal/cli/cmd/install.go

@@ -236,7 +236,7 @@ func addCommonInstallFlags(cmd *cobra.Command, params *install.Parameters) {
 	cmd.Flags().BoolVar(&params.ListVersions, "list-versions", false, "List all the available versions without actually installing")
 	cmd.Flags().BoolVar(&params.Wait, "wait", false, "Wait for helm install to finish")
 	cmd.Flags().DurationVar(&params.WaitDuration, "wait-duration", defaults.StatusWaitDuration, "Maximum time to wait for status")
-	cmd.Flags().StringSliceVar(&params.NodesWithoutCilium, "nodes-without-cilium", []string{}, "List of node names on which Cilium will not be installed. In Helm installation mode, it's assumed that the no-schedule node labels are present.")
+	cmd.Flags().StringSliceVar(&params.NodesWithoutCilium, "nodes-without-cilium", []string{}, "List of node names on which Cilium will not be installed. In Helm installation mode, it's assumed that the no-schedule node labels are present and that the infastructure has set up routing on these nodes to provide connectivity within the Cilium cluster.")


Calling this out as an FYI.

appreciating the call-out almost one year later 🙏

.github/workflows/kind.yaml

asauber

LGTM, commits made the review easy

brlbil

I am fond of this much bash magic in the workflow. It makes it hard to follow along with what is going on.
But I do not know a better way to do it myself 😄 so for workflow parts 👍

The daemonset will now be deployed when we detect that the cluster contains a node that is running without Cilium. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Signed-off-by: Chris Tarazi <chris@isovalent.com>

In Helm mode, we want to avoid changing any resource that's not part of the chart itself. This includes adding labels to the Node resource. Therefore, we add steps in the workflow to do this instead. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This is crucial for FromCIDR policy tests where a node that is outside of the cluster (running without Cilium) initiates traffic towards pods within the cluster. In order to allow this, we need to install routes on these nodes which point to the pod CIDR of each node in the cluster, via the node IP. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This commit ensures that --external-from-cidrs is not empty when --nodes-without-cilium was passed when installing Cilium. Otherwise, FromCIDR policy connectivity tests won't work properly. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Preparation for from-cidr.go to be added in subsequent commits. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Refactoring to pass the scenarios as a slice allows us to conditionally add to the slice in the future. Subsequent commits will add scenarios for fromCIDR tests, which will leverage this refactor. Signed-off-by: Chris Tarazi <chris@isovalent.com>

These should cover the tests from the Cilium (github.com/cilium/cilium) Ginkgo suite for FromCIDRs, see [1]. [1]: https://github.com/cilium/cilium/blob/8f9daa8c4e9ccf17c4badaa28aa6c1b0b51924a8/test/k8s/net_policies.go#L488-L557. Signed-off-by: Chris Tarazi <chris@isovalent.com>

The 3rd node does not run Cilium and will be used in the connectivity tests with `--nodes-without-cilium=kind-worker2`. This activates the new tests added in the previous commit. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2023-04-14T17:24:09Z

Pushed to fix up an error msg and code comments, so the push had no functional impact. The CI was already green given MLH marked it as ready to merge.

joestringer

I think that architecturally the environment configuration stuff belongs in the setup, though I do agree with Birol that embedding bash like this makes the workflows more complicated and difficult to grok. Maybe at some point we'll just reinvent the workflows as a github action with the relevant logic in it (better code reuse for cilium/cilium that way too). Anyway, now that we're not expanding the security footprint of the deployments this LGTM. I stared at the bash for a while and nothing jumped out.

joestringer · 2023-04-14T17:29:45Z

.github/workflows/kind.yaml

+          EXTERNAL_NODE_IPS=() # Nodes IPs are collected to be passed to the Cilium CLI later on.
+
+          # Loop over each pod CIDR from all nodes.
+          for i in "${!EXTERNAL_FROM_CIDRS[@]}"; do


I didn't know about getting keys with the ! syntax, TIL.

joestringer · 2023-04-14T17:30:26Z

.github/workflows/kind.yaml

+            if [[ $EXTERNAL_FROM_CIDR =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+$ ]]; then
+              IP_FAMILY="v4"
+            elif [[ $EXTERNAL_FROM_CIDR =~ ^[0-9a-fA-F:]+/[0-9]+$ ]]; then


Not perfect checks for IP address formats but good enough especially in a workflow where I presume we're controlling the inputs.

Yeah, I'm aware they aren't perfect and went for just good enough to be usable. I wanted to avoid complicating the bash code further with super long regexes, but definitely an item for followup if we can also find a way to make it more readable, i.e. using a variable instead and hide the definition of it.

The 3rd node does not run Cilium and will be used in the connectivity tests with `--nodes-without-cilium=kind-worker2`. The same was done in the cilium/cilium-cli repo, see cilium/cilium-cli#1494. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi temporarily deployed to ci April 8, 2023 01:00 — with GitHub Actions Inactive

christarazi changed the title ~~pr/christarazi/from cidr~~ Implement FromCIDR ingress tests Apr 8, 2023

christarazi force-pushed the pr/christarazi/from-cidr branch from 84626b9 to 23e52c9 Compare April 8, 2023 01:05

christarazi temporarily deployed to ci April 8, 2023 01:05 — with GitHub Actions Inactive

christarazi temporarily deployed to ci April 8, 2023 01:12 — with GitHub Actions Inactive

christarazi mentioned this pull request Apr 10, 2023

CiliumCIDRGroup followups cilium/cilium#24801

Open

christarazi force-pushed the pr/christarazi/from-cidr branch from 328ba35 to 5525ef7 Compare April 10, 2023 18:32

christarazi temporarily deployed to ci April 10, 2023 18:32 — with GitHub Actions Inactive

christarazi had a problem deploying to ci April 10, 2023 18:51 — with GitHub Actions Failure

christarazi force-pushed the pr/christarazi/from-cidr branch from 9cff12f to f39c180 Compare April 10, 2023 18:56

christarazi temporarily deployed to ci April 10, 2023 18:56 — with GitHub Actions Inactive

christarazi temporarily deployed to ci April 10, 2023 19:17 — with GitHub Actions Inactive

christarazi force-pushed the pr/christarazi/from-cidr branch from ba555c0 to e6f69cd Compare April 10, 2023 19:42

christarazi temporarily deployed to ci April 10, 2023 19:42 — with GitHub Actions Inactive

christarazi force-pushed the pr/christarazi/from-cidr branch from e6f69cd to 4a0f01d Compare April 10, 2023 22:26

christarazi temporarily deployed to ci April 10, 2023 22:26 — with GitHub Actions Inactive

christarazi force-pushed the pr/christarazi/from-cidr branch from 4a0f01d to bc65b06 Compare April 10, 2023 23:35

christarazi temporarily deployed to ci April 10, 2023 23:35 — with GitHub Actions Inactive

christarazi force-pushed the pr/christarazi/from-cidr branch from bc65b06 to 09cfe15 Compare April 11, 2023 06:18

christarazi temporarily deployed to ci April 11, 2023 06:18 — with GitHub Actions Inactive

christarazi force-pushed the pr/christarazi/from-cidr branch from 09cfe15 to ccdeaf0 Compare April 11, 2023 07:11

christarazi temporarily deployed to ci April 11, 2023 07:11 — with GitHub Actions Inactive

christarazi marked this pull request as ready for review April 11, 2023 07:20

christarazi requested review from a team as code owners April 11, 2023 07:20

christarazi requested review from asauber and brlbil April 11, 2023 07:20

michi-covalent approved these changes Apr 11, 2023

View reviewed changes

.github/workflows/kind.yaml Outdated Show resolved Hide resolved

christarazi force-pushed the pr/christarazi/from-cidr branch from ccdeaf0 to c8ea70b Compare April 11, 2023 16:58

christarazi temporarily deployed to ci April 14, 2023 00:10 — with GitHub Actions Inactive

asauber reviewed Apr 14, 2023

View reviewed changes

.github/workflows/kind.yaml Show resolved Hide resolved

christarazi commented Apr 14, 2023

View reviewed changes

asauber approved these changes Apr 14, 2023

View reviewed changes

christarazi marked this pull request as ready for review April 14, 2023 03:12

brlbil approved these changes Apr 14, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 14, 2023

christarazi added 9 commits April 14, 2023 10:22

connectivity/check: Deploy host-netns daemonset by default

8b59a58

The daemonset will now be deployed when we detect that the cluster contains a node that is running without Cilium. Signed-off-by: Chris Tarazi <chris@isovalent.com>

internal/cli/cmd: Clarify --nodes-without-cilium

54a7dd4

Signed-off-by: Chris Tarazi <chris@isovalent.com>

Rename current CIDR tests to to-cidr.go

36747fb

Preparation for from-cidr.go to be added in subsequent commits. Signed-off-by: Chris Tarazi <chris@isovalent.com>

.github: Prepare workflows for tests with a 3rd node

e0d6f35

The 3rd node does not run Cilium and will be used in the connectivity tests with `--nodes-without-cilium=kind-worker2`. This activates the new tests added in the previous commit. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/from-cidr branch from afddc3d to e0d6f35 Compare April 14, 2023 17:22

christarazi temporarily deployed to ci April 14, 2023 17:22 — with GitHub Actions Inactive

christarazi requested review from joestringer and aanm April 14, 2023 17:24

joestringer approved these changes Apr 14, 2023

View reviewed changes

maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 14, 2023

aanm approved these changes Apr 17, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 17, 2023

christarazi merged commit 65f85a7 into master Apr 17, 2023
12 checks passed

christarazi deleted the pr/christarazi/from-cidr branch April 17, 2023 20:01

ZhongRuoyu mentioned this pull request Apr 24, 2023

cilium-cli 0.14.0 Homebrew/homebrew-core#129223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FromCIDR ingress tests #1494

Implement FromCIDR ingress tests #1494

christarazi commented Apr 8, 2023 •

edited

michi-covalent left a comment

christarazi Apr 14, 2023

michi-covalent Mar 6, 2024

asauber left a comment

brlbil left a comment

christarazi commented Apr 14, 2023

joestringer left a comment

joestringer Apr 14, 2023

joestringer Apr 14, 2023

christarazi Apr 14, 2023 •

edited

Implement FromCIDR ingress tests #1494

Implement FromCIDR ingress tests #1494

Conversation

christarazi commented Apr 8, 2023 • edited

michi-covalent left a comment

Choose a reason for hiding this comment

christarazi Apr 14, 2023

Choose a reason for hiding this comment

michi-covalent Mar 6, 2024

Choose a reason for hiding this comment

asauber left a comment

Choose a reason for hiding this comment

brlbil left a comment

Choose a reason for hiding this comment

christarazi commented Apr 14, 2023

joestringer left a comment

Choose a reason for hiding this comment

joestringer Apr 14, 2023

Choose a reason for hiding this comment

joestringer Apr 14, 2023

Choose a reason for hiding this comment

christarazi Apr 14, 2023 • edited

Choose a reason for hiding this comment

christarazi commented Apr 8, 2023 •

edited

christarazi Apr 14, 2023 •

edited