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

PolicyServer CRD is missing some validation #1016

Open
1 task done
flavio opened this issue Feb 25, 2025 · 5 comments
Open
1 task done

PolicyServer CRD is missing some validation #1016

flavio opened this issue Feb 25, 2025 · 5 comments

Comments

@flavio
Copy link
Member

flavio commented Feb 25, 2025

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

It's currently possible to create a PolicyServer resource that looks like that:

apiVersion: policies.kubewarden.io/v1
kind: PolicyServer
metadata:
  name: flavio
spec:
  env:
    - name: KUBEWARDEN_LOG_LEVEL
      value: info
  replicas: 1
  serviceAccountName: policy-server

This PolicyServer instance does not specify which image has to be used (spec.image is omitted).

The PolicyServer is created, but the controller will keep failing while trying to reconcile the Deployment resource associated with it:

│ {"level":"error","ts":"2025-02-25T13:09:38Z","msg":"Reconciler error","controller":"policyserver","controllerGroup":"policies.kubewarden.io","controllerKind":"PolicyServer","PolicyServer":{"name":"flavio"},"namespace":"","name":"flavio │
│ ","reconcileID":"0d08e51e-4b31-4835-b710-b9643e9e7289","error":"error reconciling policy-server deployment: Deployment.apps \"policy-server-flavio\" is invalid: spec.template.spec.containers[0].image: Required value","stacktrace":"sigs │
│ .k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:341\nsigs.k8s.io/controller-runtime/pkg/internal/control │
│ ler.(*Controller[...]).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:288\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2\n\t/go/ │
│ pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:249"}

Expected Behavior

No response

Steps To Reproduce

  • Deploy the Kubewarden 1.22 stack
  • Crate a PolicyServer resource like the one described above
  • Look at the logs of the kubewarden-controller pod

The Deployment associated to this PolicyServer is never created.

Environment

- Any version of Kubewarden

Anything else?

We have to decide which approach we should take. I see two possible solutions:

  • Make spec.image mandatory and reject the creation of the PolicyServer resource if the mandatory fields. While we're at that, let's make sure all the mandatory fields are properly configured
  • Leave spec.image optional, let the controller fill the value using the latest stable release of the policy-server image.

The first solution is the easiest one.

The second solution has some pros and cons:

  • Pros:
    • The controller takes care of upgrading custom PolicyServer instances to use latest stable release of PolicyServer. That grants the user does not end up with a mixed - and unsupported scenario - like having the kubewarden-controller and the custom PolicyServer versions drifted too far away
  • Cons:
    • Maybe the user doesn't want this automatic upgrade to happen behind their back. I doubt that's the case, especially since we require the controller and PolicyServer versions to be in sync
    • How does the controller determines which is the default container image to be used? I would not hard code that into the source code, maybe we can use a ConfigMap to store that value. The ConfigMap would be created by the kubewarden-controller helm chart
    • Every time we do a patch release of the PolicyServer we have to remember to update the stable version string that is put into the ConfigMap
@fabriziosestito
Copy link
Contributor

@flavio

How does the controller determines which is the default container image to be used? I would not hard code that into the source code, maybe we can use a ConfigMap to store that value. The ConfigMap would be created by the kubewarden-controller helm chart

What are the advantages of doing this?
I was about to propose using ld-flags to inject the current tag version into the binary.

@flavio
Copy link
Member Author

flavio commented Feb 25, 2025

Take the recent issue with the broken metadata inside of Sigstore's TUF repository. We had to do a patch release of PolicyServer, but we didn't have to touch the controller.
Unless you want to pass the whole policy-server version as a ld-flag (I thought you wanted to just reuse the version of the controller).

However, I would prefer to have a constant defined somewhere clear inside of the source code, instead of injecting something at build time. It's less "magical", everything is clearly found when editing the source code (the IDE can bring you straight to the constant that defines the image of Policy Server to be used).

The approach that relies on the ConfigMap requires a change only inside of the helm chart (no rebuilding of the controller image). We could extend the current automation we have in place for the helm chart bumps to take the contents of this ConfigMap into account.

@jvanz
Copy link
Member

jvanz commented Feb 25, 2025

Considering that we have statement that the patch versions are free from among the components of the stack (controller, kwctl, etc). I think adding the latest version in the controller code or injected maybe not the best solution. Otherwise, everytime we patch policy server, for any reason like a security issue, also means updating the controller. Witch remove the advantage of bumping the policy server independently.

@fabriziosestito
Copy link
Contributor

Ok good point.

Maybe the user doesn't want this automatic upgrade to happen behind their back. I doubt that's the case, especially since we require the controller and PolicyServer versions to be in sync

This will not happen if we use the webhook defaulter.

@jvanz
Copy link
Member

jvanz commented Feb 25, 2025

Cannot we have just a controller CLI flag with the default policy server version to be used in case the user miss to set it in the resource definition? We can even add another CLI flag to say "please, also upgrade to this version the running policy servers"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

3 participants