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

Add v1alpha2 Hardware validating admission webhook #675

Merged
merged 1 commit into from
May 4, 2023

Conversation

chrisdoherty4
Copy link
Member

@chrisdoherty4 chrisdoherty4 commented Mar 21, 2023

Add a validating admission webhook to perform complex Hardware validation that can't be performed with CEL.

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #675 (85f430d) into main (906da94) will increase coverage by 3.43%.
The diff coverage is 75.78%.

❗ Current head 85f430d differs from pull request most recent head d8517c0. Consider uploading reports for the commit d8517c0 to get more accurate results

@@            Coverage Diff             @@
##             main     #675      +/-   ##
==========================================
+ Coverage   49.30%   52.73%   +3.43%     
==========================================
  Files          18       23       +5     
  Lines         858      986     +128     
==========================================
+ Hits          423      520      +97     
- Misses        427      454      +27     
- Partials        8       12       +4     
Impacted Files Coverage Δ
internal/hardware/admission.go 37.20% <37.20%> (ø)
internal/hardware/admission_ip.go 92.85% <92.85%> (ø)
internal/hardware/admission_mac.go 93.93% <93.93%> (ø)
internal/hardware/admission_conditional.go 100.00% <100.00%> (ø)
internal/hardware/duplicate.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@chrisdoherty4 chrisdoherty4 force-pushed the feature/webhooks branch 6 times, most recently from f6dd120 to 6990bed Compare March 22, 2023 13:18
@chrisdoherty4 chrisdoherty4 force-pushed the feature/webhooks branch 9 times, most recently from b55c6c1 to 673deef Compare March 24, 2023 02:40
@chrisdoherty4 chrisdoherty4 force-pushed the feature/webhooks branch 3 times, most recently from a82b532 to def680f Compare March 25, 2023 17:30
@chrisdoherty4 chrisdoherty4 marked this pull request as ready for review March 25, 2023 17:30
@chrisdoherty4 chrisdoherty4 added the ready-to-merge Signal to Mergify to merge the PR. label Mar 25, 2023
@chrisdoherty4 chrisdoherty4 changed the title Add v1alpha2 Hardware webhook Add v1alpha2 Hardware validating admission webhook Mar 25, 2023
@chrisdoherty4 chrisdoherty4 force-pushed the feature/webhooks branch 2 times, most recently from b5b5ed7 to 710d440 Compare March 31, 2023 02:08
Copy link
Member

@jacobweinstock jacobweinstock left a comment

Choose a reason for hiding this comment

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

Thanks @chrisdoherty4. Question, how is this deployed?

api/v1alpha2/hardware.go Outdated Show resolved Hide resolved
internal/hardware/admission.go Outdated Show resolved Hide resolved
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)

func (a *Admission) validateMACs(hw *v1alpha2.Hardware) admission.Response {
Copy link
Member

Choose a reason for hiding this comment

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

Could a cel validation be used instead? +kubebuilder:validation:XValidation:rule= https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we can run regex using the XValidation rule - I can't find anything in the link or other resources that shows how.

Even if it could, I'm not sure it would work for map keys. We have CEL validation configured on the map key type which I'd hoped would translate to applying the regex, but it seems the open API spec supported by the CustomResourceDefinition kind doesn't support validation of map keys (not that I've found yet anyway).

@chrisdoherty4
Copy link
Member Author

Thanks @chrisdoherty4. Question, how is this deployed?

Great question.

This will be registered with the new controller manager component (we already have a manager under /internal/controllers). The manager is a controller-runtime data structure that handles initialization of controllers, webhooks, clients and has responsibilities such as leader election etc.

The Admission object satisfies the Handler interface that is consumed by the Webhook data structure. The Webhook is registerable, as an http.Handler, with the webhook.Server. You can see this registration happening in the SetupWithManager function. The webhook.Server is 1 of the structures managed by the manager.

An example of how this is plugged together can be seen in the kubebuilder book.

Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>
@mergify mergify bot merged commit 4583d33 into tinkerbell:main May 4, 2023
@chrisdoherty4 chrisdoherty4 deleted the feature/webhooks branch May 15, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants