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

Reports aggregation in a separate process #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eddycharly
Copy link
Member

This PR proposes to change the way we generate reports by generating per resource reports, let Kubernetes take care of reports lifecycle, and aggregate reports into higher level reports in a separate process/controller.

It is aligned with other KDPs meant to decompose Kyverno in multiple processes.

@chipzoller
Copy link
Member

You've got some relics from another KDP you used as a template in here :)

@eddycharly
Copy link
Member Author

You've got some relics from another KDP you used as a template in here :)

Fixed the one in the Metas section

@eddycharly
Copy link
Member Author

Removed Prior art that leaked from the other KDP too.

Comment on lines +48 to +50
1. At admission time, all policies running in audit mode are run against the admission request and produce report results.
1. When a policy is created/updated/deleted, if the policy can run in background mode, reports are updated according to the policy changes.
1. Periodically, policies running in background mode are re eveluated against resources present in the cluster and reports are updated accordingly.
Copy link
Member

Choose a reason for hiding this comment

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

Index:
1.
2.
3.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will appear correctly in md viewer.

# Proposal

In this proposal, we study the possibility to change the way reports are generated by:
- creating one report per resource
Copy link
Member

Choose a reason for hiding this comment

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

Is report here a report change request or a policy report?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested with RCR but could be a policy report as well.

@Boojapho
Copy link

@eddycharly If you align the reports and TTL with each resource, then it seems like we could just populate the report in the status of the resource itself. I would think that would simplify the architecture. Aggregation would occur by reading the resource status.

@eddycharly
Copy link
Member Author

@Boojapho The status of analysed resources ?

@Boojapho
Copy link

@Boojapho The status of analysed resources ?

Just realized when you asked this that Kyverno doesn't own the status fields of the analyzed resources. So, this wouldn't work.

You could achieve a similar effect using annotations on the analyzed resource. But, that would require you to have write access to all resources, so that's not going to work either.

@eddycharly
Copy link
Member Author

Yeah, basically we want a report, makes sense that it lives in it's own type.
I prefer not to bake that in the resource itself TBH.

@chipzoller
Copy link
Member

It seems that creating a 1:1 mapping of resource:report could really put a squeeze on Kubernetes' data store (etcd). To me, it still seems like splitting reports per policy (and overflowing to more if the need arises) may be more efficient.

@eddycharly
Copy link
Member Author

Splitting per policy has some advantages too but we won't be able to use k8s native garbage collection.
Let's try to list pro/cons of both approaches.

@realshuting
Copy link
Member

Splitting per policy has some advantages too but we won't be able to use k8s native garbage collection. Let's try to list pro/cons of both approaches.

Initially I thought we were going to set ownerReference for RCRs (to solve the current RCR cleanup issue), and generate a report per namespace when we discussed offline.

@eddycharly
Copy link
Member Author

eddycharly commented Sep 12, 2022

Initially I thought we were going to set ownerReference for RCRs (to solve the current RCR cleanup issue)

That's what i have now. This doesn't make a real difference for @chipzoller point though, it's still a one:one mapping.

@realshuting
Copy link
Member

realshuting commented Sep 12, 2022

Initially I thought we were going to set ownerReference for RCRs (to solve the current RCR cleanup issue)

That's what i have now. This doesn't make a real difference for @chipzoller point though, it's still a one:one mapping.

I meant to generate an RCR per resource, and merge RCRs to a single policy report for one namespace.

@eddycharly
Copy link
Member Author

It seems that creating a 1:1 mapping of resource:report could really put a squeeze on Kubernetes' data store

True but I don't know if it's an issue, when managing a large cluster you usually size the control plane adequately.

@eddycharly
Copy link
Member Author

eddycharly commented Sep 12, 2022

I meant to generate an RCR per resource, and merge RCRs to a single policy report for one namespace.

Do you expect that we remove the RCR once merged into the report ?
My initial idea was to keep it for as long as the resource exists, hence creating a 1:1 mapping.

@realshuting
Copy link
Member

I meant to generate an RCR per resource, and merge RCRs to a single policy report for one namespace.

Do you expect that we remove the RCR once merged into the report ? My initial idea was to keep it for as long as the resource exists, hence creating a 1:1 mapping.

Do you expect that we remove the RCR once merged into the report ?

No, per this proposal, the RCR shares the same lifecycle with the resource. Just that in addition to creating the RCR per resource, we will merge RCRs into one report per namespace. But this could increase the size by x2 as we duplicate the data.

@eddycharly
Copy link
Member Author

eddycharly commented Sep 12, 2022

But this could increase the size by x2 as we duplicate the data

Are you suggesting that we don't need to aggregate at the namespace level ? ;-)
In the end if we have the reports for all resources, why do we need to aggregate ?
We could just list reports instead of get, maybe just aggregate summaries ?

@Boojapho
Copy link

You should consider minimizing the Kubernetes API call load in the solution. The more you can aggregate in memory and then write to the report once, the less probability of hitting throttling issues.

@eddycharly
Copy link
Member Author

You should consider minimizing the Kubernetes API call load in the solution

Definitely.

On the other hand, The solution we have now is not perfect, we are constantly creating/deleting RCRs instead of keeping them around and updating them when necessary.

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
@chipzoller
Copy link
Member

This has already been completed, yes?

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

4 participants