Skip to content

feat: add managed resource webhook#1152

Merged
nwnt merged 9 commits into
Azure:mainfrom
nwnt:nwnt/add-managed-resource-webhook
Jul 9, 2025
Merged

feat: add managed resource webhook#1152
nwnt merged 9 commits into
Azure:mainfrom
nwnt:nwnt/add-managed-resource-webhook

Conversation

@nwnt

@nwnt nwnt commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

Description of your changes

Fixes :
Enable the fleet dataplane to deny any create/update/delete operations on ARM managed resources. At the moment, CRP, Namespace, ResourceQuota, and NetworkPolicy can be managed resources.

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

make local-unit-test ran successfully

Special notes for your reviewer

Comment thread pkg/webhook/managedresource/managedresource_validating_webhook.go
Comment thread pkg/webhook/managedresource/managedresource_validating_webhook_test.go Outdated
Comment thread pkg/webhook/managedresource/managedresource_validating_webhook.go Outdated
@Arvindthiru

Copy link
Copy Markdown
Contributor

Comment thread pkg/webhook/managedresource/managedresource_validating_webhook_test.go Outdated
Comment thread pkg/webhook/webhook.go Outdated
@Arvindthiru

Copy link
Copy Markdown
Contributor

We should also add E2Es here https://github.com/Azure/fleet/blob/main/test/e2e/webhook_test.go

Comment thread pkg/webhook/webhook.go Outdated
@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch 2 times, most recently from de8bc2a to 35cc8f0 Compare July 2, 2025 00:24
@nwnt nwnt changed the title feat: add managed resource webhook feat: add managed resource webhook Jul 2, 2025
Comment thread pkg/webhook/webhook.go Outdated
Nont added 6 commits July 2, 2025 17:40
Signed-off-by: Nont <nont@duck.com>
Signed-off-by: Nont <nont@duck.com>
Signed-off-by: Nont <nont@duck.com>
Signed-off-by: Nont <nont@duck.com>
Signed-off-by: Nont <nont@duck.com>
Signed-off-by: Nont <nont@duck.com>
@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch from 4475ea8 to 660c99d Compare July 3, 2025 21:18
@stevekuznetsov

Copy link
Copy Markdown

You may want to look into ValidatingAdmissionPolicy - webhooks are a huge lift (and, if they are blocking, an uptime risk for the api server). VAP is extremely lightweight.

Comment thread test/e2e/webhook_test.go Outdated
Comment thread test/e2e/webhook_test.go Outdated
Comment thread test/e2e/webhook_test.go Outdated
Comment thread test/e2e/webhook_test.go
@nwnt

nwnt commented Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

You may want to look into ValidatingAdmissionPolicy - webhooks are a huge lift (and, if they are blocking, an uptime risk for the api server). VAP is extremely lightweight.

@stevekuznetsov thanks for the input. We'll go ahead with this approach for now and will be validating if it can be simplified further with the way you suggest.

@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch from 7952439 to 334f248 Compare July 7, 2025 21:13
Comment thread test/e2e/webhook_test.go Outdated
Comment thread test/e2e/webhook_test.go Outdated
@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch from 334f248 to 9103561 Compare July 8, 2025 01:38
@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch from 9103561 to c35147c Compare July 8, 2025 01:41
Comment on lines +80 to +87
if err := runtime.Convert_runtime_RawExtension_To_runtime_Object(&raw, &obj, nil); err != nil {
return nil, nil, err
}
o, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
if err != nil {
return nil, nil, err
}
u := unstructured.Unstructured{Object: o}

@Arvindthiru Arvindthiru Jul 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think using a converter doesn't improve much here. It's probably even simpler to not have a decoder just to convert from raw to runtime.Object.

Arvindthiru
Arvindthiru previously approved these changes Jul 8, 2025

@Arvindthiru Arvindthiru left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread test/e2e/webhook_test.go Outdated
Comment thread test/e2e/webhook_test.go Outdated
Comment thread test/e2e/webhook_test.go Outdated
@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch from 2b17a21 to 044e29c Compare July 8, 2025 19:14
Signed-off-by: Nont <nont@duck.com>
@nwnt
nwnt force-pushed the nwnt/add-managed-resource-webhook branch from 044e29c to d39c3b3 Compare July 8, 2025 20:13
@nwnt
nwnt merged commit d7bed83 into Azure:main Jul 9, 2025
21 of 22 checks passed
@nwnt
nwnt deleted the nwnt/add-managed-resource-webhook branch July 9, 2025 02:06
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.

4 participants