diff --git a/.golangci.yml b/.golangci.yml index 86110ffefa..ed692daa31 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -80,8 +80,6 @@ issues: - Subprocess launch(ed with variable|ing should be audited) - (G204|G104|G307) - "ST1000: at least one file in a package should have a package comment" - - "SA1019: \"sigs.k8s.io/controller-runtime/pkg/runtime/inject\"" - - "SA1019: inject.*" exclude-rules: - linters: - gosec diff --git a/alias.go b/alias.go index 35cba30be5..6217116da8 100644 --- a/alias.go +++ b/alias.go @@ -139,7 +139,7 @@ var ( // The logger, when used with controllers, can be expected to contain basic information about the object // that's being reconciled like: // - `reconciler group` and `reconciler kind` coming from the For(...) object passed in when building a controller. - // - `name` and `namespace` injected from the reconciliation request. + // - `name` and `namespace` from the reconciliation request. // // This is meant to be used with the context supplied in a struct that satisfies the Reconciler interface. LoggerFrom = log.FromContext diff --git a/doc.go b/doc.go index fa6c532c49..9b604d522b 100644 --- a/doc.go +++ b/doc.go @@ -52,7 +52,7 @@ limitations under the License. // // Every controller and webhook is ultimately run by a Manager (pkg/manager). A // manager is responsible for running controllers and webhooks, and setting up -// common dependencies (pkg/runtime/inject), like shared caches and clients, as +// common dependencies, like shared caches and clients, as // well as managing leader election (pkg/leaderelection). Managers are // generally configured to gracefully shut down controllers on pod termination // by wiring up a signal handler (pkg/manager/signals). diff --git a/example_test.go b/example_test.go index 9c82de87d5..381959d5bb 100644 --- a/example_test.go +++ b/example_test.go @@ -37,7 +37,6 @@ import ( // ReplicaSetReconciler. // // * Start the application. -// TODO(pwittrock): Update this example when we have better dependency injection support. func Example() { var log = ctrl.Log.WithName("builder-examples") @@ -75,7 +74,6 @@ func Example() { // ReplicaSetReconciler. // // * Start the application. -// TODO(pwittrock): Update this example when we have better dependency injection support. func Example_updateLeaderElectionDurations() { var log = ctrl.Log.WithName("builder-examples") leaseDuration := 100 * time.Second diff --git a/examples/builtins/main.go b/examples/builtins/main.go index cddaae24bb..8ea173b248 100644 --- a/examples/builtins/main.go +++ b/examples/builtins/main.go @@ -22,6 +22,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -30,7 +31,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/manager/signals" "sigs.k8s.io/controller-runtime/pkg/source" - "sigs.k8s.io/controller-runtime/pkg/webhook" ) func init() { @@ -71,13 +71,14 @@ func main() { os.Exit(1) } - // Setup webhooks - entryLog.Info("setting up webhook server") - hookServer := mgr.GetWebhookServer() - - entryLog.Info("registering webhooks to the webhook server") - hookServer.Register("/mutate-v1-pod", &webhook.Admission{Handler: &podAnnotator{Client: mgr.GetClient()}}) - hookServer.Register("/validate-v1-pod", &webhook.Admission{Handler: &podValidator{Client: mgr.GetClient()}}) + if err := builder.WebhookManagedBy(mgr). + For(&corev1.Pod{}). + WithDefaulter(&podAnnotator{}). + WithValidator(&podValidator{}). + Complete(); err != nil { + entryLog.Error(err, "unable to create webhook", "webhook", "Pod") + os.Exit(1) + } entryLog.Info("starting manager") if err := mgr.Start(signals.SetupSignalHandler()); err != nil { diff --git a/examples/builtins/mutatingwebhook.go b/examples/builtins/mutatingwebhook.go index 823b620877..c3e3bc396a 100644 --- a/examples/builtins/mutatingwebhook.go +++ b/examples/builtins/mutatingwebhook.go @@ -18,54 +18,31 @@ package main import ( "context" - "encoding/json" - "net/http" + "fmt" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) // +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,groups="",resources=pods,verbs=create;update,versions=v1,name=mpod.kb.io // podAnnotator annotates Pods -type podAnnotator struct { - Client client.Client - decoder *admission.Decoder -} +type podAnnotator struct{} -// podAnnotator adds an annotation to every incoming pods. -func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admission.Response { - // set up a convenient log object so we don't have to type request over and over again +func (a *podAnnotator) Default(ctx context.Context, obj runtime.Object) error { log := logf.FromContext(ctx) - - pod := &corev1.Pod{} - err := a.decoder.Decode(req, pod) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) + pod, ok := obj.(*corev1.Pod) + if !ok { + return fmt.Errorf("expected a Pod but got a %T", obj) } if pod.Annotations == nil { pod.Annotations = map[string]string{} } pod.Annotations["example-mutating-admission-webhook"] = "foo" + log.Info("Annotated Pod") - marshaledPod, err := json.Marshal(pod) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - log.Info("Annotating Pod") - - return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod) -} - -// podAnnotator implements admission.DecoderInjector. -// A decoder will be automatically injected. - -// InjectDecoder injects the decoder. -func (a *podAnnotator) InjectDecoder(d *admission.Decoder) error { - a.decoder = d return nil } diff --git a/examples/builtins/validatingwebhook.go b/examples/builtins/validatingwebhook.go index 00b3e94417..e6094598bb 100644 --- a/examples/builtins/validatingwebhook.go +++ b/examples/builtins/validatingwebhook.go @@ -19,53 +19,47 @@ package main import ( "context" "fmt" - "net/http" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) // +kubebuilder:webhook:path=/validate-v1-pod,mutating=false,failurePolicy=fail,groups="",resources=pods,verbs=create;update,versions=v1,name=vpod.kb.io // podValidator validates Pods -type podValidator struct { - Client client.Client - decoder *admission.Decoder -} +type podValidator struct{} -// podValidator admits a pod if a specific annotation exists. -func (v *podValidator) Handle(ctx context.Context, req admission.Request) admission.Response { - // set up a convenient log object so we don't have to type request over and over again +// validate admits a pod if a specific annotation exists. +func (v *podValidator) validate(ctx context.Context, obj runtime.Object) error { log := logf.FromContext(ctx) - - pod := &corev1.Pod{} - err := v.decoder.Decode(req, pod) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) + pod, ok := obj.(*corev1.Pod) + if !ok { + return fmt.Errorf("expected a Pod but got a %T", obj) } log.Info("Validating Pod") - key := "example-mutating-admission-webhook" anno, found := pod.Annotations[key] if !found { - return admission.Denied(fmt.Sprintf("missing annotation %s", key)) + return fmt.Errorf("missing annotation %s", key) } if anno != "foo" { - return admission.Denied(fmt.Sprintf("annotation %s did not have value %q", key, "foo")) + return fmt.Errorf("annotation %s did not have value %q", key, "foo") } - return admission.Allowed("") + return nil +} + +func (v *podValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error { + return v.validate(ctx, obj) } -// podValidator implements admission.DecoderInjector. -// A decoder will be automatically injected. +func (v *podValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error { + return v.validate(ctx, newObj) +} -// InjectDecoder injects the decoder. -func (v *podValidator) InjectDecoder(d *admission.Decoder) error { - v.decoder = d - return nil +func (v *podValidator) ValidateDelete(ctx context.Context, obj runtime.Object) error { + return v.validate(ctx, obj) } diff --git a/pkg/builder/example_test.go b/pkg/builder/example_test.go index 955c46b562..652c9b5833 100644 --- a/pkg/builder/example_test.go +++ b/pkg/builder/example_test.go @@ -107,7 +107,9 @@ func ExampleBuilder() { ControllerManagedBy(mgr). // Create the ControllerManagedBy For(&appsv1.ReplicaSet{}). // ReplicaSet is the Application API Owns(&corev1.Pod{}). // ReplicaSet owns Pods created by it - Complete(&ReplicaSetReconciler{}) + Complete(&ReplicaSetReconciler{ + Client: mgr.GetClient(), + }) if err != nil { log.Error(err, "could not create controller") os.Exit(1) @@ -155,8 +157,3 @@ func (a *ReplicaSetReconciler) Reconcile(ctx context.Context, req reconcile.Requ return reconcile.Result{}, nil } - -func (a *ReplicaSetReconciler) InjectClient(c client.Client) error { - a.Client = c - return nil -} diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 05bb3e2ee9..115c182029 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -164,10 +164,10 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() { func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { if defaulter := blder.withDefaulter; defaulter != nil { - return admission.WithCustomDefaulter(blder.apiType, defaulter).WithRecoverPanic(blder.recoverPanic) + return admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter).WithRecoverPanic(blder.recoverPanic) } if defaulter, ok := blder.apiType.(admission.Defaulter); ok { - return admission.DefaultingWebhookFor(defaulter).WithRecoverPanic(blder.recoverPanic) + return admission.DefaultingWebhookFor(blder.mgr.GetScheme(), defaulter).WithRecoverPanic(blder.recoverPanic) } log.Info( "skip registering a mutating webhook, object does not implement admission.Defaulter or WithDefaulter wasn't called", @@ -194,10 +194,10 @@ func (blder *WebhookBuilder) registerValidatingWebhook() { func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook { if validator := blder.withValidator; validator != nil { - return admission.WithCustomValidator(blder.apiType, validator).WithRecoverPanic(blder.recoverPanic) + return admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, validator).WithRecoverPanic(blder.recoverPanic) } if validator, ok := blder.apiType.(admission.Validator); ok { - return admission.ValidatingWebhookFor(validator).WithRecoverPanic(blder.recoverPanic) + return admission.ValidatingWebhookFor(blder.mgr.GetScheme(), validator).WithRecoverPanic(blder.recoverPanic) } log.Info( "skip registering a validating webhook, object does not implement admission.Validator or WithValidator wasn't called", diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 41d15f1bb4..2ee1e7bfb4 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -115,8 +115,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -191,8 +189,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -208,7 +204,7 @@ func runTests(admissionReviewVersion string) { By("sanity checking the response contains reasonable fields") ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) }) It("should scaffold a defaulting webhook with a custom defaulter", func() { @@ -260,8 +256,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -337,8 +331,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -411,8 +403,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -430,7 +420,7 @@ func runTests(admissionReviewVersion string) { By("sanity checking the response contains reasonable field") ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) }) It("should scaffold a validating webhook with a custom validator", func() { @@ -463,12 +453,12 @@ func runTests(admissionReviewVersion string) { "kind":{ "group":"foo.test.org", "version":"v1", - "kind":"TestDefaulter" + "kind":"TestValidator" }, "resource":{ "group":"foo.test.org", "version":"v1", - "resource":"testdefaulter" + "resource":"testvalidator" }, "namespace":"default", "name":"foo", @@ -484,8 +474,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -511,7 +499,7 @@ func runTests(admissionReviewVersion string) { By("sanity checking the response contains reasonable field") ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) - EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testdefaulter"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) + EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testvalidator"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) }) It("should scaffold defaulting and validating webhooks if the type implements both Defaulter and Validator interfaces", func() { @@ -558,8 +546,6 @@ func runTests(admissionReviewVersion string) { ctx, cancel := context.WithCancel(context.Background()) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -636,8 +622,6 @@ func runTests(admissionReviewVersion string) { }`) cancel() - // TODO: we may want to improve it to make it be able to inject dependencies, - // but not always try to load certs and return not found error. err = svr.Start(ctx) if err != nil && !os.IsNotExist(err) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -724,7 +708,7 @@ func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil } func (d *TestDefaulter) Default() { if d.Panic { - panic("injected panic") + panic("fake panic test") } if d.Replica < 2 { d.Replica = 2 @@ -767,7 +751,7 @@ var _ admission.Validator = &TestValidator{} func (v *TestValidator) ValidateCreate() error { if v.Panic { - panic("injected panic") + panic("fake panic test") } if v.Replica < 0 { return errors.New("number of replica should be greater than or equal to 0") @@ -777,7 +761,7 @@ func (v *TestValidator) ValidateCreate() error { func (v *TestValidator) ValidateUpdate(old runtime.Object) error { if v.Panic { - panic("injected panic") + panic("fake panic test") } if v.Replica < 0 { return errors.New("number of replica should be greater than or equal to 0") @@ -792,7 +776,7 @@ func (v *TestValidator) ValidateUpdate(old runtime.Object) error { func (v *TestValidator) ValidateDelete() error { if v.Panic { - panic("injected panic") + panic("fake panic test") } if v.Replica > 0 { return errors.New("number of replica should be less than or equal to 0 to delete") diff --git a/pkg/cluster/internal.go b/pkg/cluster/internal.go index a84e4526d6..d7c251c9b9 100644 --- a/pkg/cluster/internal.go +++ b/pkg/cluster/internal.go @@ -34,14 +34,8 @@ type cluster struct { // config is the rest.config used to talk to the apiserver. Required. config *rest.Config - // scheme is the scheme injected into Controllers, EventHandlers, Sources and Predicates. Defaults - // to scheme.scheme. scheme *runtime.Scheme - - cache cache.Cache - - // TODO(directxman12): Provide an escape hatch to get individual indexers - // client is the client injected into Controllers (and EventHandlers, Sources and Predicates). + cache cache.Cache client client.Client // apiReader is the reader that will make requests to the api server and not the cache. diff --git a/pkg/config/config.go b/pkg/config/config.go index 8e853d6a0f..769a9e3b28 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -83,12 +83,6 @@ func (d *DeferredFileLoader) OfKind(obj ControllerManagerConfiguration) *Deferre return d } -// InjectScheme will configure the scheme to be used for decoding the file. -func (d *DeferredFileLoader) InjectScheme(scheme *runtime.Scheme) error { - d.scheme = scheme - return nil -} - // loadFile is used from the mutex.Once to load the file. func (d *DeferredFileLoader) loadFile() { if d.scheme == nil { diff --git a/pkg/config/example_test.go b/pkg/config/example_test.go index fb1cd58b5f..715f3acebd 100644 --- a/pkg/config/example_test.go +++ b/pkg/config/example_test.go @@ -44,41 +44,10 @@ func ExampleFile() { } // This example will load the file from a custom path. -func ExampleDeferredFileLoader_atPath() { +func ExampleFile_atPath() { loader := config.File().AtPath("/var/run/controller-runtime/config.yaml") if _, err := loader.Complete(); err != nil { fmt.Println("failed to load config") os.Exit(1) } } - -// This example sets up loader with a custom scheme. -func ExampleDeferredFileLoader_injectScheme() { - loader := config.File() - err := loader.InjectScheme(scheme) - if err != nil { - fmt.Println("failed to inject scheme") - os.Exit(1) - } - - _, err = loader.Complete() - if err != nil { - fmt.Println("failed to load config") - os.Exit(1) - } -} - -// This example sets up the loader with a custom scheme and custom type. -func ExampleDeferredFileLoader_ofKind() { - loader := config.File().OfKind(&v1alpha1.CustomControllerManagerConfiguration{}) - err := loader.InjectScheme(scheme) - if err != nil { - fmt.Println("failed to inject scheme") - os.Exit(1) - } - _, err = loader.Complete() - if err != nil { - fmt.Println("failed to load config") - os.Exit(1) - } -} diff --git a/pkg/internal/controller/controller_test.go b/pkg/internal/controller/controller_test.go index 89048ec550..83a9e61a6d 100644 --- a/pkg/internal/controller/controller_test.go +++ b/pkg/internal/controller/controller_test.go @@ -212,7 +212,7 @@ var _ = Describe("controller", func() { It("should process events from source.Channel", func() { // channel to be closed when event is processed processed := make(chan struct{}) - // source channel to be injected + // source channel ch := make(chan event.GenericEvent, 1) ctx, cancel := context.WithCancel(context.TODO()) diff --git a/pkg/internal/testing/process/process_test.go b/pkg/internal/testing/process/process_test.go index 9c3aa3bf27..5b0708227a 100644 --- a/pkg/internal/testing/process/process_test.go +++ b/pkg/internal/testing/process/process_test.go @@ -138,7 +138,7 @@ var _ = Describe("Start method", func() { echo 'i started' >&2 `, } - processState.StartTimeout = 1 * time.Second + processState.StartTimeout = 5 * time.Second Expect(processState.Start(stdout, stderr)).To(Succeed()) Eventually(processState.Exited).Should(BeTrue()) diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 19c8fea6ea..967f1ffa3b 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -46,7 +46,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/internal/httpserver" intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder" "sigs.k8s.io/controller-runtime/pkg/metrics" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook" ) @@ -191,28 +190,9 @@ func (cm *controllerManager) Add(r Runnable) error { } func (cm *controllerManager) add(r Runnable) error { - // Set dependencies on the object - if err := cm.setFields(r); err != nil { - return err - } return cm.runnables.Add(r) } -// Deprecated: use the equivalent Options field to set a field. This method will be removed in v0.10. -func (cm *controllerManager) setFields(i interface{}) error { - if _, err := inject.SchemeInto(cm.cluster.GetScheme(), i); err != nil { - return err - } - if _, err := inject.InjectorInto(cm.setFields, i); err != nil { - return err - } - if _, err := inject.LoggerInto(cm.logger, i); err != nil { - return err - } - - return nil -} - // AddMetricsExtraHandler adds extra handler served on path to the http server that serves metrics. func (cm *controllerManager) AddMetricsExtraHandler(path string, handler http.Handler) error { cm.Lock() diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 2facb1c915..dd4cb4055a 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -44,7 +44,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/metrics" "sigs.k8s.io/controller-runtime/pkg/recorder" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook" ) @@ -55,8 +54,7 @@ type Manager interface { cluster.Cluster // Add will set requested dependencies on the component, and cause the component to be - // started when Start is called. Add will inject any dependencies for which the argument - // implements the inject interface - e.g. inject.Client. + // started when Start is called. // Depending on if a Runnable implements LeaderElectionRunnable interface, a Runnable can be run in either // non-leaderelection mode (always running) or leader election mode (managed by leader election if enabled). Add(Runnable) error @@ -457,13 +455,6 @@ func New(config *rest.Config, options Options) (Manager, error) { // any options already set on Options will be ignored, this is used to allow // cli flags to override anything specified in the config file. func (o Options) AndFrom(loader config.ControllerManagerConfiguration) (Options, error) { - if inj, wantsScheme := loader.(inject.Scheme); wantsScheme { - err := inj.InjectScheme(o.Scheme) - if err != nil { - return o, err - } - } - newObj, err := loader.Complete() if err != nil { return o, err diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go index 4d62aee67a..a430664509 100644 --- a/pkg/manager/manager_test.go +++ b/pkg/manager/manager_test.go @@ -52,7 +52,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/leaderelection" fakeleaderelection "sigs.k8s.io/controller-runtime/pkg/leaderelection/fake" "sigs.k8s.io/controller-runtime/pkg/metrics" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/recorder" "sigs.k8s.io/controller-runtime/pkg/webhook" ) @@ -1485,12 +1484,6 @@ var _ = Describe("manger.Manager", func() { <-c1 }) - It("should fail if SetFields fails", func() { - m, err := New(cfg, Options{}) - Expect(err).NotTo(HaveOccurred()) - Expect(m.Add(&failRec{})).To(HaveOccurred()) - }) - It("should fail if attempted to start a second time", func() { m, err := New(cfg, Options{}) Expect(err).NotTo(HaveOccurred()) @@ -1623,22 +1616,6 @@ var _ = Describe("manger.Manager", func() { }) }) -var _ reconcile.Reconciler = &failRec{} - -type failRec struct{} - -func (*failRec) Reconcile(context.Context, reconcile.Request) (reconcile.Result, error) { - return reconcile.Result{}, nil -} - -func (*failRec) Start(context.Context) error { - return nil -} - -func (*failRec) InjectScheme(*runtime.Scheme) error { - return fmt.Errorf("expected error") -} - type runnableError struct { } diff --git a/pkg/predicate/predicate_test.go b/pkg/predicate/predicate_test.go index 17aa808769..6bbf21adf0 100644 --- a/pkg/predicate/predicate_test.go +++ b/pkg/predicate/predicate_test.go @@ -21,7 +21,6 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -827,12 +826,6 @@ var _ = Describe("Predicate", func() { passFuncs := funcs(true) failFuncs := funcs(false) - var injectFunc inject.Func - injectFunc = func(i interface{}) error { - _, err := inject.InjectorInto(injectFunc, i) - return err - } - Describe("When checking an And predicate", func() { It("should return false when one of its predicates returns false", func() { a := predicate.And(passFuncs, failFuncs) diff --git a/pkg/runtime/inject/inject.go b/pkg/runtime/inject/inject.go deleted file mode 100644 index 1721232ede..0000000000 --- a/pkg/runtime/inject/inject.go +++ /dev/null @@ -1,86 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Package inject is used by a Manager to inject types into Sources, EventHandlers, Predicates, and Reconciles. -// -// Deprecated: Use manager.Options fields directly. This package will be removed in a future version. -package inject - -import ( - "github.com/go-logr/logr" - "k8s.io/apimachinery/pkg/runtime" -) - -// Scheme is used by the ControllerManager to inject Scheme into Sources, EventHandlers, Predicates, and -// Reconciles. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -type Scheme interface { - InjectScheme(scheme *runtime.Scheme) error -} - -// SchemeInto will set scheme and return the result on i if it implements Scheme. Returns -// false if i does not implement Scheme. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -func SchemeInto(scheme *runtime.Scheme, i interface{}) (bool, error) { - if is, ok := i.(Scheme); ok { - return true, is.InjectScheme(scheme) - } - return false, nil -} - -// Func injects dependencies into i. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -type Func func(i interface{}) error - -// Injector is used by the ControllerManager to inject Func into Controllers. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -type Injector interface { - InjectFunc(f Func) error -} - -// InjectorInto will set f and return the result on i if it implements Injector. Returns -// false if i does not implement Injector. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -func InjectorInto(f Func, i interface{}) (bool, error) { - if ii, ok := i.(Injector); ok { - return true, ii.InjectFunc(f) - } - return false, nil -} - -// Logger is used to inject Loggers into components that need them -// and don't otherwise have opinions. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -type Logger interface { - InjectLogger(l logr.Logger) error -} - -// LoggerInto will set the logger on the given object if it implements inject.Logger, -// returning true if a InjectLogger was called, and false otherwise. -// -// Deprecated: Dependency injection methods are deprecated and going to be removed in a future version. -func LoggerInto(l logr.Logger, i interface{}) (bool, error) { - if injectable, wantsLogger := i.(Logger); wantsLogger { - return true, injectable.InjectLogger(l) - } - return false, nil -} diff --git a/pkg/runtime/inject/inject_suite_test.go b/pkg/runtime/inject/inject_suite_test.go deleted file mode 100644 index ce612f3177..0000000000 --- a/pkg/runtime/inject/inject_suite_test.go +++ /dev/null @@ -1,29 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package inject - -import ( - "testing" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -func TestSource(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Runtime Injection Suite") -} diff --git a/pkg/runtime/inject/inject_test.go b/pkg/runtime/inject/inject_test.go deleted file mode 100644 index 14d48ae172..0000000000 --- a/pkg/runtime/inject/inject_test.go +++ /dev/null @@ -1,103 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package inject - -import ( - "fmt" - "reflect" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/runtime" -) - -var instance *testSource -var uninjectable *failSource -var errInjectFail = fmt.Errorf("injection fails") - -var _ = Describe("runtime inject", func() { - - BeforeEach(func() { - instance = &testSource{} - uninjectable = &failSource{} - }) - - It("should set dependencies", func() { - - f := func(interface{}) error { return nil } - - By("Validating injecting dependencies") - res, err := InjectorInto(f, instance) - Expect(err).NotTo(HaveOccurred()) - Expect(res).To(Equal(true)) - Expect(reflect.ValueOf(f).Pointer()).To(Equal(reflect.ValueOf(instance.GetFunc()).Pointer())) - - By("Returning false if the type does not implement inject.Injector") - res, err = InjectorInto(f, uninjectable) - Expect(err).NotTo(HaveOccurred()) - Expect(res).To(Equal(false)) - Expect(uninjectable.GetFunc()).To(BeNil()) - - By("Returning an error if dependencies injection fails") - res, err = InjectorInto(nil, instance) - Expect(err).To(Equal(errInjectFail)) - Expect(res).To(Equal(true)) - }) - -}) - -type testSource struct { - scheme *runtime.Scheme - f Func -} - -func (s *testSource) InjectScheme(scheme *runtime.Scheme) error { - if scheme != nil { - s.scheme = scheme - return nil - } - return fmt.Errorf("injection fails") -} - -func (s *testSource) InjectFunc(f Func) error { - if f != nil { - s.f = f - return nil - } - return fmt.Errorf("injection fails") -} - -func (s *testSource) GetScheme() *runtime.Scheme { - return s.scheme -} - -func (s *testSource) GetFunc() Func { - return s.f -} - -type failSource struct { - scheme *runtime.Scheme - f Func -} - -func (s *failSource) GetScheme() *runtime.Scheme { - return s.scheme -} - -func (s *failSource) GetFunc() Func { - return s.f -} diff --git a/pkg/webhook/admission/decode.go b/pkg/webhook/admission/decode.go index c7cb71b755..445a304293 100644 --- a/pkg/webhook/admission/decode.go +++ b/pkg/webhook/admission/decode.go @@ -32,8 +32,11 @@ type Decoder struct { } // NewDecoder creates a Decoder given the runtime.Scheme. -func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) { - return &Decoder{codecs: serializer.NewCodecFactory(scheme)}, nil +func NewDecoder(scheme *runtime.Scheme) *Decoder { + if scheme == nil { + panic("scheme should never be nil") + } + return &Decoder{codecs: serializer.NewCodecFactory(scheme)} } // Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object. diff --git a/pkg/webhook/admission/decode_test.go b/pkg/webhook/admission/decode_test.go index f92db10a6c..66586da790 100644 --- a/pkg/webhook/admission/decode_test.go +++ b/pkg/webhook/admission/decode_test.go @@ -32,9 +32,7 @@ var _ = Describe("Admission Webhook Decoder", func() { var decoder *Decoder BeforeEach(func() { By("creating a new decoder for a scheme") - var err error - decoder, err = NewDecoder(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) + decoder = NewDecoder(scheme.Scheme) Expect(decoder).NotTo(BeNil()) }) diff --git a/pkg/webhook/admission/defaulter.go b/pkg/webhook/admission/defaulter.go index e4e0778f57..a3b7207168 100644 --- a/pkg/webhook/admission/defaulter.go +++ b/pkg/webhook/admission/defaulter.go @@ -33,9 +33,9 @@ type Defaulter interface { } // DefaultingWebhookFor creates a new Webhook for Defaulting the provided type. -func DefaultingWebhookFor(defaulter Defaulter) *Webhook { +func DefaultingWebhookFor(scheme *runtime.Scheme, defaulter Defaulter) *Webhook { return &Webhook{ - Handler: &mutatingHandler{defaulter: defaulter}, + Handler: &mutatingHandler{defaulter: defaulter, decoder: NewDecoder(scheme)}, } } @@ -44,16 +44,11 @@ type mutatingHandler struct { decoder *Decoder } -var _ DecoderInjector = &mutatingHandler{} - -// InjectDecoder injects the decoder into a mutatingHandler. -func (h *mutatingHandler) InjectDecoder(d *Decoder) error { - h.decoder = d - return nil -} - // Handle handles admission requests. func (h *mutatingHandler) Handle(ctx context.Context, req Request) Response { + if h.decoder == nil { + panic("decoder should never be nil") + } if h.defaulter == nil { panic("defaulter should never be nil") } diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index 7007984245..5f697e7dce 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -34,9 +34,9 @@ type CustomDefaulter interface { } // WithCustomDefaulter creates a new Webhook for a CustomDefaulter interface. -func WithCustomDefaulter(obj runtime.Object, defaulter CustomDefaulter) *Webhook { +func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter CustomDefaulter) *Webhook { return &Webhook{ - Handler: &defaulterForType{object: obj, defaulter: defaulter}, + Handler: &defaulterForType{object: obj, defaulter: defaulter, decoder: NewDecoder(scheme)}, } } @@ -46,15 +46,11 @@ type defaulterForType struct { decoder *Decoder } -var _ DecoderInjector = &defaulterForType{} - -func (h *defaulterForType) InjectDecoder(d *Decoder) error { - h.decoder = d - return nil -} - // Handle handles admission requests. func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { + if h.decoder == nil { + panic("decoder should never be nil") + } if h.defaulter == nil { panic("defaulter should never be nil") } diff --git a/pkg/webhook/admission/defaulter_test.go b/pkg/webhook/admission/defaulter_test.go index 63635c8456..cf7571663c 100644 --- a/pkg/webhook/admission/defaulter_test.go +++ b/pkg/webhook/admission/defaulter_test.go @@ -10,16 +10,13 @@ import ( admissionv1 "k8s.io/api/admission/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Defaulter Handler", func() { It("should return ok if received delete verb in defaulter handler", func() { obj := &TestDefaulter{} - handler := DefaultingWebhookFor(obj) - Expect(inject.LoggerInto(log, handler)).To(BeTrue()) + handler := DefaultingWebhookFor(admissionScheme, obj) resp := handler.Handle(context.TODO(), Request{ AdmissionRequest: admissionv1.AdmissionRequest{ diff --git a/pkg/webhook/admission/doc.go b/pkg/webhook/admission/doc.go index 0b274dd02b..8dc0cbec6f 100644 --- a/pkg/webhook/admission/doc.go +++ b/pkg/webhook/admission/doc.go @@ -20,9 +20,3 @@ Package admission provides implementation for admission webhook and methods to i See examples/mutatingwebhook.go and examples/validatingwebhook.go for examples of admission webhooks. */ package admission - -import ( - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" -) - -var log = logf.RuntimeLog.WithName("admission") diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index f23c553e56..be10aea459 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -29,9 +29,6 @@ import ( . "github.com/onsi/gomega" admissionv1 "k8s.io/api/admission/v1" - - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Admission Webhooks", func() { @@ -50,8 +47,6 @@ var _ = Describe("Admission Webhooks", func() { respRecorder = &httptest.ResponseRecorder{ Body: bytes.NewBuffer(nil), } - _, err := inject.LoggerInto(log.WithName("test-webhook"), webhook) - Expect(err).NotTo(HaveOccurred()) }) It("should return bad-request when given an empty body", func() { @@ -96,7 +91,6 @@ var _ = Describe("Admission Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} @@ -112,7 +106,6 @@ var _ = Describe("Admission Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} @@ -128,7 +121,6 @@ var _ = Describe("Admission Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} @@ -152,7 +144,6 @@ var _ = Describe("Admission Webhooks", func() { return Allowed(ctx.Value(key).(string)) }, }, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"message":%q,"code":200}}} @@ -180,7 +171,6 @@ var _ = Describe("Admission Webhooks", func() { WithContextFunc: func(ctx context.Context, r *http.Request) context.Context { return context.WithValue(ctx, key, r.Header["Content-Type"][0]) }, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"message":%q,"code":200}}} @@ -199,7 +189,6 @@ var _ = Describe("Admission Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } bw := &brokenWriter{ResponseWriter: respRecorder} @@ -219,20 +208,8 @@ type nopCloser struct { func (nopCloser) Close() error { return nil } type fakeHandler struct { - invoked bool - fn func(context.Context, Request) Response - decoder *Decoder - injectedString string -} - -func (h *fakeHandler) InjectDecoder(d *Decoder) error { - h.decoder = d - return nil -} - -func (h *fakeHandler) InjectString(s string) error { - h.injectedString = s - return nil + invoked bool + fn func(context.Context, Request) Response } func (h *fakeHandler) Handle(ctx context.Context, req Request) Response { diff --git a/pkg/webhook/admission/inject.go b/pkg/webhook/admission/inject.go deleted file mode 100644 index d5af0d598f..0000000000 --- a/pkg/webhook/admission/inject.go +++ /dev/null @@ -1,31 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package admission - -// DecoderInjector is used by the ControllerManager to inject decoder into webhook handlers. -type DecoderInjector interface { - InjectDecoder(*Decoder) error -} - -// InjectDecoderInto will set decoder on i and return the result if it implements Decoder. Returns -// false if i does not implement Decoder. -func InjectDecoderInto(decoder *Decoder, i interface{}) (bool, error) { - if s, ok := i.(DecoderInjector); ok { - return true, s.InjectDecoder(decoder) - } - return false, nil -} diff --git a/pkg/webhook/admission/multi.go b/pkg/webhook/admission/multi.go index 26900cf2eb..2f7820d04b 100644 --- a/pkg/webhook/admission/multi.go +++ b/pkg/webhook/admission/multi.go @@ -25,8 +25,6 @@ import ( jsonpatch "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) type multiMutating []Handler @@ -62,31 +60,6 @@ func (hs multiMutating) Handle(ctx context.Context, req Request) Response { } } -// InjectFunc injects the field setter into the handlers. -func (hs multiMutating) InjectFunc(f inject.Func) error { - // inject directly into the handlers. It would be more correct - // to do this in a sync.Once in Handle (since we don't have some - // other start/finalize-type method), but it's more efficient to - // do it here, presumably. - for _, handler := range hs { - if err := f(handler); err != nil { - return err - } - } - - return nil -} - -// InjectDecoder injects the decoder into the handlers. -func (hs multiMutating) InjectDecoder(d *Decoder) error { - for _, handler := range hs { - if _, err := InjectDecoderInto(d, handler); err != nil { - return err - } - } - return nil -} - // MultiMutatingHandler combines multiple mutating webhook handlers into a single // mutating webhook handler. Handlers are called in sequential order, and the first // `allowed: false` response may short-circuit the rest. Users must take care to @@ -120,28 +93,3 @@ func (hs multiValidating) Handle(ctx context.Context, req Request) Response { func MultiValidatingHandler(handlers ...Handler) Handler { return multiValidating(handlers) } - -// InjectFunc injects the field setter into the handlers. -func (hs multiValidating) InjectFunc(f inject.Func) error { - // inject directly into the handlers. It would be more correct - // to do this in a sync.Once in Handle (since we don't have some - // other start/finalize-type method), but it's more efficient to - // do it here, presumably. - for _, handler := range hs { - if err := f(handler); err != nil { - return err - } - } - - return nil -} - -// InjectDecoder injects the decoder into the handlers. -func (hs multiValidating) InjectDecoder(d *Decoder) error { - for _, handler := range hs { - if _, err := InjectDecoderInto(d, handler); err != nil { - return err - } - } - return nil -} diff --git a/pkg/webhook/admission/validator.go b/pkg/webhook/admission/validator.go index 4b27e75ede..43ea3ee65f 100644 --- a/pkg/webhook/admission/validator.go +++ b/pkg/webhook/admission/validator.go @@ -35,9 +35,9 @@ type Validator interface { } // ValidatingWebhookFor creates a new Webhook for validating the provided type. -func ValidatingWebhookFor(validator Validator) *Webhook { +func ValidatingWebhookFor(scheme *runtime.Scheme, validator Validator) *Webhook { return &Webhook{ - Handler: &validatingHandler{validator: validator}, + Handler: &validatingHandler{validator: validator, decoder: NewDecoder(scheme)}, } } @@ -46,16 +46,11 @@ type validatingHandler struct { decoder *Decoder } -var _ DecoderInjector = &validatingHandler{} - -// InjectDecoder injects the decoder into a validatingHandler. -func (h *validatingHandler) InjectDecoder(d *Decoder) error { - h.decoder = d - return nil -} - // Handle handles admission requests. func (h *validatingHandler) Handle(ctx context.Context, req Request) Response { + if h.decoder == nil { + panic("decoder should never be nil") + } if h.validator == nil { panic("validator should never be nil") } diff --git a/pkg/webhook/admission/validator_custom.go b/pkg/webhook/admission/validator_custom.go index d2256acc5f..755e464a91 100644 --- a/pkg/webhook/admission/validator_custom.go +++ b/pkg/webhook/admission/validator_custom.go @@ -35,9 +35,9 @@ type CustomValidator interface { } // WithCustomValidator creates a new Webhook for validating the provided type. -func WithCustomValidator(obj runtime.Object, validator CustomValidator) *Webhook { +func WithCustomValidator(scheme *runtime.Scheme, obj runtime.Object, validator CustomValidator) *Webhook { return &Webhook{ - Handler: &validatorForType{object: obj, validator: validator}, + Handler: &validatorForType{object: obj, validator: validator, decoder: NewDecoder(scheme)}, } } @@ -47,16 +47,11 @@ type validatorForType struct { decoder *Decoder } -var _ DecoderInjector = &validatorForType{} - -// InjectDecoder injects the decoder into a validatingHandler. -func (h *validatorForType) InjectDecoder(d *Decoder) error { - h.decoder = d - return nil -} - // Handle handles admission requests. func (h *validatorForType) Handle(ctx context.Context, req Request) Response { + if h.decoder == nil { + panic("decoder should never be nil") + } if h.validator == nil { panic("validator should never be nil") } diff --git a/pkg/webhook/admission/validator_test.go b/pkg/webhook/admission/validator_test.go index 782b3a14ec..3da8f0b46a 100644 --- a/pkg/webhook/admission/validator_test.go +++ b/pkg/webhook/admission/validator_test.go @@ -37,7 +37,7 @@ var fakeValidatorVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v var _ = Describe("validatingHandler", func() { - decoder, _ := NewDecoder(scheme.Scheme) + decoder := NewDecoder(scheme.Scheme) Context("when dealing with successful results", func() { diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index dcff043db6..13bb0f0cc7 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -21,20 +21,16 @@ import ( "errors" "fmt" "net/http" + "sync" "github.com/go-logr/logr" "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/json" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/client-go/kubernetes/scheme" "k8s.io/klog/v2" - - internallog "sigs.k8s.io/controller-runtime/pkg/internal/log" logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) @@ -142,13 +138,8 @@ type Webhook struct { // decoder is constructed on receiving a scheme and passed down to then handler decoder *Decoder - log logr.Logger -} - -// InjectLogger gets a handle to a logging instance, hopefully with more info about this particular webhook. -func (wh *Webhook) InjectLogger(l logr.Logger) error { - wh.log = l - return nil + setupLogOnce sync.Once + log logr.Logger } // WithRecoverPanic takes a bool flag which indicates whether the panic caused by webhook should be recovered. @@ -189,6 +180,12 @@ func (wh *Webhook) Handle(ctx context.Context, req Request) (response Response) // getLogger constructs a logger from the injected log and LogConstructor. func (wh *Webhook) getLogger(req *Request) logr.Logger { + wh.setupLogOnce.Do(func() { + if wh.log.GetSink() == nil { + wh.log = logf.Log.WithName("admission") + } + }) + logConstructor := wh.LogConstructor if logConstructor == nil { logConstructor = DefaultLogConstructor @@ -207,70 +204,14 @@ func DefaultLogConstructor(base logr.Logger, req *Request) logr.Logger { return base } -// InjectScheme injects a scheme into the webhook, in order to construct a Decoder. -func (wh *Webhook) InjectScheme(s *runtime.Scheme) error { - // TODO(directxman12): we should have a better way to pass this down - - var err error - wh.decoder, err = NewDecoder(s) - if err != nil { - return err - } - - // inject the decoder here too, just in case the order of calling this is not - // scheme first, then inject func - if wh.Handler != nil { - if _, err := InjectDecoderInto(wh.GetDecoder(), wh.Handler); err != nil { - return err - } - } - - return nil -} - // GetDecoder returns a decoder to decode the objects embedded in admission requests. // It may be nil if we haven't received a scheme to use to determine object types yet. func (wh *Webhook) GetDecoder() *Decoder { return wh.decoder } -// InjectFunc injects the field setter into the webhook. -func (wh *Webhook) InjectFunc(f inject.Func) error { - // inject directly into the handlers. It would be more correct - // to do this in a sync.Once in Handle (since we don't have some - // other start/finalize-type method), but it's more efficient to - // do it here, presumably. - - // also inject a decoder, and wrap this so that we get a setFields - // that injects a decoder (hopefully things don't ignore the duplicate - // InjectorInto call). - - var setFields inject.Func - setFields = func(target interface{}) error { - if err := f(target); err != nil { - return err - } - - if _, err := inject.InjectorInto(setFields, target); err != nil { - return err - } - - if _, err := InjectDecoderInto(wh.GetDecoder(), target); err != nil { - return err - } - - return nil - } - - return setFields(wh.Handler) -} - // StandaloneOptions let you configure a StandaloneWebhook. type StandaloneOptions struct { - // Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources - // Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always better - // idea to pass your own scheme in. See the documentation in pkg/scheme for more information. - Scheme *runtime.Scheme // Logger to be used by the webhook. // If none is set, it defaults to log.Log global logger. Logger logr.Logger @@ -290,19 +231,9 @@ type StandaloneOptions struct { // in your own server/mux. In order to be accessed by a kubernetes cluster, // all webhook servers require TLS. func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, error) { - if opts.Scheme == nil { - opts.Scheme = scheme.Scheme - } - - if err := hook.InjectScheme(opts.Scheme); err != nil { - return nil, err - } - - if opts.Logger.GetSink() == nil { - opts.Logger = internallog.RuntimeLog.WithName("webhook") + if opts.Logger.GetSink() != nil { + hook.log = opts.Logger } - hook.log = opts.Logger - if opts.MetricsPath == "" { return hook, nil } diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index 6c140d4728..5d603a5e6f 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -29,13 +29,10 @@ import ( admissionv1 "k8s.io/api/admission/v1" authenticationv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" machinerytypes "k8s.io/apimachinery/pkg/types" - internallog "sigs.k8s.io/controller-runtime/pkg/internal/log" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Admission Webhooks", func() { @@ -61,7 +58,6 @@ var _ = Describe("Admission Webhooks", func() { } webhook := &Webhook{ Handler: handler, - log: internallog.RuntimeLog.WithName("webhook"), } return webhook @@ -111,7 +107,6 @@ var _ = Describe("Admission Webhooks", func() { }, } }), - log: internallog.RuntimeLog.WithName("webhook"), } By("invoking the webhook") @@ -128,7 +123,6 @@ var _ = Describe("Admission Webhooks", func() { Handler: HandlerFunc(func(ctx context.Context, req Request) Response { return Patched("", jsonpatch.Operation{Operation: "add", Path: "/a", Value: 2}, jsonpatch.Operation{Operation: "replace", Path: "/b", Value: 4}) }), - log: internallog.RuntimeLog.WithName("webhook"), } By("invoking the webhook") @@ -204,86 +198,17 @@ var _ = Describe("Admission Webhooks", func() { Eventually(logBuffer).Should(gbytes.Say(`"msg":"Received request","operation":"CREATE","requestID":"test123"}`)) }) - Describe("dependency injection", func() { - It("should set dependencies passed in on the handler", func() { - By("setting up a webhook and injecting it with a injection func that injects a string") - setFields := func(target interface{}) error { - inj, ok := target.(stringInjector) - if !ok { - return nil - } - - return inj.InjectString("something") - } - handler := &fakeHandler{} - webhook := &Webhook{ - Handler: handler, - log: internallog.RuntimeLog.WithName("webhook"), - } - Expect(setFields(webhook)).To(Succeed()) - Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) - - By("checking that the string was injected") - Expect(handler.injectedString).To(Equal("something")) - }) - - It("should inject a decoder into the handler", func() { - By("setting up a webhook and injecting it with a injection func that injects a scheme") - setFields := func(target interface{}) error { - if _, err := inject.SchemeInto(runtime.NewScheme(), target); err != nil { - return err - } - return nil - } - handler := &fakeHandler{} - webhook := &Webhook{ - Handler: handler, - log: internallog.RuntimeLog.WithName("webhook"), - } - Expect(setFields(webhook)).To(Succeed()) - Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) - - By("checking that the decoder was injected") - Expect(handler.decoder).NotTo(BeNil()) - }) - - It("should pass a setFields that also injects a decoder into sub-dependencies", func() { - By("setting up a webhook and injecting it with a injection func that injects a scheme") - setFields := func(target interface{}) error { - if _, err := inject.SchemeInto(runtime.NewScheme(), target); err != nil { - return err - } - return nil - } - handler := &handlerWithSubDependencies{ - Handler: HandlerFunc(func(ctx context.Context, req Request) Response { - return Response{} - }), - dep: &subDep{}, - } - webhook := &Webhook{ - Handler: handler, - } - Expect(setFields(webhook)).To(Succeed()) - Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) - - By("checking that setFields sets the decoder as well") - Expect(handler.dep.decoder).NotTo(BeNil()) - }) - }) - Describe("panic recovery", func() { It("should recover panic if RecoverPanic is true", func() { panicHandler := func() *Webhook { handler := &fakeHandler{ fn: func(ctx context.Context, req Request) Response { - panic("injected panic") + panic("fake panic test") }, } webhook := &Webhook{ Handler: handler, RecoverPanic: true, - log: internallog.RuntimeLog.WithName("webhook"), } return webhook @@ -298,19 +223,18 @@ var _ = Describe("Admission Webhooks", func() { By("checking that it errored the request") Expect(resp.Allowed).To(BeFalse()) Expect(resp.Result.Code).To(Equal(int32(http.StatusInternalServerError))) - Expect(resp.Result.Message).To(Equal("panic: injected panic [recovered]")) + Expect(resp.Result.Message).To(Equal("panic: fake panic test [recovered]")) }) It("should not recover panic if RecoverPanic is false by default", func() { panicHandler := func() *Webhook { handler := &fakeHandler{ fn: func(ctx context.Context, req Request) Response { - panic("injected panic") + panic("fake panic test") }, } webhook := &Webhook{ Handler: handler, - log: internallog.RuntimeLog.WithName("webhook"), } return webhook @@ -342,25 +266,3 @@ var _ = Describe("Should be able to write/read admission.Request to/from context Expect(err).To(Not(HaveOccurred())) Expect(gotRequest).To(Equal(testRequest)) }) - -type stringInjector interface { - InjectString(s string) error -} - -type handlerWithSubDependencies struct { - Handler - dep *subDep -} - -func (h *handlerWithSubDependencies) InjectFunc(f inject.Func) error { - return f(h.dep) -} - -type subDep struct { - decoder *Decoder -} - -func (d *subDep) InjectDecoder(dec *Decoder) error { - d.decoder = dec - return nil -} diff --git a/pkg/webhook/authentication/doc.go b/pkg/webhook/authentication/doc.go index a1b45c1aef..d2b85f378c 100644 --- a/pkg/webhook/authentication/doc.go +++ b/pkg/webhook/authentication/doc.go @@ -21,9 +21,3 @@ methods to implement authentication webhook handlers. See examples/tokenreview/ for an example of authentication webhooks. */ package authentication - -import ( - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" -) - -var log = logf.RuntimeLog.WithName("authentication") diff --git a/pkg/webhook/authentication/http.go b/pkg/webhook/authentication/http.go index 59832e8a07..dfed3efea0 100644 --- a/pkg/webhook/authentication/http.go +++ b/pkg/webhook/authentication/http.go @@ -52,7 +52,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { var reviewResponse Response if r.Body == nil { err = errors.New("request body is empty") - wh.log.Error(err, "bad request") + wh.getLogger(nil).Error(err, "bad request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -60,7 +60,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { defer r.Body.Close() if body, err = io.ReadAll(r.Body); err != nil { - wh.log.Error(err, "unable to read the body from the incoming request") + wh.getLogger(nil).Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -69,7 +69,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { // verify the content type is accurate if contentType := r.Header.Get("Content-Type"); contentType != "application/json" { err = fmt.Errorf("contentType=%s, expected application/json", contentType) - wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) + wh.getLogger(nil).Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -89,16 +89,16 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { ar.SetGroupVersionKind(authenticationv1.SchemeGroupVersion.WithKind("TokenReview")) _, actualTokRevGVK, err := authenticationCodecs.UniversalDeserializer().Decode(body, nil, &ar) if err != nil { - wh.log.Error(err, "unable to decode the request") + wh.getLogger(&req).Error(err, "unable to decode the request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return } - wh.log.V(1).Info("received request", "UID", req.UID, "kind", req.Kind) + wh.getLogger(&req).V(1).Info("received request", "UID", req.UID, "kind", req.Kind) if req.Spec.Token == "" { err = errors.New("token is empty") - wh.log.Error(err, "bad request") + wh.getLogger(&req).Error(err, "bad request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -131,12 +131,12 @@ func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, tokRevGVK // writeTokenResponse writes ar to w. func (wh *Webhook) writeTokenResponse(w io.Writer, ar authenticationv1.TokenReview) { if err := json.NewEncoder(w).Encode(ar); err != nil { - wh.log.Error(err, "unable to encode the response") + wh.getLogger(nil).Error(err, "unable to encode the response") wh.writeResponse(w, Errored(err)) } res := ar - if log := wh.log; log.V(1).Enabled() { - log.V(1).Info("wrote response", "UID", res.UID, "authenticated", res.Status.Authenticated) + if wh.getLogger(nil).V(1).Enabled() { + wh.getLogger(nil).V(1).Info("wrote response", "UID", res.UID, "authenticated", res.Status.Authenticated) } } diff --git a/pkg/webhook/authentication/http_test.go b/pkg/webhook/authentication/http_test.go index 32227ee144..86bd5d0153 100644 --- a/pkg/webhook/authentication/http_test.go +++ b/pkg/webhook/authentication/http_test.go @@ -27,9 +27,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" authenticationv1 "k8s.io/api/authentication/v1" - - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Authentication Webhooks", func() { @@ -47,8 +44,6 @@ var _ = Describe("Authentication Webhooks", func() { respRecorder = &httptest.ResponseRecorder{ Body: bytes.NewBuffer(nil), } - _, err := inject.LoggerInto(log.WithName("test-webhook"), webhook) - Expect(err).NotTo(HaveOccurred()) }) It("should return bad-request when given an empty body", func() { @@ -107,7 +102,6 @@ var _ = Describe("Authentication Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"authenticated":true,"user":{}}} @@ -125,7 +119,6 @@ var _ = Describe("Authentication Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"authenticated":true,"user":{}}} @@ -150,7 +143,6 @@ var _ = Describe("Authentication Webhooks", func() { return Authenticated(ctx.Value(key).(string), authenticationv1.UserInfo{}) }, }, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"authenticated":true,"user":{},"error":%q}} @@ -179,7 +171,6 @@ var _ = Describe("Authentication Webhooks", func() { WithContextFunc: func(ctx context.Context, r *http.Request) context.Context { return context.WithValue(ctx, key, r.Header["Content-Type"][0]) }, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"authenticated":true,"user":{},"error":%q}} @@ -200,14 +191,8 @@ type nopCloser struct { func (nopCloser) Close() error { return nil } type fakeHandler struct { - invoked bool - fn func(context.Context, Request) Response - injectedString string -} - -func (h *fakeHandler) InjectString(s string) error { - h.injectedString = s - return nil + invoked bool + fn func(context.Context, Request) Response } func (h *fakeHandler) Handle(ctx context.Context, req Request) Response { diff --git a/pkg/webhook/authentication/webhook.go b/pkg/webhook/authentication/webhook.go index b1229e422e..7c5e627aa5 100644 --- a/pkg/webhook/authentication/webhook.go +++ b/pkg/webhook/authentication/webhook.go @@ -20,11 +20,13 @@ import ( "context" "errors" "net/http" + "sync" "github.com/go-logr/logr" authenticationv1 "k8s.io/api/authentication/v1" + "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + logf "sigs.k8s.io/controller-runtime/pkg/log" ) var ( @@ -85,45 +87,39 @@ type Webhook struct { // headers thus allowing you to read them from within the handler WithContextFunc func(context.Context, *http.Request) context.Context - log logr.Logger -} - -// InjectLogger gets a handle to a logging instance, hopefully with more info about this particular webhook. -func (wh *Webhook) InjectLogger(l logr.Logger) error { - wh.log = l - return nil + setupLogOnce sync.Once + log logr.Logger } // Handle processes TokenReview. func (wh *Webhook) Handle(ctx context.Context, req Request) Response { resp := wh.Handler.Handle(ctx, req) if err := resp.Complete(req); err != nil { - wh.log.Error(err, "unable to encode response") + wh.getLogger(&req).Error(err, "unable to encode response") return Errored(errUnableToEncodeResponse) } return resp } -// InjectFunc injects the field setter into the webhook. -func (wh *Webhook) InjectFunc(f inject.Func) error { - // inject directly into the handlers. It would be more correct - // to do this in a sync.Once in Handle (since we don't have some - // other start/finalize-type method), but it's more efficient to - // do it here, presumably. - - var setFields inject.Func - setFields = func(target interface{}) error { - if err := f(target); err != nil { - return err +// getLogger constructs a logger from the injected log and LogConstructor. +func (wh *Webhook) getLogger(req *Request) logr.Logger { + wh.setupLogOnce.Do(func() { + if wh.log.GetSink() == nil { + wh.log = logf.Log.WithName("authentication") } + }) - if _, err := inject.InjectorInto(setFields, target); err != nil { - return err - } + return logConstructor(wh.log, req) +} - return nil +// logConstructor adds some commonly interesting fields to the given logger. +func logConstructor(base logr.Logger, req *Request) logr.Logger { + if req != nil { + return base.WithValues("object", klog.KRef(req.Namespace, req.Name), + "namespace", req.Namespace, "name", req.Name, + "user", req.Status.User.Username, + ) } - - return setFields(wh.Handler) + return base } diff --git a/pkg/webhook/authentication/webhook_test.go b/pkg/webhook/authentication/webhook_test.go index e80096395e..3df446d898 100644 --- a/pkg/webhook/authentication/webhook_test.go +++ b/pkg/webhook/authentication/webhook_test.go @@ -25,9 +25,6 @@ import ( authenticationv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" machinerytypes "k8s.io/apimachinery/pkg/types" - - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Authentication Webhooks", func() { @@ -45,7 +42,6 @@ var _ = Describe("Authentication Webhooks", func() { } webhook := &Webhook{ Handler: handler, - log: logf.RuntimeLog.WithName("webhook"), } return webhook @@ -97,7 +93,6 @@ var _ = Describe("Authentication Webhooks", func() { }, } }), - log: logf.RuntimeLog.WithName("webhook"), } By("invoking the webhook") @@ -108,33 +103,4 @@ var _ = Describe("Authentication Webhooks", func() { Expect(resp.Status.Authenticated).To(BeTrue()) Expect(resp.Status.Error).To(Equal("Ground Control to Major Tom")) }) - - Describe("dependency injection", func() { - It("should set dependencies passed in on the handler", func() { - By("setting up a webhook and injecting it with a injection func that injects a string") - setFields := func(target interface{}) error { - inj, ok := target.(stringInjector) - if !ok { - return nil - } - - return inj.InjectString("something") - } - handler := &fakeHandler{} - webhook := &Webhook{ - Handler: handler, - log: logf.RuntimeLog.WithName("webhook"), - } - Expect(setFields(webhook)).To(Succeed()) - Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) - - By("checking that the string was injected") - Expect(handler.injectedString).To(Equal("something")) - }) - - }) }) - -type stringInjector interface { - InjectString(s string) error -} diff --git a/pkg/webhook/conversion/conversion.go b/pkg/webhook/conversion/conversion.go index 879aae3c9b..cab6878b1b 100644 --- a/pkg/webhook/conversion/conversion.go +++ b/pkg/webhook/conversion/conversion.go @@ -45,18 +45,6 @@ type Webhook struct { decoder *Decoder } -// InjectScheme injects a scheme into the webhook, in order to construct a Decoder. -func (wh *Webhook) InjectScheme(s *runtime.Scheme) error { - var err error - wh.scheme = s - wh.decoder, err = NewDecoder(s) - if err != nil { - return err - } - - return nil -} - // ensure Webhook implements http.Handler var _ http.Handler = &Webhook{} diff --git a/pkg/webhook/conversion/conversion_test.go b/pkg/webhook/conversion/conversion_test.go index 6a6c9a7611..ec7e4ab990 100644 --- a/pkg/webhook/conversion/conversion_test.go +++ b/pkg/webhook/conversion/conversion_test.go @@ -43,7 +43,7 @@ var _ = Describe("Conversion Webhook", func() { var respRecorder *httptest.ResponseRecorder var decoder *Decoder var scheme *runtime.Scheme - webhook := Webhook{} + var webhook *Webhook BeforeEach(func() { respRecorder = &httptest.ResponseRecorder{ @@ -55,12 +55,9 @@ var _ = Describe("Conversion Webhook", func() { Expect(jobsv1.AddToScheme(scheme)).To(Succeed()) Expect(jobsv2.AddToScheme(scheme)).To(Succeed()) Expect(jobsv3.AddToScheme(scheme)).To(Succeed()) - Expect(webhook.InjectScheme(scheme)).To(Succeed()) - - var err error - decoder, err = NewDecoder(scheme) - Expect(err).NotTo(HaveOccurred()) + decoder = NewDecoder(scheme) + webhook = &Webhook{scheme: scheme, decoder: decoder} }) doRequest := func(convReq *apix.ConversionReview) *apix.ConversionReview { diff --git a/pkg/webhook/conversion/decoder.go b/pkg/webhook/conversion/decoder.go index 6a9e9c2365..b6bb8bd938 100644 --- a/pkg/webhook/conversion/decoder.go +++ b/pkg/webhook/conversion/decoder.go @@ -30,8 +30,11 @@ type Decoder struct { } // NewDecoder creates a Decoder given the runtime.Scheme -func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) { - return &Decoder{codecs: serializer.NewCodecFactory(scheme)}, nil +func NewDecoder(scheme *runtime.Scheme) *Decoder { + if scheme == nil { + panic("scheme should never be nil") + } + return &Decoder{codecs: serializer.NewCodecFactory(scheme)} } // Decode decodes the inlined object. diff --git a/pkg/webhook/example_test.go b/pkg/webhook/example_test.go index e7872ae5da..7c9fbfe24b 100644 --- a/pkg/webhook/example_test.go +++ b/pkg/webhook/example_test.go @@ -20,7 +20,6 @@ import ( "context" "net/http" - "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/manager/signals" @@ -87,7 +86,7 @@ func Example() { // Note that this assumes and requires a valid TLS // cert and key at the default locations // tls.crt and tls.key. -func ExampleServer_StartStandalone() { +func ExampleServer_Start() { // Create a webhook server hookServer := &Server{ Port: 8443, @@ -98,7 +97,7 @@ func ExampleServer_StartStandalone() { hookServer.Register("/validating", validatingHook) // Start the server without a manger - err := hookServer.StartStandalone(signals.SetupSignalHandler(), scheme.Scheme) + err := hookServer.Start(signals.SetupSignalHandler()) if err != nil { // handle error panic(err) @@ -118,7 +117,6 @@ func ExampleStandaloneWebhook() { // Create the standalone HTTP handlers from our webhooks mutatingHookHandler, err := admission.StandaloneWebhook(mutatingHook, admission.StandaloneOptions{ - Scheme: scheme.Scheme, // Logger let's you optionally pass // a custom logger (defaults to log.Log global Logger) Logger: logf.RuntimeLog.WithName("mutating-webhook"), @@ -134,7 +132,6 @@ func ExampleStandaloneWebhook() { } validatingHookHandler, err := admission.StandaloneWebhook(validatingHook, admission.StandaloneOptions{ - Scheme: scheme.Scheme, Logger: logf.RuntimeLog.WithName("validating-webhook"), MetricsPath: "/validating", }) diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index d9d8ca6ad8..6b68ae3ed5 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -29,12 +29,9 @@ import ( "sync" "time" - "k8s.io/apimachinery/pkg/runtime" - kscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/internal/httpserver" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) @@ -83,13 +80,9 @@ type Server struct { // WebhookMux is the multiplexer that handles different webhooks. WebhookMux *http.ServeMux - // webhooks keep track of all registered webhooks for dependency injection, - // and to provide better panic messages on duplicate webhook registration. + // webhooks keep track of all registered webhooks webhooks map[string]http.Handler - // setFields allows injecting dependencies from an external source - setFields inject.Func - // defaultingOnce ensures that the default fields are only ever set once. defaultingOnce sync.Once @@ -141,51 +134,11 @@ func (s *Server) Register(path string, hook http.Handler) { if _, found := s.webhooks[path]; found { panic(fmt.Errorf("can't register duplicate path: %v", path)) } - // TODO(directxman12): call setfields if we've already started the server s.webhooks[path] = hook s.WebhookMux.Handle(path, metrics.InstrumentedHook(path, hook)) regLog := log.WithValues("path", path) regLog.Info("Registering webhook") - - // we've already been "started", inject dependencies here. - // Otherwise, InjectFunc will do this for us later. - if s.setFields != nil { - if err := s.setFields(hook); err != nil { - // TODO(directxman12): swallowing this error isn't great, but we'd have to - // change the signature to fix that - regLog.Error(err, "unable to inject fields into webhook during registration") - } - - baseHookLog := log.WithName("webhooks") - - // NB(directxman12): we don't propagate this further by wrapping setFields because it's - // unclear if this is how we want to deal with log propagation. In this specific instance, - // we want to be able to pass a logger to webhooks because they don't know their own path. - if _, err := inject.LoggerInto(baseHookLog.WithValues("webhook", path), hook); err != nil { - regLog.Error(err, "unable to logger into webhook during registration") - } - } -} - -// StartStandalone runs a webhook server without -// a controller manager. -func (s *Server) StartStandalone(ctx context.Context, scheme *runtime.Scheme) error { - // Use the Kubernetes client-go scheme if none is specified - if scheme == nil { - scheme = kscheme.Scheme - } - - if err := s.InjectFunc(func(i interface{}) error { - if _, err := inject.SchemeInto(scheme, i); err != nil { - return err - } - return nil - }); err != nil { - return err - } - - return s.Start(ctx) } // tlsVersion converts from human-readable TLS version (for example "1.1") @@ -324,24 +277,3 @@ func (s *Server) StartedChecker() healthz.Checker { return nil } } - -// InjectFunc injects the field setter into the server. -func (s *Server) InjectFunc(f inject.Func) error { - s.setFields = f - - // inject fields here that weren't injected in Register because we didn't have setFields yet. - baseHookLog := log.WithName("webhooks") - for hookPath, webhook := range s.webhooks { - if err := s.setFields(webhook); err != nil { - return err - } - - // NB(directxman12): we don't propagate this further by wrapping setFields because it's - // unclear if this is how we want to deal with log propagation. In this specific instance, - // we want to be able to pass a logger to webhooks because they don't know their own path. - if _, err := inject.LoggerInto(baseHookLog.WithValues("webhook", hookPath), webhook); err != nil { - return err - } - } - return nil -} diff --git a/pkg/webhook/server_test.go b/pkg/webhook/server_test.go index 68b4dcf076..e9b40a1542 100644 --- a/pkg/webhook/server_test.go +++ b/pkg/webhook/server_test.go @@ -26,7 +26,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -83,15 +82,6 @@ var _ = Describe("Webhook Server", func() { return err }).Should(Succeed()) - // this is normally called before Start by the manager - Expect(server.InjectFunc(func(i interface{}) error { - boolInj, canInj := i.(interface{ InjectBool(bool) error }) - if !canInj { - return nil - } - return boolInj.InjectBool(true) - })).To(Succeed()) - return doneCh } @@ -145,17 +135,6 @@ var _ = Describe("Webhook Server", func() { ctxCancel() Eventually(doneCh, "4s").Should(BeClosed()) }) - - It("should inject dependencies eventually, given an inject func is eventually provided", func() { - handler := &testHandler{} - server.Register("/somepath", handler) - doneCh := startServer() - - Eventually(func() bool { return handler.injectedField }).Should(BeTrue()) - - ctxCancel() - Eventually(doneCh, "4s").Should(BeClosed()) - }) }) Context("when registering webhooks after starting", func() { @@ -179,34 +158,6 @@ var _ = Describe("Webhook Server", func() { Expect(io.ReadAll(resp.Body)).To(Equal([]byte("gadzooks!"))) }) - - It("should inject dependencies, if an inject func has been provided already", func() { - handler := &testHandler{} - server.Register("/somepath", handler) - Expect(handler.injectedField).To(BeTrue()) - }) - }) - - It("should be able to serve in unmanaged mode", func() { - server = &webhook.Server{ - Host: servingOpts.LocalServingHost, - Port: servingOpts.LocalServingPort, - CertDir: servingOpts.LocalServingCertDir, - } - server.Register("/somepath", &testHandler{}) - doneCh := genericStartServer(func(ctx context.Context) { - Expect(server.StartStandalone(ctx, scheme.Scheme)) - }) - - Eventually(func() ([]byte, error) { - resp, err := client.Get(fmt.Sprintf("https://%s/somepath", testHostPort)) - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - return io.ReadAll(resp.Body) - }).Should(Equal([]byte("gadzooks!"))) - - ctxCancel() - Eventually(doneCh, "4s").Should(BeClosed()) }) It("should respect passed in TLS configurations", func() { @@ -230,7 +181,7 @@ var _ = Describe("Webhook Server", func() { } server.Register("/somepath", &testHandler{}) doneCh := genericStartServer(func(ctx context.Context) { - Expect(server.StartStandalone(ctx, scheme.Scheme)) + Expect(server.Start(ctx)) }) Eventually(func() ([]byte, error) { @@ -251,13 +202,8 @@ var _ = Describe("Webhook Server", func() { }) type testHandler struct { - injectedField bool } -func (t *testHandler) InjectBool(val bool) error { - t.injectedField = val - return nil -} func (t *testHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) { if _, err := resp.Write([]byte("gadzooks!")); err != nil { panic("unable to write http response!") diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index fd545aae06..1d3d228278 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -19,11 +19,6 @@ package webhook_test import ( "context" "crypto/tls" - "errors" - "net" - "net/http" - "path/filepath" - "strconv" "strings" "time" @@ -33,15 +28,10 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/internal/httpserver" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/admissiontest" ) var _ = Describe("Webhook", func() { @@ -90,7 +80,7 @@ var _ = Describe("Webhook", func() { }) // we need manager here just to leverage manager.SetFields Expect(err).NotTo(HaveOccurred()) server := m.GetWebhookServer() - server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{}}) + server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{d: admission.NewDecoder(testenv.Scheme)}}) ctx, cancel := context.WithCancel(context.Background()) go func() { @@ -114,7 +104,7 @@ var _ = Describe("Webhook", func() { }) // we need manager here just to leverage manager.SetFields Expect(err).NotTo(HaveOccurred()) server := m.GetWebhookServer() - server.Register("/failing", &webhook.Admission{Handler: admission.MultiValidatingHandler(&rejectingValidator{})}) + server.Register("/failing", &webhook.Admission{Handler: admission.MultiValidatingHandler(&rejectingValidator{d: admission.NewDecoder(testenv.Scheme)})}) ctx, cancel := context.WithCancel(context.Background()) go func() { @@ -137,11 +127,11 @@ var _ = Describe("Webhook", func() { Host: testenv.WebhookInstallOptions.LocalServingHost, CertDir: testenv.WebhookInstallOptions.LocalServingCertDir, } - server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{}}) + server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{d: admission.NewDecoder(testenv.Scheme)}}) ctx, cancel := context.WithCancel(context.Background()) go func() { - err := server.StartStandalone(ctx, scheme.Scheme) + err := server.Start(ctx) Expect(err).NotTo(HaveOccurred()) }() @@ -150,62 +140,6 @@ var _ = Describe("Webhook", func() { return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden }, 1*time.Second).Should(BeTrue()) - cancel() - }) - }) - Context("when running a standalone webhook", func() { - It("should reject create request for webhook that rejects all requests", func() { - ctx, cancel := context.WithCancel(context.Background()) - - By("generating the TLS config") - certPath := filepath.Join(testenv.WebhookInstallOptions.LocalServingCertDir, "tls.crt") - keyPath := filepath.Join(testenv.WebhookInstallOptions.LocalServingCertDir, "tls.key") - - certWatcher, err := certwatcher.New(certPath, keyPath) - Expect(err).NotTo(HaveOccurred()) - go func() { - Expect(certWatcher.Start(ctx)).NotTo(HaveOccurred()) - }() - - cfg := &tls.Config{ - NextProtos: []string{"h2"}, - GetCertificate: certWatcher.GetCertificate, - MinVersion: tls.VersionTLS12, - } - - By("generating the listener") - listener, err := tls.Listen("tcp", - net.JoinHostPort(testenv.WebhookInstallOptions.LocalServingHost, - strconv.Itoa(testenv.WebhookInstallOptions.LocalServingPort)), cfg) - Expect(err).NotTo(HaveOccurred()) - - By("creating and registering the standalone webhook") - hook, err := admission.StandaloneWebhook(admission.ValidatingWebhookFor( - &admissiontest.FakeValidator{ - ErrorToReturn: errors.New("Always denied"), - GVKToReturn: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - }), admission.StandaloneOptions{}) - Expect(err).NotTo(HaveOccurred()) - http.Handle("/failing", hook) - - By("running the http server") - srv := httpserver.New(nil) - go func() { - idleConnsClosed := make(chan struct{}) - go func() { - <-ctx.Done() - Expect(srv.Shutdown(context.Background())).NotTo(HaveOccurred()) - close(idleConnsClosed) - }() - _ = srv.Serve(listener) - <-idleConnsClosed - }() - - Eventually(func() bool { - err = c.Create(context.TODO(), obj) - return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden - }, 1*time.Second).Should(BeTrue()) - cancel() }) }) @@ -215,11 +149,6 @@ type rejectingValidator struct { d *admission.Decoder } -func (v *rejectingValidator) InjectDecoder(d *admission.Decoder) error { - v.d = d - return nil -} - func (v *rejectingValidator) Handle(ctx context.Context, req admission.Request) admission.Response { var obj appsv1.Deployment if err := v.d.Decode(req, &obj); err != nil {