feat: webhook: Check if DataImportCronTemplate is correct #1162

akrejcir · 2024-12-05T12:14:45Z

What this PR does / why we need it:
Added webhook code to check if DataImportCronTemplate is correct.
The code is copied from CDI webhook. They don't export the code, so we cannot import it as a module.

Which issue(s) this PR fixes:
Fixes: https://issues.redhat.com/browse/CNV-48792

Release note:

None

kubevirt-bot · 2024-12-05T12:14:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from akrejcir. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akrejcir · 2024-12-05T12:15:34Z

/cc @0xFelix @jcanocan @codingben

codingben · 2024-12-05T20:03:37Z

The code is copied from CDI webhook. They don't export the code, so we cannot import it as a module.

Isn't exportable at all? What would be @akalenyu's opinion about this?

0xFelix · 2024-12-06T08:18:43Z

webhooks/data-import-cron.go

+func validateNumberOfSources(source *cdiv1beta1.DataVolumeSource) error {
+	numberOfSources := 0
+	s := reflect.ValueOf(source).Elem()
+	for i := 0; i < s.NumField(); i++ {


nit: Use for i := range s.NumField()

Done.
TIL that range can have int argument. I assumed only slices and maps can be there.

But golangci-lint complains about it:

golangci/golangci-lint info installed /workspace/bin/golangci-lint /workspace/bin/golangci-lint run --timeout 5m webhooks/data-import-cron.go:170:17: cannot range over s.NumField() (value of type int) (typecheck) for i := range s.NumField() { ^ make: *** [Makefile:327: lint] Error 1

Even if it compiles and works: https://go.dev/play/p/KbogyWbaifX?v=goprev

Maybe we need to update the linter. For now, I will revert it.

IIRC that was introduced with golang 1.22. :)

Maybe the problem is that the linter is already a bit old. IIRC, we are using golangci-lint 1.55.2, which is based in golang 1.21.3. Could you please try to update the golangci-lint version up to 1.62.2 and try again?

I will do it in a separate PR.

webhooks/data-import-cron.go

0xFelix · 2024-12-06T08:20:40Z

webhooks/data-import-cron.go

+	sourceURL := sourceRegistry.URL
+	sourceIS := sourceRegistry.ImageStream


Up to you but I'd inline most of these vars

I'd keep them this way. It would be too verbose if inlined.

0xFelix · 2024-12-06T08:22:36Z

webhooks/data-import-cron_test.go

+			Expect(err).To(MatchError(ContainSubstring("unsupported value:")))
+		})
+
+		It("should fail if there are multiple access modes in PVC", func() {


This case is missing for Storage?

Can the cases not be split e.g. like other ones?

It is allowed to have multiple access modes for Storage. At least the CDI webhook allows it.

Can the cases not be split e.g. like other ones?

What do you mean?

webhooks/ssp_webhook.go

Added webhook code to check if DataImportCronTemplate is correct. The code is copied from CDI webhook. They don't export the code, so we cannot import it as a module. Signed-off-by: Andrej Krejcir <[email protected]>

Added unit tests for DataImportCronTemplate check in the webhook. Signed-off-by: Andrej Krejcir <[email protected]>

sonarqubecloud · 2024-12-06T14:05:58Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

akalenyu · 2024-12-08T08:49:22Z

The code is copied from CDI webhook. They don't export the code, so we cannot import it as a module.

Isn't exportable at all? What would be @akalenyu's opinion about this?

Yeah I don't think there is a good reason to export it. We did export the authorization bits at one point kubevirt/containerized-data-importer#2636 but that is a must for any project that embeds DataVolumes for security reasons.

My general thoughts are, with all the recent focus on webhooks, I am not sure we want to copy the webhook,
when we could instead just propagate the creation error properly to the status. Would that not suffice?
Another option is dry running the creation.

0xFelix · 2024-12-09T09:05:26Z

I like the idea of using the dry running option.

@akrejcir Can you explain how having these checks in SSP fixes the initial issue? Won't it still fail its reconciliation because the supplied DIC templates are not valid?

akrejcir · 2024-12-09T10:48:53Z

My general thoughts are, with all the recent focus on webhooks, I am not sure we want to copy the webhook,
when we could instead just propagate the creation error properly to the status. Would that not suffice?
Another option is dry running the creation.

Ok, I will look into propagating the error into status. Rejecting an incorrect SSP in webhook would be faster, but we need a lot of copied code.

I didn't want to call dry-run create API in the webhook, because the webhook should be fast, and there is a set timeout for it. There can be any number of DataImportCronTemplates specified in the SSP, and doing many API calls in the webhook could take too long.

@akrejcir Can you explain how having these checks in SSP fixes the initial issue? Won't it still fail its reconciliation because the supplied DIC templates are not valid?

It will not fail reconciliation, bacuase the invalid SSP object is rejected in webhook.

akalenyu · 2024-12-09T10:53:04Z

Rejecting an incorrect SSP in webhook would be faster

As a project we kind of decided that this is not enough of an advantage versus all the bad stuff webhooks bring.
I see no issue with solving the bug by reporting the error in the status (users should be checking that)

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 5, 2024

kubevirt-bot requested review from 0xFelix and ksimon1 December 5, 2024 12:14

kubevirt-bot added the size/XXL label Dec 5, 2024

kubevirt-bot requested review from codingben and jcanocan December 5, 2024 12:15

akrejcir force-pushed the webhook-dict branch from 709ee48 to c8ede89 Compare December 5, 2024 12:30

0xFelix reviewed Dec 6, 2024

View reviewed changes

akrejcir force-pushed the webhook-dict branch 2 times, most recently from f020458 to 0043522 Compare December 6, 2024 13:20

akrejcir added 2 commits December 6, 2024 14:48

webhook: Check DataImportCronTemplate

c6e6e25

Added webhook code to check if DataImportCronTemplate is correct. The code is copied from CDI webhook. They don't export the code, so we cannot import it as a module. Signed-off-by: Andrej Krejcir <[email protected]>

tests: Add unit tests for DICT webhook

f3255a2

Added unit tests for DataImportCronTemplate check in the webhook. Signed-off-by: Andrej Krejcir <[email protected]>

akrejcir force-pushed the webhook-dict branch from 0043522 to f3255a2 Compare December 6, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: webhook: Check if DataImportCronTemplate is correct #1162

feat: webhook: Check if DataImportCronTemplate is correct #1162

akrejcir commented Dec 5, 2024

kubevirt-bot commented Dec 5, 2024

akrejcir commented Dec 5, 2024

codingben commented Dec 5, 2024

0xFelix Dec 6, 2024

akrejcir Dec 6, 2024

akrejcir Dec 6, 2024 •

edited

Loading

0xFelix Dec 9, 2024

jcanocan Dec 9, 2024

akrejcir Dec 9, 2024

0xFelix Dec 6, 2024

akrejcir Dec 6, 2024

0xFelix Dec 6, 2024

0xFelix Dec 6, 2024

akrejcir Dec 6, 2024

akrejcir Dec 6, 2024

sonarqubecloud bot commented Dec 6, 2024

akalenyu commented Dec 8, 2024

0xFelix commented Dec 9, 2024 •

edited

Loading

akrejcir commented Dec 9, 2024

akalenyu commented Dec 9, 2024

		sourceURL := sourceRegistry.URL
		sourceIS := sourceRegistry.ImageStream

feat: webhook: Check if DataImportCronTemplate is correct #1162

Are you sure you want to change the base?

feat: webhook: Check if DataImportCronTemplate is correct #1162

Conversation

akrejcir commented Dec 5, 2024

kubevirt-bot commented Dec 5, 2024

akrejcir commented Dec 5, 2024

codingben commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 6, 2024

Quality Gate passed

akalenyu commented Dec 8, 2024

0xFelix commented Dec 9, 2024 • edited Loading

akrejcir commented Dec 9, 2024

akalenyu commented Dec 9, 2024

akrejcir Dec 6, 2024 •

edited

Loading

0xFelix commented Dec 9, 2024 •

edited

Loading