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

fix: ur not cleaned up when deleting namespaced policy #17

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

sandeshlmore
Copy link

@sandeshlmore sandeshlmore commented Oct 27, 2022

Signed-off-by: Sandesh More [email protected]

Explanation

Currently same handler function is used in both ClusterPolicy and Policy deletion, due to which a type error prevents clean up of UR while policy deletion.

Related issue

closes: 5101

Milestone of this PR

What type of PR is this

bug

Proposed Changes

Added seperate handler for Namespaced Policy deletion which takes care of UR clean up.

Proof Manifests

  1. Create the poltest Namespace.
  2. Create this Policy.
apiVersion: kyverno.io/v2beta1
kind: Policy
metadata:
  name: pol-sync-data
  namespace: poltest
spec:
  rules:
  - name: gen-zk
    match:
      any:
      - resources:
          kinds:
          - Secret
    generate:
      synchronize: true
      apiVersion: v1
      kind: ConfigMap
      name: zk
      namespace: poltest
      data:
        kind: ConfigMap
        metadata:
          labels:
            somekey: somevalue
        data:
          ZK_ADDRESS: "192.168.10.10:2181,192.168.10.11:2181,192.168.10.12:2181"
          KAFKA_ADDRESS: "192.168.10.13:9092,192.168.10.14:9092,192.168.10.15:9092"
  1. Create a Secret which triggers this rule.
    $ k -n poltest create secret generic noodles --from-literal=foo=bar
  2. Delete the Policy.
    $ k -n poltest delete pol pol-sync-data
  3. See the complete UR is removed:
    $ k -n kyverno get ur

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

@sandeshlmore sandeshlmore self-assigned this Oct 27, 2022
@sandeshlmore sandeshlmore marked this pull request as ready for review October 28, 2022 04:45
Copy link

@shahpratikr shahpratikr left a comment

Choose a reason for hiding this comment

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

don't we need to add any unit test for this?

pkg/background/update_request_controller.go Outdated Show resolved Hide resolved
pkg/background/update_request_controller.go Outdated Show resolved Hide resolved
pkg/background/update_request_controller.go Outdated Show resolved Hide resolved
@sandeshlmore sandeshlmore force-pushed the issue_5101 branch 2 times, most recently from 1fbb596 to 8748a78 Compare October 31, 2022 08:52
@sandeshlmore sandeshlmore force-pushed the issue_5101 branch 3 times, most recently from 0d69c9f to 7f2bd21 Compare November 3, 2022 03:41
logger.V(4).Info("deleting policy", "name", p.Name)
key, err := cache.MetaNamespaceKeyFunc(kubeutils.GetObjectWithTombstone(obj))
if err != nil {
logger.Error(err, "failed to load policy key")
Copy link

Choose a reason for hiding this comment

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

can we put a return here, so that rest of the code can come out of else block?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -370,6 +370,50 @@ func (c *controller) deletePolicy(obj interface{}) {
}
}

func (c *controller) deleteNSPolicy(obj interface{}) {
Copy link

Choose a reason for hiding this comment

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

It will be good if we can write unit tests for deleteNSPolicy function?

Copy link
Author

Choose a reason for hiding this comment

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

Unit test cant be added for this function. i tried with fake client as well.

eddycharly and others added 4 commits November 11, 2022 12:16
Signed-off-by: Charles-Edouard Brétéché <[email protected]>
Closes kyverno#5187

The test command was resetting the return value to "pass", even if it
was already marked failed, in some cases. This solves by moving the
"pass" into an else-if clause.

Signed-off-by: Eric Miller <[email protected]>

Signed-off-by: Eric Miller <[email protected]>
Co-authored-by: Vyankatesh Kudtarkar <[email protected]>
Co-authored-by: shuting <[email protected]>
* Adds tests for fixes in kyverno#4767

Signed-off-by: Tobias Dahlberg <[email protected]>

Signed-off-by: Tobias Dahlberg <[email protected]>
Co-authored-by: shuting <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants