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

pkg/policy: remove fromEntities and toEntities from rule type #3375

Merged
merged 1 commit into from Mar 29, 2018

Conversation

ianvernon
Copy link
Member

@ianvernon ianvernon commented Mar 29, 2018

Due to a regression introduced with the calling of rule sanitization functions,
rule.sanitize() (different than Rule.sanitize) was never called at runtime,
only during unit tests. As a result, any rule with toEntities or fromEntities
was not properly populated during runtime. This was because the type
pkg/policy:rule only populated these fields during rule.sanitize(), which as
mentioned before, was not called outside of unit tests. Remove the toEntities
and fromEntities fields, and just use the ToEntities and FromEntities within
Rule.Ingress and Rule.Egress accordingly. While this involves a map lookup
to map entities to their corresponding EndpointSelector, this means that we now
only have one code path for validating rules; having multiple ones, as shown
by the regression, is error-prone. Update the policy resolution functions
to account for this change, as well as unit tests.

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

Fixes: #3358

Testing done:

Manually tested. This needs CI end-to-end coverage ASAP so we do not face regressions like this again! Will work on adding those ASAP.

  1. Deploy the following resources in K8s using the following YAML:
---
apiVersion: v1
kind: Service
metadata:
  name: app1-service
spec:
  ports:
  - port: 80
  selector:
    id: app1
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: app1
spec:
  replicas: 2
  template:
    metadata:
      labels:
        id: app1
    spec:
      containers:
      - name: web
        image: cilium/demo-httpd
        ports:
        - containerPort: 80
---
apiVersion: v1
kind: Pod
metadata:
  name: app2
  labels:
    id: app2
spec:
  containers:
  - name: app-frontend
    image: cilium/demo-client
    command: [ "sleep" ]
    args:
      - "1000h"
---
apiVersion: v1
kind: Pod
metadata:
  name: app3
  labels:
    id: app3
spec:
  containers:
  - name: app-frontend
    image: cilium/demo-client
    command: [ "sleep" ]
    args:
      - "1000h"
  1. Create policy with toEntities selecting a pod in K8s:
$ cat policy.yaml 
apiVersion: "cilium.io/v2"                                                 
kind: CiliumNetworkPolicy                                                  
description: "L7 policy for getting started using Kubernetes guide"        
metadata:                                                                  
  name: "rule1"                                                            
spec:                                                                      
  endpointSelector:                                                        
    matchLabels:                                                           
      id: app2                                                             
  egress:                                                                  
  - toEntities:                                                            
    - host                                                                 
    - world 
$ kubectl create -f policy.yaml
  1. Policy appears in both K8s and Cilium:
vagrant@k8s1:~/go/src/github.com/cilium/cilium$ kubectl get cnp -o json
{
    "apiVersion": "v1",
    "items": [
        {
            "apiVersion": "cilium.io/v2",
            "kind": "CiliumNetworkPolicy",
            "metadata": {
                "clusterName": "",
                "creationTimestamp": "2018-03-29T05:15:00Z",
                "generation": 0,
                "name": "rule1",
                "namespace": "default",
                "resourceVersion": "5807",
                "selfLink": "/apis/cilium.io/v2/namespaces/default/ciliumnetworkpolicies/rule1",
                "uid": "2158fd87-3310-11e8-8214-0800279ce2d5"
            },
            "spec": {
                "egress": [
                    {
                        "toEntities": [
                            "host",
                            "world"
                        ]
                    }
                ],
                "endpointSelector": {
                    "matchLabels": {
                        "any:id": "app2"
                    }
                }
            },
            "status": {
                "nodes": {
                    "k8s1": {
                        "enforcing": true,
                        "lastUpdated": "2018-03-29T06:22:42.217258397Z",
                        "localPolicyRevision": 6,
                        "ok": true
                    }
                }
            }
        }
    ],
    "kind": "List",
    "metadata": {
        "resourceVersion": "",
        "selfLink": ""
    }
}
vagrant@k8s1:~/go/src/github.com/cilium/cilium$ cilium policy get
[
  {
    "endpointSelector": {
      "matchLabels": {
        "any:id": "app2",
        "k8s:io.kubernetes.pod.namespace": "default"
      }
    },
    "egress": [
      {
        "toEntities": [
          "host",
          "world"
        ]
      }
    ],
    "labels": [
      {
        "key": "io.cilium.k8s.policy.name",
        "value": "rule1",
        "source": "unspec"
      },
      {
        "key": "io.cilium.k8s.policy.namespace",
        "value": "default",
        "source": "unspec"
      }
    ]
  }
]
Revision: 12
  1. The entities are translated to CIDRs in the policy for the endpoint:
