k8s: Fix CCNP for host policies

[pchaigno]

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]

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

[pchaigno]

test-me-please

[christarazi]

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

[aanm]

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

[pchaigno]

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

No, that's known flake #11512 AFAICS.

[pchaigno]

test-me-please

[pchaigno]

test-focus K8sFQDNTest Validate that multiple specs are working correctly

[pchaigno]

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]

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

[pchaigno]

@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:

1. 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"

2. 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

1. needs to be addressed
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)

[pchaigno]

1. Fixed.
2. 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]

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]

@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]

@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:

1. 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
2. Set preserveUnknownFields: false while we're on v1beta1 CRDs. (more graceful transitional solution)

Correct me if I'm wrong @aanm.

[pchaigno]

retest-4.19

[pchaigno]

retest-gke

[pchaigno]

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]

@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]

@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:

1. 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
2. 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