pkg/k8s: handle nil PodSelector and NamespaceSelector within ingress rules #2699

ianvernon · 2018-02-02T00:45:50Z

Kubernetes NetworkPolicyPeer allows for PodSelector and NamespaceSelector fields to be optional.
Gracefully handle when these objects are nil when we are parsing NetworkPolicy.

Signed-off by: Ian Vernon ian@cilium.io

Fixes: #2698

Kubernetes NetworkPolicyPeer allows for PodSelector and NamespaceSelector fields to be optional.
Gracefully handle when these objects are nil when we are parsing NetworkPolicy.

How to test:

Start up the Kubernetes developer VM:
K8S=1 ./contrib/vagrant/start.sh
Import the following NetworkPolicy:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: andre-example-policy
  namespace: default
spec:
  podSelector:
    matchLabels:
      id: app1
  policyTypes:
  - Ingress
  ingress:
  - from:
    - ipBlock:
        cidr: 172.17.0.0/16
        except:
        - 172.17.1.0/24

kubectl create -f <policy file>
3. Cilium doesn't crash and the policy is present in cilium policy get

[
  {
    "endpointSelector": {
      "matchLabels": {
        "k8s:id": "app1",
        "k8s:io.kubernetes.pod.namespace": "default"
      }
    },
    "ingress": [
      {
        "fromCIDRSet": [
          {
            "cidr": "172.17.0.0/16",
            "except": [
              "172.17.1.0/24"
            ]
          }
        ]
      }
    ],
    "labels": [
      {
        "key": "io.cilium.k8s-policy-name",
        "value": "andre-example-policy",
        "source": "unspec"
      },
      {
        "key": "io.cilium.k8s-policy-namespace",
        "value": "default",
        "source": "unspec"
      }
    ]
  }
]
Revision: 12

ianvernon · 2018-02-02T00:55:54Z

test-me-please

tgraf · 2018-02-02T01:11:39Z

pkg/k8s/network_policy.go

+				endpointSelector := parseNetworkPolicyPeer(namespace, &rule)
+
+				// Case where no label-based selectors were in rule.
+				if endpointSelector != nil {


Please add a debug message that we are skipping to import a rule here

I'll add a log saying that the rule has no PodSelector / NamespaceSelector in ParseNetworkPolicyPeer. We aren't skipping to import a rule because the rule could also have IPBlock restrictions in it which are parsed following this line. Is that OK?

Ah, the comment is incorrect, I'll move it accordingly as well.

aanm · 2018-02-02T11:18:10Z

pkg/k8s/network_policy_test.go

+
+	rules, err := ParseNetworkPolicy(&np)
+	c.Assert(err, IsNil)
+	c.Assert(rules, NotNil)


Create a golang struct with your expectations of what will be the result of ParseNetworkPolicy and assert for deep equalness between your expectations and the result of ParseNetworkPolicy

nebril · 2018-02-02T11:29:46Z

pkg/k8s/network_policy_test.go

+
+	// Ingress with neither pod nor namespace selector set.
+	ex1 := []byte(`{
+"kind": "NetworkPolicy",


Minor nitpick - since it's json you can indent this string, but it's nothing of great importance.

ianvernon · 2018-02-02T18:35:36Z

test-me-please

ianvernon · 2018-02-02T19:18:06Z

test-me-please

ianvernon · 2018-02-02T22:57:20Z

test-me-please

aanm · 2018-02-03T01:13:20Z

pkg/k8s/network_policy_test.go

+	}
+
+	rules, err := ParseNetworkPolicy(&np)
+	fmt.Printf("parsedRule: %v", rules[0])


Leftovers fmt.Printfs

ianvernon · 2018-02-03T03:54:56Z

test-me-please

aanm · 2018-02-05T12:04:48Z

test-me-please

Kubernetes NetworkPolicyPeer allows for PodSelector and NamespaceSelector fields to be optional. Gracefully handle when these objects are nil when we are parsing NetworkPolicy. Signed-off by: Ian Vernon <ian@cilium.io>

Signed-off by: Ian Vernon <ian@cilium.io>

ianvernon · 2018-02-05T17:11:38Z

test-me-please

ianvernon · 2018-02-05T18:58:22Z

test-me-please

ianvernon · 2018-02-05T19:00:05Z

Keep hitting issues that were fixed by #2686 :

17:37:10 ==> runtime: cp: cannot open '/src/pkg/envoy/lds.sock' for reading: Input/output error
17:37:10 ==> runtime: cp: cannot open '/src/pkg/envoy/access_log.sock' for reading: Input/output error
17:37:10 ==> runtime: cp: cannot open '/src/pkg/envoy/rds.sock' for reading: Input/output error

This is in no way related to my changes, so I tried again. I notice that the node used is workhouse-0, which is always running as an agent of Jenkins. This means that while my branch is rebased, the sockets are left around within the workspace from before I rebased :/

ianvernon added stable/needs-backport priority/high This is considered vital to an upcoming release. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Feb 2, 2018

ianvernon requested a review from a team February 2, 2018 00:45

ianvernon added the pending-review label Feb 2, 2018

tgraf requested changes Feb 2, 2018

View reviewed changes

aanm requested changes Feb 2, 2018

View reviewed changes

nebril approved these changes Feb 2, 2018

View reviewed changes

ianvernon changed the title ~~pkg/k8s: handle nil PodSelector and NamespaceSelector~~ pkg/k8s: handle nil PodSelector and NamespaceSelector within ingress rules Feb 2, 2018

ianvernon force-pushed the 2698-allow-nil-selector-networkpolicy branch from 68d39cb to d3a99ca Compare February 2, 2018 18:35

ianvernon force-pushed the 2698-allow-nil-selector-networkpolicy branch from d3a99ca to 6b563f8 Compare February 2, 2018 22:57

ianvernon requested review from tgraf and aanm February 2, 2018 22:57

aanm requested changes Feb 3, 2018

View reviewed changes

ianvernon force-pushed the 2698-allow-nil-selector-networkpolicy branch from 6b563f8 to bfcf076 Compare February 3, 2018 03:54

ianvernon requested a review from aanm February 3, 2018 03:55

tgraf approved these changes Feb 5, 2018

View reviewed changes

aanm approved these changes Feb 5, 2018

View reviewed changes

Ian Vernon added 3 commits February 5, 2018 09:10

pkg/k8s: handle nil PodSelector and NamespaceSelector

2fdc134

Kubernetes NetworkPolicyPeer allows for PodSelector and NamespaceSelector fields to be optional. Gracefully handle when these objects are nil when we are parsing NetworkPolicy. Signed-off by: Ian Vernon <ian@cilium.io>

pkg/k8s: log when NetworkPolicy does not have Pod / Namespace Selector

f33d09b

Signed-off by: Ian Vernon <ian@cilium.io>

pkg/k8s: add DeepEquals check in TestParseNetworkPolicyNoSelectors

4d28106

Signed-off by: Ian Vernon <ian@cilium.io>

ianvernon force-pushed the 2698-allow-nil-selector-networkpolicy branch from bfcf076 to 4d28106 Compare February 5, 2018 17:11

ianvernon merged commit 142add0 into master Feb 5, 2018

tgraf removed the stable/needs-backport label Feb 15, 2018

tgraf deleted the 2698-allow-nil-selector-networkpolicy branch March 31, 2018 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/k8s: handle nil PodSelector and NamespaceSelector within ingress rules #2699

pkg/k8s: handle nil PodSelector and NamespaceSelector within ingress rules #2699

ianvernon commented Feb 2, 2018 •

edited

ianvernon commented Feb 2, 2018

tgraf Feb 2, 2018

ianvernon Feb 2, 2018

ianvernon Feb 2, 2018

aanm Feb 2, 2018

ianvernon Feb 2, 2018

nebril Feb 2, 2018

ianvernon commented Feb 2, 2018

ianvernon commented Feb 2, 2018

ianvernon commented Feb 2, 2018

aanm Feb 3, 2018

ianvernon Feb 3, 2018

ianvernon commented Feb 3, 2018

aanm commented Feb 5, 2018

ianvernon commented Feb 5, 2018

ianvernon commented Feb 5, 2018

ianvernon commented Feb 5, 2018

pkg/k8s: handle nil PodSelector and NamespaceSelector within ingress rules #2699

pkg/k8s: handle nil PodSelector and NamespaceSelector within ingress rules #2699

Conversation

ianvernon commented Feb 2, 2018 • edited

ianvernon commented Feb 2, 2018

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

ianvernon commented Feb 2, 2018

ianvernon commented Feb 2, 2018

ianvernon commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Feb 3, 2018

aanm commented Feb 5, 2018

ianvernon commented Feb 5, 2018

ianvernon commented Feb 5, 2018

ianvernon commented Feb 5, 2018

ianvernon commented Feb 2, 2018 •

edited