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

Add KDP - Mutate Existing Resources #4

Merged
merged 11 commits into from
Jun 1, 2022

Conversation

realshuting
Copy link
Member

@realshuting realshuting commented Mar 23, 2022

Signed-off-by: ShutingZhao <shuting@nirmata.com>
@chipzoller
Copy link
Member

What I don't see here is a pseudo policy on the background mutation use case (i.e., where there is no triggering resource). I think that's the primary use case for background mutation here. Couple suggestions on that:

  1. needs to be a way in a policy to differentiate select between "new" resources which can be mutated (CREATE or UPDATE) and "old" resources which should be mutated (those which already exist)
  2. needs parallel support in the Kyverno CLI with the apply command, similar to background generation, to allow users to understand the impact such a policy may have on existing resources.

Signed-off-by: ShutingZhao <shuting@nirmata.com>
Signed-off-by: ShutingZhao <shuting@nirmata.com>
@realshuting
Copy link
Member Author

@chipzoller - sorry I missed your comments. For some reason the notification goes into spam :(

To answer your questions:

  1. needs to be a way in a policy to differentiate select between "new" resources which can be mutated (CREATE or UPDATE) and "old" resources which should be mutated (those which already exist)

1.1. ""new" resources which can be mutated" - can be defined in spec.rules.mutate.targets;

1.2. ""old" resources which should be mutated (those which already exist)" - I assume old and new resources are the same in this case? If so, such use cases can be addressed by example 3, when the policy is installed, Kyverno will mutate existing resources.

  1. needs parallel support in the Kyverno CLI with the apply command, similar to background generation, to allow users to understand the impact such a policy may have on existing resources.

Good point! I've added it in the requirement.

Signed-off-by: ShutingZhao <shuting@nirmata.com>
@chipzoller
Copy link
Member

@chipzoller - sorry I missed your comments. For some reason the notification goes into spam :(

To answer your questions:

  1. needs to be a way in a policy to differentiate select between "new" resources which can be mutated (CREATE or UPDATE) and "old" resources which should be mutated (those which already exist)

1.1. ""new" resources which can be mutated" - can be defined in spec.rules.mutate.targets;

But doesn't mutate.targets imply the use case where the target is different from the trigger? What if that's not the case and trigger = target? Seems that the existing way to write a mutate policy would suffice here for "new" resources.

1.2. ""old" resources which should be mutated (those which already exist)" - I assume old and new resources are the same in this case? If so, such use cases can be addressed by example 3, when the policy is installed, Kyverno will mutate existing resources.

I think there needs to be a clear way to distinguish between the two. There are three permutations here:

  1. I want to mutate only "new" (incoming) resources but not pre-existing ones.
  2. I want to mutate both "new" and "old" resources.
  3. I want to mutate only "old" resources.

@realshuting
Copy link
Member Author

  1. I want to mutate only "new" (incoming) resources but not pre-existing ones.

The existing mutate policy can support this.

  1. I want to mutate both "new" and "old" resources.

2.1 "new" and "old" are the same - example 3, setting mutateExisting=true

2.2 "new" and "old" are different - example 1 or 2

  1. I want to mutate only "old" resources.

Good point, we can let the user set mutateExisting=false to support this. And we set the default value to nil/null.

Signed-off-by: ShutingZhao <shuting@nirmata.com>
@chipzoller
Copy link
Member

chipzoller commented Apr 2, 2022

I went back and studied this a bit more and will add more thoughts and comments. I identify three use cases for this enhancement. Let me know if these sound right:

  1. Mutate existing resources once
  2. Mutate existing resources upon policy update (sync mutation)
  3. Mutate resources which are different from the triggering resource

Should we provide for distinction between use cases 1 and 2? Use case for 1 is a one-time mutation whereas use case for 2 is a continual mutation, similar to a generate rule with sync: true enabled.

Also at least one use case that we might want to call as out of scope:

  1. Rolling back mutations in any of the above use cases

Couple additions I thought of we should consider:

  1. Generate an event on existing resources if they were mutated.
  2. Have a new (multiple?) metric corresponding to existing resources that were mutated if not covered by a current metric.

Signed-off-by: ShutingZhao <shuting@nirmata.com>
@realshuting
Copy link
Member Author

Should we provide for distinction between use cases 1 and 2? Use case for 1 is a one-time mutation whereas use case for 2 is a continual mutation, similar to a generate rule with sync: true enabled.

Just a clarification of the current proposal, there's no support as of now to mutate resources once. Like the existing mutate policies, resources will be mutated whenever there's an admission operation. The only difference with this feature is that the trigger can be from two sources:

  • trigger resource events via admission requests
  • policy events

If one-time mutation is something we want to support, we can extend this flag spec.rules.mutate.mutateExisting to accept different modes, say "Always" and "Once", instead of true and flase.

@realshuting
Copy link
Member Author

Also at least one use case that we might want to call as out of scope:

Rolling back mutations in any of the above use cases

This is an interesting use case, I'm not sure if the previous state is stored and we can use that for rolling back since mutation happens during admission review. Do you know if kubectl rollout can revert admission review changes?

Generate an event on existing resources if they were mutated.

Yes, will support.

Have a new (multiple?) metric corresponding to existing resources that were mutated if not covered by a current metric.

Good catch, will need to check this during implementation.

Signed-off-by: ShutingZhao <shuting@nirmata.com>
Signed-off-by: ShutingZhao <shuting@nirmata.com>
Signed-off-by: ShutingZhao <shuting@nirmata.com>
@realshuting realshuting changed the title [WIP] Add KDP - Mutate Existing Resources Add KDP - Mutate Existing Resources Apr 20, 2022
Signed-off-by: ShutingZhao <shuting@nirmata.com>
@realshuting
Copy link
Member Author

realshuting commented Apr 25, 2022

I've updated mutate.target structure to re-use the same target in generate policies. Otherwise changes were completed.

Can we merge this @prateekpandey14 @chipzoller @JimBugwadia ?

@chipzoller
Copy link
Member

I think so. I have resolved my comments.

@realshuting
Copy link
Member Author

@prateekpandey14 @JimBugwadia - needs another approval :)

@JimBugwadia
Copy link
Member

Should we provide for distinction between use cases 1 and 2? Use case for 1 is a one-time mutation whereas use case for 2 is a continual mutation, similar to a generate rule with sync: true enabled.

Just a clarification of the current proposal, there's no support as of now to mutate resources once. Like the existing mutate policies, resources will be mutated whenever there's an admission operation. The only difference with this feature is that the trigger can be from two sources:

  • trigger resource events via admission requests
  • policy events

If one-time mutation is something we want to support, we can extend this flag spec.rules.mutate.mutateExisting to accept different modes, say "Always" and "Once", instead of true and flase.

I don't follow....if the user wants to apply the rule on existing resources in the cluster, will we not apply it as a background operation when the rule is created or modified?

@realshuting
Copy link
Member Author

realshuting commented Apr 25, 2022

I don't follow....if the user wants to apply the rule on existing resources in the cluster, will we not apply it as a background operation when the rule is created or modified?

As stated here:

This feature is can be triggered from two sources:

  • trigger resource events via admission requests
  • policy events

The "policy events" means when a policy/rule is added/updated/removed.

@chipzoller
Copy link
Member

@realshuting what do we do with this now it's implemented yet the PR was never merged? Do we merge for posterity's sake or close?

@realshuting
Copy link
Member Author

@realshuting what do we do with this now it's implemented yet the PR was never merged? Do we merge for posterity's sake or close?

I'll update to the current implementation today and we can then merge.

Signed-off-by: ShutingZhao <shuting@nirmata.com>
@realshuting
Copy link
Member Author

@chipzoller - I have updated the KDP to reflect the current implementation.

@chipzoller
Copy link
Member

Needs second review by @JimBugwadia

@JimBugwadia JimBugwadia merged commit 5475022 into kyverno:main Jun 1, 2022
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.

None yet

3 participants