feat: validate crd versions #1919

sadath-12 · 2023-12-28T09:15:44Z

Issue

This pr Closes #1412

Description

If there is a change in the crd yaml files and if CustomResourceDefinitionSchemaVersion value is not updated accordingly in pkg/k8s/apis/cilium.io/v1alpha1/version.go or vice-versa . The CI will fail

How to test locally ?

follow this #1919 (review)

for the event.json thing set it as

  "pull_request": {
    "head": {
      "ref": "test-dev-2"
    },
    "base": {
      "ref": "test-dev",
      "sha":"58acf63b51238a93cb2d13cba90a80d6662d45bd"
    }
  }
        }

you can replace the sha field by the latest commit on your base branch

here I added CustomResourceDefinitionSchemaVersion to pkg/k8s/apis/cilium.io/v1alpha1/version.go in both branches

and changed yaml files pkg/k8s/apis/cilium.io/v1alpha1/version.go and CustomResourceDefinitionSchemaVersion in HEAD branch (test-dev-2)

change the branch

git checkout test-dev-2 (the branch that makes the pr request because we have HEAD in the yaml)

then you should see the success output as I saw

[Check CRD version update/check-version] ⭐ Run Main actions/checkout@v3
[Check CRD version update/check-version]   🐳  docker cp src=/home/sadath/go/src/tetragon/. dst=/home/sadath/go/src/tetragon
[Check CRD version update/check-version]   ✅  Success - Main actions/checkout@v3
[Check CRD version update/check-version] ⭐ Run Main Check for CRD changes and version update
[Check CRD version update/check-version]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/2] user= workdir=
| old_version=0.10.1
| new_version=0.10.4
| crd_changed=1
| version_changed=1
[Check CRD version update/check-version]   ✅  Success - Main Check for CRD changes and version update
[Check CRD version update/check-version] Cleaning up container for job check-version
[Check CRD version update/check-version] 🏁  Job succeeded

if you changed crd yaml files but not version you will see the failure output something as I see here

[Check CRD version update/check-version]   ✅  Success - Main Install git
[Check CRD version update/check-version] ⭐ Run Main actions/checkout@v3
[Check CRD version update/check-version]   🐳  docker cp src=/home/sadath/go/src/tetragon/. dst=/home/sadath/go/src/tetragon
[Check CRD version update/check-version]   ✅  Success - Main actions/checkout@v3
[Check CRD version update/check-version] ⭐ Run Main Check for CRD changes and version update
[Check CRD version update/check-version]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/2] user= workdir=
| old_version=0.10.1
| new_version=0.10.1
| CRD changed but version not updated
[Check CRD version update/check-version]   ❌  Failure - Main Check for CRD changes and version update
[Check CRD version update/check-version] exitcode '1': failure
[Check CRD version update/check-version] 🏁  Job failed
Error: Job 'check-version' failed

And for vice-versa

[Check CRD version update/check-version] ⭐ Run Main actions/checkout@v3
[Check CRD version update/check-version]   🐳  docker cp src=/home/sadath/go/src/tetragon/. dst=/home/sadath/go/src/tetragon
[Check CRD version update/check-version]   ✅  Success - Main actions/checkout@v3
[Check CRD version update/check-version] ⭐ Run Main Check for CRD changes and version update
[Check CRD version update/check-version]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/2] user= workdir=
| old_version=0.10.1
| new_version=0.10.7
| Version updated but CRD not changed
[Check CRD version update/check-version]   ❌  Failure - Main Check for CRD changes and version update
[Check CRD version update/check-version] exitcode '1': failure
[Check CRD version update/check-version] 🏁  Job failed
Error: Job 'check-version' failed

lambdanis

Thank you for tackling this @sadath-12.

Ideally I would imagine this check as a Makefile rule, so that we can run it in CI, but also when validating changes locally. Can you define such a rule in this PR?

Please also squash the commits, so that the git history is more readable :)

.github/workflows/validate-crd.yaml

sadath-12 · 2024-01-03T17:06:33Z

Thank you for tackling this @sadath-12.

Ideally I would imagine this check as a Makefile rule, so that we can run it in CI, but also when validating changes locally. Can you define such a rule in this PR?

Please also squash the commits, so that the git history is more readable :)

can we check the commit difference in makefile ?

lambdanis · 2024-01-04T13:30:43Z

can we check the commit difference in makefile ?

Good question - we could, but it would be likely confusing what we're actually checking :) Maybe it's better to leave it as is.

.github/workflows/validate-crd.yaml

netlify · 2024-01-08T17:16:51Z

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	`d89d947`
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/659c2dec72f43100089f404a
😎 Deploy Preview	https://deploy-preview-1919--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sadath-12 · 2024-01-08T18:14:29Z

Updated with other action

kkourt

Thank you for taking on this issue!
I wanted to provide a starting point on how you can start solving this issue.

First, let's create a simple workflow. I'll start with a clean repo to make things easier.

$ git clone [email protected]:cilium/tetragon.git
$ cd tetragon
$ git switch -c crd-validation

Let's add a starting point for the action:

$ curl -s https://gist.githubusercontent.com/kkourt/f886b0a4dff79b472a179c9200205c16/raw/c4c3d1cf35c90a917e514e8ebd1b3078a0ca7bc0/validate-crd.yaml | tee .github/workflows/validate-crd.yaml

You can see the file here: https://gist.github.com/kkourt/f886b0a4dff79b472a179c9200205c16

We can now use acct to execute the action. First, we need to properly set the base and head refs (see https://nektosact.com/beginner/index.html#using-event-file-to-provide-complete-event-payload).

To do so we add and events.json file


  "pull_request": {
    "head": {
      "ref": "crd-validation"
    },
    "base": {
      "ref": "origin/main"
    }
  }
}

Now we can use acct:

$ gh act -W .github/workflows/validate-crd.yaml --eventpath event.json

Which will result in:

| origin/main
| Differences in yaml files:
| Differences in schema:

Since there are no differences.
We can start simple by creating the differences. Here's a simple commit that modifies the yaml file:

commit 9a90ffbd8063a4f1ba8caa89fc315566ba232f3a (HEAD -> crd-validation)
Author: Kornilios Kourtis <[email protected]>
Date:   Tue Jan 9 13:51:27 2024 +0100

    dummy
    
    Signed-off-by: Kornilios Kourtis <[email protected]>

diff --git a/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.yaml b/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.yaml
index 31ad2495..b95412aa 100644
--- a/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.yaml
+++ b/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.yaml
@@ -1390,6 +1390,7 @@ spec:
                                   - State
                                   - InMap
                                   - NotInMap
+                                  - IsPizza
                                   type: string
                                 values:
                                   description: Value to compare the argument against.

Now running act, we get:

| Differences in yaml files:
| diff --git a/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpol
| icies.yaml b/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolici
| es.yaml
| index b95412aa..31ad2495 100644
| --- a/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.y
| aml
| +++ b/pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.y
| aml
| @@ -1390,7 +1390,6 @@ spec:
|                                    - State
|                                    - InMap
|                                    - NotInMap
| -                                  - IsPizza
|                                    type: string
|                                  values:
|                                    description: Value to compare the argument ag
| ainst.
| Differences in schema:

So we can see that there are differences in the yaml files.

Please use above a starting point to develop a solution. Once finished, please provide the necessary steps for folk reviewing this PR to test the PR using acct for the two cases that we need covered: i) changes in the resources but no changes in the version, and ii) changes in both the resources and the versions. In (i) the action should fail, and in (ii) the action should succeed.

.github/workflows/validate-crd.yaml

sadath-12 · 2024-01-09T15:38:22Z

wow this really needs to be appreciated thanks @kkourt for taking time and finding it valuable to explain me. ill work on it

sadath-12 · 2024-01-09T18:36:39Z

Thank you for helping me improve @kkourt . Updated the changes and guide in description

lambdanis

Thanks for the updates, I left some comments regarding UX.

.github/workflows/validate-crd.yaml

sadath-12 · 2024-01-10T14:30:55Z

Thank you @lambdanis for improving

willfindlay

LGTM but we should make sure to test this locally using act before merging.

.github/workflows/validate-crd.yaml

If there is a change in the crd yaml files and if CustomResourceDefinitionSchemaVersion value is not updated accordingly in pkg/k8s/apis/cilium.io/v1alpha1/register.go or vice-versa . The CI will fail Signed-off-by: sadath-12 <[email protected]>

kkourt

LGTM, thanks!

kkourt · 2024-01-31T10:51:21Z

failures seem unrelated, so merging.

sadath-12 · 2024-01-31T15:19:33Z

@kkourt issue closed but this pr not merged

sadath-12 requested review from willfindlay and a team as code owners December 28, 2023 09:15

lambdanis added area/ci Related to CI release-note/ci This PR makes changes to the CI. labels Jan 3, 2024

lambdanis reviewed Jan 3, 2024

View reviewed changes

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

lambdanis reviewed Jan 4, 2024

View reviewed changes

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

sadath-12 force-pushed the validate-crd branch 3 times, most recently from cbb92be to d89d947 Compare January 8, 2024 17:16

kkourt reviewed Jan 9, 2024

View reviewed changes

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

.github/workflows/validate-crd.yaml Show resolved Hide resolved

sadath-12 force-pushed the validate-crd branch from d89d947 to 82c7f63 Compare January 9, 2024 18:35

lambdanis reviewed Jan 10, 2024

View reviewed changes

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

sadath-12 force-pushed the validate-crd branch from 82c7f63 to ea0b42e Compare January 10, 2024 14:30

willfindlay approved these changes Jan 18, 2024

View reviewed changes

sadath-12 requested a review from kkourt January 18, 2024 15:45

kkourt requested changes Jan 24, 2024

View reviewed changes

.github/workflows/validate-crd.yaml Outdated Show resolved Hide resolved

CI: validate-crd

9ad6135

If there is a change in the crd yaml files and if CustomResourceDefinitionSchemaVersion value is not updated accordingly in pkg/k8s/apis/cilium.io/v1alpha1/register.go or vice-versa . The CI will fail Signed-off-by: sadath-12 <[email protected]>

sadath-12 force-pushed the validate-crd branch from ea0b42e to 9ad6135 Compare January 24, 2024 09:30

sadath-12 requested a review from kkourt January 24, 2024 09:31

kkourt approved these changes Jan 31, 2024

View reviewed changes

kkourt mentioned this pull request Jan 31, 2024

CI: check that the CRD version is updated #1412

Closed

kkourt merged commit d9d4946 into cilium:main Jan 31, 2024
28 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: validate crd versions #1919

feat: validate crd versions #1919

sadath-12 commented Dec 28, 2023 •

edited

Loading

lambdanis left a comment

sadath-12 commented Jan 3, 2024

lambdanis commented Jan 4, 2024

netlify bot commented Jan 8, 2024

sadath-12 commented Jan 8, 2024

kkourt left a comment

sadath-12 commented Jan 9, 2024 •

edited

Loading

sadath-12 commented Jan 9, 2024 •

edited

Loading

lambdanis left a comment

sadath-12 commented Jan 10, 2024

willfindlay left a comment

kkourt left a comment

kkourt commented Jan 31, 2024

sadath-12 commented Jan 31, 2024

feat: validate crd versions #1919

feat: validate crd versions #1919

Conversation

sadath-12 commented Dec 28, 2023 • edited Loading

Issue

Description

lambdanis left a comment

Choose a reason for hiding this comment

sadath-12 commented Jan 3, 2024

lambdanis commented Jan 4, 2024

netlify bot commented Jan 8, 2024

✅ Deploy Preview for tetragon ready!

sadath-12 commented Jan 8, 2024

kkourt left a comment

Choose a reason for hiding this comment

sadath-12 commented Jan 9, 2024 • edited Loading

sadath-12 commented Jan 9, 2024 • edited Loading

lambdanis left a comment

Choose a reason for hiding this comment

sadath-12 commented Jan 10, 2024

willfindlay left a comment

Choose a reason for hiding this comment

kkourt left a comment

Choose a reason for hiding this comment

kkourt commented Jan 31, 2024

sadath-12 commented Jan 31, 2024

sadath-12 commented Dec 28, 2023 •

edited

Loading

sadath-12 commented Jan 9, 2024 •

edited

Loading

sadath-12 commented Jan 9, 2024 •

edited

Loading