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 ClusterIssuer and Issuer solvers schema #2249

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

Conversation

simonklb
Copy link
Contributor

@simonklb simonklb commented Aug 27, 2024

Warning

This is a public repository, ensure not to disclose:

  • personal data beyond what is necessary for interacting with this pull request, nor
  • business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

  • kind/feature
  • kind/improvement
  • kind/deprecation
  • kind/documentation
  • kind/clean-up
  • kind/bug
  • kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

  • kind/admin-change
  • kind/dev-change
  • kind/security
  • kind/adr

What does this PR do / why do we need this PR?

I wanted to validate my cluster issuer solver config but noticed that we did not support that. This fixes that.

Information to reviewers

The schema change is really large since we don't narrow down the solvers config. I'm not sure if this is acceptable or not. Let me know what you think!

Checklist

  • Proper commit message prefix on all commits
  • Change checks:
    • The change is transparent
    • The change is disruptive
    • The change requires no migration steps
    • The change requires migration steps
    • The change upgrades CRDs
    • The change updates the config and the schema
  • Metrics checks:
    • The metrics are still exposed and present in Grafana after the change
    • The metrics names didn't change (Grafana dashboards and Prometheus alerts are not affected)
    • The metrics names did change (Grafana dashboards and Prometheus alerts were fixed)
  • Logs checks:
    • The logs do not show any errors after the change
  • Pod Security Policy checks:
    • Any changed pod is covered by Pod Security Admission
    • Any changed pod is covered by Gatekeeper Pod Security Policies
    • The change does not cause any pods to be blocked by Pod Security Admission or Policies
  • Network Policy checks:
    • Any changed pod is covered by Network Policies
    • The change does not cause any dropped packets in the NetworkPolicy Dashboard
  • Audit checks:
    • The change does not cause any unnecessary Kubernetes audit events
    • The change requires changes to Kubernetes audit policy
  • Falco checks:
    • The change does not cause any alerts to be generated by Falco
  • Bug checks:
    • The bug fix is covered by regression tests

@aarnq
Copy link
Contributor

aarnq commented Aug 29, 2024

My thinking is that this is way to much to be able to include a resource in the schema and I will see if we can make this easier somehow.

@simonklb
Copy link
Contributor Author

My thinking is that this is way to much to be able to include a resource in the schema and I will see if we can make this easier somehow.

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

@aarnq
Copy link
Contributor

aarnq commented Aug 29, 2024

My thinking is that this is way to much to be able to include a resource in the schema and I will see if we can make this easier somehow.

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

I am on the opposite side, I want to pull in the relevant parts of the CRD as verbatim as possible and automatically update it from the chart, so it gets tracked if there are changes. 😅

@simonklb
Copy link
Contributor Author

My thinking is that this is way to much to be able to include a resource in the schema and I will see if we can make this easier somehow.

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

I am on the opposite side, I want to pull in the relevant parts of the CRD as verbatim as possible and automatically update it from the chart, so it gets tracked if there are changes. 😅

Regarding solving the schema explosion being able to directly reference upstream schemas would be fantastic.

Regarding maintainability of ck8s, keeping the config as small as possible I think would be preferable since then we can more accurately predict what users have put in their config so that we don't break it in future releases.

@aarnq
Copy link
Contributor

aarnq commented Aug 29, 2024

My thinking is that this is way to much to be able to include a resource in the schema and I will see if we can make this easier somehow.

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

I am on the opposite side, I want to pull in the relevant parts of the CRD as verbatim as possible and automatically update it from the chart, so it gets tracked if there are changes. 😅

Regarding solving the schema explosion being able to directly reference upstream schemas would be fantastic.

Regarding maintainability of ck8s, keeping the config as small as possible I think would be preferable since then we can more accurately predict what users have put in their config so that we don't break it in future releases.

Yeah, I agree, but given that we would track this schema more closely it would be easy to see the entirety of how it changes.
And it we truly don't want to support config migrations for upstream schema changes then we can always add a big caution message that says that this has to migrated manually.

@simonklb
Copy link
Contributor Author

My thinking is that this is way to much to be able to include a resource in the schema and I will see if we can make this easier somehow.

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

I am on the opposite side, I want to pull in the relevant parts of the CRD as verbatim as possible and automatically update it from the chart, so it gets tracked if there are changes. 😅

Regarding solving the schema explosion being able to directly reference upstream schemas would be fantastic.
Regarding maintainability of ck8s, keeping the config as small as possible I think would be preferable since then we can more accurately predict what users have put in their config so that we don't break it in future releases.

Yeah, I agree, but given that we would track this schema more closely it would be easy to see the entirety of how it changes. And it we truly don't want to support config migrations for upstream schema changes then we can always add a big caution message that says that this has to migrated manually.

I'm good with that! At least if we keep track of what parameters are available in the schema it will be noticable. I would love to see a big red CAUTION: THIS IS OUT OF OUR HAND in front of all "additional properties", "extra config" and what not where everything goes. 😄

@Zash
Copy link
Contributor

Zash commented Aug 29, 2024

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers.

Simplifying config, reducing complexity is nice.

I want to pull in the relevant parts of the CRD as verbatim as possible and automatically update it from the chart, so it gets tracked if there are changes. 😅

Makes sense where we do pass things through as-is.

@Elias-elastisys
Copy link
Contributor

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

I like the idea of this, trying to keep the possible config as small as possible. But I feel as if we would need to do the same then for every other part of config that accepts arbitrary yaml (extra args etc) which I feel would make the schema explode and hard to maintain but also might lock us in too much. Since we rely so much on upstream I worry it might make chart upgrades and other upstream changes more difficult to deal with, resulting in us avoiding or delaying upgrades and falling behind.

@simonklb
Copy link
Contributor Author

simonklb commented Sep 2, 2024

I'd love to see us reducing the different possible config options here to only what we know and support instead of everything that cert-manager allows in solvers. Should hopefully be easier to know what to add tests for and realize when something breaks upstream.

I like the idea of this, trying to keep the possible config as small as possible. But I feel as if we would need to do the same then for every other part of config that accepts arbitrary yaml (extra args etc) which I feel would make the schema explode and hard to maintain but also might lock us in too much. Since we rely so much on upstream I worry it might make chart upgrades and other upstream changes more difficult to deal with, resulting in us avoiding or delaying upgrades and falling behind.

The counter-point to this is that for every part of the config that accepts arbitrary yaml we have no idea what is being set by the user. That means that when we upgrade something we risk breaking deployments without having a migration path.

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.

4 participants