Implement the AdminNetworkPolicy api CRD #30

astoycos · 2022-02-28T16:28:32Z

This PR serves to implement the AdminNetworkPolicy CRD described in further detail with kubernetes/enhancements#2091.

The goal is to ensure this does not merge without solving all of the TODO's described in the KEP.

k8s-ci-robot · 2022-02-28T16:28:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [astoycos]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

api/v1alpha1/adminnetworkpolicy_types.go

astoycos · 2022-03-01T14:14:03Z

/assign @Dyanngg

k8s-ci-robot · 2022-03-01T14:14:05Z

@astoycos: GitHub didn't allow me to assign the following users: Dyanngg.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Dyanngg

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

danwinship · 2022-03-03T22:04:18Z

Maybe you should merge a PR with all the vendor stuff first so that this one can be just the actual API?

astoycos · 2022-03-03T22:07:44Z

Sure I can do that @danwinship, thanks!

astoycos · 2022-03-03T22:23:02Z

For now I broke the API changes explicitly out in it's own commit 389fc55

astoycos · 2022-03-04T17:53:33Z

/assign @thockin @caseydavenport @danwinship

astoycos · 2022-03-14T13:40:29Z

/assign @Dyanngg

k8s-ci-robot · 2022-03-14T13:40:30Z

@astoycos: GitHub didn't allow me to assign the following users: Dyanngg.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Dyanngg

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

astoycos · 2022-03-14T13:46:28Z

/assign @abhiraut

caseydavenport

first pass

caseydavenport · 2022-03-14T18:32:58Z

api/v1alpha1/adminnetworkpolicy_types.go

+	metav1.ObjectMeta `json:"metadata,omitempty"`
+
+	// Specification of the desired behavior of AdminNetworkPolicy.
+	// +optional


Is the spec really optional?

For NetworkPolicy and a few other types in the core API it is, but I agree logically it doesn't make sense

api/v1alpha1/adminnetworkpolicy_types.go

caseydavenport · 2022-03-14T18:53:09Z

api/v1alpha1/adminnetworkpolicy_types.go

