Clusterwide K8s Cilium Network Policies #9381

fristonio · 2019-10-09T13:54:39Z

CRD for CiliumClusterwideNetworkPolicy
Controller for watching events related to CiliumClusterwideNetworkPolicy
RBAC registration for newly created CRD
Tests for the new policy CRD.
Add documentation for CiliumClusterwideNetworkPolicy resource.

This change is

aanm · 2019-10-09T13:58:51Z

pkg/k8s/apis/cilium.io/v2/types.go

+// CiliumGlobalNetworkPolicy is a Kubernetes third-party resource with an extended version
+// of CiliumNetworkPolicy which is cluster scoped rather than namespace scoped.
+type CiliumGlobalNetworkPolicy struct {
+	// +k8s:openapi-gen=false


If you do this we get all of the methods already implemented for CNP for free.

type CiliumGlobalNetworkPolicy struct { CiliumNetworkPolicy }

pkg/k8s/apis/cilium.io/v2/types.go

coveralls · 2019-10-09T14:16:01Z

Coverage decreased (-0.2%) to 45.418% when pulling 1f7036f on fristonio:pr/fristonio/global-policies into 28002d5 on cilium:master.

houndci-bot · 2019-10-21T18:38:20Z

pkg/k8s/watchers/watcher.go

+	k8sAPIGroupCRD                              = "CustomResourceDefinition"
+	k8sAPIGroupNodeV1Core                       = "core/v1::Node"
+	k8sAPIGroupNamespaceV1Core                  = "core/v1::Namespace"
+	K8sAPIGroupServiceV1Core                    = "core/v1::Service"


exported const K8sAPIGroupServiceV1Core should have comment (or a comment on this block) or be unexported

nebril · 2019-10-22T10:17:42Z

Hi @fristonio , sorry for moving the pr from draft, I had to check something in regards to CI failure.

aanm

All good so far only small nits. Next step CI! 💯

aanm · 2019-10-22T14:26:42Z

pkg/k8s/apis/cilium.io/v2/types.go

+// rules. This is similar to the parse function used in CiliumNetworkPolicy with
+// the only difference that since the CRD is cluster scoped we don't send any namespace
+// here.
+func (r *CiliumClusterwideNetworkPolicy) Parse() (api.Rules, error) {


Was this removed in follow up commits?

aanm · 2019-10-22T14:26:57Z

pkg/k8s/apis/cilium.io/v2/types.go

+}
+
+// GetControllerName returns the unique name for the controller manager.
+func (r *CiliumClusterwideNetworkPolicy) GetControllerName() string {


Was this removed in follow up commits?

aanm · 2019-10-22T14:27:00Z

pkg/k8s/apis/cilium.io/v2/types.go

+}
+
+// GetIdentityLabels returns all rule labels in the CiliumNetworkPolicy.
+func (r *CiliumClusterwideNetworkPolicy) GetIdentityLabels() labels.LabelArray {


Was this removed in follow up commits?

aanm · 2019-10-22T14:27:29Z

pkg/k8s/apis/cilium.io/v2/types.go

@@ -276,58 +276,6 @@ type CiliumClusterwideNetworkPolicy struct {
 	CiliumNetworkPolicy
 }

-// Parse parses a CiliumClusterwideNetworkPolicy and returns a list of cilium policy


you can squash this change with the previous commit

pkg/k8s/factory_functions.go

aanm · 2019-10-22T14:30:21Z

pkg/k8s/factory_functions.go

@@ -386,24 +387,39 @@ func ConvertToCNPWithStatus(obj interface{}) interface{} {
 		return &types.SlimCNP{


Please create a new function dedicated for the conversion of CiliumClusterwideNetworkPolicy to types.SlimpCNP

aanm · 2019-10-28T12:45:24Z

test-me-please

aanm · 2019-10-29T09:32:29Z

test-me-please

aanm · 2019-10-29T13:31:56Z

test-me-please

fristonio · 2019-10-29T20:38:47Z

test-me-please

fristonio · 2019-10-29T23:23:39Z

test-me-please

fristonio · 2019-10-30T16:35:11Z

test-me-please

aanm · 2019-11-04T11:54:07Z

test-me-please

aanm · 2019-11-04T14:19:30Z

test-upstream-k8s

aanm · 2019-11-04T14:19:36Z

test-missed-k8s

previous test https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-K8s/2603/

aanm · 2019-11-04T19:38:12Z

test-missed-k8s

fristonio · 2019-11-04T22:15:37Z

test-upstream-k8s

ianvernon · 2019-11-06T01:23:58Z

Upstream k8s tests got stuck:

23:09:25  Running pods 0
23:09:26  Running pods 0
23:09:27  Running pods 0
23:09:28  Running pods 0
23:09:29  Running pods 0
23:09:30  Sending interrupt signal to process
23:09:31  Running pods 0
23:09:31  Connection to 127.0.0.1 closed by remote host.
23:09:31  script returned exit code 255

I hit this in a recent PR as well; re-running.

ianvernon · 2019-11-06T01:24:04Z

test-upstream-k8s

ianvernon

Overall, looks great! I have a few nits mainly surrounding documentation grammar. Thanks for this contribution!!

ianvernon · 2019-11-06T01:26:32Z

Documentation/kubernetes/policy.rst

@@ -24,6 +24,11 @@ with Kubernetes:
  `CustomResourceDefinition` which supports specification of policies
  at Layers 3-7 for both ingress and egress.

+- The `CiliumClusterwideNetworkPolicy` format which is cluster scoped


a cluster-scoped

ianvernon · 2019-11-06T01:26:48Z

Documentation/kubernetes/policy.rst

@@ -24,6 +24,11 @@ with Kubernetes:
  `CustomResourceDefinition` which supports specification of policies
  at Layers 3-7 for both ingress and egress.

+- The `CiliumClusterwideNetworkPolicy` format which is cluster scoped
+  `CustomResourceDefinition` for specifying cluster wide policies to be enforced


cluster-wide

ianvernon · 2019-11-06T01:27:04Z

Documentation/kubernetes/policy.rst

+
+`CiliumClusterwideNetworkPolicy` is same as that of `CiliumNetworkPolicy` with the only
+difference in the scope of the policy. Policies defined by `CiliumClusterwideNetworkPolicy`
+are non namespaced and are cluster scoped. Internally the policy is composed of


non-namespaces and are cluster-scoped.

ianvernon · 2019-11-06T01:27:50Z

Documentation/policy/language.rst

+--------------------
+
+`CiliumNetworkPolicy` only allows to bind a policy restricted to a particular namespace. There can be situations
+where one wants to have a cluster scoped effect of the policy, this can be done using cilium's


cluster-scoped

ianvernon · 2019-11-06T01:28:01Z

Documentation/policy/language.rst

+--------------------
+
+`CiliumNetworkPolicy` only allows to bind a policy restricted to a particular namespace. There can be situations
+where one wants to have a cluster scoped effect of the policy, this can be done using cilium's


..., which can be done using Cilium's...

ianvernon · 2019-11-06T01:28:07Z

Documentation/policy/language.rst

+
+`CiliumNetworkPolicy` only allows to bind a policy restricted to a particular namespace. There can be situations
+where one wants to have a cluster scoped effect of the policy, this can be done using cilium's
+`CiliumClusterwideNetworkPolicy` kubernetes custom resource. The specification of the policy is same as that


kubernetes --> Kubernetes

ianvernon · 2019-11-06T01:28:42Z

Documentation/policy/language.rst

+`CiliumNetworkPolicy` only allows to bind a policy restricted to a particular namespace. There can be situations
+where one wants to have a cluster scoped effect of the policy, this can be done using cilium's
+`CiliumClusterwideNetworkPolicy` kubernetes custom resource. The specification of the policy is same as that
+of `CiliumNetworkPolicy` with the only difference that it is not namespaced.


with the only difference that it is not namespace --> except that it is not namespaced.

ianvernon · 2019-11-06T01:30:28Z

pkg/k8s/apis/cilium.io/v2/register.go

+		CustomResourceDefinitionPluralName = "ciliumclusterwidenetworkpolicies"
+
+		// CustomResourceDefinitionShortNames are the abbreviated names to refer to this CRD's instances
+		CustomResourceDefinitionShortNames = []string{"ccnp", "ciliumcnp"}


I don't think ciliumcnp is necessary here - CNP is almost always used to abbeviate CiliumNetworkPolicy, while here it refers to ClusterwideNetworkPolicy. I'd prefer we keep only ccnp.

fristonio · 2019-11-09T19:29:32Z

test-me-please

fristonio · 2019-11-10T07:20:18Z

test-me-please

tgraf

Fantastic work. The only change request I have is regarding the name. I think shorter is better but I'm also fine with the current proposal.

tgraf · 2019-11-10T23:51:18Z

pkg/k8s/apis/cilium.io/utils/utils.go

+
+	// ResourceTypeCiliumClusterwideNetworkPolicy is the resource type used for the
+	// PolicyLabelDerivedFrom label
+	ResourceTypeCiliumClusterwideNetworkPolicy = "CiliumClusterwideNetworkPolicy"


I would call this CiliumClusterNetworkPolicy. It's shorter and also clear.

Discussed offline. The naming was discussed in SIG-policy and the conclusion was CiliumClusterwideNetworkPolicy. I'm OK with this.

* Add CiliumGlobalNetowrkPolicy type to Cilium k8s api client. * Update k8s utilities to incorporate changes corresponding to cilium global policies. Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

* Update k8s_watcher.go to include controller to watch for CiliumClusterwideNetworkPolicy CRD. * Update ExtractNamespace function in k8s utils to account for the fact that any object with empty namespace is a cluster scoped object and we should not automatically add default namespace to such objects. Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

* Added k8s manifests for ccnp polices to test common policies * Updated tests to include ccnp policy test manifests Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

aanm · 2019-11-11T13:42:04Z

test-me-please

aanm reviewed Oct 9, 2019

View reviewed changes

fristonio force-pushed the pr/fristonio/global-policies branch 2 times, most recently from 8dc8a24 to 800d637 Compare October 9, 2019 17:10

aanm changed the title ~~Global k8s policies for Cilium~~ Clusterwide K8s Cilium Network Policies Oct 10, 2019

fristonio force-pushed the pr/fristonio/global-policies branch 3 times, most recently from 8b979c5 to 79f2142 Compare October 16, 2019 00:56

fristonio force-pushed the pr/fristonio/global-policies branch 2 times, most recently from 559fb57 to 2f59927 Compare October 21, 2019 18:38

houndci-bot reviewed Oct 21, 2019

View reviewed changes

nebril marked this pull request as ready for review October 22, 2019 10:13

nebril requested a review from a team as a code owner October 22, 2019 10:13

nebril requested a review from a team October 22, 2019 10:13

aanm requested changes Oct 22, 2019

View reviewed changes

fristonio force-pushed the pr/fristonio/global-policies branch 2 times, most recently from 002ddd5 to d39c134 Compare October 22, 2019 18:29

fristonio requested a review from a team as a code owner October 26, 2019 20:09

fristonio force-pushed the pr/fristonio/global-policies branch from a0df602 to dd5297d Compare October 29, 2019 02:18

fristonio force-pushed the pr/fristonio/global-policies branch from dd5297d to 1ac45e2 Compare October 29, 2019 13:01

fristonio force-pushed the pr/fristonio/global-policies branch from 1ac45e2 to 9b16060 Compare October 29, 2019 20:25

fristonio force-pushed the pr/fristonio/global-policies branch 2 times, most recently from d8082f8 to a96bdf9 Compare October 30, 2019 16:02

aanm approved these changes Nov 4, 2019

View reviewed changes

aanm added the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Nov 4, 2019

aanm removed the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Nov 4, 2019

ianvernon reviewed Nov 6, 2019

View reviewed changes

fristonio force-pushed the pr/fristonio/global-policies branch 2 times, most recently from 581835d to 3ac09b4 Compare November 9, 2019 19:14

fristonio requested review from ianvernon and tgraf November 10, 2019 16:17

tgraf requested changes Nov 11, 2019

View reviewed changes

tgraf approved these changes Nov 11, 2019

View reviewed changes

tgraf added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 11, 2019

fristonio added 6 commits November 11, 2019 18:45

policy: add cilium clusterwide network policy crd and types

97848c2

* Add CiliumGlobalNetowrkPolicy type to Cilium k8s api client. * Update k8s utilities to incorporate changes corresponding to cilium global policies. Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

policy: add rbac rules for clusterwide network policy

98c5728

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

test: add CI tests for Clusterwide policies

7b05ea7

* Added k8s manifests for ccnp polices to test common policies * Updated tests to include ccnp policy test manifests Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

policy: fix clusterwide policy status update issue for k8s<1.13

1368913

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

docs: add cilium clusterwide policy documentation

1f7036f

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

fristonio force-pushed the pr/fristonio/global-policies branch from 3ac09b4 to 1f7036f Compare November 11, 2019 13:15

aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 11, 2019

aanm merged commit f62e455 into cilium:master Nov 11, 2019

		@@ -386,24 +387,39 @@ func ConvertToCNPWithStatus(obj interface{}) interface{} {
		return &types.SlimCNP{

Clusterwide K8s Cilium Network Policies #9381

Clusterwide K8s Cilium Network Policies #9381

Conversation

fristonio commented Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

coveralls commented Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

nebril commented Oct 22, 2019

aanm left a comment • edited Loading

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

aanm commented Oct 28, 2019

aanm commented Oct 29, 2019

aanm commented Oct 29, 2019

fristonio commented Oct 29, 2019

fristonio commented Oct 29, 2019

fristonio commented Oct 30, 2019

aanm commented Nov 4, 2019

aanm commented Nov 4, 2019

aanm commented Nov 4, 2019 • edited Loading

aanm commented Nov 4, 2019

fristonio commented Nov 4, 2019

ianvernon commented Nov 6, 2019

ianvernon commented Nov 6, 2019

ianvernon 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

fristonio commented Nov 9, 2019

fristonio commented Nov 10, 2019

tgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgraf Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

aanm commented Nov 11, 2019

fristonio commented Oct 9, 2019 •

edited

Loading

coveralls commented Oct 9, 2019 •

edited

Loading

aanm left a comment •

edited

Loading

aanm commented Nov 4, 2019 •

edited

Loading

tgraf Nov 11, 2019 •

edited

Loading