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

test: Extend coverage for host policies enforcement #14822

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Feb 1, 2021

We currently have a couple of host firewall tests, but they don't cover all possible packet paths. #12621 added two tests for the path node <-> world (one with fromCIDR+toPorts and one combined with NodePort handling). Other firewall tests (cf. #14255) are only validating correct loading without enforcing policies.

This pull request fills this gap by adding VXLAN and direct routing tests for the host firewall, with L3+L4 policies enforced on the paths node <-> local pod, node <-> remote pod, and node <-> remote node.

The test design draws inspiration from early host firewall bugs and regressions:

  • Test ingress and egress at the same time with restrictions on allowed ports. This is meant to ensure we detect a regression where only one direction bypasses policy enforcement. If such a case arises, we will fail because the source port won't be allowed and the connection will be dropped.
  • Allow connections to/from world and pods not used in tests. This is meant to reduce the risk of bricking the nodes. Node to node
    communications are still strongly restricted, but the ports defined there have been stable for a while.
  • Test connections to local and remote pods separately. They follow very different paths through our datapath.

@pchaigno pchaigno added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. area/host-firewall Impacts the host firewall or the host endpoint. labels Feb 1, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 1, 2021
@pchaigno pchaigno added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Feb 1, 2021
@pchaigno pchaigno force-pushed the pr/pchaigno/hostfw-basic-tests branch 4 times, most recently from 0ce63d4 to 5de54ec Compare February 4, 2021 15:46
@pchaigno pchaigno marked this pull request as ready for review February 4, 2021 16:46
@pchaigno pchaigno requested a review from a team February 4, 2021 16:46
@pchaigno pchaigno requested a review from a team as a code owner February 4, 2021 16:46
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

LGTM, though I'm a complete newbie to Ginkgo, so I'll ask for a second review :)

test/k8sT/DatapathConfiguration.go Show resolved Hide resolved
@nbusseneau nbusseneau requested a review from a team February 5, 2021 13:14
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Couple of minor concerns below. On the defer side, apparently we're using defer quite a bit in CI but I don't know if we've fully considered that. Either way it seems like we should double-check that defer is OK and there's a bunch of other code to clean up unrelated to this PR, so I don't really feel it is so fair to push back on this particular PR for the same thing that other tests already do.

test/k8sT/DatapathConfiguration.go Outdated Show resolved Hide resolved
test/k8sT/DatapathConfiguration.go Outdated Show resolved Hide resolved
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

LGTM (except the autoDirectNodeRoutes related comment posted by Joe)

@pchaigno pchaigno force-pushed the pr/pchaigno/hostfw-basic-tests branch from 5de54ec to 059d4dd Compare February 8, 2021 21:02
We currently have a couple of host firewall tests, but they don't cover
all possible packet paths. [1] added two tests for the path node <-> world
(one with fromCIDR+toPorts and one combined with NodePort handling).
Other firewall tests [2] are only validating correct loading without
enforcing policies.

This commit fills this gap by adding VXLAN and direct routing tests for
the host firewall, with L3+L4 policies enforced on the paths node <->
local pod, node <-> remote pod, and node <-> remote node.

The test design draws inspiration from early host firewall bugs and
regressions:
- Test ingress and egress at the same time with restrictions on allowed
  ports. This is meant to ensure we detect a regression where only one
  direction bypasses policy enforcement. If such a case arises, we will
  fail because the source port won't be allowed and the connection
  will be dropped.
- Allow connections to/from world and pods not used in tests. This is
  meant to reduce the risk of bricking the nodes. Node to node
  communications are still strongly restricted, but the ports defined
  there have been stable for a while.
- Test connections to local and remote pods separately. They follow very
  different paths through our datapath.

1 - #12621
2 - #14255
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/hostfw-basic-tests branch from 059d4dd to 6d557b9 Compare February 8, 2021 21:18
@pchaigno
Copy link
Member Author

pchaigno commented Feb 8, 2021

test-me-please

@borkmann borkmann merged commit 6f59f4f into master Feb 9, 2021
@borkmann borkmann deleted the pr/pchaigno/hostfw-basic-tests branch February 9, 2021 13:39
@vadorovsky
Copy link
Member

Needs to be backported to 1.9 to solve conflicts with #16064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow area/host-firewall Impacts the host firewall or the host endpoint. release-note/ci This PR makes changes to the CI. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants