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

feat(webhook): add webhook to validate kepler resource #362

Merged

Conversation

sthaha
Copy link
Collaborator

@sthaha sthaha commented Feb 22, 2024

Previously, when user created kepler with name other kepler, k8s API server would accept the resource and create it. The operator then updates the status of the resource to be invalid. This is now changed by adding Validating webhook that rejects the resource if the name does not match kepler.

@sthaha
Copy link
Collaborator Author

sthaha commented Feb 22, 2024

https://github.com/sustainable-computing-io/kepler-operator/actions/runs/7998493408/job/21844787506?pr=362#step:8:780
Nice!

@sthaha sthaha force-pushed the feat-validate-kepler branch 2 times, most recently from 0dc7a28 to 7253831 Compare February 26, 2024 04:45
Previously, when user created kepler with name other `kepler`, k8s API
server would accept the resource and create it. The operator then
updates the status of the resource to be `invalid`. This is now changed
by adding Validating webhook that rejects the resource if the name does
not match `kepler`.

Signed-off-by: Sunil Thaha <sthaha@redhat.com>
@sthaha sthaha force-pushed the feat-validate-kepler branch from 7253831 to 16dce4f Compare February 26, 2024 05:06
@sthaha sthaha requested a review from vimalk78 February 27, 2024 02:27
@@ -416,6 +416,24 @@ ensure_deploy_img_is_always_pulled() {
ok "Operator deployment imagePullPolicy is Always"
}

reject_invalid() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we add this to our e2e tests so that It could be used downstream?

Copy link
Collaborator

@vimalk78 vimalk78 left a comment

Choose a reason for hiding this comment

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

/lgtm

KeplerInstanceName = "kepler"
)

// log is for logging in this package.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: keplerlog is for logging in this package
or should rename it to webhookLogger ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

auto-generated :)
but I think there is a point to calling it keplerLog since we will create the same for keplerInternal_webhook

func (r *Kepler) Default() {
keplerlog.Info("default", "name", r.Name)

// TODO(user): fill in your defaulting logic.
Copy link
Collaborator

Choose a reason for hiding this comment

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

any defaults we want to add? what is the guideline for defaults? use kubebuilder based defaults or a webhook?
perhaps Defaulter webhook is for complex apis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, as much as possible, lets rely only CRD defaults. And use this for complex cases where CRD defaults don't work.

@vprashar2929
Copy link
Collaborator

Getting error while creating Kepler on OpenShift
Screenshot 2024-02-27 at 4 52 38 PM

@vprashar2929
Copy link
Collaborator

Tested again on fresh OpenShift cluster and validating webhooks are working as expected.

@vimalk78 vimalk78 merged commit 4d2756f into sustainable-computing-io:v1alpha1 Mar 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants