k8s: Fix CCNP for host policies by pchaigno · Pull Request #11638 · cilium/cilium

pchaigno · 2020-05-21T09:34:23Z

This commit fixes the CNP/CCNP parsing to accept NodeSelectors, used to define host policies. NodeSelectors are only accepted in CCNP resources. Because host policies are never limited to a namespace, it would be misleading to accept NodeSelectors in CNP resources.

This commit also adds a new CCNP example of host policies in the same directory as the existing JSON example.

Fixes: #11507

coveralls · 2020-05-21T10:05:32Z

Coverage decreased (-0.04%) to 36.918% when pulling 0b5ff4f on pr/pchaigno/fix-host-policies-ccnp into e871f28 on master.

pchaigno · 2020-05-21T10:05:58Z

test-me-please

christarazi

Non-blocking comments, feel free to implement or ignore.

aanm · 2020-05-25T12:18:17Z

@pchaigno it seems the PR failed in the privileged tests. Is this a new failure?

pchaigno · 2020-05-25T12:20:13Z

@pchaigno it seems the PR failed in the privileged tests. Is this a new failure?

No, that's known flake #11512 AFAICS.

pchaigno · 2020-05-27T10:06:36Z

test-me-please

pchaigno · 2020-05-27T12:59:25Z

test-focus K8sFQDNTest Validate that multiple specs are working correctly

pchaigno · 2020-05-27T13:02:06Z

Among the required builds, only K8s-1.11-Kernel-netnext failed with:

Can't connect to to a valid target when it should work
Expected command: kubectl exec -n default app2-64975875bc-5zkgw -- curl --path-as-is -s -D /dev/stderr --fail --connect-timeout 5 --max-time 8 http://vagrant-cache.ci.cilium.io -w "time-> DNS: '%{time_namelookup}(%{remote_ip})', Connect: '%{time_connect}',Transfer '%{time_starttransfer}', total '%{time_total}'"

I've seen other failures with vagrant-cache.ci.cilium.io a few minutes ago, so probably some transient network outage. I've relaunched that test with test-focus.

pchaigno · 2020-05-27T14:41:12Z

test-focus passed. I'm ~~marking as ready to merge.~~ patiently waiting for André's review :-)

pchaigno · 2020-05-27T14:43:02Z

@nebril It looks like Cilium-PR-Kubernetes-Upstream and Cilium-PR-Ginkgo-Tests-K8s were executed when I typed tets-focus 🤔

aanm

Doesn't require a 2nd review from my side but things that are missing:

Needs an increment of

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

Lines 55 to 57 in f9c205d

    
           // CustomResourceDefinitionSchemaVersion is semver-conformant version of CRD schema 
        
           // Used to determine if CRD needs to be updated in cluster 
        
           CustomResourceDefinitionSchemaVersion = "1.19"

I was able to deploy this invalid policy:

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
description: "Allow ports for kube-dns, kube-api, Cilium, and SSH, as well as queries to Google's DNS on UDP/53 only"
metadata:
  name: "allow-google-dns-on-udp-53-only-2"
spec:
  endpointSelector:
    matchLabels:
      {}
  nodeSelector:
    matchLabels:
      {}
  ingress:
    - toPorts:
        - ports:
            - port: "6443"
              protocol: TCP
            - port: "22"
              protocol: TCP
  egress:
    - toPorts:
        - ports:
            - port: "4240"
              protocol: TCP
            - port: "8080"
              protocol: TCP
    - toCIDR:
        - "8.8.0.0/16"
      toPorts:
        - ports:
            - port: "53"
              protocol: UDP

needs to be addressed
can be done as a follow up as I suspect this is an existing bug (and not related with this new schema) cc @christarazi FYI this might be related with the fact that the schema contains all field properties in its root and/or the fact that we are allowing unknown fields (which should not be supported in a v1 CRD but can be on v1beta1)

This commit fixes the CCNP parsing to accept NodeSelectors, used to define host policies. NodeSelectors are only accepted in CCNP resources. Because host policies are never limited to a namespace, it would be misleading to accept NodeSelectors in CNP resources. This commit also adds a new CCNP example of host policies in the same directory as the existing JSON example. Fixes: f9c205d ("pkg/policy: Host network policies") Signed-off-by: Paul Chaignon <paul@cilium.io>

pchaigno · 2020-05-27T20:58:18Z

Fixed.
I noticed that too. I had a quick look and it seemed "expected" that any unspecified field was accepted. I figured it wasn't that bad since the policy will be rejected later on (endpointSelector and nodeSelector cannot be both specified as checked by Rule.Sanitize()). I checked that case was rejected with kubectl describe cnp.

christarazi · 2020-05-27T21:14:35Z

2. can be done as a follow up as I suspect this is an existing bug (and not related with this new schema) cc @christarazi FYI this might be related with the fact that the schema contains all field properties in its root and/or the fact that we are allowing unknown fields (which should not be supported in a v1 CRD but can be on v1beta1)

Unknown fields are allowed as long as preserveUnknownFields: true is set and using v1beta1 CRD. That is the default for v1beta1 also. Which explains why @pchaigno was seeing unspecified fields being accepted.

In v1 CRDs, preserveUnknownFields: true will no longer be allowed, or even set.

@aanm What about your example is invalid?

pchaigno · 2020-05-27T21:19:04Z

@aanm What about your example is invalid?

@christarazi nodeSelector is an unspecified field in the context of CNPs. It's only specified in CCNPs in this PR.

christarazi · 2020-05-27T21:25:03Z

@aanm What about your example is invalid?

@christarazi nodeSelector is an unspecified field in the context of CNPs. It's only specified in CCNPs in this PR.

I see. I believe the only way to prevent this or to throw a validation error, is to either:

Move to v1 CRDs (more of a breaking change, there's still an open question as to how far back we can go in terms of K8s versions)
or
Set preserveUnknownFields: false while we're on v1beta1 CRDs. (more graceful transitional solution)

Correct me if I'm wrong @aanm.

pchaigno · 2020-05-28T09:19:14Z

retest-4.19

pchaigno · 2020-05-28T09:19:30Z

retest-gke

pchaigno · 2020-05-28T09:24:31Z

net-next build failed with https://jenkins.cilium.io/job/Cilium-PR-K8s-oldest-net-next/431/testReport/Suite-k8s-1/11/K8sPolicyTest_Basic_Test_Validate_to_entities_policies_Validate_toEntities_All/. According to CI dashboard, we've hit this in master before (cf. https://jenkins.cilium.io/job/Ginkgo-CI-Tests-k8s1.18-Pipeline/434/testReport/Suite-k8s-1/18/K8sPolicyTest_Basic_Test_Validate_to_entities_policies_Validate_toEntities_All/); probably just a connectivity blip.

test-focus K8sPolicyTest Basic Test Validate to-entities policies Validate toEntities All

It passed in test-focus so all CI builds are okay now 🎉

pchaigno · 2020-05-28T19:59:25Z

@aanm said:

Doesn't require a 2nd review from my side but things that are missing:

and the version has been bumped. Marking as ready to merge.

aanm · 2020-05-28T20:10:46Z

@aanm What about your example is invalid?

@christarazi nodeSelector is an unspecified field in the context of CNPs. It's only specified in CCNPs in this PR.

I see. I believe the only way to prevent this or to throw a validation error, is to either:

Move to v1 CRDs (more of a breaking change, there's still an open question as to how far back we can go in terms of K8s versions)
or

Set preserveUnknownFields: false while we're on v1beta1 CRDs. (more graceful transitional solution)

Correct me if I'm wrong @aanm.

@christarazi It's probably better if we do this after 1.8 at the risk of breaking things

pchaigno added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/misc This PR makes changes that have no direct user impact. labels May 21, 2020

pchaigno requested a review from aanm May 21, 2020 09:34

pchaigno commented May 21, 2020

View reviewed changes

Comment thread pkg/k8s/apis/cilium.io/v2/types.go Outdated

christarazi approved these changes May 21, 2020

View reviewed changes

Comment thread pkg/k8s/apis/cilium.io/v2/types.go Outdated

Comment thread pkg/k8s/apis/cilium.io/v2/types.go Outdated

Comment thread pkg/k8s/apis/cilium.io/utils/utils.go Outdated

pchaigno marked this pull request as ready for review May 21, 2020 20:30

pchaigno requested review from a team as code owners May 21, 2020 20:30

pchaigno requested a review from a team May 21, 2020 20:30

nebril approved these changes May 22, 2020

View reviewed changes

aanm requested changes May 25, 2020

View reviewed changes

Comment thread pkg/k8s/apis/cilium.io/v2/types.go Outdated

aanm approved these changes May 25, 2020

View reviewed changes

pchaigno commented May 25, 2020

View reviewed changes

Comment thread examples/policies/l3/host/host-policy.yaml

pchaigno added the priority/release-blocker label May 26, 2020

pchaigno force-pushed the pr/pchaigno/fix-host-policies-ccnp branch from a59f0ba to be6baf9 Compare May 26, 2020 18:13

pchaigno requested a review from aanm May 26, 2020 18:14

aanm requested changes May 26, 2020

View reviewed changes

Comment thread pkg/k8s/apis/cilium.io/v2/client/register.go Outdated

pchaigno force-pushed the pr/pchaigno/fix-host-policies-ccnp branch from be6baf9 to 437b696 Compare May 27, 2020 10:01

pchaigno requested a review from aanm May 27, 2020 10:06

aanm requested changes May 27, 2020

View reviewed changes

pchaigno force-pushed the pr/pchaigno/fix-host-policies-ccnp branch from 437b696 to 0b5ff4f Compare May 27, 2020 20:55

pchaigno requested a review from aanm May 28, 2020 13:19

pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 28, 2020

aanm merged commit 9b0ae85 into master May 28, 2020

aanm deleted the pr/pchaigno/fix-host-policies-ccnp branch May 28, 2020 20:10

pchaigno mentioned this pull request Jun 1, 2020

Followups for host endpoint and firewall #11799

Closed

34 tasks

pchaigno mentioned this pull request Jul 13, 2020

Reject Cilium network policies with unknown fields #12516

Closed

pchaigno added the area/host-firewall Impacts the host firewall or the host endpoint. label Jul 20, 2020

	// CustomResourceDefinitionSchemaVersion is semver-conformant version of CRD schema
	// Used to determine if CRD needs to be updated in cluster
	CustomResourceDefinitionSchemaVersion = "1.19"

Conversation

pchaigno commented May 21, 2020

Uh oh!

Uh oh!

coveralls commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno commented May 21, 2020

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aanm commented May 25, 2020

Uh oh!

pchaigno commented May 25, 2020

Uh oh!

Uh oh!

Uh oh!

pchaigno commented May 27, 2020

Uh oh!

pchaigno commented May 27, 2020

Uh oh!

pchaigno commented May 27, 2020

Uh oh!

pchaigno commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno commented May 27, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

pchaigno commented May 27, 2020

Uh oh!

christarazi commented May 27, 2020

Uh oh!

pchaigno commented May 27, 2020

Uh oh!

christarazi commented May 27, 2020

Uh oh!

pchaigno commented May 28, 2020

Uh oh!

pchaigno commented May 28, 2020

Uh oh!

pchaigno commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno commented May 28, 2020

Uh oh!

aanm commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented May 21, 2020 •

edited

Loading

pchaigno commented May 27, 2020 •

edited

Loading

pchaigno commented May 28, 2020 •

edited

Loading