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.

Sign up for GitHub

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

Jump to bottom

[Add] CEL rule for validating App and PackageInstall Spec #1382

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

[Add] CEL rule for validating App and PackageInstall Spec #1382

wants to merge 1 commit into from

Conversation

varshaprasad96
Copy link

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.:

@prembhaskal
Copy link
Contributor

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
prembhaskal reviewed Oct 28, 2023
View reviewed changes
config/config/crds.yml Outdated Show resolved Hide resolved
praveenrewar
praveenrewar reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

@varshaprasad96 Could you run './hack/build.sh', './hack/gen.sh', and './hack/gen-apiserver.sh' and add the changes to the generated files as well.
With these changes,

  • we will error out when serviceAccount and cluster both are not provided (✅ )
  • we will error out when both serviceAccount and cluster are provided (Even though this is how it should be, like @prembhaskal mentioned this might be a breaking change for some folks, for example if they have an App CR where they have mentioned both serviceAccount and cluster, then they will start seeing this error when they upgrade to the newer version of kc.)

Is current behaviour to choose serviceAccount over cluster config by design?

I am not sure if this was intended for long term, but yes, this is the current expected behaviour (I think it's mentioned somewhere in the docs as well)

cc @neil-hickey @joaopapereira

config/config/crds.yml Outdated Show resolved Hide resolved
@varshaprasad96 varshaprasad96 changed the title [Add] CEL rule for validating App Spec [Add] CEL rule for validating App and PackageInstall Spec Oct 30, 2023
@varshaprasad96
Copy link
Author

varshaprasad96 commented Oct 30, 2023 •
edited

@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 varshaprasad96 force-pushed the add/app-spec-validation branch 2 times, most recently from a1cb874 to 821176c Compare November 1, 2023 18:06
praveenrewar
praveenrewar reviewed Nov 2, 2023
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

Thank you so much for the PR @varshaprasad96. The changes look good to me, could you also add a quick e2e test here.

prembhaskal
prembhaskal reviewed Nov 2, 2023
View reviewed changes
Copy link
Contributor

@prembhaskal prembhaskal left a comment

Choose a reason for hiding this comment

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

Changes look good.

prembhaskal
prembhaskal previously approved these changes Nov 2, 2023
View reviewed changes
@prembhaskal prembhaskal dismissed their stale review November 2, 2023 13:38

I saw praveen's comment late.

praveenrewar
praveenrewar reviewed Nov 8, 2023
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

(A few minor changes)

test/e2e/kappcontroller/packageinstall_test.go Outdated Show resolved Hide resolved
test/e2e/kappcontroller/packageinstall_test.go Outdated Show resolved Hide resolved
test/e2e/kappcontroller/packageinstall_test.go Outdated Show resolved Hide resolved
@prembhaskal
Copy link
Contributor

prembhaskal commented Nov 13, 2023 •
edited

all seems fine.
Well test is failing actually.

@prembhaskal
Copy link
Contributor

I think test is failing because k8s version of minikube is set to 1.20 which does not support CEL validation.

kapp-controller/.github/workflows/test-gh.yml

Line 49 in b4886df

minikube start --driver=docker --kubernetes-version="1.20.15"

@praveenrewar
Copy link
Member

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 praveenrewar linked an issue Nov 24, 2023 that may be closed by this pull request
App CR should require one of serviceAccountName or cluster. #598
Open
@varshaprasad96
[Add] CEL rules for validating App and PackageInstall Spec
38f6e7b 
This PR:
- Adds KB marker to ensure that either spec.ServiceAccount
  or spec.Cluster is present in App and PackageInstall CR.
- Bumps controller-tools to 0.12.1 to support CEL based
  validation marker. That is the latest version compatible with
  the k8s release the project is currently at.
- Adds test to validate the logic introduced with the kubebuilder
  marker.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@varshaprasad96
Copy link
Author

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.

@praveenrewar
Copy link
Member

@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).

@prembhaskal
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@praveenrewar praveenrewar Awaiting requested review from praveenrewar

@prembhaskal prembhaskal Awaiting requested review from prembhaskal

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Carvel
Status: In Progress
Milestone
No milestone
3 participants
@varshaprasad96 @prembhaskal @praveenrewar