kapp-controller icon indicating copy to clipboard operation
kapp-controller copied to clipboard
verified publisher icon carvel-dev

[Add] CEL rule for validating App and PackageInstall Spec

Open varshaprasad96 opened this issue 2 years ago • 8 comments

What this PR does / why we need it:

This PR:

  • Adds KB marker to ensure that either spec.ServiceAccount or spec.Cluster is present in App CR.
  • Bumps controller-tools to 0.10.0 to support CEL based validation marker. That is the latest version compatible with the k8s release the project is currently at.

Which issue(s) this PR fixes:

Fixes #1380

Does this PR introduce a user-facing change?

No. The validation already exists, this change performs the same action before the resource is created.

Additional Notes for your reviewer:

Review Checklist:
  • [ ] Follows the developer guidelines
  • [ ] Relevant tests are added or updated
  • [ ] Relevant docs in this repo added or updated
  • [ ] Relevant carvel.dev docs added or updated in a separate PR and there's a link to that PR
  • [ ] Code is at least as readable and maintainable as it was before this change

Additional documentation e.g., Proposal, usage docs, etc.:

varshaprasad96 avatar Oct 27 '23 22:10 varshaprasad96

one comment based on my first glance. currently if both serviceAccount and cluster are specified in a PackageInstall CRD, the serviceAccount is read and used. if none are specified, we get an error.

so validation in PR might break some existing configuration. @praveenrewar @100mik Is current behaviour to choose serviceAccount over cluster config by design?

prembhaskal avatar Oct 28 '23 10:10 prembhaskal

@prembhaskal @praveenrewar I've modified the validation to just check the existence of atleast one of it. This should retain the current behaviour and fix the failing tests.

varshaprasad96 avatar Oct 30 '23 22:10 varshaprasad96

~~all seems fine.~~ Well test is failing actually.

prembhaskal avatar Nov 13 '23 02:11 prembhaskal

I think test is failing because k8s version of minikube is set to 1.20 which does not support CEL validation. https://github.com/carvel-dev/kapp-controller/blob/b4886dfaff8050fc7221e1d7bce9e69e2a4264e3/.github/workflows/test-gh.yml#L49

prembhaskal avatar Nov 15 '23 08:11 prembhaskal

We could bump k8s version in the tests, but I don't want us to indicate that we don't support any version before 1.26. Since the kind tests with the latest version are passing, I think we can add a check in this test to skip if k8s version is <1.26.x (as the validation won't work on older versions but there shouldn't be any other issue because of it).

praveenrewar avatar Nov 17 '23 11:11 praveenrewar

Sorry for not being able to follow up due to other commitments. The recent changes should skip the test if the server version is less than 1.26.

varshaprasad96 avatar Dec 19 '23 13:12 varshaprasad96

@varshaprasad96 The test seems to be failing while getting the server version (I haven't taken a closer look so not sure exactly which part).

praveenrewar avatar Dec 27 '23 12:12 praveenrewar

cel_validation_test.go:26: Error getting Kubernetes version: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined

failing because the test code is not running on the kind cluster.

prembhaskal avatar Dec 29 '23 06:12 prembhaskal