From c90869657b2c36018edf8f1eacb720cc39ad31c6 Mon Sep 17 00:00:00 2001 From: cbarbian-sap <52255556+cbarbian-sap@users.noreply.github.com> Date: Sun, 6 Oct 2024 18:39:01 +0200 Subject: [PATCH] Refactoring misc (#156) * refactor client factory * refactor context package * add explaining comments * refactor deep merging of maps * rename test suite descriptions * refactor version handling for executables --- .github/workflows/publish-scaffold.yaml | 6 +- internal/cluster/client.go | 34 +++++ internal/cluster/factory.go | 26 +--- internal/helm/suite_test.go | 2 +- internal/version/version.go | 40 +++++ pkg/component/context.go | 30 ++-- pkg/component/reconciler.go | 4 +- pkg/component/target.go | 2 +- pkg/manifests/helm/generator.go | 2 + pkg/manifests/kustomize/generator.go | 2 + pkg/manifests/suite_test.go | 18 +++ pkg/manifests/util.go | 24 ++- pkg/manifests/util_test.go | 192 ++++++++++++++++++++++++ pkg/status/suite_test.go | 2 +- scaffold/scaffold.go | 8 +- 15 files changed, 340 insertions(+), 52 deletions(-) create mode 100644 internal/version/version.go create mode 100644 pkg/manifests/suite_test.go create mode 100644 pkg/manifests/util_test.go diff --git a/.github/workflows/publish-scaffold.yaml b/.github/workflows/publish-scaffold.yaml index 10c219f..9e9265c 100644 --- a/.github/workflows/publish-scaffold.yaml +++ b/.github/workflows/publish-scaffold.yaml @@ -28,7 +28,11 @@ jobs: for arch in amd64 arm64; do file=bin/scaffold-$os-$arch echo "Building $file ..." - GOOS=$os GOARCH=$arch go build -o $file -ldflags "-X main.version=${{ github.event.release.tag_name }}" ./scaffold + LDFLAGS="" + LDFLAGS+=" -X github.com/sap/component-operator-runtime/internal/version.version=${{ github.event.release.tag_name }}" + LDFLAGS+=" -X github.com/sap/component-operator-runtime/internal/version.gitCommit=${{ github.sha }}" + LDFLAGS+=" -X github.com/sap/component-operator-runtime/internal/version.gitTreeState=clean" + GOOS=$os GOARCH=$arch go build -o $file -ldflags "$LDFLAGS" ./scaffold done done diff --git a/internal/cluster/client.go b/internal/cluster/client.go index 39bdee7..ad3d70c 100644 --- a/internal/cluster/client.go +++ b/internal/cluster/client.go @@ -8,7 +8,12 @@ package cluster import ( "time" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/discovery" + "k8s.io/client-go/kubernetes" + typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -21,6 +26,35 @@ func NewClient(clnt client.Client, discoveryClient discovery.DiscoveryInterface, } } +func NewClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (Client, error) { + return newClientFor(config, scheme, name) +} + +func newClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*clientImpl, error) { + httpClient, err := rest.HTTPClientFor(config) + if err != nil { + return nil, err + } + ctrlClient, err := client.New(config, client.Options{HTTPClient: httpClient, Scheme: scheme}) + if err != nil { + return nil, err + } + clientset, err := kubernetes.NewForConfigAndClient(config, httpClient) + if err != nil { + return nil, err + } + eventBroadcaster := record.NewBroadcaster() + eventBroadcaster.StartRecordingToSink(&typedcorev1.EventSinkImpl{Interface: clientset.CoreV1().Events("")}) + eventRecorder := eventBroadcaster.NewRecorder(scheme, corev1.EventSource{Component: name}) + clnt := &clientImpl{ + Client: ctrlClient, + discoveryClient: clientset, + eventBroadcaster: eventBroadcaster, + eventRecorder: eventRecorder, + } + return clnt, nil +} + type clientImpl struct { client.Client discoveryClient discovery.DiscoveryInterface diff --git a/internal/cluster/factory.go b/internal/cluster/factory.go index 57df99d..ef5bc83 100644 --- a/internal/cluster/factory.go +++ b/internal/cluster/factory.go @@ -14,17 +14,12 @@ import ( "sync" "time" - corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" - typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" - "k8s.io/client-go/tools/record" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/sap/component-operator-runtime/internal/metrics" "github.com/sap/component-operator-runtime/pkg/types" @@ -132,28 +127,11 @@ func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, imperso return rt.RoundTrip(r) }) }) - httpClient, err := rest.HTTPClientFor(config) + clnt, err := newClientFor(config, f.scheme, f.name) if err != nil { return nil, err } - ctrlClient, err := client.New(config, client.Options{HTTPClient: httpClient, Scheme: f.scheme}) - if err != nil { - return nil, err - } - clientset, err := kubernetes.NewForConfigAndClient(config, httpClient) - if err != nil { - return nil, err - } - eventBroadcaster := record.NewBroadcaster() - eventBroadcaster.StartRecordingToSink(&typedcorev1.EventSinkImpl{Interface: clientset.CoreV1().Events("")}) - eventRecorder := eventBroadcaster.NewRecorder(f.scheme, corev1.EventSource{Component: f.name}) - clnt := &clientImpl{ - Client: ctrlClient, - discoveryClient: clientset, - eventBroadcaster: eventBroadcaster, - eventRecorder: eventRecorder, - validUntil: time.Now().Add(validity), - } + clnt.validUntil = time.Now().Add(validity) f.clients[key] = clnt metrics.CreatedClients.WithLabelValues(f.controllerName).Inc() metrics.ActiveClients.WithLabelValues(f.controllerName).Set(float64(len(f.clients))) diff --git a/internal/helm/suite_test.go b/internal/helm/suite_test.go index 72c2000..045c131 100644 --- a/internal/helm/suite_test.go +++ b/internal/helm/suite_test.go @@ -14,5 +14,5 @@ import ( func TestComponent(t *testing.T) { RegisterFailHandler(Fail) - RunSpecs(t, "Component Suite") + RunSpecs(t, "Package tests") } diff --git a/internal/version/version.go b/internal/version/version.go new file mode 100644 index 0000000..5b4fcc9 --- /dev/null +++ b/internal/version/version.go @@ -0,0 +1,40 @@ +/* +SPDX-FileCopyrightText: 2023 SAP SE or an SAP affiliate company and component-operator-runtime contributors +SPDX-License-Identifier: Apache-2.0 +*/ + +package version + +import ( + "runtime" +) + +var ( + version = "latest" + metadata = "" + gitCommit = "" + gitTreeState = "" +) + +type BuildInfo struct { + Version string `json:"version,omitempty"` + GitCommit string `json:"gitCommit,omitempty"` + GitTreeState string `json:"gitTreeState,omitempty"` + GoVersion string `json:"goVersion,omitempty"` +} + +func GetVersion() string { + if metadata == "" { + return version + } + return version + "+" + metadata +} + +func GetBuildInfo() BuildInfo { + return BuildInfo{ + Version: GetVersion(), + GitCommit: gitCommit, + GitTreeState: gitTreeState, + GoVersion: runtime.Version(), + } +} diff --git a/pkg/component/context.go b/pkg/component/context.go index a86b4e4..c49a810 100644 --- a/pkg/component/context.go +++ b/pkg/component/context.go @@ -26,28 +26,36 @@ var ( componentDigestContextKey = componentDigestContextKeyType{} ) -func newContext(ctx context.Context) *reconcileContext { - return &reconcileContext{Context: ctx} +type Context interface { + context.Context + WithReconcilerName(reconcilerName string) Context + WithClient(clnt cluster.Client) Context + WithComponent(component Component) Context + WithComponentDigest(componentDigest string) Context +} + +func NewContext(ctx context.Context) Context { + return &contextImpl{Context: ctx} } -type reconcileContext struct { +type contextImpl struct { context.Context } -func (c *reconcileContext) WithReconcilerName(reconcilerName string) *reconcileContext { - return &reconcileContext{Context: context.WithValue(c, reconcilerNameContextKey, reconcilerName)} +func (c *contextImpl) WithReconcilerName(reconcilerName string) Context { + return &contextImpl{Context: context.WithValue(c, reconcilerNameContextKey, reconcilerName)} } -func (c *reconcileContext) WithClient(clnt cluster.Client) *reconcileContext { - return &reconcileContext{Context: context.WithValue(c, clientContextKey, clnt)} +func (c *contextImpl) WithClient(clnt cluster.Client) Context { + return &contextImpl{Context: context.WithValue(c, clientContextKey, clnt)} } -func (c *reconcileContext) WithComponent(component Component) *reconcileContext { - return &reconcileContext{Context: context.WithValue(c, componentContextKey, component)} +func (c *contextImpl) WithComponent(component Component) Context { + return &contextImpl{Context: context.WithValue(c, componentContextKey, component)} } -func (c *reconcileContext) WithComponentDigest(componentDigest string) *reconcileContext { - return &reconcileContext{Context: context.WithValue(c, componentDigestContextKey, componentDigest)} +func (c *contextImpl) WithComponentDigest(componentDigest string) Context { + return &contextImpl{Context: context.WithValue(c, componentDigestContextKey, componentDigest)} } func ReconcilerNameFromContext(ctx context.Context) (string, error) { diff --git a/pkg/component/reconciler.go b/pkg/component/reconciler.go index 0d0cf84..7f2ad23 100644 --- a/pkg/component/reconciler.go +++ b/pkg/component/reconciler.go @@ -311,7 +311,7 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result // note: it's important that this happens after deferring the status handler // TODO: enhance ctx with tailored logger and event recorder // TODO: enhance ctx with the local client - hookCtx := newContext(ctx).WithReconcilerName(r.name) + hookCtx := NewContext(ctx).WithReconcilerName(r.name) for hookOrder, hook := range r.postReadHooks { if err := hook(hookCtx, r.client, component); err != nil { return ctrl.Result{}, errors.Wrapf(err, "error running post-read hook (%d)", hookOrder) @@ -337,7 +337,7 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result }) // TODO: enhance ctx with tailored logger and event recorder // TODO: enhance ctx with the local client - hookCtx = newContext(ctx).WithReconcilerName(r.name).WithClient(targetClient) + hookCtx = NewContext(ctx).WithReconcilerName(r.name).WithClient(targetClient) // do the reconciliation if component.GetDeletionTimestamp().IsZero() { diff --git a/pkg/component/target.go b/pkg/component/target.go index af8c2ca..f1af9a8 100644 --- a/pkg/component/target.go +++ b/pkg/component/target.go @@ -52,7 +52,7 @@ func (t *reconcileTarget[T]) Apply(ctx context.Context, component T) (bool, stri componentDigest := calculateComponentDigest(component) // TODO: enhance ctx with local client - generateCtx := newContext(ctx). + generateCtx := NewContext(ctx). WithReconcilerName(t.reconcilerName). WithClient(t.client). WithComponent(component). diff --git a/pkg/manifests/helm/generator.go b/pkg/manifests/helm/generator.go index 610c284..98ea6e5 100644 --- a/pkg/manifests/helm/generator.go +++ b/pkg/manifests/helm/generator.go @@ -25,6 +25,8 @@ import ( // HelmGenerator is a Generator implementation that basically renders a given Helm chart. // A few restrictions apply to the provided Helm chart: it must not contain any subcharts, some template functions are not supported, // some bultin variables are not supported, and hooks are processed in a slightly different fashion. +// Note: HelmGenerator's Generate() method expects client and reconciler name to be set in the passed context; +// see: Context.WithClient() and Context.WithReconcilerName() in package pkg/component. type HelmGenerator struct { client client.Client chart *helm.Chart diff --git a/pkg/manifests/kustomize/generator.go b/pkg/manifests/kustomize/generator.go index 4d2fdb4..2b4b40e 100644 --- a/pkg/manifests/kustomize/generator.go +++ b/pkg/manifests/kustomize/generator.go @@ -53,6 +53,8 @@ type KustomizeGeneratorOptions struct { } // KustomizeGenerator is a Generator implementation that basically renders a given Kustomization. +// Note: KustomizeGenerator's Generate() method expects client and component to be set in the passed context; +// see: Context.WithClient() and Context.WithComponent() in package pkg/component. type KustomizeGenerator struct { kustomizer *krusty.Kustomizer files map[string][]byte diff --git a/pkg/manifests/suite_test.go b/pkg/manifests/suite_test.go new file mode 100644 index 0000000..48db289 --- /dev/null +++ b/pkg/manifests/suite_test.go @@ -0,0 +1,18 @@ +/* +SPDX-FileCopyrightText: 2023 SAP SE or an SAP affiliate company and component-operator-runtime contributors +SPDX-License-Identifier: Apache-2.0 +*/ + +package manifests_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestComponent(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Package tests") +} diff --git a/pkg/manifests/util.go b/pkg/manifests/util.go index 746724d..acf759f 100644 --- a/pkg/manifests/util.go +++ b/pkg/manifests/util.go @@ -14,26 +14,34 @@ import ( // Deep-merge two maps with the usual logic and return the result. // The first map (x) must be deeply JSON (i.e. consist deeply of JSON values only). // The maps given as input will not be changed. +// Both maps can be passed as nil. func MergeMaps(x, y map[string]any) map[string]any { if x == nil { x = make(map[string]any) } else { x = runtime.DeepCopyJSON(x) } - for k, w := range y { - if v, ok := x[k]; ok { - if v, ok := v.(map[string]any); ok { - if _w, ok := w.(map[string]any); ok { - x[k] = MergeMaps(v, _w) + MergeMapInto(x, y) + return x +} + +// Deep-merge second map (y) over first map (x) with the usual logic. +// The first map will be changed (unless y is empty or nil), the second map will not be changed. +// The first map must not be nil, the second map is allowed to be nil. +func MergeMapInto(x map[string]any, y map[string]any) { + for k := range y { + if _, ok := x[k]; ok { + if v, ok := x[k].(map[string]any); ok { + if w, ok := y[k].(map[string]any); ok { + MergeMapInto(v, w) } else { x[k] = w } } else { - x[k] = w + x[k] = y[k] } } else { - x[k] = w + x[k] = y[k] } } - return x } diff --git a/pkg/manifests/util_test.go b/pkg/manifests/util_test.go new file mode 100644 index 0000000..4ccc6dc --- /dev/null +++ b/pkg/manifests/util_test.go @@ -0,0 +1,192 @@ +/* +SPDX-FileCopyrightText: 2023 SAP SE or an SAP affiliate company and component-operator-runtime contributors +SPDX-License-Identifier: Apache-2.0 +*/ + +package manifests_test + +import ( + "encoding/json" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/sap/component-operator-runtime/pkg/manifests" +) + +var _ = Describe("testing: util.go", func() { + DescribeTable("testing: MergeMaps()", + func(xs, ys, rs string) { + x := jsonUnmarshal(xs) + y := jsonUnmarshal(ys) + Expect(manifests.MergeMaps(x, y)).To(Equal(jsonUnmarshal(rs))) + Expect(x).To(Equal(jsonUnmarshal(xs))) + Expect(y).To(Equal(jsonUnmarshal(ys))) + }, + + Entry(nil, + `{ + "a": { + "a": { + "x": 1 + }, + "b": { + "x": 1 + }, + "u": [ + 1 + ], + "v": [ + 1 + ], + "x": 1, + "y": 1 + }, + "b": { + "x": 1 + }, + "u": [ + 1 + ], + "v": [ + 1 + ], + "x": 1, + "y": 1 + }`, + `{ + "a": { + "a": { + "x": 2, + "z": 2 + }, + "u": 2, + "x": 2, + "z": 2 + }, + "u": 2, + "x": 2, + "z": 2 + }`, + `{ + "a": { + "a": { + "x": 2, + "z": 2 + }, + "b": { + "x": 1 + }, + "u": 2, + "v": [ + 1 + ], + "x": 2, + "y": 1, + "z": 2 + }, + "b": { + "x": 1 + }, + "u": 2, + "v": [ + 1 + ], + "x": 2, + "y": 1, + "z": 2 + }`, + ), + ) + + DescribeTable("testing: MergeMapInto()", + func(xs, ys, rs string) { + x := jsonUnmarshal(xs) + y := jsonUnmarshal(ys) + manifests.MergeMapInto(x, y) + Expect(x).To(Equal(jsonUnmarshal(rs))) + Expect(y).To(Equal(jsonUnmarshal(ys))) + }, + + Entry(nil, + `{ + "a": { + "a": { + "x": 1 + }, + "b": { + "x": 1 + }, + "u": [ + 1 + ], + "v": [ + 1 + ], + "x": 1, + "y": 1 + }, + "b": { + "x": 1 + }, + "u": [ + 1 + ], + "v": [ + 1 + ], + "x": 1, + "y": 1 + }`, + `{ + "a": { + "a": { + "x": 2, + "z": 2 + }, + "u": 2, + "x": 2, + "z": 2 + }, + "u": 2, + "x": 2, + "z": 2 + }`, + `{ + "a": { + "a": { + "x": 2, + "z": 2 + }, + "b": { + "x": 1 + }, + "u": 2, + "v": [ + 1 + ], + "x": 2, + "y": 1, + "z": 2 + }, + "b": { + "x": 1 + }, + "u": 2, + "v": [ + 1 + ], + "x": 2, + "y": 1, + "z": 2 + }`, + ), + ) +}) + +func jsonUnmarshal(s string) (x map[string]any) { + if err := json.Unmarshal([]byte(s), &x); err != nil { + panic(err) + } + return +} diff --git a/pkg/status/suite_test.go b/pkg/status/suite_test.go index d5dad39..29e8b9e 100644 --- a/pkg/status/suite_test.go +++ b/pkg/status/suite_test.go @@ -14,5 +14,5 @@ import ( func TestComponent(t *testing.T) { RegisterFailHandler(Fail) - RunSpecs(t, "Component Suite") + RunSpecs(t, "Package tests") } diff --git a/scaffold/scaffold.go b/scaffold/scaffold.go index e35f5a6..e4bd63a 100644 --- a/scaffold/scaffold.go +++ b/scaffold/scaffold.go @@ -21,9 +21,12 @@ import ( "github.com/Masterminds/sprig/v3" "github.com/spf13/pflag" + apimeta "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" kyaml "sigs.k8s.io/yaml" + + "github.com/sap/component-operator-runtime/internal/version" ) type Config struct { @@ -83,7 +86,6 @@ var templates embed.FS // default verions var ( goVersion = "1.21.7" - version = "latest" kubernetesVersion = "v0.29.2" controllerRuntimeVersion = "v0.17.2" controllerToolsVersion = "v0.14.0" @@ -96,7 +98,7 @@ func main() { errlog := log.New(os.Stderr, "", 0) showVersion := false - config := Config{Version: version} + config := Config{Version: version.GetVersion()} outputDir := "" skipPostProcessing := false @@ -131,7 +133,7 @@ func main() { outputDir = pflag.Arg(0) if showVersion { - fmt.Println(version) + fmt.Println(version.GetVersion()) os.Exit(0) }