vagrant@k8s1:~/go/src/github.com/cilium/cilium$ cilium endpoint get 49508
[
  {
...
    "policy": {
      "allowed-egress-identities": [
        1,
        2
      ],
      "allowed-ingress-identities": [
        1
      ],
      "build": 12,
      "cidr-policy": {
        "egress": [
          {
            "derived-from-rules": [
              [
                "unspec:io.cilium.k8s.policy.name=rule1",
                "unspec:io.cilium.k8s.policy.namespace=default"
              ]
            ],
            "rule": "0.0.0.0/0"
          },
          {
            "derived-from-rules": [
              [
                "unspec:io.cilium.k8s.policy.name=rule1",
                "unspec:io.cilium.k8s.policy.namespace=default"
              ]
            ],
            "rule": "::/0"
          }
        ],
        "ingress": []
      },
      "id": 17030,
      "l4": {
        "egress": [],
        "ingress": []
      }
    },
    "policy-enabled": "egress",
    "policy-revision": 12,
    "proxy-statistics": [],
    "state": "ready",
    "status": [
      {
        "code": "OK",
        "message": "Successfully regenerated endpoint program due to endpoint policy updated & changes were needed",
        "state": "ready",
        "timestamp": "2018-03-29T06:47:43Z"
      }
    ]
  }
]
  1. Access to world succeeds:
$ kubectl exec -ti app2 -- curl -s -o /dev/null -w "%{http_code}" http://www.google.com
200

Due to a regression introduced with the calling of rule sanitization functions,
rule.sanitize() (different than Rule.sanitize) was never called at runtime,
only during unit tests. As a result, any rule with toEntities or fromEntities
was not properly populated during runtime. This was because the type
pkg/policy:rule only populated these fields during rule.sanitize(), which as
mentioned before, was not called outside of unit tests. Remove the toEntities
and fromEntities fields, and just use the ToEntities and FromEntities within
Rule.Ingress and Rule.Egress accordingly. While this involves a map lookup
to map entities to their corresponding EndpointSelector, this means that we now
only have one code path for validating rules; having multiple ones, as shown
by the regression, is error-prone. Update the policy resolution functions
to account for this change, as well as unit tests.

Signed-off by: Ian Vernon <ian@cilium.io>
@ianvernon ianvernon requested a review from a team March 29, 2018 06:55
@ianvernon ianvernon requested a review from a team as a code owner March 29, 2018 06:55
@ianvernon ianvernon requested review from a team March 29, 2018 06:55
@ianvernon ianvernon added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. priority/1.0-blocker labels Mar 29, 2018
@ianvernon
Copy link
Member Author

test-me-please

@eloycoto eloycoto added the needs/e2e-test This issue is not covered by existing CI tests, but should be. label Mar 29, 2018
Copy link
Contributor

@raybejjani raybejjani left a comment

Choose a reason for hiding this comment

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

Please make more PRs like this. I will buy you ice-cream and coffee. :)

@eloycoto
Copy link
Member

Tested the branch locally and it works as expected

@eloycoto
Copy link
Member

test-me-please

@eloycoto
Copy link
Member

eloycoto commented Mar 29, 2018

Failing test unrelated:

08:19:45 [Runtime] • Failure [219.585 seconds]
08:19:45 [Runtime] RuntimeValidatedConntrackTest
08:19:45 [Runtime] /root/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:238
08:19:45 [Runtime]   Conntrack-related configuration options for endpoints [It]
08:19:45 [Runtime]   /root/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:270
08:19:45 [Runtime] 
08:19:45 [Runtime]   The result of "netperf -l 3 -t UDP_RR -H 10.15.213.77" from container "client" to server does not match
08:19:45 [Runtime]   Expected
08:19:45 [Runtime]       <bool>: false
08:19:45 [Runtime]   to be true
08:19:45 [Runtime] 
08:19:45 [Runtime]   /root/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/runtime/connectivity.go:540
08:19:45 [Runtime] ------------------------------
08:19:45 [Runtime] SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
08:19:45 [Runtime] 
08:19:45 [Runtime] Summarizing 1 Failure:
08:19:45 [Runtime] 
08:19:45 [Runtime] [Fail] RuntimeValidatedConntrackTest [It] Conntrack-related configuration options for endpoints 
08:19:45 [Runtime] /root/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/runtime/connectivity.go:540

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/1728/console

Fixed by #3333

@tgraf tgraf merged commit 35145e6 into master Mar 29, 2018
@tgraf tgraf deleted the 3358-egress-toEntities branch March 29, 2018 14:09
@tgraf tgraf added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/e2e-test This issue is not covered by existing CI tests, but should be. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants