From 2a2b561ab0b4cf0fa575b124e58c9d9c8f2085d2 Mon Sep 17 00:00:00 2001 From: justinsb Date: Mon, 5 Feb 2024 08:50:57 -0500 Subject: [PATCH 1/2] Allow http client reuse in dynamic client By using ForConfigAndClient, we are more likely to reuse the http connection. We expose an HTTPClient, but also we default to the manager HTTPClient. --- pkg/patterns/declarative/watch.go | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/pkg/patterns/declarative/watch.go b/pkg/patterns/declarative/watch.go index f41a020a..ec93bb3c 100644 --- a/pkg/patterns/declarative/watch.go +++ b/pkg/patterns/declarative/watch.go @@ -55,6 +55,9 @@ type WatchChildrenOptions struct { // RESTConfig is the configuration for connecting to the cluster. RESTConfig *rest.Config + // HTTPClient is the HTTP client to use for requests. + HTTPClient *http.Client + // LabelMaker is used to build the labels we should watch on. LabelMaker LabelMaker @@ -88,6 +91,21 @@ func WatchChildren(options WatchChildrenOptions) error { return fmt.Errorf("labelMaker is required to scope watches") } + var httpClient *http.Client + if options.HTTPClient != nil { + httpClient = options.HTTPClient + } else { + if options.RESTConfig != nil { + hc, err := rest.HTTPClientFor(options.RESTConfig) + if err != nil { + return err + } + httpClient = hc + } else if options.Manager != nil { + httpClient = options.Manager.GetHTTPClient() + } + } + if options.RESTConfig == nil { if options.Manager != nil { options.RESTConfig = options.Manager.GetConfig() @@ -100,23 +118,19 @@ func WatchChildren(options WatchChildrenOptions) error { if options.Manager != nil { restMapper = options.Manager.GetRESTMapper() } else { - client, err := rest.HTTPClientFor(options.RESTConfig) - if err != nil { - return err - } - rm, err := commonclient.NewDiscoveryRESTMapper(options.RESTConfig, client) + rm, err := commonclient.NewDiscoveryRESTMapper(options.RESTConfig, httpClient) if err != nil { return err } restMapper = rm } - client, err := dynamic.NewForConfig(options.RESTConfig) + dynamicClient, err := dynamic.NewForConfigAndClient(options.RESTConfig, httpClient) if err != nil { return err } - dw, events, err := watch.NewDynamicWatch(restMapper, client) + dw, events, err := watch.NewDynamicWatch(restMapper, dynamicClient) if err != nil { return fmt.Errorf("creating dynamic watch: %v", err) } From d5c69924df9c0a6282d77e62168cf65bd2eeafd0 Mon Sep 17 00:00:00 2001 From: justinsb Date: Mon, 5 Feb 2024 08:58:14 -0500 Subject: [PATCH 2/2] Add developer hints We've evolved a lot over time, and we could start nudging developers to use the now-recommended approaches and avoid the less-recommended approaches (without breaking compatability). Introduce a "hints" package that enables this. --- pkg/hints/log.go | 33 +++++++++++++++++++ .../declarative/pkg/applier/applylib.go | 2 ++ pkg/patterns/declarative/watch.go | 2 ++ 3 files changed, 37 insertions(+) create mode 100644 pkg/hints/log.go diff --git a/pkg/hints/log.go b/pkg/hints/log.go new file mode 100644 index 00000000..8c511441 --- /dev/null +++ b/pkg/hints/log.go @@ -0,0 +1,33 @@ +package hints + +import ( + "context" + "fmt" + "os" + + "k8s.io/klog/v2" +) + +// EnvRecommendationsAreStrict is the environment variable that can be set +// to make developer recommendations a panic. It can be set by developers +// while developing to help them keep up with changes to +// kubebuilder-declarative-pattern and any upstream changes. +const EnvRecommendationsAreStrict = "FAIL_ON_DEVELOPER_RECOMMENDATIONS" + +// DeveloperRecommendation will log a recommendation for developers; +// helping developers keep up with changes here and upstream. +// If EnvRecommendationsAreStrict is set, we will panic. +func DeveloperRecommendation(ctx context.Context, msg string, keysAndValues ...any) { + log := klog.FromContext(ctx) + log.Info(msg, keysAndValues...) + if os.Getenv(EnvRecommendationsAreStrict) != "" { + s := fmt.Sprintf("recommendation for developers with %s environment variable set, so treating as a failure: %s", EnvRecommendationsAreStrict, msg) + for i := 0; (i + 1) < len(keysAndValues); i++ { + if i != 0 { + s += ", " + } + s += fmt.Sprintf("%v:%v", keysAndValues[i], keysAndValues[i+1]) + } + panic(s) + } +} diff --git a/pkg/patterns/declarative/pkg/applier/applylib.go b/pkg/patterns/declarative/pkg/applier/applylib.go index 5592e303..08cb1b22 100644 --- a/pkg/patterns/declarative/pkg/applier/applylib.go +++ b/pkg/patterns/declarative/pkg/applier/applylib.go @@ -12,6 +12,7 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/klog/v2" "sigs.k8s.io/kubebuilder-declarative-pattern/applylib/applyset" + "sigs.k8s.io/kubebuilder-declarative-pattern/pkg/hints" ) type ApplysetOptions struct { @@ -57,6 +58,7 @@ func (a *ApplySetApplier) Apply(ctx context.Context, opt ApplierOptions) error { dynamicClient := opt.DynamicClient if dynamicClient == nil { + hints.DeveloperRecommendation(context.Background(), "consider setting ApplierOptions.DynamicClient", "applierOptions", opt) d, err := dynamic.NewForConfig(opt.RESTConfig) if err != nil { return fmt.Errorf("error building dynamic client: %w", err) diff --git a/pkg/patterns/declarative/watch.go b/pkg/patterns/declarative/watch.go index ec93bb3c..7e19f94a 100644 --- a/pkg/patterns/declarative/watch.go +++ b/pkg/patterns/declarative/watch.go @@ -33,6 +33,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/source" "sigs.k8s.io/kubebuilder-declarative-pattern/commonclient" + "sigs.k8s.io/kubebuilder-declarative-pattern/pkg/hints" "sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/watch" ) @@ -96,6 +97,7 @@ func WatchChildren(options WatchChildrenOptions) error { httpClient = options.HTTPClient } else { if options.RESTConfig != nil { + hints.DeveloperRecommendation(context.Background(), "consider setting WatchChildrenOptions.HTTPClient, if setting RESTConfig", "options", options) hc, err := rest.HTTPClientFor(options.RESTConfig) if err != nil { return err