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

Introduce flag to control application of policy in background mode #566

Closed
shivdudhani opened this issue Dec 18, 2019 · 9 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@shivdudhani
Copy link
Contributor

shivdudhani commented Dec 18, 2019

A kyverno policy is applied in multiple ways:

  • Admission-Control: Use webhook configurations to evaluate policies on incoming API-request.
  • Policy-Controller: Runs a re-conciliation loop to evaluate the policies on existing resources.

With variable substitution to be introduced in #533 and userInfo filtering in #345, we can have policies that refer to userInfo attributes in evaluation on the policy. Since the userInfo is only available in incoming api-request and at admission control, we cannot refer it in the policy.

Currently, userInfo filtering is skipped while processing an existing resource.

For consistent behavior, we would introduce a flag background in the policy.
If True then the policy will be evaluated in the background i.e. policy-controller as well, but the policy cannot use variables or userInfo in the policy. This will be validated during policy resource validation.
If False then the policy will only be evaluated in admission-control.

The policy store can be also modified to add a filter based on the background flag in policy.

@realshuting
Copy link
Member

realshuting commented Jan 3, 2020

This background flag, by default, needs to set to true, as one major function of Kyverno is to report violations for the existing resources. If it's not set, it will be true as well. Only when the user sets it to false, we would turn off the background check for that policy.

  • Need to update CRD to set the default value to true
  • When the policy has userInfo defined, and the flag is not false, we need to warn them to set it to false.

@realshuting realshuting reopened this Jan 3, 2020
@realshuting realshuting mentioned this issue Jan 3, 2020
@JimBugwadia
Copy link
Member

When the policy has userInfo defined, and the flag is not false, we need to warn them to set it to false.

This should be a validation error, not a warning i.e. the policy rule should not be allowed

@shivdudhani
Copy link
Contributor Author

As the type of the attribute is boolean, we cannot differentiate between its default value false & user explicitly setting it to false. So we have the default as false.
Do we need to set the default to true here? (If, yes we'll have to change the type to string and make the corresponding changes)

Regarding the use of userInfo in filters & variables with request.userInfo, if they are defined and policy Background is set as true, we tag this as an error in policy resource validation. (this is already present as a part of PR #569 )

@realshuting
Copy link
Member

@shivdudhani
Copy link
Contributor Author

The feature is available as stable since 1.17 and beta in 1.16 with featureGate CustomResourceDefaulting.
The apiVersion for current CRD we use is apiextensions.k8s.io/v1beta1, with 1.17 its apiVersion apiextensions.k8s.io/v1

Does the default value have to be true ? Curious to know the reason

@realshuting
Copy link
Member

If it's not feasible through openAPI, as we still support k8s prior to 1.16, we can handle this default value internally, using a boolean. Found similar:
https://github.com/kubernetes/api/blob/7dc09db16fb8ff2eee16c65dc066c85ab3abb7ce/core/v1/types.go#L3020-L3022

The main concern to set the default to false is: the violation won't be reported on the existing resources, we are losing a major functionality of what we are doing today.

@shivdudhani
Copy link
Contributor Author

Will change it to a pointer to differentiate between empty and default values.

We don't lose on major functionality, we already have validation checks to make sure userInfo is not used in the background. Just documenting it well is needed, so now instead of skipping userInfo check we now do a check. This is to be configured by the user when the policy is defined. Having true or false as default is just behavior, the question was the comparison between them and if the default is set to false wat are things to be handled.

@realshuting
Copy link
Member

As the default is false, we are not pushing the policy to the queue:
https://github.com/nirmata/kyverno/blob/622d007e18dbcd743d2211a6a0f364eef86bf7b5/pkg/policy/controller.go#L162-L164
Then before the controller runs the sync loop, it extracts the policy from the queue. From what I observed from testing, this Get() would wait, return the key until it gets the policy from the queue. That's what I mean we are losing that functionality if we have the flag default to false.
https://github.com/nirmata/kyverno/blob/622d007e18dbcd743d2211a6a0f364eef86bf7b5/pkg/policy/controller.go#L253

@realshuting
Copy link
Member

Close via #569, #593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants