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

global context proposal #47

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

global context proposal #47

wants to merge 30 commits into from

Conversation

JimBugwadia
Copy link
Member

No description provided.

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
@MariamFahmy98
Copy link
Contributor

I can't find the resource cache proposal in the KDP.
Am I missing something?

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
@JimBugwadia
Copy link
Member Author

I can't find the resource cache proposal in the KDP. Am I missing something?

Oops...forgot to add the file!

As they say, good design is invisible :-)

Added.

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Copy link
Contributor

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

It seems like it may also be a good idea to register a new metric which indicates how many total cache informers exist at a given time to assist users (and maintainers) in troubleshooting.

README.md Outdated Show resolved Hide resolved
proposals/resource_cache.md Show resolved Hide resolved
proposals/resource_cache.md Outdated Show resolved Hide resolved
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
@eddycharly
Copy link
Member

Ooops, I missed that, will look at it next week !

Copy link
Contributor

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

What will happen if a resourceCache context variable is declared yet there are no corresponding resources in the cache? Does the context var fail to be evaluated, does it evaluate automatically to [], something else?

proposals/resource_cache.md Outdated Show resolved Hide resolved
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>

### Metrics

It would be useful to add cache metrics for observability and troubleshooting.
Copy link
Collaborator

@MarcelMue MarcelMue Sep 26, 2023

Choose a reason for hiding this comment

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

Especially cache size would be very important. With this current proposal I see the risk of Kyverno Policies being created which have a large influence on overall cache size.

Comment on lines 232 to 234
API calls do not leverage caching by default.

If needed, we can add a separate caching mechanism for API calls in the future.
Copy link
Member

Choose a reason for hiding this comment

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

Updating drawbacks of this approach

Suggested change
API calls do not leverage caching by default.
If needed, we can add a separate caching mechanism for API calls in the future.
1. API calls do not leverage caching by default. If needed, we can add a separate caching mechanism for API calls in the future.
2. Using `cache.kyverno.io/enabled: "true"` can cause issues when users forgets to add them to the resource.
1. It is easy to miss a resource when adding the label. `apiCall` will return all the resources of the given type while `resourceCache` will return only those resources that have the label. In case of 1-10k resources.
2. Users might not want to have a kyverno specific label in all their resources across all namespaces.
3. For the usecase mentioned in [motivation](#motivation), we need to add resource to the cache when the policies is applied, otherwise, when the resource is applied for the first time, it will fail because of the timeout like it currently does. This will take away the abilities to have substitutions in `resourceCache` (e.g. `namespace: "{{request.namespace}}"`) and the `resourceCache` field will have to be static.

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: shuting <shuting@nirmata.com>
Signed-off-by: shuting <shuting@nirmata.com>
Signed-off-by: shuting <shuting@nirmata.com>
vishal-chdhry and others added 15 commits January 30, 2024 07:57
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
fix: update resource cache proposal to global context

Users can manage which resources to cache by creating a new custom resource called `GlobalContextEntry` provided by Kyverno. This will decouple the creation and usage of a global entry.

A `GlobalContextEntry` will can be either of the following types:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A `GlobalContextEntry` will can be either of the following types:
A `GlobalContextEntry` can be either of the following types:

@chipzoller
Copy link
Contributor

The two things I, as a user, really need answered here are:

  1. what happens when I try and fetch something from global cache and it's empty?
  2. what does the returned data look like when when the cache is populated? I want it to look the same as if I did a raw API call to the K8s API so I can avoid having to write separate JMESPath queries.

@realshuting
Copy link
Member

The two things I, as a user, really need answered here are:

  1. what happens when I try and fetch something from global cache and it's empty?
  2. what does the returned data look like when when the cache is populated? I want it to look the same as if I did a raw API call to the K8s API so I can avoid having to write separate JMESPath queries.

cc @vishal-chdhry @KhaledEmaraDev

@vishal-chdhry
Copy link
Member

  1. In the original implementation, if the context entry is not present, the policy would have returned an error. Khaled can you verify that this is the current behaviour.

  2. Unfortunately, there is a slight difference, when we do an API call and dont mention the resource, we get a list in return. But in the global context, we are using imformers and informer returns an array of object. Basically the output of global context is .Items of the corresponding apicall

@KhaledEmaraDev
Copy link

KhaledEmaraDev commented Feb 6, 2024

The two things I, as a user, really need answered here are:

  1. what happens when I try and fetch something from global cache and it's empty?
  2. what does the returned data look like when when the cache is populated? I want it to look the same as if I did a raw API call to the K8s API so I can avoid having to write separate JMESPath queries.

cc @vishal-chdhry @KhaledEmaraDev

  1. Just like Vishal mentioned. Plus, it would set the policy to NotReady and with the correct reason of the Cache Entry not existing.

  2. Again Vishal is correct here. APICall would return a DeploymentList array each of which would have its own items. So it has to be queried the old way for instance [0].items | length(@) while the resource would return items to begin with.

We could edit the response of the APICall. But it would be hacky as sometimes it will be a list sometimes not. Depending on the query.

@JimBugwadia JimBugwadia changed the title add resource cache proposal global context proposal Aug 17, 2024
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.

8 participants