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

Normal policy support for host-* endpoints #2228

Merged
merged 46 commits into from
Apr 7, 2020

Conversation

lmm
Copy link
Contributor

@lmm lmm commented Mar 5, 2020

Description

* hostendpoints currently only support pre-DNAT global network policy. This PR will (eventually) add normal policy support for * hostendpoints.

Some notes on semantics:

  • If a * hostendpoint and a "named" (e.g. eth0) hostendpoint exist, the named hostendpoint will be used. (This is existing behaviour and isn't changing here.)
  • Host-> pod traffic is always allowed. With this change, we would continue to have default-allow semantics. (This desired behaviour to be confirmed)

Todos

  • Unit tests (full coverage)
  • Integration tests (delete as appropriate) In plan/Not needed/Done
  • Documentation
  • Backport
  • Release note

Release Note

all-interfaces host endpoints now supports normal network policy in addition to pre-dnat policy

rules/dispatch.go Outdated Show resolved Hide resolved
Copy link
Member

@nelljerram nelljerram left a comment

Choose a reason for hiding this comment

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

@lmm I think the shape of this is looking pretty good. I've made one specific comment. Apart from that, my concern is just to be sure about what we decided in the spec about:

  • defaults
  • ordering (applying workload policy before WHEP policy, for traffic from a workload to its own host, and meshing with the existing hardcoded rules)
  • optionally deduplication (in the case where the same policy applies to the workload and the WHEP)

and then obviously making sure that the code reflects that, ideally with copious tests.

rules/dispatch.go Outdated Show resolved Hide resolved
@lmm lmm changed the title wip: normal policy support for host-* endpoints Normal policy support for host-* endpoints Mar 19, 2020
@lmm lmm marked this pull request as ready for review March 19, 2020 15:44
@lmm lmm requested a review from a team as a code owner March 19, 2020 15:44
Copy link
Member

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

Few questions from me. Also, this will need thorough tests in the FV framework

rules/dispatch_test.go Show resolved Hide resolved
rules/endpoints.go Outdated Show resolved Hide resolved
rules/endpoints.go Outdated Show resolved Hide resolved
fv/vxlan_test.go Show resolved Hide resolved
rules/dispatch.go Outdated Show resolved Hide resolved
lmm added 3 commits March 24, 2020 12:27
- toEndForwardRules was updated with wrong rules
- in the `fromOnly` case (for pre-DNAT) we use HostFromEndpointPfx not HostFromEndpointForwardPfx,
  so the end rules passed to the dispatch chain builder were incorrect.
@lmm lmm force-pushed the lmm-all-hep-policy branch 4 times, most recently from 1c342e7 to 068aad0 Compare April 6, 2020 06:17
Copy link
Member

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

We discussed the AoF case on slack so I think we got to the bottom of that (test is failing due to AoF policy applying at workload egress for WHEPs, but not HEPs).

Good emphasis on testing in this PR; with lots of test being upgraded to cover the new case. Well worth that effort, I think!

Found a few nits in the tests.

fv/apply_on_forward_test.go Outdated Show resolved Hide resolved
fv/host_port_test.go Show resolved Hide resolved
fv/host_port_test.go Outdated Show resolved Hide resolved
fv/host_port_test.go Outdated Show resolved Hide resolved
fv/host_port_test.go Outdated Show resolved Hide resolved
fv/hostendpoints_test.go Outdated Show resolved Hide resolved
fv/hostendpoints_test.go Outdated Show resolved Hide resolved
fv/hostendpoints_test.go Outdated Show resolved Hide resolved
fv/vxlan_test.go Outdated Show resolved Hide resolved
rules/endpoints.go Outdated Show resolved Hide resolved
Copy link
Member

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

LGTM

fv/ipip_test.go Outdated Show resolved Hide resolved
@nelljerram nelljerram dismissed their stale review April 7, 2020 14:22

Concerns have now been addressed.

@lmm lmm merged commit 79efdd9 into projectcalico:master Apr 7, 2020
@lmm lmm deleted the lmm-all-hep-policy branch April 7, 2020 15:58
@caseydavenport caseydavenport added this to the Calico v3.14.0 milestone Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants