Fix: handled skip rule processing in anyPattern field #5191

ansalamdaniel · 2022-11-02T05:41:16Z

Signed-off-by: ansalamdaniel ansalam.daniel@infracloud.io

Explanation

Global anchor with anyPattern fails when none of the pattern matches. The global anchor generally skips when the criteria is not matched and when used in the anyPattern it throws failure message. But here the generated skip error is not handled in the anyPattern validation function.
This PR aims to fix the global anchor functioning with the anyPattern field.

Related issue

Closes #4221

Milestone of this PR

What type of PR is this

/kind bug

Proposed Changes

Handling the skip error in in the anyPattern field.

Proof Manifests

Below attached test file grabbed from slack discussion. Before this fix, the last test must be skipped but it fails instead.

Before fix

$ go run ./cmd/cli/kubectl-kyverno/main.go test ./test_policy

Executing validate-service-loadbalancer-test...
applying 1 policy to 7 resources... 

│───│───────────────────────────────│─────────────────────────────│──────────────────────────────────│────────│
│ # │ POLICY                        │ RULE                        │ RESOURCE                         │ RESULT │
│───│───────────────────────────────│─────────────────────────────│──────────────────────────────────│────────│
│ 1 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-internal-pass   │ Pass   │
│ 2 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-internal-2-pass │ Pass   │
│ 3 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-public-pass     │ Pass   │
│ 4 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-public-2-pass   │ Pass   │
│ 5 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-public-fail     │ Pass   │
│ 6 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-internal-fail   │ Pass   │
│ 7 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-clusterip-pass  │ Pass   │
│ 8 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-clusterip-pass  │ Fail   │
│───│───────────────────────────────│─────────────────────────────│──────────────────────────────────│────────│

Test Summary: 7 tests passed and 1 tests failed

Aggregated Failed Test Cases : 
│───│───────────────────────────────│───────────────────────────│─────────────────────────────────│────────│
│ # │ POLICY                        │ RULE                      │ RESOURCE                        │ RESULT │
│───│───────────────────────────────│───────────────────────────│─────────────────────────────────│────────│
│ 8 │ validate-service-loadbalancer │ check-loadbalancer-public │ /Service/service-clusterip-pass │ Fail   │
│───│───────────────────────────────│───────────────────────────│─────────────────────────────────│────────│
exit status 1

policy.yaml

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: validate-service-loadbalancer
spec:
  validationFailureAction: enforce
  rules:
  - name: check-loadbalancer-internal
    match:
      resources:
        kinds:
        - Service
    validate:
      message: "Service of type 'LoadBalancer' is internal and does not explicitly define network security. To set the LB to internal, use annotation 'service.beta.kubernetes.io/aws-load-balancer-internal' with value 'true' or '0.0.0.0/0' "
      pattern:
        spec:
          <(type): LoadBalancer
        metadata: # If an annotation of type service.beta.kubernetes.io/aws-load-balancer-internal exists, then it must be either of those values https://kyverno.io/docs/writing-policies/validate/#anchors-and-child-elements-conditional-and-equality
          annotations:
            =(service.beta.kubernetes.io/aws-load-balancer-internal): "0.0.0.0/0|true"
  - name: check-loadbalancer-public
    match:
      resources:
        kinds:
        - Service
    validate:
      message: "Service of type 'LoadBalancer' is public and does not explicitly define network security. To use a public LB you must supply either spec[loadBalancerSourceRanges] or the 'service.beta.kubernetes.io/aws-load-balancer-security-groups' annotation."
      anyPattern:
      - spec:
          <(type): LoadBalancer
        metadata:
          annotations:
            service.beta.kubernetes.io/aws-load-balancer-security-groups: "?*"
      - spec:
          <(type): LoadBalancer
          loadBalancerSourceRanges: "*"

resources.yaml

apiVersion: v1
kind: Service
metadata:
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0
  labels:
    app.kubernetes.io/managed-by: Helm
  name: service-internal-pass
spec:
  type: LoadBalancer
  clusterIP: 100.69.148.11
  loadBalancerSourceRanges:
  - 3.224.166.65/32
  - 3.210.193.151/32
  - 3.226.136.65/32
  - 10.0.0.0/8
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-internal: "true"
  labels:
    app.kubernetes.io/managed-by: Helm
  name: service-internal-2-pass
spec:
  type: LoadBalancer
  clusterIP: 100.69.148.11
  loadBalancerSourceRanges:
  - 3.224.166.65/32
  - 3.210.193.151/32
  - 3.226.136.65/32
  - 10.0.0.0/8
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    meta.helm.sh/release-name: app-my-release
  labels:
    app.kubernetes.io/managed-by: Helm
  name: service-public-pass
spec:
  type: LoadBalancer
  loadBalancerSourceRanges:
  - 3.224.166.65/32
  - 3.210.193.151/32
  - 3.226.136.65/32
  ports:
  - name: http
    nodePort: 31207
    port: 80
    protocol: TCP
    targetPort: 3000
  - name: https
    nodePort: 30400
    port: 443
    protocol: TCP
    targetPort: 3000
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: "true"
    service.beta.kubernetes.io/aws-load-balancer-security-groups: sg-01d98fabb057f8842
  labels:
    app.kubernetes.io/instance: app-hello-node-test
  name: service-public-2-pass
spec:
  type: LoadBalancer
  clusterIP: 100.67.234.84
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    external-dns.alpha.kubernetes.io/hostname: hello-node-public-test.[REDACTED]
    meta.helm.sh/release-name: app-hello-node-test
    meta.helm.sh/release-namespace: app-hello-node-test
    service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: Environment=test,ops_environment=test,ops_costcenter=220,service=hello-node,kubernetesManager=true,region=us-east-1,elb_name=public
    service.beta.kubernetes.io/aws-load-balancer-backend-protocol: http
    service.beta.kubernetes.io/aws-load-balancer-connection-draining-enabled: "true"
    service.beta.kubernetes.io/aws-load-balancer-connection-draining-timeout: "60"
    service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: "true"
    service.beta.kubernetes.io/aws-load-balancer-healthcheck-healthy-threshold: "2"
    service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval: "5"
    service.beta.kubernetes.io/aws-load-balancer-healthcheck-timeout: "2"
    service.beta.kubernetes.io/aws-load-balancer-healthcheck-unhealthy-threshold: "2"
    service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
  labels:
    app.kubernetes.io/managed-by: Helm
  name: service-public-fail
spec:
  type: LoadBalancer
  clusterIP: 100.67.234.84
  clusterIPs:
  - 100.67.234.84
  externalTrafficPolicy: Cluster
  ipFamilies:
  - IPv4
  ipFamilyPolicy: SingleStack
  ports:
  - name: http
    nodePort: 31207
    port: 80
    protocol: TCP
    targetPort: 3000
  - name: https
    nodePort: 30400
    port: 443
    protocol: TCP
    targetPort: 3000
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-internal: "anything"
  labels:
    app.kubernetes.io/managed-by: Helm
  name: service-internal-fail
spec:
  type: LoadBalancer
  clusterIP: 10.96.0.2
  ports:
  - name: http
    nodePort: 31207
    port: 80
    protocol: TCP
    targetPort: 3000
---
apiVersion: v1
kind: Service
metadata:
  labels:
    app.kubernetes.io/managed-by: Helm
  name: service-clusterip-pass
spec:
  type: ClusterIP
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: 3000

test.yaml

name: validate-service-loadbalancer-test
policies:
  - policy.yaml
resources:
  - resources.yaml
results:
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-internal
    resource: service-internal-pass
    kind: Service
    result: pass
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-internal
    resource: service-internal-2-pass
    kind: Service
    result: pass
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-public
    resource: service-public-pass
    kind: Service
    result: pass
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-public
    resource: service-public-2-pass
    kind: Service
    result: pass
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-public
    resource: service-public-fail
    kind: Service
    result: fail
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-internal
    resource: service-internal-fail
    kind: Service
    result: fail
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-internal
    resource: service-clusterip-pass
    kind: Service
    result: skip
  - policy: validate-service-loadbalancer
    rule: check-loadbalancer-public
    resource: service-clusterip-pass
    kind: Service
    result: skip

Kyverno test output (After fix)

$ go run ./cmd/cli/kubectl-kyverno/main.go test ./test_policy

Executing validate-service-loadbalancer-test...
applying 1 policy to 7 resources... 

│───│───────────────────────────────│─────────────────────────────│──────────────────────────────────│────────│
│ # │ POLICY                        │ RULE                        │ RESOURCE                         │ RESULT │
│───│───────────────────────────────│─────────────────────────────│──────────────────────────────────│────────│
│ 1 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-internal-pass   │ Pass   │
│ 2 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-internal-2-pass │ Pass   │
│ 3 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-public-pass     │ Pass   │
│ 4 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-public-2-pass   │ Pass   │
│ 5 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-public-fail     │ Pass   │
│ 6 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-internal-fail   │ Pass   │
│ 7 │ validate-service-loadbalancer │ check-loadbalancer-internal │ /Service/service-clusterip-pass  │ Pass   │
│ 8 │ validate-service-loadbalancer │ check-loadbalancer-public   │ /Service/service-clusterip-pass  │ Pass   │
│───│───────────────────────────────│─────────────────────────────│──────────────────────────────────│────────│

Test Summary: 8 tests passed and 0 tests failed

Running cli tests
For cli tests, used only anypattern related rule for testing.

$ make test-cli-local         
Go fmt...
Go vet...
Build cli binary...

Executing admission-user-info...
applying 1 policy to 3 resources... 

│───│─────────────────────│────────────────────│─────────────────│────────│
│ # │ POLICY              │ RULE               │ RESOURCE        │ RESULT │
│───│─────────────────────│────────────────────│─────────────────│────────│
│ 1 │ disallow-latest-tag │ require-image-tag  │ /Pod/myapp-pod1 │ Pass   │
│ 2 │ disallow-latest-tag │ require-image-tag  │ /Pod/myapp-pod2 │ Pass   │
│ 3 │ disallow-latest-tag │ require-image-tag  │ /Pod/myapp-pod3 │ Pass   │
│ 4 │ disallow-latest-tag │ validate-image-tag │ /Pod/myapp-pod1 │ Pass   │
│ 5 │ disallow-latest-tag │ validate-image-tag │ /Pod/myapp-pod2 │ Pass   │
│ 6 │ disallow-latest-tag │ validate-image-tag │ /Pod/myapp-pod3 │ Pass   │
│───│─────────────────────│────────────────────│─────────────────│────────│

Executing disallow-protected-namespaces...
applying 1 policy to 3 resources... 

<snip>

Executing validate-service-loadbalancer...
applying 1 policy to 4 resources... 

│───│───────────────────────────────│───────────────────────────│─────────────────────────────────│────────│
│ # │ POLICY                        │ RULE                      │ RESOURCE                        │ RESULT │
│───│───────────────────────────────│───────────────────────────│─────────────────────────────────│────────│
│ 1 │ validate-service-loadbalancer │ check-loadbalancer-public │ /Service/service-public-pass    │ Pass   │
│ 2 │ validate-service-loadbalancer │ check-loadbalancer-public │ /Service/service-public-2-pass  │ Pass   │
│ 3 │ validate-service-loadbalancer │ check-loadbalancer-public │ /Service/service-public-fail    │ Pass   │
│ 4 │ validate-service-loadbalancer │ check-loadbalancer-public │ /Service/service-clusterip-skip │ Pass   │
│───│───────────────────────────────│───────────────────────────│─────────────────────────────────│────────│

<snip>

Executing wildcard-support-in-matchlabels...
applying 1 policy to 2 resources... 

│───│─────────────────│─────────────────│───────────────────────────│────────│
│ # │ POLICY          │ RULE            │ RESOURCE                  │ RESULT │
│───│─────────────────│─────────────────│───────────────────────────│────────│
│ 1 │ mutate-wildcard │ mutate-wildcard │ /Pod/wildcard-mutate      │ Pass   │
│ 2 │ mutate-wildcard │ mutate-wildcard │ /Pod/wildcard-mutate-fail │ Pass   │
│───│─────────────────│─────────────────│───────────────────────────│────────│

Test Summary: 142 tests passed and 0 tests failed

Checklist

I have read the contributing guidelines.
I have read the PR documentation guide and followed the process including adding proof manifests to this PR.
This is a bug fix and I have added unit tests that prove my fix is effective.
This is a feature and I have added CLI tests that are applicable.
My PR needs to be cherry picked to a specific release branch which is .
My PR contains new or altered behavior to Kyverno and
- CLI support should be added and my PR doesn't contain that functionality.
- I have added or changed the documentation myself in an existing PR and the link is:
- I have raised an issue in kyverno/website to track the documentation update and the link is:

Further Comments

codecov · 2022-11-02T05:48:29Z

Codecov Report

Merging #5191 (6a20ab6) into main (dfded5c) will increase coverage by 0.09%.
The diff coverage is 84.21%.

❗ Current head 6a20ab6 differs from pull request most recent head 2ba5690. Consider uploading reports for the commit 2ba5690 to get more accurate results

@@            Coverage Diff             @@
##             main    #5191      +/-   ##
==========================================
+ Coverage   36.44%   36.53%   +0.09%     
==========================================
  Files         171      172       +1     
  Lines       19082    19089       +7     
==========================================
+ Hits         6955     6975      +20     
+ Misses      11332    11319      -13     
  Partials      795      795

Impacted Files	Coverage Δ
pkg/engine/validation.go	`62.36% <84.21%> (+2.87%)`	⬆️
pkg/webhooks/resource/handlers.go	`45.83% <0.00%> (-3.06%)`	⬇️
pkg/engine/mutate/patch/patchesUtils.go	`91.83% <0.00%> (-0.25%)`	⬇️
pkg/policy/validate.go	`51.59% <0.00%> (-0.06%)`	⬇️
pkg/policy/existing.go	`0.00% <0.00%> (ø)`
pkg/config/metricsconfig.go	`0.00% <0.00%> (ø)`
pkg/webhooks/utils/metrics.go	`0.00% <0.00%> (ø)`
pkg/webhooks/handlers/filter.go	`0.00% <0.00%> (ø)`
pkg/webhooks/policy/handlers.go	`0.00% <0.00%> (ø)`
pkg/webhooks/handlers/metrics.go	`0.00% <0.00%> (ø)`
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

realshuting · 2022-11-02T08:45:52Z

Thanks for the work @ansalamdaniel , the changes look good to me.

@vyankyGH can you help verify the webhook behavior please?

/assign @vyankyGH

chipzoller · 2022-11-06T13:03:59Z

We need to incorporate tests for this when merged. It seems kyverno/kyverno is probably best.

ansalamdaniel · 2022-11-07T05:52:41Z

We need to incorporate tests for this when merged. It seems kyverno/kyverno is probably best.

@chipzoller Could you please explain how to implement the tests you mentioned?

chipzoller · 2022-11-07T13:01:43Z

Here we have CLI tests which can be used to place a test case to ensure, once this issue is fixed, it does not reappear: https://github.com/kyverno/kyverno/tree/main/test/cli

ansalamdaniel · 2022-11-08T11:25:55Z

Added the test cases. Please do let me know if anything needs to be added or modified.

chipzoller

Thank you for CLI tests! One very minor tweak.

test/cli/test/anypattern_skip_error/policy.yaml

vyankyGH · 2022-11-09T11:42:29Z

@ansalamdaniel can you please rebase the branch?

vyankyGH · 2022-11-23T05:20:15Z

LGTM

realshuting · 2022-11-28T09:35:52Z

@ansalamdaniel - the branch is out-to-date, can you rebase the main branch ?

Signed-off-by: ansalamdaniel <ansalam.daniel@infracloud.io>

ansalamdaniel requested a review from prateekpandey14 as a code owner November 2, 2022 05:41

github-actions bot assigned vyankyGH Nov 2, 2022

ansalamdaniel force-pushed the issue-4221 branch 2 times, most recently from ad904ee to e23bb5e Compare November 3, 2022 13:01

ansalamdaniel force-pushed the issue-4221 branch from e23bb5e to 5a9b7ac Compare November 7, 2022 04:01

ansalamdaniel force-pushed the issue-4221 branch from 5a9b7ac to 7017086 Compare November 8, 2022 06:16

ansalamdaniel requested review from vyankyGH, chipzoller and eddycharly as code owners November 8, 2022 06:16

chipzoller requested changes Nov 8, 2022

View reviewed changes

test/cli/test/anypattern_skip_error/policy.yaml Outdated Show resolved Hide resolved

ansalamdaniel force-pushed the issue-4221 branch from 7017086 to 18baf1f Compare November 9, 2022 04:15

ansalamdaniel requested review from chipzoller and removed request for prateekpandey14, vyankyGH and eddycharly November 9, 2022 05:45

vyankyGH previously approved these changes Nov 9, 2022

View reviewed changes

ansalamdaniel dismissed vyankyGH’s stale review via 81b1e51 November 9, 2022 11:55

ansalamdaniel force-pushed the issue-4221 branch from 18baf1f to 81b1e51 Compare November 9, 2022 11:55

vyankyGH previously approved these changes Nov 9, 2022

View reviewed changes

vyankyGH enabled auto-merge (squash) November 9, 2022 12:43

ansalamdaniel force-pushed the issue-4221 branch from 81b1e51 to d5f5a4b Compare November 11, 2022 05:05

prateekpandey14 dismissed vyankyGH’s stale review via d5f5a4b November 11, 2022 06:46

ansalamdaniel force-pushed the issue-4221 branch from d5f5a4b to 32ffaf4 Compare November 14, 2022 04:54

vyankyGH added the milestone 1.9.0 label Nov 15, 2022

ansalamdaniel force-pushed the issue-4221 branch 2 times, most recently from 0807e49 to 346b71c Compare November 18, 2022 05:08

ansalamdaniel force-pushed the issue-4221 branch from 346b71c to 8add394 Compare November 23, 2022 05:05

vyankyGH previously approved these changes Nov 23, 2022

View reviewed changes

ansalamdaniel dismissed vyankyGH’s stale review via a6a863b November 25, 2022 04:34

ansalamdaniel force-pushed the issue-4221 branch from 6a20ab6 to a6a863b Compare November 25, 2022 04:34

Fix: handled skip rule processing in anyPattern field

2ba5690

Signed-off-by: ansalamdaniel <ansalam.daniel@infracloud.io>

ansalamdaniel force-pushed the issue-4221 branch from a6a863b to 2ba5690 Compare November 28, 2022 11:47

realshuting approved these changes Nov 28, 2022

View reviewed changes

chipzoller approved these changes Nov 28, 2022

View reviewed changes

vyankyGH merged commit 2ba4def into kyverno:main Nov 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: handled skip rule processing in anyPattern field #5191

Fix: handled skip rule processing in anyPattern field #5191

ansalamdaniel commented Nov 2, 2022 •

edited

codecov bot commented Nov 2, 2022 •

edited

realshuting commented Nov 2, 2022

chipzoller commented Nov 6, 2022

ansalamdaniel commented Nov 7, 2022

chipzoller commented Nov 7, 2022

ansalamdaniel commented Nov 8, 2022

chipzoller left a comment

vyankyGH commented Nov 9, 2022

vyankyGH commented Nov 23, 2022

realshuting commented Nov 28, 2022

Fix: handled skip rule processing in anyPattern field #5191

Fix: handled skip rule processing in anyPattern field #5191

Conversation

ansalamdaniel commented Nov 2, 2022 • edited

Explanation

Related issue

Milestone of this PR

What type of PR is this

Proposed Changes

Proof Manifests

Checklist

Further Comments

codecov bot commented Nov 2, 2022 • edited

Codecov Report

realshuting commented Nov 2, 2022

chipzoller commented Nov 6, 2022

ansalamdaniel commented Nov 7, 2022

chipzoller commented Nov 7, 2022

ansalamdaniel commented Nov 8, 2022

chipzoller left a comment

Choose a reason for hiding this comment

vyankyGH commented Nov 9, 2022

vyankyGH commented Nov 23, 2022

realshuting commented Nov 28, 2022

ansalamdaniel commented Nov 2, 2022 •

edited

codecov bot commented Nov 2, 2022 •

edited