add support to create webhook configuration when webhooks is enabled #120905

kmala · 2023-09-26T23:40:11Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The changes would create/update the validation webhook configuration if webhooks are enabled as part of the cloud controller and is an extension of #108838.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added the ability to create a validating webhook configuration as part of the cloud controller manager bootstrap.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: [KEP-2699](https://github.com/kubernetes/enhancements/tree/master/keps/sig-cloud-provider/2699-add-webhook-hosting-to-ccm)

k8s-ci-robot · 2023-09-26T23:40:20Z

Hi @kmala. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kmala · 2023-09-26T23:43:48Z

/sig cloud-provider

elmiko · 2023-09-27T16:05:13Z

/triage accepted

nckturner · 2023-10-05T16:59:01Z

staging/src/k8s.io/cloud-provider/app/controllermanager.go

+			CABundle: []byte(webhooksConfig.CaBundle),
+		}
+	}
+	kubeClient := clientBuilder.ClientOrDie("ccm")


Is ccm the right client name? I think that name shows up in the audit logs, so I'm wondering if we want a suffix to make it clear what aspect of the ccm is making these calls.

ccm-webhook ?

nckturner · 2023-10-05T17:05:47Z

staging/src/k8s.io/cloud-provider/options/webhook.go

+				}
+			}
+		}
+		if webhookConfigs.Len() != 0 {


So the behavior here is that we need a 1:1 mapping of enabled webhooks to webhook configs, right? I think that makes sense. The only case I'm thinking of is where a user wants to disable the webhook and reenable it later, but I think its better to force them to remove the config because that way its explicit what webhooks exist based on the configuration and the webhook names.

yes, that correct. Since both are configs provided by the user i felt its better to force to update both places.

nckturner · 2023-10-05T17:06:23Z

staging/src/k8s.io/cloud-provider/config/types.go

+	// CaBundle is the ca certs to be used by apiserver for validating the webhook server certs.
+	CaBundle string
+	// ValidationWebhookConfiguration is the config used for creating validation webhook config object.
+	ValidationWebhookConfiguration *admissionregistrationv1.ValidatingWebhookConfiguration


Do we want to allow the user to choose between validating and mutating webhooks on a per-webhook basis?

yeah i think its possible to do it by taking another input here https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cloud-provider/app/webhooks.go#L56-L59

Per @cheftako we may want to allow providers to choose, but they may not want their customers to choose.

andrewsykim · 2023-10-25T16:23:57Z

/assign @cheftako

elmiko · 2023-12-20T17:08:53Z

discussed today at sig cloud provider meetings, needs a few more reviews here.

@cheftako has offered to take point on the review requests

kmala · 2024-02-23T19:08:45Z

@cheftako can you please review when you get chance

dims · 2024-03-07T20:43:26Z

/ok-to-test

kmala · 2024-03-15T16:33:18Z

/retest

kmala · 2024-03-15T18:19:03Z

/retest

kmala · 2024-03-21T22:20:31Z

/test pull-kubernetes-dependencies

k8s-ci-robot · 2024-03-22T18:55:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kmala
Once this PR has been reviewed and has the lgtm label, please ask for approval from cheftako and additionally assign msau42 for approval. 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:

api/OWNERS
pkg/generated/openapi/OWNERS
staging/src/k8s.io/cloud-provider/OWNERS

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

kmala · 2024-03-22T20:03:12Z

/retest-required

k8s-ci-robot · 2024-03-22T20:44:41Z

@kmala: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-providerless	`3c2a8c6`	link	false	`/test pull-kubernetes-e2e-gce-providerless`
pull-kubernetes-verify	`3c2a8c6`	link	true	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

elmiko

this is generally making sense to me, and i don't see anything in the code that is concerning. i would like to play with the unit tests and run this locally to get a feel for how it works.

but i think on balance this is looking good to me

liggitt · 2024-06-26T23:18:25Z

staging/src/k8s.io/cloud-provider/config/v1alpha1/types.go

+	// CaBundle is the ca certs to be used by apiserver for validating the webhook server certs.
+	CaBundle string
+	// ValidatingWebhookConfiguration is the config used for creating validation webhook config object.
+	ValidatingWebhookConfiguration *admissionregistrationv1.ValidatingWebhookConfiguration


I'm very confused about what is going on here... why are we expecting to apply defaulting from REST APIs to fields of this config file?

liggitt · 2024-06-26T23:24:57Z

staging/src/k8s.io/cloud-provider/app/controllermanager.go

@@ -162,6 +168,97 @@ the cloud specific control loops shipped with Kubernetes.`,
 	return cmd
 }

+func createOrUpdateWebhookConfiguration(ctx context.Context, webhooks map[string]WebhookHandler, webhooksConfig cloudproviderconfig.WebhookConfiguration, clientBuilder clientbuilder.SimpleControllerClientBuilder) error {


This self-registration of webhooks is really tricky to get right... I had no idea this was being contemplated to be added into cloud-controller-manager

some quick observations:

it's not clear why having CCM create this programatically is better than applying a manifest

does having CCM do this mean that multiple CCM controllers duel when the configuration changes?

is the WebhookAddress expected to point to the CCM instance? how does routing work when there are multiple CCM instances?

how do multiple CCM controllers handle rollout of a new webhook / configuration? all instances have to understand the new endpoints or fail open when requests to unknown endpoints arrive

great questions @liggitt , i was not aware of the complexity around the self-registration but it makes perfect sense once you've laid it out. i think this might cause us to reconsider the methodology here, it seems like using a manifest would make a better experience for users and help with some of the ordering/dueling issues.

thanks for the review!

liggitt · 2024-06-26T23:25:50Z

staging/src/k8s.io/cloud-provider/app/controllermanager.go

+			currentConfiguration, err := kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, webhooksConfig.ValidatingWebhookConfiguration.Name, metav1.GetOptions{})
+			if err != nil {
+				klog.ErrorS(err, "Unable to create validating webhook configuration with API server, error getting existing webhook configuration", "webhookconfiguration", webhooksConfig.ValidatingWebhookConfiguration.Name)
+				return fmt.Errorf("unable to get validating webhook configuration from API server %w", err)
+			}
+			currentConfiguration.Webhooks = webhooksConfig.ValidatingWebhookConfiguration.Webhooks
+
+			_, err = kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(ctx, currentConfiguration, metav1.UpdateOptions{})


how does this avoid dueling registration from other CCM instances?

liggitt · 2024-06-26T23:26:25Z

staging/src/k8s.io/cloud-provider/config/v1alpha1/defaults.go

+// SetDefaults_ValidatingWebhook sets defaults for webhook validating. This function
+// is duplicated from "k8s.io/kubernetes/pkg/apis/admissionregistration/v1/defaults.go"
+// in order for in-tree cloud providers to not depend on internal packages.
+func SetDefaults_ValidatingWebhook(obj *admissionregistrationv1.ValidatingWebhook) {


I don't think any defaulting is required here... the defaulting gets applied when creating against the server

k8s-ci-robot · 2024-09-06T00:56:54Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-triage-robot · 2024-12-05T01:43:07Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

elmiko · 2024-12-05T15:54:40Z

i think this is still an important feature to work on, but i'm not sure we have the time/resources to push it further currently.

/remove-lifecycle stale

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 26, 2023

k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. area/cloudprovider and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 26, 2023

k8s-ci-robot requested review from deads2k and justinsb September 26, 2023 23:46

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 27, 2023

nckturner reviewed Oct 5, 2023

View reviewed changes

k8s-ci-robot assigned cheftako Oct 25, 2023

kmala force-pushed the ccmwebhook branch from ad9ef65 to d22ac65 Compare November 7, 2023 01:11

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 7, 2024

kmala force-pushed the ccmwebhook branch from bdaf315 to eb61d23 Compare March 12, 2024 19:52

kmala force-pushed the ccmwebhook branch from 8a272fe to 4a66724 Compare March 14, 2024 23:55

kmala force-pushed the ccmwebhook branch 2 times, most recently from 54442a5 to 612c052 Compare March 15, 2024 17:30

kmala force-pushed the ccmwebhook branch 2 times, most recently from 737a281 to 9164613 Compare March 15, 2024 21:02

k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Mar 15, 2024

kmala added 2 commits March 21, 2024 13:12

add support to create webhook configuration when webhooks is enabled

4f557af

add option to specify mutating webhook

dc7883d

kmala force-pushed the ccmwebhook branch 3 times, most recently from 4a86b5a to 57f032d Compare March 21, 2024 21:28

fix code gen and lint

3c2a8c6

kmala force-pushed the ccmwebhook branch from 57f032d to 3c2a8c6 Compare March 22, 2024 18:54

elmiko reviewed May 16, 2024

View reviewed changes

liggitt reviewed Jun 26, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 5, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support to create webhook configuration when webhooks is enabled #120905

add support to create webhook configuration when webhooks is enabled #120905

kmala commented Sep 26, 2023

k8s-ci-robot commented Sep 26, 2023

kmala commented Sep 26, 2023

elmiko commented Sep 27, 2023

nckturner Oct 5, 2023

kmala Oct 5, 2023

cheftako Oct 30, 2023

nckturner Oct 5, 2023

kmala Oct 5, 2023

nckturner Oct 5, 2023

kmala Oct 5, 2023

bridgetkromhout Oct 25, 2023

andrewsykim commented Oct 25, 2023

elmiko commented Dec 20, 2023

kmala commented Feb 23, 2024

dims commented Mar 7, 2024

kmala commented Mar 15, 2024

kmala commented Mar 15, 2024

kmala commented Mar 21, 2024

k8s-ci-robot commented Mar 22, 2024

kmala commented Mar 22, 2024

k8s-ci-robot commented Mar 22, 2024 •

edited

Loading

elmiko left a comment

liggitt Jun 26, 2024

liggitt Jun 26, 2024

elmiko Jun 27, 2024

liggitt Jun 26, 2024

liggitt Jun 26, 2024

k8s-ci-robot commented Sep 6, 2024

k8s-triage-robot commented Dec 5, 2024

elmiko commented Dec 5, 2024

add support to create webhook configuration when webhooks is enabled #120905

Are you sure you want to change the base?

add support to create webhook configuration when webhooks is enabled #120905

Conversation

kmala commented Sep 26, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 26, 2023

kmala commented Sep 26, 2023

elmiko commented Sep 27, 2023

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

andrewsykim commented Oct 25, 2023

elmiko commented Dec 20, 2023

kmala commented Feb 23, 2024

dims commented Mar 7, 2024

kmala commented Mar 15, 2024

kmala commented Mar 15, 2024

kmala commented Mar 21, 2024

k8s-ci-robot commented Mar 22, 2024

kmala commented Mar 22, 2024

k8s-ci-robot commented Mar 22, 2024 • edited Loading

elmiko left a 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

k8s-ci-robot commented Sep 6, 2024

k8s-triage-robot commented Dec 5, 2024

elmiko commented Dec 5, 2024

k8s-ci-robot commented Mar 22, 2024 •

edited

Loading