+
+// NamespaceSetAndPod defines a flexible way to select Namespaces and pods in a
+// cluster. The `Namespaces` and `Pods` fields are required and must not be empty.
+type NamespaceAndPodSet struct {


I think I missed this point in the original KEP somehow... what's the intended difference between NamespaceAndPodSet and NamespacedPodSubject?

They seem like very similar structs so not sure what the purpose of having both is.

NamespacedPodSubject is just a simple NamespaceSelector AND PodSelector but the abstraction allows AdminNetworkPolicySubject to still be built in a future-proof way. We don't use NamespaceSet(self, notself) here since the semantics don't work the same for defining the Subject pods of the policy vs defining a peer.

With NamespaceAndPodSet you can define the set of pods selected in a much more dynamic way since you have all the mechanisms provided by the NamespaceSet object(self, notself etc.) AND PodSet (currently just a generic PodSelector but possibly a PodSelector And ServiceSelector in the future). It also gives us a safe way to expand the API in the future if more selectors are added to either NamespaceSet or PodSet

astoycos · 2022-05-02T20:48:09Z

Pushed changes in response to Dan's Review and meeting last week

Fix Nits from Tim
Rework AdminNetworkPolicyPort
Add More validation to ensure
all lists are bound, and that
only one field may be set when
necessary

astoycos · 2022-05-02T20:48:43Z

apis/v1alpha1/shared_types.go

+// Exactly one field must be set.
+// +kubebuilder:validation:MaxProperties=1
+// +kubebuilder:validation:MinProperties=1
+type AdminNetworkPolicyPort struct {


@Dyanngg @danwinship @thockin PTAL

AdminNetworkPolicyPort spec looks good

apis/v1alpha1/adminnetworkpolicy_types.go

Dyanngg · 2022-05-02T21:14:40Z

apis/v1alpha1/shared_types.go

+// Exactly one field must be set.
+// +kubebuilder:validation:MaxProperties=1
+// +kubebuilder:validation:MinProperties=1
+type AdminNetworkPolicyPort struct {


AdminNetworkPolicyPort spec looks good

apis/v1alpha1/shared_types.go

Dyanngg · 2022-05-02T21:17:52Z

@astoycos Could you also retry assigning me? I'm in the K8s org now, probably need you to add me as a maintainer for this repo

astoycos · 2022-05-02T21:33:33Z

/assign @Dyanngg

Update the API in response to another round of reviews... Sepcifically - Fix Nits from Tim - Rework AdminNetworkPolicyPort - Add More validation to ensure all lists are bound, and then only one field may be set when necessary Signed-off-by: astoycos <[email protected]>

apis/v1alpha1/adminnetworkpolicy_types.go

danwinship · 2022-05-09T17:30:12Z

apis/v1alpha1/adminnetworkpolicy_types.go

+	// Name is an identifier for this rule, that may be no more than 100 characters
+	// in length.
+	// +optional
+	// +kubebuilder:validation:MaxLength=100
+	Name string `json:"name,omitempty"`


again, this needs to explain what the field is for, or the user will have no clue what to do with this.

Sounds good, I was planning on including this rational in the accompanying docs, however I will also add a blurb on it here as well.

danwinship · 2022-05-09T17:35:10Z

apis/v1alpha1/adminnetworkpolicy_types.go

+	// +kubebuilder:validation:MaxLength=100
+	Name string `json:"name,omitempty"`
+
+	// Action specifies whether this rule must pass, allow or deny traffic.


"specifies the action taken on matched traffic?"

Is it guaranteed that action will always be Allow, Deny, or Pass; or is it possible other actions will be added in the future? ("Log"?) Might be good to be clear.

Yeah it should be worded to be extensible in the future, I will alter this a bit!

apis/v1alpha1/adminnetworkpolicy_types.go

apis/v1alpha1/shared_types.go

danwinship · 2022-05-09T18:05:24Z

apis/v1alpha1/shared_types.go

+	// follows standard label selector semantics; if present but empty, it selects
+	// all Namespaces.
+	// +optional
+	Selector *metav1.LabelSelector `json:"selector,omitempty"`


So:

subject: namespaces: matchLabels: admin: "true"

but

from: namespaces: selector: <-- extra matchLabels: admin: "true"

also weird:

from: namespaces: selector: <-- matchLabels: admin: "true"

but

from: pods: pods: <-- UGH podSelector: <-- why not just "selector"? matchLabels: admin: "true"

danwinship · 2022-05-09T18:12:54Z

apis/v1alpha1/shared_types.go

+	Namespaces NamespacedPeer `json:"namespaces"`
+
+	// Pods is used to select a set of Pods in the set of Namespaces.
+	Pods PodPeer `json:"pods"`


Why is PodPeer needed? Why can't this just be PodSelector *metav1.LabelSelector? If we want to define some other way of selecting pods in the future, then that should be a new option under AdminNetworkPolicyPeer, not a new option inside NamespacedPodPeer.

After taking about this in the SIG meeting we decided to remove the PodPeer level of indirection

Fix some nits Remove a level of indirection: `PodPeer` Change Ports protocol default Update some comment documentation Signed-off-by: Andrew Stoycos <[email protected]>

astoycos · 2022-05-10T16:22:41Z

Next round of changes have been pushed, I will work to create some sample yaml's for us to study this week.

apis/v1alpha1/adminnetworkpolicy_types.go

danwinship · 2022-05-24T11:31:10Z

apis/v1alpha1/adminnetworkpolicy_types.go

+	// List of Egress rules to be applied to the selected pods BEFORE any
+	// NetworkPolicy or BaselineAdminNetworkPolicy rules have been applied.


maybe instead of repeating this on both Ingress and Egress it should be mentioned on Priority instead? ("All AdminNetworkPolicy rules have higher precedence than NetworkPolicy or BaselineAdminNetworkPolicy rules".)

+1 This seems a bit repetitive

However for BANP I'll leave the description on the rule fields since there is no priority field

apis/v1alpha1/adminnetworkpolicy_types.go

apis/v1alpha1/shared_types.go

danwinship · 2022-05-24T12:51:39Z

apis/v1alpha1/shared_types.go

+	// +optional
+	NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"`
+
+	// SameLabels is used to select a set of Namespaces that share the same values


hm... it's too bad we managed to squash Self/NotSelf but didn't manage to squash SameLabels/NotSameLabels.

It's also too bad that one of them uses "Self" and the other uses "Same" (which is confusing enough that you already got it wrong, above).

I don't have any suggestions for v1alpha1, but I think this might be something we could improve later...

Yeah I'll think on this a bit more, We probably could combine into a RelatedLabels or something of the sort with a list of labels and a single Enum shared with Related possibly

danwinship · 2022-05-24T12:53:35Z

apis/v1alpha1/shared_types.go

+	// for a set of labels.
+	// To be selected a Namespace must have all of the labels defined in SameLabels,
+	// and they must all have the same value as the subject of this policy.
+	// If Samelabels is Empty then nothing is selected.


as with the Ports thing above, we need to correctly distinguish nil (does not match on labels) from [] (matches if these 0 labels are all the same, which is to say, matches everything)

For now i'll follow the same convention I did with Ports, i.e make it a pointer to a slice

apis/v1alpha1/shared_types.go

- Some smaller changes in grammer and wording. - Internal ordering - TODOs, figure out if Pointer to a slice is valid, and Combine Same/NotSame Labels into a single field Signed-off-by: astoycos <[email protected]>

astoycos · 2022-06-06T15:31:05Z

@danwinship I should have gotten to most of your comments in the latest commit :)

danwinship · 2022-06-06T20:04:02Z

/lgtm
/hold
would be good to get another review from Tim, especially around the *[]

astoycos · 2022-06-08T19:45:20Z

CC @thockin

astoycos · 2022-06-27T21:24:05Z

/unhold

astoycos · 2022-06-27T21:25:35Z

We discussed this with Tim today in the impromptu sig-network-policy api meeting and he agreed we should go ahead and merge as is, and he will review/ open issues if needed.

Thanks for all the help everyone!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 28, 2022

k8s-ci-robot requested review from abhiraut and rikatz February 28, 2022 16:28

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2022

astoycos changed the title ~~Implment the AdminNetworkPolicy api CRD~~ [WIP] Implement the AdminNetworkPolicy api CRD Feb 28, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2022

astoycos commented Feb 28, 2022

View reviewed changes

api/v1alpha1/adminnetworkpolicy_types.go Outdated Show resolved Hide resolved

astoycos force-pushed the admin-network-policy-api branch from 8007eb0 to ef25b93 Compare February 28, 2022 17:26

astoycos force-pushed the admin-network-policy-api branch from ef25b93 to 688335d Compare March 3, 2022 22:22

k8s-ci-robot assigned caseydavenport, danwinship and thockin Mar 4, 2022

astoycos mentioned this pull request Mar 7, 2022

Setup API base for Implementation of AdminNetworkPolicyAPI #31

Closed

astoycos force-pushed the admin-network-policy-api branch from 688335d to 389fc55 Compare March 7, 2022 20:04

astoycos changed the title ~~[WIP] Implement the AdminNetworkPolicy api CRD~~ Implement the AdminNetworkPolicy api CRD Mar 7, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2022

k8s-ci-robot assigned abhiraut Mar 14, 2022

caseydavenport suggested changes Mar 14, 2022

View reviewed changes

gracenng mentioned this pull request Mar 15, 2022

AdminNetworkPolicy support kubernetes/enhancements#2091

Closed

astoycos commented May 2, 2022

View reviewed changes

Dyanngg reviewed May 2, 2022

View reviewed changes

k8s-ci-robot assigned Dyanngg May 2, 2022

astoycos force-pushed the admin-network-policy-api branch from 9e02bd7 to 7b7c961 Compare May 3, 2022 13:48

astoycos requested review from danwinship and thockin May 9, 2022 06:28

danwinship reviewed May 9, 2022

View reviewed changes

Respond to Round 2 of Dan Comments

ce92791

Fix some nits Remove a level of indirection: `PodPeer` Change Ports protocol default Update some comment documentation Signed-off-by: Andrew Stoycos <[email protected]>

astoycos mentioned this pull request May 23, 2022

KEP-2091: Update ANP KEP in response to building the API kubernetes/enhancements#3318

Closed

danwinship reviewed May 24, 2022

View reviewed changes

Respond to Round 3 of Dan's Comments

274184a

- Some smaller changes in grammer and wording. - Internal ordering - TODOs, figure out if Pointer to a slice is valid, and Combine Same/NotSame Labels into a single field Signed-off-by: astoycos <[email protected]>

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 6, 2022

danwinship mentioned this pull request Jun 22, 2022

Stateless Ingress node firewall support using eBPF proposal openshift/enhancements#1158

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2022

k8s-ci-robot merged commit c0938a6 into kubernetes-sigs:master Jun 27, 2022

astoycos mentioned this pull request Aug 18, 2022

Add conformance tests for the AdminNetworkPolicy API #35

Closed

elevran mentioned this pull request Oct 27, 2022

Introduce support for (cluster scoped) AdminNetworkPolicy API. np-guard/netpol-analyzer#30

Closed

astoycos mentioned this pull request May 8, 2023

Cut v0.1.0 of the API and accompanying generated clients #90

Closed

astoycos mentioned this pull request Oct 12, 2023

Implement Cluster Egress Traffic semantics (ANP&BANP NorthBound Support) - PART1 - Nodes #143

Merged

		// List of Egress rules to be applied to the selected pods BEFORE any
		// NetworkPolicy or BaselineAdminNetworkPolicy rules have been applied.

Implement the AdminNetworkPolicy api CRD #30

Implement the AdminNetworkPolicy api CRD #30

Uh oh!

Conversation

astoycos commented Feb 28, 2022

Uh oh!

k8s-ci-robot commented Feb 28, 2022

Uh oh!

Uh oh!

astoycos commented Mar 1, 2022

Uh oh!

k8s-ci-robot commented Mar 1, 2022

Uh oh!

danwinship commented Mar 3, 2022

Uh oh!

astoycos commented Mar 3, 2022

Uh oh!

astoycos commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astoycos commented Mar 4, 2022

Uh oh!

astoycos commented Mar 14, 2022

Uh oh!

k8s-ci-robot commented Mar 14, 2022

Uh oh!

astoycos commented Mar 14, 2022

Uh oh!

caseydavenport left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astoycos Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astoycos commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dyanngg commented May 2, 2022

Uh oh!

astoycos commented May 2, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

astoycos commented Mar 3, 2022 •

edited

Loading

astoycos Mar 18, 2022 •

edited

Loading

astoycos commented May 2, 2022 •

edited

Loading

astoycos commented Jun 27, 2022 •

edited

Loading