Skip to content

Conversation

Shreyas-s14
Copy link
Contributor

@Shreyas-s14 Shreyas-s14 commented Feb 3, 2025

How to categorize this PR?

/area quality
/area control-plane
/area open-source
/kind enhancement

What this PR does / why we need it:

  • Performs validations for the etcd resource via kubebuilder markers, using a combination of CEL Expressions via the x-validation and the validation markers.
  • Verifies the effectiveness of the validation rules using integration tests.
  • To support k8s clusters < 1.29 and >=1.29 it generates 2 versions of CRDs one with CEL expressions and one without.
  • api/core/crds is changed to now have API methods which takes in a k8s version to provide the appropriate CRD.
  • Enhances make deploy-* and make prepare-helm-chart targets to work with CRDs with and without CEL expressions.
  • Updates the documentation to include instructions on how to properly use the CRDs when setting up etcd-druid.
  • Adapts IT tests setup to take the correct CRDs.

Which issue(s) this PR fixes:
Fixes #989

Special notes for your reviewer:

Release note:

Introduces 'Etcd` custom resource validations using CEL expressions. This will be available for kubernetes clusters with version >= 1.29 only. For kubernetes clusters with version < 1.29, `Etcd` CRD will not contain validations using CEL expressions.
Programatic access to the CRDs via the API is available using the `api/core/crds/GetAll()` function, which now accepts k8sVersion as a parameter, in order to return the correct CRD, either with CEL validations for k8s versions v1.29 and above, or without CEL validation for k8s versions below 1.29.
The field `etcd.spec.selector` field is now deprecated.

@Shreyas-s14 Shreyas-s14 requested a review from a team as a code owner February 3, 2025 20:49
@gardener-robot gardener-robot added needs/review Needs review area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/enhancement Enhancement, improvement, extension labels Feb 3, 2025
@gardener-robot
Copy link

@Shreyas-s14 Thank you for your contribution.

@gardener-robot gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Feb 3, 2025
@gardener-robot-ci-1
Copy link
Contributor

Thank you @Shreyas-s14 for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@shreyas-s-rao shreyas-s-rao added this to the v0.28.0 milestone Feb 4, 2025
Copy link
Contributor

@seshachalam-yv seshachalam-yv 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, @Shreyas-s14, for adding the validations for the etcd CR. I've provided a few initial review comments based on my first glance. I will share more detailed feedback after a thorough review.

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Feb 5, 2025
@Shreyas-s14
Copy link
Contributor Author

/test pull-etcd-druid-e2e-kind

@shreyas-s-rao shreyas-s-rao self-assigned this Feb 24, 2025
Shreyas-s14 and others added 11 commits February 27, 2025 10:43
* Added `itTestEnv.StartManager()` to start the manager in `setupTestEnvironment()`.

* Upgraded teh env-test k8s version to `1.30`.

* Changed the timeout to `20s` in `assertReconcileSuspensionEventRecorded`
  to prevent the test timing out.

* Removed an unnecessary `time.Sleep` in `TestValidateSpecReplicas`.

* Uncommented previously commented options which caused regressions.
2) added version compatibility docs
3) reduced code reuse in specUpdate_test.go
1) increased value of timeout passed in test/it/helper.go createAndAssertEtcdAndAllManagedResources
2) increased the timeout in test/e2e/suite_test.go to 20 minutes each
Reverted the timeout value in test/it/helper.go which was increased to handle failure of prow jobs
@gardener-robot gardener-robot removed the needs/second-opinion Needs second review by someone else label Mar 7, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 7, 2025
Copy link
Contributor

@seshachalam-yv seshachalam-yv 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, @Shreyas-s14, for addressing my review comments.
/lgtm

@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Mar 8, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 8, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 8, 2025
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@unmarshall thanks for the fix.
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Mar 9, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 9, 2025
Copy link
Contributor

@unmarshall unmarshall left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Mar 10, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 10, 2025
Copy link
Contributor

@seshachalam-yv seshachalam-yv left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Mar 10, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 10, 2025
@unmarshall unmarshall merged commit c083042 into gardener:master Mar 10, 2025
15 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRD Validation for ETCD
9 participants