Add AdminNetworkPolicy #9206

mazdakn · 2024-09-03T16:44:29Z

Description

Add support for the new k8s AdminNetworkPolicy API. This PR only adds the core functionalities.

Related issues/PRs

Tier support: #9085
Default action in tiers: #9232
Operator: tigera/operator#3501
AdminNetworkPolicy egress networks: #9276
AdminNetworkPolicy named port: #9254

Todos

Tests
Documentation
Release note

Release Note

Add support for the core functionalities of the new k8s AdminNetworkPolicy API.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

libcalico-go/lib/backend/k8s/conversion/constants.go

fasaxc

Took a first pass. Mostly looks good but I think there are a few details of the spec for ANP that differ from the other policy that we support, mainly to allow for future enhancements to ANP. We should try to follow those behaviours, even if they're no-ops right now.

I think we should include support for CIDR matches and named ports in the first release. They should both be trivial to do and, to be correct according to the spec, I think if you don't support them, the behaviour needs to change (you should drop allow rules/convert deny rules to deny all).

api/pkg/apis/projectcalico/v3/policy_common.go

libcalico-go/Makefile

fasaxc · 2024-09-12T09:56:51Z

libcalico-go/lib/backend/k8s/conversion/conversion.go

+	for _, r := range rule.To {
+		// We need to add a copy of the peer so all the rules don't
+		// point to the same location.
+		peer := &adminpolicy.AdminNetworkPolicyEgressPeer{Namespaces: r.Namespaces}


Egress peers also have Networks (list of CIDRs).

Do we need the copy step in recent golang? I think loop variables are now unique per loop so you may not need to copy anything.

There's also "Nodes". I suppose we could translate that to something that would match the right auto-HEPs (but if we're not going to support it, should we validate against it?)

Note https://github.com/kubernetes-sigs/network-policy-api/blob/v0.1.5/apis/v1alpha1/shared_types.go#L148C1-L150C46 for forwards compatibility, we should check that something is set in the rule and fail closed if we see nothing.

// Exactly one of the selector pointers must be set for a given peer. If a
// consumer observes none of its fields are set, they must assume an unknown
// option has been specified and fail closed.

fasaxc · 2024-09-12T10:09:08Z

libcalico-go/lib/validator/v3/validator.go

@@ -1567,6 +1567,18 @@ func validateTier(structLevel validator.StructLevel) {
 		}
 	}

+	if tier.Name == names.AdminNetworkPolicyTierName {


Do we want to validate this on read? If Felix reads a bad value it'll drop the resource and that'll break policy. I feel like we only want to check this on write so that, if bad data gets into the datastore, we still do something sensible.

We don't validate this on read. In the Tier implementation of Clientv3, we only call validator on create and update.

calico/libcalico-go/lib/clientv3/tier.go

Line 146 in 1e41064

func (r tiers) Get(ctx context.Context, name string, opts options.GetOptions) (*apiv3.Tier, error) {

Felix/Typha both validate in their calc graphs, I think.

Right. It seems in Typha we also validate in reads:

calico/typha/pkg/calc/validation_filter.go

Line 64 in 4ad72b7

if err := validatorFunc(elem.Interface()); err != nil {

Also in Felix:

calico/felix/calc/validation_filter.go

Line 67 in 4ad72b7

if err := validatorFunc(elem.Interface()); err != nil {

Originally the check was like this: https://github.com/tigera/calico-private/blame/d7f58624426ef41c64b6981b6017cf2956b50a25/libcalico-go/lib/clientv3/tier.go#L51

However, we changed it to v3 validator since the return error code, Operation not supported, is not technically correct, as the create/update operations are supported. It just feels the right place for these type of checks is in validator.

Will this be an issue if create and update does not allow the incorrect value for AdminNeworkPolicy tier in first place?

We can also use DefaultTierFields here: https://github.com/projectcalico/calico/blob/master/libcalico-go/lib/resources/tier.go#L21
to make sure values are correct. This function is called by k8s client: https://github.com/projectcalico/calico/blob/master/libcalico-go/lib/backend/k8s/resources/tiers.go

btw, we create adminneworkpolicy tier ourselves at the beginning of the day here:

calico/libcalico-go/lib/clientv3/client.go

Line 421 in 018190a

func (c client) ensureAdminNetworkPolicyTierExists(ctx context.Context) error {

fasaxc · 2024-09-12T10:13:36Z

libcalico-go/lib/backend/k8s/conversion/conversion.go

+	for _, r := range anp.Spec.Ingress {
+		rules, err := c.k8sANPIngressRuleToCalico(r)
+		if err != nil {
+			log.WithError(err).Warn("dropping k8s rule that couldn't be converted.")


I think the intent in the NPEP is to "fail closed" when we don't understand a rule in order to deal with forwards compatibility more cleanly. I take that to mean:

If we can't understand an Allow rule, we should log a warning and drop it.

If we can't understand a Deny rule, we should log a warning and convert that to a "Deny all" rule.

WDYT?

Make sense for a fail closed system. For allow & pass action it's clear that we should drop the rule.
For deny action, I think we should only add the Deny all for the direction of failed rule, right?

only add the Deny all for the direction of failed rule, right?

Yes, exactly, just that one rule gets converted to deny all (or at least, that's my interpretation).

libcalico-go/lib/names/policy.go

fasaxc · 2024-09-12T12:29:41Z

libcalico-go/lib/backend/k8s/conversion/conversion.go

+		protocol = c.k8sProtocolToCalico(&port.PortRange.Protocol)
+		return
+	}
+	return


NamedPort is "Extended" but we support named ports in Felix so I think that's best to do now.

libcalico-go/lib/backend/k8s/conversion/conversion.go

fasaxc · 2024-09-12T13:15:12Z

libcalico-go/lib/backend/k8s/conversion/conversion.go

+		}
+	}
+
+	// If there no peers, or no ports, represent that as nil.


I think the spec requires there to be at least one peer (although that means it's hard to do basic things like allow all/deny all!). I filed an issue upstream for that: kubernetes-sigs/network-policy-api#248

felix/rules/endpoints.go

mazdakn · 2024-09-13T15:39:43Z

e2e/cmd/adminpolicy/e2e_test.go

@@ -0,0 +1,72 @@
+// Copyright (c) 2024 Tigera, Inc. All rights reserved.


I copied this file from here: https://github.com/kubernetes-sigs/network-policy-api/blob/main/conformance/conformance_test.go
I believe I should use their copy right clause, right?

Yes, always keep external copyright notices intact. We can also add ours as and when we make changes (but best to make changes in a second file where possible to make it easier to unpick what's ours and what's theirs).

felix/dataplane/linux/bpf_ep_mgr.go

libcalico-go/lib/backend/k8s/conversion/conversion.go

libcalico-go/lib/backend/k8s/conversion/constants.go

libcalico-go/lib/backend/k8s/conversion/conversion.go

libcalico-go/lib/names/policy.go

fasaxc

Approved as an increment but, as discussed, you're planning to:

Solve the end of tier ACTION PROPERLY.
Add tiers to Dikastes.

mazdakn requested a review from a team as a code owner September 3, 2024 16:44

marvin-tigera added this to the Calico v3.29.0 milestone Sep 3, 2024

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Sep 3, 2024

mazdakn commented Sep 6, 2024

View reviewed changes

libcalico-go/lib/backend/k8s/conversion/constants.go Outdated Show resolved Hide resolved

Add support for AdminNeworkPolicy

a9626f1

mazdakn force-pushed the adminnetworkpolicy branch from ab29eb0 to a9626f1 Compare September 10, 2024 03:59

mazdakn added 9 commits September 10, 2024 13:41

Update RBAC

f604895

Merge remote-tracking branch 'os/master' into adminnetworkpolicy

eeed505

update test

041b35a

fix test

fd8de8b

fix felix fvs

055b826

Fix tests

3d893cc

add conformance test

dc0a3ba

fix

2c5bffd

update

fa6c6d4

fasaxc requested changes Sep 12, 2024

View reviewed changes

mazdakn added 5 commits September 12, 2024 11:56

fix conformance

05bf7b3

fix

b2f08d5

more

395e878

update crds

2f07624

Merge remote-tracking branch 'os/master' into adminnetworkpolicy

ccfd4b8

fasaxc reviewed Sep 13, 2024

View reviewed changes

felix/rules/endpoints.go Outdated Show resolved Hide resolved

mazdakn commented Sep 13, 2024

View reviewed changes

update

43d804b

mazdakn requested a review from fasaxc September 16, 2024 21:42

Some more changes

018190a

mazdakn commented Sep 16, 2024

View reviewed changes

felix/dataplane/linux/bpf_ep_mgr.go Show resolved Hide resolved

mazdakn added 2 commits September 16, 2024 16:50

remove comment

2cbc93f

Merge remote-tracking branch 'os/master' into adminnetworkpolicy

ad15cc2

fasaxc reviewed Sep 17, 2024

View reviewed changes

fasaxc approved these changes Sep 17, 2024

View reviewed changes

mazdakn added 2 commits September 17, 2024 10:01

Address comments

82f0fe4

fix fv

f594284

caseydavenport merged commit 800b96f into projectcalico:master Sep 17, 2024
2 of 3 checks passed

mazdakn deleted the adminnetworkpolicy branch September 17, 2024 21:02

This was referenced Sep 17, 2024

Add support for AdminNetworkPolicy tigera/operator#3501

Merged

Support Admin Network Policy API from upstream K8s #7578

Open

Support AdminNetworkPolicy named ports #9254

Open

AdminNetworkPolicy network cidr support #9276

Merged

coutinhop added docs-completed Docs PR has been merged for this change and removed docs-pr-required Change is not yet documented labels Oct 28, 2024

fasaxc mentioned this pull request Nov 19, 2024

Add support for BaselineAdminNetworkPolicy. #9464

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AdminNetworkPolicy #9206

Add AdminNetworkPolicy #9206

mazdakn commented Sep 3, 2024 •

edited by coutinhop

Loading

fasaxc left a comment

fasaxc Sep 12, 2024

fasaxc Sep 12, 2024

fasaxc Sep 12, 2024

fasaxc Sep 12, 2024

mazdakn Sep 12, 2024

fasaxc Sep 16, 2024

mazdakn Sep 16, 2024

mazdakn Sep 16, 2024

mazdakn Sep 16, 2024

mazdakn Sep 16, 2024

mazdakn Sep 16, 2024

fasaxc Sep 12, 2024

mazdakn Sep 13, 2024

fasaxc Sep 16, 2024

fasaxc Sep 12, 2024

fasaxc Sep 12, 2024

mazdakn Sep 13, 2024

fasaxc Sep 16, 2024

fasaxc left a comment

		@@ -0,0 +1,72 @@
		// Copyright (c) 2024 Tigera, Inc. All rights reserved.

Add AdminNetworkPolicy #9206

Add AdminNetworkPolicy #9206

Conversation

mazdakn commented Sep 3, 2024 • edited by coutinhop Loading

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

fasaxc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasaxc left a comment

Choose a reason for hiding this comment

mazdakn commented Sep 3, 2024 •

edited by coutinhop

Loading