From ba812bfa8da8133ad4740b56d140a1258bde56bc Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Tue, 19 Sep 2023 14:31:53 +0100 Subject: [PATCH 1/5] fix: Handle multiple meta mutators cleanly This applies each of the mutators before generating patches. --- .golangci.yml | 4 + cmd/main.go | 2 +- .../handlers/cluster_context.go | 48 ++++ .../clustertopology/handlers/mutation/meta.go | 70 +++-- .../handlers/mutation/meta_test.go | 265 +++++++++--------- common/pkg/testutils/capitest/patches.go | 65 +---- .../pkg/testutils/capitest/request/items.go | 69 +++++ .../testutils/capitest/serializer/tojson.go | 21 ++ pkg/handlers/auditpolicy/inject.go | 131 ++++----- pkg/handlers/auditpolicy/inject_test.go | 3 +- pkg/handlers/etcd/inject.go | 121 ++++---- pkg/handlers/etcd/inject_test.go | 7 +- pkg/handlers/extraapiservercertsans/inject.go | 101 +++---- .../extraapiservercertsans/inject_test.go | 3 +- pkg/handlers/httpproxy/inject.go | 166 +++++------ pkg/handlers/httpproxy/inject_test.go | 7 +- .../kubernetesimagerepository/inject.go | 101 +++---- .../kubernetesimagerepository/inject_test.go | 3 +- 18 files changed, 644 insertions(+), 543 deletions(-) create mode 100644 common/pkg/capi/clustertopology/handlers/cluster_context.go create mode 100644 common/pkg/testutils/capitest/request/items.go create mode 100644 common/pkg/testutils/capitest/serializer/tojson.go diff --git a/.golangci.yml b/.golangci.yml index 3d698e04e..5d5de98bb 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -85,3 +85,7 @@ issues: - path: "api/*" linters: - gochecknoinits + # Idiomatic to use init functions to register APIs with scheme + - text: "hugeParam: holderRef is heavy" + linters: + - gocritic diff --git a/cmd/main.go b/cmd/main.go index 1bb58d9f6..4788b9928 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -136,7 +136,7 @@ func main() { } // This metaPatchHandlers combines all other patch and variable handlers under a single handler. // It allows to specify configuration under a single variable. - metaPatchHandlers := []mutation.GeneratePatches{ + metaPatchHandlers := []mutation.MetaMutater{ httpproxy.NewMetaPatch(mgr.GetClient()), extraapiservercertsans.NewMetaPatch(), auditpolicy.NewPatch(), diff --git a/common/pkg/capi/clustertopology/handlers/cluster_context.go b/common/pkg/capi/clustertopology/handlers/cluster_context.go new file mode 100644 index 000000000..31567d2e5 --- /dev/null +++ b/common/pkg/capi/clustertopology/handlers/cluster_context.go @@ -0,0 +1,48 @@ +// Copyright 2023 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package handlers + +import ( + "context" + "errors" + + capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// clusterKeyContextKey is how we find a cluster key from a context.Context. +type clusterKeyContextKey struct{} + +func ClusterKeyFrom(ctx context.Context) (client.ObjectKey, error) { + clusterKey, ok := ctx.Value(clusterKeyContextKey{}).(client.ObjectKey) + if !ok || clusterKey.Name == "" { + return client.ObjectKey{}, errors.New( + "failed to detect cluster name from GeneratePatch request", + ) + } + + return clusterKey, nil +} + +func ClusterKeyInto( + ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, +) context.Context { + clusterKey := client.ObjectKey{} + + for i := range req.Items { + item := req.Items[i] + if item.HolderReference.Kind == "Cluster" && + item.HolderReference.APIVersion == capiv1.GroupVersion.String() { + clusterKey.Name = item.HolderReference.Name + clusterKey.Namespace = item.HolderReference.Namespace + } + } + + if clusterKey.Name != "" { + return context.WithValue(ctx, clusterKeyContextKey{}, clusterKey) + } + + return ctx +} diff --git a/common/pkg/capi/clustertopology/handlers/mutation/meta.go b/common/pkg/capi/clustertopology/handlers/mutation/meta.go index 28681a027..b4bb2748c 100644 --- a/common/pkg/capi/clustertopology/handlers/mutation/meta.go +++ b/common/pkg/capi/clustertopology/handlers/mutation/meta.go @@ -5,22 +5,44 @@ package mutation import ( "context" - "strings" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" + controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + "sigs.k8s.io/cluster-api/exp/runtime/topologymutation" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers" ) +type MetaMutater interface { + Mutate( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error +} + type metaGeneratePatches struct { - name string - wrappedHandlers []GeneratePatches + name string + decoder runtime.Decoder + mutaters []MetaMutater } -func NewMetaGeneratePatchesHandler(name string, gp ...GeneratePatches) handlers.Named { +func NewMetaGeneratePatchesHandler(name string, m ...MetaMutater) handlers.Named { + scheme := runtime.NewScheme() + _ = bootstrapv1.AddToScheme(scheme) + _ = controlplanev1.AddToScheme(scheme) return metaGeneratePatches{ - name: name, - wrappedHandlers: gp, + name: name, + decoder: serializer.NewCodecFactory(scheme).UniversalDecoder( + controlplanev1.GroupVersion, + bootstrapv1.GroupVersion, + ), + mutaters: m, } } @@ -33,20 +55,26 @@ func (mgp metaGeneratePatches) GeneratePatches( req *runtimehooksv1.GeneratePatchesRequest, resp *runtimehooksv1.GeneratePatchesResponse, ) { - for _, h := range mgp.wrappedHandlers { - wrappedResp := &runtimehooksv1.GeneratePatchesResponse{} - h.GeneratePatches(ctx, req, wrappedResp) - resp.Items = append(resp.Items, wrappedResp.Items...) - if wrappedResp.Message != "" { - resp.Message = strings.TrimPrefix(resp.Message+"\n"+wrappedResp.Message, "\n") - } - resp.Status = wrappedResp.Status - if resp.Status == runtimehooksv1.ResponseStatusFailure { - return - } - } + ctx = handlers.ClusterKeyInto(ctx, req) - if resp.Status == "" { - resp.Status = runtimehooksv1.ResponseStatusSuccess - } + topologymutation.WalkTemplates( + ctx, + mgp.decoder, + req, + resp, + func( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error { + for _, h := range mgp.mutaters { + if err := h.Mutate(ctx, obj, vars, holderRef); err != nil { + return err + } + } + + return nil + }, + ) } diff --git a/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go b/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go index f31f26ed0..7d47365f7 100644 --- a/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go +++ b/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go @@ -5,26 +5,76 @@ package mutation import ( "context" + "encoding/json" + "fmt" "testing" + "github.com/go-logr/logr" "github.com/onsi/gomega" + "gomodules.xyz/jsonpatch/v2" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/runtime" + bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" + controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/patches" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/patches/selectors" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/request" ) type testHandler struct { - resp *runtimehooksv1.GeneratePatchesResponse + returnErr bool + mutateControlPlane bool } -var _ GeneratePatches = &testHandler{} +var _ MetaMutater = &testHandler{} -func (h *testHandler) GeneratePatches( +func (h *testHandler) Mutate( _ context.Context, - _ *runtimehooksv1.GeneratePatchesRequest, - resp *runtimehooksv1.GeneratePatchesResponse, -) { - resp.Items = append(resp.Items, h.resp.Items...) - resp.Message = h.resp.Message - resp.Status = h.resp.Status + obj runtime.Object, + _ map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, +) error { + if h.returnErr { + return fmt.Errorf("This is a failure") + } + + if h.mutateControlPlane { + return patches.Generate( + obj, nil, &holderRef, selectors.ControlPlane(), logr.Discard(), + func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { + obj.Spec.Template.Spec.KubeadmConfigSpec.PostKubeadmCommands = append( + obj.Spec.Template.Spec.KubeadmConfigSpec.PostKubeadmCommands, + fmt.Sprintf( + "control-plane-extra-post-kubeadm-%d", + len(obj.Spec.Template.Spec.KubeadmConfigSpec.PostKubeadmCommands), + ), + ) + return nil + }, + ) + } + + return patches.Generate( + obj, machineVars(), &holderRef, selectors.AllWorkersSelector(), logr.Discard(), + func(obj *bootstrapv1.KubeadmConfigTemplate) error { + obj.Spec.Template.Spec.PostKubeadmCommands = append( + obj.Spec.Template.Spec.PostKubeadmCommands, + fmt.Sprintf( + "worker-extra-post-kubeadm-%d", + len(obj.Spec.Template.Spec.PostKubeadmCommands), + ), + ) + return nil + }, + ) +} + +func machineVars() map[string]apiextensionsv1.JSON { + return map[string]apiextensionsv1.JSON{ + "builtin": {Raw: []byte(`{"machineDeployment": {"class": "a-worker"}}`)}, + } } func TestMetaGeneratePatches(t *testing.T) { @@ -32,7 +82,7 @@ func TestMetaGeneratePatches(t *testing.T) { tests := []struct { name string - wrappedHandlers []GeneratePatches + mutaters []MetaMutater expectedResponse *runtimehooksv1.GeneratePatchesResponse }{{ name: "no handlers", @@ -40,45 +90,46 @@ func TestMetaGeneratePatches(t *testing.T) { CommonResponse: runtimehooksv1.CommonResponse{ Status: runtimehooksv1.ResponseStatusSuccess, }, + Items: []runtimehooksv1.GeneratePatchesResponseItem{{ + UID: "kubeadm-config", + PatchType: runtimehooksv1.JSONPatchType, + Patch: jsonPatch([]jsonpatch.Operation{}...), + }, { + UID: "kubeadm-control-plane", + PatchType: runtimehooksv1.JSONPatchType, + Patch: jsonPatch([]jsonpatch.Operation{}...), + }}, }, }, { name: "single success handler", - wrappedHandlers: []GeneratePatches{ - &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusSuccess, - Message: "This is a success", - }, - Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), - }}, - }, - }, + mutaters: []MetaMutater{ + &testHandler{}, }, expectedResponse: &runtimehooksv1.GeneratePatchesResponse{ CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusSuccess, - Message: "This is a success", + Status: runtimehooksv1.ResponseStatusSuccess, }, Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", + UID: "kubeadm-config", + PatchType: runtimehooksv1.JSONPatchType, + Patch: jsonPatch( + jsonpatch.NewOperation( + "add", + "/spec/template/spec/postKubeadmCommands", + []string{"worker-extra-post-kubeadm-0"}, + ), + ), + }, { + UID: "kubeadm-control-plane", PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), + Patch: jsonPatch([]jsonpatch.Operation{}...), }}, }, }, { name: "single failure handler", - wrappedHandlers: []GeneratePatches{ + mutaters: []MetaMutater{ &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusFailure, - Message: "This is a failure", - }, - }, + returnErr: true, }, }, expectedResponse: &runtimehooksv1.GeneratePatchesResponse{ @@ -89,131 +140,57 @@ func TestMetaGeneratePatches(t *testing.T) { }, }, { name: "multiple success handlers", - wrappedHandlers: []GeneratePatches{ - &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusSuccess, - Message: "This is a success", - }, - Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), - }, { - UID: "12345", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is another patch`), - }}, - }, - }, - &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusSuccess, - Message: "This is also a success", - }, - Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "123456", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is also a patch`), - }}, - }, - }, + mutaters: []MetaMutater{ + &testHandler{}, + &testHandler{}, + &testHandler{mutateControlPlane: true}, }, expectedResponse: &runtimehooksv1.GeneratePatchesResponse{ CommonResponse: runtimehooksv1.CommonResponse{ Status: runtimehooksv1.ResponseStatusSuccess, - Message: `This is a success -This is also a success`, }, Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), - }, { - UID: "12345", + UID: "kubeadm-config", PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is another patch`), + Patch: jsonPatch( + jsonpatch.NewOperation( + "add", + "/spec/template/spec/postKubeadmCommands", + []string{"worker-extra-post-kubeadm-0", "worker-extra-post-kubeadm-1"}, + ), + ), }, { - UID: "123456", + UID: "kubeadm-control-plane", PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is also a patch`), + Patch: jsonPatch( + jsonpatch.NewOperation( + "add", + "/spec/template/spec/kubeadmConfigSpec/postKubeadmCommands", + []string{"control-plane-extra-post-kubeadm-0"}, + )), }}, }, }, { name: "success handler followed by failure handler", - wrappedHandlers: []GeneratePatches{ + mutaters: []MetaMutater{ + &testHandler{}, &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusSuccess, - Message: "This is a success", - }, - Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), - }, { - UID: "12345", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is another patch`), - }}, - }, - }, - &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusFailure, - Message: "This is a failure", - }, - }, + returnErr: true, }, }, expectedResponse: &runtimehooksv1.GeneratePatchesResponse{ CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusFailure, - Message: `This is a success -This is a failure`, + Status: runtimehooksv1.ResponseStatusFailure, + Message: "This is a failure", }, - Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), - }, { - UID: "12345", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is another patch`), - }}, }, }, { name: "failure handler followed by success handler", - wrappedHandlers: []GeneratePatches{ - &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusFailure, - Message: "This is a failure", - }, - }, - }, + mutaters: []MetaMutater{ &testHandler{ - resp: &runtimehooksv1.GeneratePatchesResponse{ - CommonResponse: runtimehooksv1.CommonResponse{ - Status: runtimehooksv1.ResponseStatusSuccess, - Message: "This is a success", - }, - Items: []runtimehooksv1.GeneratePatchesResponseItem{{ - UID: "1234", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is a patch`), - }, { - UID: "12345", - PatchType: runtimehooksv1.JSONPatchType, - Patch: []byte(`this is another patch`), - }}, - }, + returnErr: true, }, + &testHandler{}, }, expectedResponse: &runtimehooksv1.GeneratePatchesResponse{ CommonResponse: runtimehooksv1.CommonResponse{ @@ -231,12 +208,24 @@ This is a failure`, g := gomega.NewWithT(t) - h := NewMetaGeneratePatchesHandler("", tt.wrappedHandlers...).(GeneratePatches) + h := NewMetaGeneratePatchesHandler("", tt.mutaters...).(GeneratePatches) resp := &runtimehooksv1.GeneratePatchesResponse{} - h.GeneratePatches(context.Background(), &runtimehooksv1.GeneratePatchesRequest{}, resp) + h.GeneratePatches(context.Background(), &runtimehooksv1.GeneratePatchesRequest{ + Items: []runtimehooksv1.GeneratePatchesRequestItem{ + request.NewKubeadmConfigTemplateRequestItem("kubeadm-config"), + request.NewKubeadmControlPlaneTemplateRequestItem("kubeadm-control-plane"), + }, + }, resp) g.Expect(resp).To(gomega.Equal(tt.expectedResponse)) }) } } + +func jsonPatch(operations ...jsonpatch.Operation) []byte { + b, _ := json.Marshal( //nolint:errchkjson // OK to do this in a test when the type can be marshalled. + operations, + ) + return b +} diff --git a/common/pkg/testutils/capitest/patches.go b/common/pkg/testutils/capitest/patches.go index 57da4d1ec..64231043d 100644 --- a/common/pkg/testutils/capitest/patches.go +++ b/common/pkg/testutils/capitest/patches.go @@ -4,7 +4,6 @@ package capitest import ( - "bytes" "context" "encoding/json" "testing" @@ -14,14 +13,10 @@ import ( gomegatypes "github.com/onsi/gomega/types" "gomodules.xyz/jsonpatch/v2" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/uuid" - bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" - controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/serializer" ) type PatchTestDef struct { @@ -100,62 +95,6 @@ func ValidateGeneratePatches[T mutation.GeneratePatches]( func VariableWithValue(name string, value any) runtimehooksv1.Variable { return runtimehooksv1.Variable{ Name: name, - Value: apiextensionsv1.JSON{Raw: toJSON(value)}, + Value: apiextensionsv1.JSON{Raw: serializer.ToJSON(value)}, } } - -func toJSON(v any) []byte { - data, err := json.Marshal(v) - if err != nil { - panic(err) - } - compacted := &bytes.Buffer{} - if err := json.Compact(compacted, data); err != nil { - panic(err) - } - return compacted.Bytes() -} - -// requestItem returns a GeneratePatchesRequestItem with the given variables and object. -func requestItem( - object any, - holderRef *runtimehooksv1.HolderReference, -) runtimehooksv1.GeneratePatchesRequestItem { - return runtimehooksv1.GeneratePatchesRequestItem{ - UID: uuid.NewUUID(), - Object: runtime.RawExtension{ - Raw: toJSON(object), - }, - HolderReference: *holderRef, - } -} - -func NewKubeadmConfigTemplateRequestItem() runtimehooksv1.GeneratePatchesRequestItem { - return requestItem( - &bootstrapv1.KubeadmConfigTemplate{ - TypeMeta: metav1.TypeMeta{ - Kind: "KubeadmConfigTemplate", - APIVersion: bootstrapv1.GroupVersion.String(), - }, - }, - &runtimehooksv1.HolderReference{ - Kind: "MachineDeployment", - FieldPath: "spec.template.spec.infrastructureRef", - }, - ) -} - -func NewKubeadmControlPlaneTemplateRequestItem() runtimehooksv1.GeneratePatchesRequestItem { - return requestItem( - &controlplanev1.KubeadmControlPlaneTemplate{ - TypeMeta: metav1.TypeMeta{ - Kind: "KubeadmControlPlaneTemplate", - APIVersion: controlplanev1.GroupVersion.String(), - }, - }, - &runtimehooksv1.HolderReference{ - Kind: "Cluster", - FieldPath: "spec.controlPlaneRef", - }, - ) -} diff --git a/common/pkg/testutils/capitest/request/items.go b/common/pkg/testutils/capitest/request/items.go new file mode 100644 index 000000000..6eba55ca3 --- /dev/null +++ b/common/pkg/testutils/capitest/request/items.go @@ -0,0 +1,69 @@ +// Copyright 2023 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package request + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" + bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" + controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/serializer" +) + +// requestItem returns a GeneratePatchesRequestItem with the given variables and object. +func requestItem( + object any, + holderRef *runtimehooksv1.HolderReference, + uid types.UID, +) runtimehooksv1.GeneratePatchesRequestItem { + if uid == "" { + uid = uuid.NewUUID() + } + + return runtimehooksv1.GeneratePatchesRequestItem{ + UID: uid, + Object: runtime.RawExtension{ + Raw: serializer.ToJSON(object), + }, + HolderReference: *holderRef, + } +} + +func NewKubeadmConfigTemplateRequestItem(uid types.UID) runtimehooksv1.GeneratePatchesRequestItem { + return requestItem( + &bootstrapv1.KubeadmConfigTemplate{ + TypeMeta: metav1.TypeMeta{ + Kind: "KubeadmConfigTemplate", + APIVersion: bootstrapv1.GroupVersion.String(), + }, + }, + &runtimehooksv1.HolderReference{ + Kind: "MachineDeployment", + FieldPath: "spec.template.spec.infrastructureRef", + }, + uid, + ) +} + +func NewKubeadmControlPlaneTemplateRequestItem( + uid types.UID, +) runtimehooksv1.GeneratePatchesRequestItem { + return requestItem( + &controlplanev1.KubeadmControlPlaneTemplate{ + TypeMeta: metav1.TypeMeta{ + Kind: "KubeadmControlPlaneTemplate", + APIVersion: controlplanev1.GroupVersion.String(), + }, + }, + &runtimehooksv1.HolderReference{ + Kind: "Cluster", + FieldPath: "spec.controlPlaneRef", + }, + uid, + ) +} diff --git a/common/pkg/testutils/capitest/serializer/tojson.go b/common/pkg/testutils/capitest/serializer/tojson.go new file mode 100644 index 000000000..815c9b75f --- /dev/null +++ b/common/pkg/testutils/capitest/serializer/tojson.go @@ -0,0 +1,21 @@ +// Copyright 2023 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package serializer + +import ( + "bytes" + "encoding/json" +) + +func ToJSON(v any) []byte { + data, err := json.Marshal(v) + if err != nil { + panic(err) + } + compacted := &bytes.Buffer{} + if err := json.Compact(compacted, data); err != nil { + panic(err) + } + return compacted.Bytes() +} diff --git a/pkg/handlers/auditpolicy/inject.go b/pkg/handlers/auditpolicy/inject.go index 9db9da3fe..74c8ffff0 100644 --- a/pkg/handlers/auditpolicy/inject.go +++ b/pkg/handlers/auditpolicy/inject.go @@ -35,6 +35,7 @@ type auditPolicyPatchHandler struct { var ( _ handlers.Named = &auditPolicyPatchHandler{} _ mutation.GeneratePatches = &auditPolicyPatchHandler{} + _ mutation.MetaMutater = &auditPolicyPatchHandler{} //go:embed embedded/apiserver-audit-policy.yaml auditPolicy string @@ -58,6 +59,71 @@ func (h *auditPolicyPatchHandler) Name() string { return HandlerNamePatch } +func (h *auditPolicyPatchHandler) Mutate( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, +) error { + log := ctrl.LoggerFrom(ctx).WithValues( + "holderRef", holderRef, + ) + + return patches.Generate( + obj, vars, &holderRef, selectors.ControlPlane(), log, + func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { + log.WithValues( + "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), + "patchedObjectName", client.ObjectKeyFromObject(obj), + ).Info("adding files and updating API server extra args in kubeadm config spec") + + obj.Spec.Template.Spec.KubeadmConfigSpec.Files = append( + obj.Spec.Template.Spec.KubeadmConfigSpec.Files, + bootstrapv1.File{ + Path: auditPolicyPath, + Permissions: "0600", + Content: auditPolicy, + }, + ) + + if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} + } + apiServer := &obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer + if apiServer.ExtraArgs == nil { + apiServer.ExtraArgs = make(map[string]string, 5) + } + + apiServer.ExtraArgs["audit-log-path"] = "/var/log/audit/kube-apiserver-audit.log" + apiServer.ExtraArgs["audit-log-maxage"] = "30" + apiServer.ExtraArgs["audit-log-maxbackup"] = "10" + apiServer.ExtraArgs["audit-log-maxsize"] = "100" + apiServer.ExtraArgs["audit-policy-file"] = auditPolicyPath + + if apiServer.ExtraVolumes == nil { + apiServer.ExtraVolumes = make([]bootstrapv1.HostPathMount, 0, 2) + } + + apiServer.ExtraVolumes = append( + apiServer.ExtraVolumes, + bootstrapv1.HostPathMount{ + Name: "audit-policy", + HostPath: "/etc/kubernetes/audit-policy/", + MountPath: "/etc/kubernetes/audit-policy/", + ReadOnly: true, + }, + bootstrapv1.HostPathMount{ + Name: "audit-logs", + HostPath: "/var/log/kubernetes/audit", + MountPath: "/var/log/audit/", + }, + ) + + return nil + }, + ) +} + func (h *auditPolicyPatchHandler) GeneratePatches( ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, @@ -68,69 +134,6 @@ func (h *auditPolicyPatchHandler) GeneratePatches( h.decoder, req, resp, - func( - ctx context.Context, - obj runtime.Object, - vars map[string]apiextensionsv1.JSON, - holderRef runtimehooksv1.HolderReference, - ) error { - log := ctrl.LoggerFrom(ctx).WithValues( - "holderRef", holderRef, - ) - - return patches.Generate( - obj, vars, &holderRef, selectors.ControlPlane(), log, - func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { - log.WithValues( - "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), - "patchedObjectName", client.ObjectKeyFromObject(obj), - ).Info("adding files and updating API server extra args in kubeadm config spec") - - obj.Spec.Template.Spec.KubeadmConfigSpec.Files = append( - obj.Spec.Template.Spec.KubeadmConfigSpec.Files, - bootstrapv1.File{ - Path: auditPolicyPath, - Permissions: "0600", - Content: auditPolicy, - }, - ) - - if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} - } - apiServer := &obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer - if apiServer.ExtraArgs == nil { - apiServer.ExtraArgs = make(map[string]string, 5) - } - - apiServer.ExtraArgs["audit-log-path"] = "/var/log/audit/kube-apiserver-audit.log" - apiServer.ExtraArgs["audit-log-maxage"] = "30" - apiServer.ExtraArgs["audit-log-maxbackup"] = "10" - apiServer.ExtraArgs["audit-log-maxsize"] = "100" - apiServer.ExtraArgs["audit-policy-file"] = auditPolicyPath - - if apiServer.ExtraVolumes == nil { - apiServer.ExtraVolumes = make([]bootstrapv1.HostPathMount, 0, 2) - } - - apiServer.ExtraVolumes = append( - apiServer.ExtraVolumes, - bootstrapv1.HostPathMount{ - Name: "audit-policy", - HostPath: "/etc/kubernetes/audit-policy/", - MountPath: "/etc/kubernetes/audit-policy/", - ReadOnly: true, - }, - bootstrapv1.HostPathMount{ - Name: "audit-logs", - HostPath: "/var/log/kubernetes/audit", - MountPath: "/var/log/audit/", - }, - ) - - return nil - }, - ) - }, + h.Mutate, ) } diff --git a/pkg/handlers/auditpolicy/inject_test.go b/pkg/handlers/auditpolicy/inject_test.go index 2d14b5a76..9bb457426 100644 --- a/pkg/handlers/auditpolicy/inject_test.go +++ b/pkg/handlers/auditpolicy/inject_test.go @@ -9,6 +9,7 @@ import ( . "github.com/onsi/gomega" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/request" ) func TestGeneratePatches(t *testing.T) { @@ -20,7 +21,7 @@ func TestGeneratePatches(t *testing.T) { }, capitest.PatchTestDef{ Name: "http proxy set for KubeadmControlPlaneTemplate", - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/kubeadmConfigSpec/files", diff --git a/pkg/handlers/etcd/inject.go b/pkg/handlers/etcd/inject.go index 2fb1bc399..155b3e99f 100644 --- a/pkg/handlers/etcd/inject.go +++ b/pkg/handlers/etcd/inject.go @@ -40,6 +40,7 @@ type etcdPatchHandler struct { var ( _ commonhandlers.Named = &etcdPatchHandler{} _ mutation.GeneratePatches = &etcdPatchHandler{} + _ mutation.MetaMutater = &etcdPatchHandler{} ) func NewPatch() *etcdPatchHandler { @@ -71,6 +72,66 @@ func (h *etcdPatchHandler) Name() string { return HandlerNamePatch } +func (h *etcdPatchHandler) Mutate( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, +) error { + log := ctrl.LoggerFrom(ctx).WithValues( + "holderRef", holderRef, + ) + + etcd, found, err := variables.Get[v1alpha1.Etcd]( + vars, + h.variableName, + h.variableFieldPath..., + ) + if err != nil { + return err + } + if !found { + log.V(5).Info("etcd variable not defined") + return nil + } + + log = log.WithValues( + "variableName", + h.variableName, + "variableFieldPath", + h.variableFieldPath, + "variableValue", + etcd, + ) + + return patches.Generate( + obj, vars, &holderRef, selectors.ControlPlane(), log, + func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { + log.WithValues( + "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), + "patchedObjectName", client.ObjectKeyFromObject(obj), + ).Info("setting etcd configuration in kubeadm config spec") + + if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} + } + if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local == nil { + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = &bootstrapv1.LocalEtcd{} + } + + localEtcd := obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local + if etcd.Image != nil && etcd.Image.Tag != "" { + localEtcd.ImageTag = etcd.Image.Tag + } + if etcd.Image != nil && etcd.Image.Repository != "" { + localEtcd.ImageRepository = etcd.Image.Repository + } + + return nil + }, + ) +} + func (h *etcdPatchHandler) GeneratePatches( ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, @@ -81,64 +142,6 @@ func (h *etcdPatchHandler) GeneratePatches( h.decoder, req, resp, - func( - ctx context.Context, - obj runtime.Object, - vars map[string]apiextensionsv1.JSON, - holderRef runtimehooksv1.HolderReference, - ) error { - log := ctrl.LoggerFrom(ctx).WithValues( - "holderRef", holderRef, - ) - - etcd, found, err := variables.Get[v1alpha1.Etcd]( - vars, - h.variableName, - h.variableFieldPath..., - ) - if err != nil { - return err - } - if !found { - log.V(5).Info("etcd variable not defined") - return nil - } - - log = log.WithValues( - "variableName", - h.variableName, - "variableFieldPath", - h.variableFieldPath, - "variableValue", - etcd, - ) - - return patches.Generate( - obj, vars, &holderRef, selectors.ControlPlane(), log, - func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { - log.WithValues( - "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), - "patchedObjectName", client.ObjectKeyFromObject(obj), - ).Info("setting etcd configuration in kubeadm config spec") - - if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} - } - if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local == nil { - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = &bootstrapv1.LocalEtcd{} - } - - localEtcd := obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local - if etcd.Image != nil && etcd.Image.Tag != "" { - localEtcd.ImageTag = etcd.Image.Tag - } - if etcd.Image != nil && etcd.Image.Repository != "" { - localEtcd.ImageRepository = etcd.Image.Repository - } - - return nil - }, - ) - }, + h.Mutate, ) } diff --git a/pkg/handlers/etcd/inject_test.go b/pkg/handlers/etcd/inject_test.go index 930d9220a..f7dc4fab3 100644 --- a/pkg/handlers/etcd/inject_test.go +++ b/pkg/handlers/etcd/inject_test.go @@ -12,6 +12,7 @@ import ( "github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/request" ) func TestGeneratePatches(t *testing.T) { @@ -34,7 +35,7 @@ func TestGeneratePatches(t *testing.T) { }, ), }, - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{ { Operation: "add", @@ -63,7 +64,7 @@ func TestGeneratePatches(t *testing.T) { }, ), }, - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{ { Operation: "add", @@ -91,7 +92,7 @@ func TestGeneratePatches(t *testing.T) { }, ), }, - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{ { Operation: "add", diff --git a/pkg/handlers/extraapiservercertsans/inject.go b/pkg/handlers/extraapiservercertsans/inject.go index 2fd163b06..49caeeff1 100644 --- a/pkg/handlers/extraapiservercertsans/inject.go +++ b/pkg/handlers/extraapiservercertsans/inject.go @@ -40,6 +40,7 @@ type extraAPIServerCertSANsPatchHandler struct { var ( _ commonhandlers.Named = &extraAPIServerCertSANsPatchHandler{} _ mutation.GeneratePatches = &extraAPIServerCertSANsPatchHandler{} + _ mutation.MetaMutater = &extraAPIServerCertSANsPatchHandler{} ) func NewPatch() *extraAPIServerCertSANsPatchHandler { @@ -71,6 +72,56 @@ func (h *extraAPIServerCertSANsPatchHandler) Name() string { return HandlerNamePatch } +func (h *extraAPIServerCertSANsPatchHandler) Mutate( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, +) error { + log := ctrl.LoggerFrom(ctx).WithValues( + "holderRef", holderRef, + ) + + extraAPIServerCertSANsVar, found, err := variables.Get[v1alpha1.ExtraAPIServerCertSANs]( + vars, + h.variableName, + h.variableFieldPath..., + ) + if err != nil { + return err + } + if !found { + log.V(5).Info("Extra API server cert SANs variable not defined") + return nil + } + + log = log.WithValues( + "variableName", + h.variableName, + "variableFieldPath", + h.variableFieldPath, + "variableValue", + extraAPIServerCertSANsVar, + ) + + return patches.Generate( + obj, vars, &holderRef, selectors.ControlPlane(), log, + func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { + log.WithValues( + "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), + "patchedObjectName", client.ObjectKeyFromObject(obj), + ).Info("adding API server extra cert SANs in kubeadm config spec") + + if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} + } + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer.CertSANs = extraAPIServerCertSANsVar + + return nil + }, + ) +} + func (h *extraAPIServerCertSANsPatchHandler) GeneratePatches( ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, @@ -81,54 +132,6 @@ func (h *extraAPIServerCertSANsPatchHandler) GeneratePatches( h.decoder, req, resp, - func( - ctx context.Context, - obj runtime.Object, - vars map[string]apiextensionsv1.JSON, - holderRef runtimehooksv1.HolderReference, - ) error { - log := ctrl.LoggerFrom(ctx).WithValues( - "holderRef", holderRef, - ) - - extraAPIServerCertSANsVar, found, err := variables.Get[v1alpha1.ExtraAPIServerCertSANs]( - vars, - h.variableName, - h.variableFieldPath..., - ) - if err != nil { - return err - } - if !found { - log.V(5).Info("Extra API server cert SANs variable not defined") - return nil - } - - log = log.WithValues( - "variableName", - h.variableName, - "variableFieldPath", - h.variableFieldPath, - "variableValue", - extraAPIServerCertSANsVar, - ) - - return patches.Generate( - obj, vars, &holderRef, selectors.ControlPlane(), log, - func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { - log.WithValues( - "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), - "patchedObjectName", client.ObjectKeyFromObject(obj), - ).Info("adding API server extra cert SANs in kubeadm config spec") - - if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} - } - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer.CertSANs = extraAPIServerCertSANsVar - - return nil - }, - ) - }, + h.Mutate, ) } diff --git a/pkg/handlers/extraapiservercertsans/inject_test.go b/pkg/handlers/extraapiservercertsans/inject_test.go index b539bc6f8..ada9faf96 100644 --- a/pkg/handlers/extraapiservercertsans/inject_test.go +++ b/pkg/handlers/extraapiservercertsans/inject_test.go @@ -12,6 +12,7 @@ import ( "github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/request" ) func TestGeneratePatches(t *testing.T) { @@ -29,7 +30,7 @@ func TestGeneratePatches(t *testing.T) { v1alpha1.ExtraAPIServerCertSANs{"a.b.c.example.com", "d.e.f.example.com"}, ), }, - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/kubeadmConfigSpec/clusterConfiguration", diff --git a/pkg/handlers/httpproxy/inject.go b/pkg/handlers/httpproxy/inject.go index 96ea737cb..34f099ab8 100644 --- a/pkg/handlers/httpproxy/inject.go +++ b/pkg/handlers/httpproxy/inject.go @@ -5,14 +5,12 @@ package httpproxy import ( "context" - "errors" "fmt" "strings" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/apimachinery/pkg/types" capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" @@ -87,108 +85,96 @@ func (h *httpProxyPatchHandler) Name() string { return HandlerNamePatch } -func (h *httpProxyPatchHandler) GeneratePatches( +func (h *httpProxyPatchHandler) Mutate( ctx context.Context, - req *runtimehooksv1.GeneratePatchesRequest, - resp *runtimehooksv1.GeneratePatchesResponse, -) { - log := ctrl.LoggerFrom(ctx) - noProxy, err := h.detectNoProxy(ctx, req) + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, +) error { + log := ctrl.LoggerFrom(ctx, "holderRef", holderRef) + + noProxy, err := h.detectNoProxy(ctx) if err != nil { log.Error(err, "failed to resolve no proxy value") } - topologymutation.WalkTemplates( - ctx, - h.decoder, - req, - resp, - func( - ctx context.Context, - obj runtime.Object, - vars map[string]apiextensionsv1.JSON, - holderRef runtimehooksv1.HolderReference, - ) error { - log = log.WithValues( - "holderRef", holderRef, - ) + httpProxyVariable, found, err := variables.Get[v1alpha1.HTTPProxy]( + vars, + h.variableName, + h.variableFieldPath..., + ) + if err != nil { + return err + } + if !found { + log.Info("http proxy variable not defined") + return nil + } - httpProxyVariable, found, err := variables.Get[v1alpha1.HTTPProxy]( - vars, - h.variableName, - h.variableFieldPath..., - ) - if err != nil { - return err - } - if !found { - log.Info("http proxy variable not defined") - return nil - } - - log = log.WithValues( - "variableName", - h.variableName, - "variableFieldPath", - h.variableFieldPath, - "variableValue", - httpProxyVariable, - ) + log = log.WithValues( + "variableName", + h.variableName, + "variableFieldPath", + h.variableFieldPath, + "variableValue", + httpProxyVariable, + ) - if err := patches.Generate( - obj, vars, &holderRef, selectors.ControlPlane(), log, - func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { - log.WithValues( - "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), - "patchedObjectName", ctrlclient.ObjectKeyFromObject(obj), - ).Info("adding files to control plane kubeadm config spec") - obj.Spec.Template.Spec.KubeadmConfigSpec.Files = append( - obj.Spec.Template.Spec.KubeadmConfigSpec.Files, - generateSystemdFiles(httpProxyVariable, noProxy)..., - ) - return nil - }); err != nil { - return err - } - - if err := patches.Generate( - obj, vars, &holderRef, selectors.AllWorkersSelector(), log, - func(obj *bootstrapv1.KubeadmConfigTemplate) error { - log.WithValues( - "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), - "patchedObjectName", ctrlclient.ObjectKeyFromObject(obj), - ).Info("adding files to worker node kubeadm config template") - obj.Spec.Template.Spec.Files = append( - obj.Spec.Template.Spec.Files, - generateSystemdFiles(httpProxyVariable, noProxy)..., - ) - return nil - }); err != nil { - return err - } + if err := patches.Generate( + obj, vars, &holderRef, selectors.ControlPlane(), log, + func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { + log.WithValues( + "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), + "patchedObjectName", ctrlclient.ObjectKeyFromObject(obj), + ).Info("adding files to control plane kubeadm config spec") + obj.Spec.Template.Spec.KubeadmConfigSpec.Files = append( + obj.Spec.Template.Spec.KubeadmConfigSpec.Files, + generateSystemdFiles(httpProxyVariable, noProxy)..., + ) + return nil + }); err != nil { + return err + } + if err := patches.Generate( + obj, vars, &holderRef, selectors.AllWorkersSelector(), log, + func(obj *bootstrapv1.KubeadmConfigTemplate) error { + log.WithValues( + "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), + "patchedObjectName", ctrlclient.ObjectKeyFromObject(obj), + ).Info("adding files to worker node kubeadm config template") + obj.Spec.Template.Spec.Files = append( + obj.Spec.Template.Spec.Files, + generateSystemdFiles(httpProxyVariable, noProxy)..., + ) return nil - }, - ) + }); err != nil { + return err + } + + return nil } -func (h *httpProxyPatchHandler) detectNoProxy( +func (h *httpProxyPatchHandler) GeneratePatches( ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, -) ([]string, error) { - clusterKey := types.NamespacedName{} - - for i := range req.Items { - item := req.Items[i] - if item.HolderReference.Kind == "Cluster" && - item.HolderReference.APIVersion == capiv1.GroupVersion.String() { - clusterKey.Name = item.HolderReference.Name - clusterKey.Namespace = item.HolderReference.Namespace - } - } + resp *runtimehooksv1.GeneratePatchesResponse, +) { + ctx = commonhandlers.ClusterKeyInto(ctx, req) - if clusterKey.Name == "" { - return nil, errors.New("failed to detect cluster name from GeneratePatch request") + topologymutation.WalkTemplates( + ctx, + h.decoder, + req, + resp, + h.Mutate, + ) +} + +func (h *httpProxyPatchHandler) detectNoProxy(ctx context.Context) ([]string, error) { + clusterKey, err := commonhandlers.ClusterKeyFrom(ctx) + if err != nil { + return nil, err } cluster := &capiv1.Cluster{} diff --git a/pkg/handlers/httpproxy/inject_test.go b/pkg/handlers/httpproxy/inject_test.go index d61c95180..24eb329a6 100644 --- a/pkg/handlers/httpproxy/inject_test.go +++ b/pkg/handlers/httpproxy/inject_test.go @@ -15,6 +15,7 @@ import ( "github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/request" ) func TestGeneratePatches(t *testing.T) { @@ -47,7 +48,7 @@ func TestGeneratePatches(t *testing.T) { }, ), }, - RequestItem: capitest.NewKubeadmConfigTemplateRequestItem(), + RequestItem: request.NewKubeadmConfigTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/files", @@ -74,7 +75,7 @@ func TestGeneratePatches(t *testing.T) { }, ), }, - RequestItem: capitest.NewKubeadmConfigTemplateRequestItem(), + RequestItem: request.NewKubeadmConfigTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/files", @@ -93,7 +94,7 @@ func TestGeneratePatches(t *testing.T) { }, ), }, - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/kubeadmConfigSpec/files", diff --git a/pkg/handlers/kubernetesimagerepository/inject.go b/pkg/handlers/kubernetesimagerepository/inject.go index 995e775ef..153501f5b 100644 --- a/pkg/handlers/kubernetesimagerepository/inject.go +++ b/pkg/handlers/kubernetesimagerepository/inject.go @@ -40,6 +40,7 @@ type imageRepositoryPatchHandler struct { var ( _ commonhandlers.Named = &imageRepositoryPatchHandler{} _ mutation.GeneratePatches = &imageRepositoryPatchHandler{} + _ mutation.MetaMutater = &imageRepositoryPatchHandler{} ) func NewPatch() *imageRepositoryPatchHandler { @@ -71,6 +72,56 @@ func (h *imageRepositoryPatchHandler) Name() string { return HandlerNamePatch } +func (h *imageRepositoryPatchHandler) Mutate( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, +) error { + log := ctrl.LoggerFrom(ctx).WithValues( + "holderRef", holderRef, + ) + + imageRepositoryVar, found, err := variables.Get[v1alpha1.KubernetesImageRepository]( + vars, + h.variableName, + h.variableFieldPath..., + ) + if err != nil { + return err + } + if !found { + log.V(5).Info("kubernetesImageRepository variable not defined") + return nil + } + + log = log.WithValues( + "variableName", + h.variableName, + "variableFieldPath", + h.variableFieldPath, + "variableValue", + imageRepositoryVar, + ) + + return patches.Generate( + obj, vars, &holderRef, selectors.ControlPlane(), log, + func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { + log.WithValues( + "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), + "patchedObjectName", client.ObjectKeyFromObject(obj), + ).Info("setting imageRepository in kubeadm config spec") + + if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} + } + obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.ImageRepository = imageRepositoryVar.String() + + return nil + }, + ) +} + func (h *imageRepositoryPatchHandler) GeneratePatches( ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, @@ -81,54 +132,6 @@ func (h *imageRepositoryPatchHandler) GeneratePatches( h.decoder, req, resp, - func( - ctx context.Context, - obj runtime.Object, - vars map[string]apiextensionsv1.JSON, - holderRef runtimehooksv1.HolderReference, - ) error { - log := ctrl.LoggerFrom(ctx).WithValues( - "holderRef", holderRef, - ) - - imageRepositoryVar, found, err := variables.Get[v1alpha1.KubernetesImageRepository]( - vars, - h.variableName, - h.variableFieldPath..., - ) - if err != nil { - return err - } - if !found { - log.V(5).Info("kubernetesImageRepository variable not defined") - return nil - } - - log = log.WithValues( - "variableName", - h.variableName, - "variableFieldPath", - h.variableFieldPath, - "variableValue", - imageRepositoryVar, - ) - - return patches.Generate( - obj, vars, &holderRef, selectors.ControlPlane(), log, - func(obj *controlplanev1.KubeadmControlPlaneTemplate) error { - log.WithValues( - "patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(), - "patchedObjectName", client.ObjectKeyFromObject(obj), - ).Info("setting imageRepository in kubeadm config spec") - - if obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration == nil { - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{} - } - obj.Spec.Template.Spec.KubeadmConfigSpec.ClusterConfiguration.ImageRepository = imageRepositoryVar.String() - - return nil - }, - ) - }, + h.Mutate, ) } diff --git a/pkg/handlers/kubernetesimagerepository/inject_test.go b/pkg/handlers/kubernetesimagerepository/inject_test.go index f7e67e1a2..11dc53345 100644 --- a/pkg/handlers/kubernetesimagerepository/inject_test.go +++ b/pkg/handlers/kubernetesimagerepository/inject_test.go @@ -12,6 +12,7 @@ import ( "github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest" + "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest/request" ) func TestGeneratePatches(t *testing.T) { @@ -29,7 +30,7 @@ func TestGeneratePatches(t *testing.T) { v1alpha1.KubernetesImageRepository("my-registry.io/my-org/my-repo"), ), }, - RequestItem: capitest.NewKubeadmControlPlaneTemplateRequestItem(), + RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(""), ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/kubeadmConfigSpec/clusterConfiguration", From b3905436c7ea1692ad42a4a0dd848fb0afcdd423 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Tue, 19 Sep 2023 15:23:24 +0100 Subject: [PATCH 2/5] fixup! test: Add initial postkubeadm command to test additional values --- .../clustertopology/handlers/mutation/meta_test.go | 13 +++++++++---- common/pkg/testutils/capitest/request/items.go | 7 +++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go b/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go index 7d47365f7..a9a98adf7 100644 --- a/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go +++ b/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go @@ -115,8 +115,8 @@ func TestMetaGeneratePatches(t *testing.T) { Patch: jsonPatch( jsonpatch.NewOperation( "add", - "/spec/template/spec/postKubeadmCommands", - []string{"worker-extra-post-kubeadm-0"}, + "/spec/template/spec/postKubeadmCommands/1", + "worker-extra-post-kubeadm-1", ), ), }, { @@ -155,8 +155,13 @@ func TestMetaGeneratePatches(t *testing.T) { Patch: jsonPatch( jsonpatch.NewOperation( "add", - "/spec/template/spec/postKubeadmCommands", - []string{"worker-extra-post-kubeadm-0", "worker-extra-post-kubeadm-1"}, + "/spec/template/spec/postKubeadmCommands/1", + "worker-extra-post-kubeadm-1", + ), + jsonpatch.NewOperation( + "add", + "/spec/template/spec/postKubeadmCommands/2", + "worker-extra-post-kubeadm-2", ), ), }, { diff --git a/common/pkg/testutils/capitest/request/items.go b/common/pkg/testutils/capitest/request/items.go index 6eba55ca3..c21f3417d 100644 --- a/common/pkg/testutils/capitest/request/items.go +++ b/common/pkg/testutils/capitest/request/items.go @@ -41,6 +41,13 @@ func NewKubeadmConfigTemplateRequestItem(uid types.UID) runtimehooksv1.GenerateP Kind: "KubeadmConfigTemplate", APIVersion: bootstrapv1.GroupVersion.String(), }, + Spec: bootstrapv1.KubeadmConfigTemplateSpec{ + Template: bootstrapv1.KubeadmConfigTemplateResource{ + Spec: bootstrapv1.KubeadmConfigSpec{ + PostKubeadmCommands: []string{"initial-post-kubeadm"}, + }, + }, + }, }, &runtimehooksv1.HolderReference{ Kind: "MachineDeployment", From cf3d78959780c0183c75f7a1978ee4382c5577a5 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Tue, 19 Sep 2023 15:46:48 +0100 Subject: [PATCH 3/5] fixup! refactor: Explicitly pass cluster key instead of using context --- .../handlers/cluster_context.go | 35 ++++--------------- .../clustertopology/handlers/mutation/meta.go | 6 ++-- .../handlers/mutation/meta_test.go | 2 ++ pkg/handlers/auditpolicy/inject.go | 10 +++++- pkg/handlers/etcd/inject.go | 10 +++++- pkg/handlers/extraapiservercertsans/inject.go | 10 +++++- pkg/handlers/httpproxy/inject.go | 24 ++++++++----- .../kubernetesimagerepository/inject.go | 10 +++++- 8 files changed, 63 insertions(+), 44 deletions(-) diff --git a/common/pkg/capi/clustertopology/handlers/cluster_context.go b/common/pkg/capi/clustertopology/handlers/cluster_context.go index 31567d2e5..85d56579c 100644 --- a/common/pkg/capi/clustertopology/handlers/cluster_context.go +++ b/common/pkg/capi/clustertopology/handlers/cluster_context.go @@ -4,45 +4,22 @@ package handlers import ( - "context" - "errors" - capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" ) -// clusterKeyContextKey is how we find a cluster key from a context.Context. -type clusterKeyContextKey struct{} - -func ClusterKeyFrom(ctx context.Context) (client.ObjectKey, error) { - clusterKey, ok := ctx.Value(clusterKeyContextKey{}).(client.ObjectKey) - if !ok || clusterKey.Name == "" { - return client.ObjectKey{}, errors.New( - "failed to detect cluster name from GeneratePatch request", - ) - } - - return clusterKey, nil -} - -func ClusterKeyInto( - ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, -) context.Context { - clusterKey := client.ObjectKey{} - +func ClusterKeyFromReq(req *runtimehooksv1.GeneratePatchesRequest) client.ObjectKey { for i := range req.Items { item := req.Items[i] if item.HolderReference.Kind == "Cluster" && item.HolderReference.APIVersion == capiv1.GroupVersion.String() { - clusterKey.Name = item.HolderReference.Name - clusterKey.Namespace = item.HolderReference.Namespace + return client.ObjectKey{ + Namespace: item.HolderReference.Namespace, + Name: item.HolderReference.Name, + } } } - if clusterKey.Name != "" { - return context.WithValue(ctx, clusterKeyContextKey{}, clusterKey) - } - - return ctx + return client.ObjectKey{} } diff --git a/common/pkg/capi/clustertopology/handlers/mutation/meta.go b/common/pkg/capi/clustertopology/handlers/mutation/meta.go index b4bb2748c..e47930294 100644 --- a/common/pkg/capi/clustertopology/handlers/mutation/meta.go +++ b/common/pkg/capi/clustertopology/handlers/mutation/meta.go @@ -13,6 +13,7 @@ import ( controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/exp/runtime/topologymutation" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers" ) @@ -23,6 +24,7 @@ type MetaMutater interface { obj runtime.Object, vars map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + clusterKey client.ObjectKey, ) error } @@ -55,7 +57,7 @@ func (mgp metaGeneratePatches) GeneratePatches( req *runtimehooksv1.GeneratePatchesRequest, resp *runtimehooksv1.GeneratePatchesResponse, ) { - ctx = handlers.ClusterKeyInto(ctx, req) + clusterKey := handlers.ClusterKeyFromReq(req) topologymutation.WalkTemplates( ctx, @@ -69,7 +71,7 @@ func (mgp metaGeneratePatches) GeneratePatches( holderRef runtimehooksv1.HolderReference, ) error { for _, h := range mgp.mutaters { - if err := h.Mutate(ctx, obj, vars, holderRef); err != nil { + if err := h.Mutate(ctx, obj, vars, holderRef, clusterKey); err != nil { return err } } diff --git a/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go b/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go index a9a98adf7..24f1534a0 100644 --- a/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go +++ b/common/pkg/capi/clustertopology/handlers/mutation/meta_test.go @@ -17,6 +17,7 @@ import ( bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/patches" "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/patches/selectors" @@ -35,6 +36,7 @@ func (h *testHandler) Mutate( obj runtime.Object, _ map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + _ client.ObjectKey, ) error { if h.returnErr { return fmt.Errorf("This is a failure") diff --git a/pkg/handlers/auditpolicy/inject.go b/pkg/handlers/auditpolicy/inject.go index 74c8ffff0..f1fd85b41 100644 --- a/pkg/handlers/auditpolicy/inject.go +++ b/pkg/handlers/auditpolicy/inject.go @@ -64,6 +64,7 @@ func (h *auditPolicyPatchHandler) Mutate( obj runtime.Object, vars map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + _ client.ObjectKey, ) error { log := ctrl.LoggerFrom(ctx).WithValues( "holderRef", holderRef, @@ -134,6 +135,13 @@ func (h *auditPolicyPatchHandler) GeneratePatches( h.decoder, req, resp, - h.Mutate, + func( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error { + return h.Mutate(ctx, obj, vars, holderRef, client.ObjectKey{}) + }, ) } diff --git a/pkg/handlers/etcd/inject.go b/pkg/handlers/etcd/inject.go index 155b3e99f..89513e6e0 100644 --- a/pkg/handlers/etcd/inject.go +++ b/pkg/handlers/etcd/inject.go @@ -77,6 +77,7 @@ func (h *etcdPatchHandler) Mutate( obj runtime.Object, vars map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + _ client.ObjectKey, ) error { log := ctrl.LoggerFrom(ctx).WithValues( "holderRef", holderRef, @@ -142,6 +143,13 @@ func (h *etcdPatchHandler) GeneratePatches( h.decoder, req, resp, - h.Mutate, + func( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error { + return h.Mutate(ctx, obj, vars, holderRef, client.ObjectKey{}) + }, ) } diff --git a/pkg/handlers/extraapiservercertsans/inject.go b/pkg/handlers/extraapiservercertsans/inject.go index 49caeeff1..8455137c3 100644 --- a/pkg/handlers/extraapiservercertsans/inject.go +++ b/pkg/handlers/extraapiservercertsans/inject.go @@ -77,6 +77,7 @@ func (h *extraAPIServerCertSANsPatchHandler) Mutate( obj runtime.Object, vars map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + _ client.ObjectKey, ) error { log := ctrl.LoggerFrom(ctx).WithValues( "holderRef", holderRef, @@ -132,6 +133,13 @@ func (h *extraAPIServerCertSANsPatchHandler) GeneratePatches( h.decoder, req, resp, - h.Mutate, + func( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error { + return h.Mutate(ctx, obj, vars, holderRef, client.ObjectKey{}) + }, ) } diff --git a/pkg/handlers/httpproxy/inject.go b/pkg/handlers/httpproxy/inject.go index 34f099ab8..1cd676861 100644 --- a/pkg/handlers/httpproxy/inject.go +++ b/pkg/handlers/httpproxy/inject.go @@ -90,10 +90,11 @@ func (h *httpProxyPatchHandler) Mutate( obj runtime.Object, vars map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + clusterKey ctrlclient.ObjectKey, ) error { log := ctrl.LoggerFrom(ctx, "holderRef", holderRef) - noProxy, err := h.detectNoProxy(ctx) + noProxy, err := h.detectNoProxy(ctx, clusterKey) if err != nil { log.Error(err, "failed to resolve no proxy value") } @@ -160,23 +161,28 @@ func (h *httpProxyPatchHandler) GeneratePatches( req *runtimehooksv1.GeneratePatchesRequest, resp *runtimehooksv1.GeneratePatchesResponse, ) { - ctx = commonhandlers.ClusterKeyInto(ctx, req) + clusterKey := commonhandlers.ClusterKeyFromReq(req) topologymutation.WalkTemplates( ctx, h.decoder, req, resp, - h.Mutate, + func( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error { + return h.Mutate(ctx, obj, vars, holderRef, clusterKey) + }, ) } -func (h *httpProxyPatchHandler) detectNoProxy(ctx context.Context) ([]string, error) { - clusterKey, err := commonhandlers.ClusterKeyFrom(ctx) - if err != nil { - return nil, err - } - +func (h *httpProxyPatchHandler) detectNoProxy( + ctx context.Context, + clusterKey ctrlclient.ObjectKey, +) ([]string, error) { cluster := &capiv1.Cluster{} if err := h.client.Get(ctx, clusterKey, cluster); err != nil { return nil, err diff --git a/pkg/handlers/kubernetesimagerepository/inject.go b/pkg/handlers/kubernetesimagerepository/inject.go index 153501f5b..c21d91793 100644 --- a/pkg/handlers/kubernetesimagerepository/inject.go +++ b/pkg/handlers/kubernetesimagerepository/inject.go @@ -77,6 +77,7 @@ func (h *imageRepositoryPatchHandler) Mutate( obj runtime.Object, vars map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference, + _ client.ObjectKey, ) error { log := ctrl.LoggerFrom(ctx).WithValues( "holderRef", holderRef, @@ -132,6 +133,13 @@ func (h *imageRepositoryPatchHandler) GeneratePatches( h.decoder, req, resp, - h.Mutate, + func( + ctx context.Context, + obj runtime.Object, + vars map[string]apiextensionsv1.JSON, + holderRef runtimehooksv1.HolderReference, + ) error { + return h.Mutate(ctx, obj, vars, holderRef, client.ObjectKey{}) + }, ) } From dc2e66474235b8bddb37113516155fe2fc3652e9 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Tue, 19 Sep 2023 15:58:15 +0100 Subject: [PATCH 4/5] fixup! test: Refactor http inject test --- pkg/handlers/httpproxy/inject_test.go | 263 +++++++++++++------------- 1 file changed, 127 insertions(+), 136 deletions(-) diff --git a/pkg/handlers/httpproxy/inject_test.go b/pkg/handlers/httpproxy/inject_test.go index 24eb329a6..a6731edd9 100644 --- a/pkg/handlers/httpproxy/inject_test.go +++ b/pkg/handlers/httpproxy/inject_test.go @@ -6,7 +6,7 @@ package httpproxy import ( "testing" - . "github.com/onsi/gomega" + "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" "k8s.io/apiserver/pkg/storage/names" capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -52,7 +52,7 @@ func TestGeneratePatches(t *testing.T) { ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/files", - ValueMatcher: HaveLen(2), + ValueMatcher: gomega.HaveLen(2), }}, }, capitest.PatchTestDef{ @@ -79,7 +79,7 @@ func TestGeneratePatches(t *testing.T) { ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/files", - ValueMatcher: HaveLen(2), + ValueMatcher: gomega.HaveLen(2), }}, }, capitest.PatchTestDef{ @@ -98,7 +98,7 @@ func TestGeneratePatches(t *testing.T) { ExpectedPatchMatchers: []capitest.JSONPatchMatcher{{ Operation: "add", Path: "/spec/template/spec/kubeadmConfigSpec/files", - ValueMatcher: HaveLen(2), + ValueMatcher: gomega.HaveLen(2), }}, }, ) @@ -106,178 +106,169 @@ func TestGeneratePatches(t *testing.T) { func TestGenerateNoProxy(t *testing.T) { t.Parallel() - g := NewWithT(t) testCases := []struct { name string cluster *capiv1.Cluster expectedNoProxy []string - }{ - { - name: "no networking config", - cluster: &capiv1.Cluster{}, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", - }, + }{{ + name: "no networking config", + cluster: &capiv1.Cluster{}, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", }, - { - name: "custom pod network", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - ClusterNetwork: &capiv1.ClusterNetwork{ - Pods: &capiv1.NetworkRanges{ - CIDRBlocks: []string{"10.0.0.0/24", "10.0.1.0/24"}, - }, + }, { + name: "custom pod network", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + ClusterNetwork: &capiv1.ClusterNetwork{ + Pods: &capiv1.NetworkRanges{ + CIDRBlocks: []string{"10.0.0.0/24", "10.0.1.0/24"}, }, }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "10.0.0.0/24", "10.0.1.0/24", "kubernetes", - "kubernetes.default", ".svc", ".svc.cluster.local", - }, }, - { - name: "Unknown infrastructure cluster", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - InfrastructureRef: &v1.ObjectReference{ - Kind: "SomeFakeInfrastructureCluster", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "10.0.0.0/24", "10.0.1.0/24", "kubernetes", + "kubernetes.default", ".svc", ".svc.cluster.local", + }, + }, { + name: "Unknown infrastructure cluster", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + InfrastructureRef: &v1.ObjectReference{ + Kind: "SomeFakeInfrastructureCluster", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", - }, }, - { - name: "AWS cluster", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - InfrastructureRef: &v1.ObjectReference{ - Kind: "AWSCluster", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", + }, + }, { + name: "AWS cluster", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + InfrastructureRef: &v1.ObjectReference{ + Kind: "AWSCluster", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", "169.254.169.254", ".elb.amazonaws.com", - }, }, - { - name: "AWS managed (EKS) cluster", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - InfrastructureRef: &v1.ObjectReference{ - Kind: "AWSManagedCluster", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", "169.254.169.254", ".elb.amazonaws.com", + }, + }, { + name: "AWS managed (EKS) cluster", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + InfrastructureRef: &v1.ObjectReference{ + Kind: "AWSManagedCluster", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", "169.254.169.254", ".elb.amazonaws.com", - }, }, - { - name: "Azure cluster", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - InfrastructureRef: &v1.ObjectReference{ - Kind: "AzureCluster", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", "169.254.169.254", ".elb.amazonaws.com", + }, + }, { + name: "Azure cluster", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + InfrastructureRef: &v1.ObjectReference{ + Kind: "AzureCluster", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", "169.254.169.254", - }, }, - { - name: "Azure managed (AKS) cluster", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - InfrastructureRef: &v1.ObjectReference{ - Kind: "AzureCluster", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", "169.254.169.254", + }, + }, { + name: "Azure managed (AKS) cluster", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + InfrastructureRef: &v1.ObjectReference{ + Kind: "AzureCluster", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", "169.254.169.254", - }, }, - { - name: "GCP cluster", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - InfrastructureRef: &v1.ObjectReference{ - Kind: "GCPCluster", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", "169.254.169.254", + }, + }, { + name: "GCP cluster", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + InfrastructureRef: &v1.ObjectReference{ + Kind: "GCPCluster", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.cluster.local", "169.254.169.254", "metadata", "metadata.google.internal", - }, }, - { - name: "custom service network", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - ClusterNetwork: &capiv1.ClusterNetwork{ - Services: &capiv1.NetworkRanges{ - CIDRBlocks: []string{"172.16.0.0/24", "172.16.1.0/24"}, - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.cluster.local", "169.254.169.254", "metadata", "metadata.google.internal", + }, + }, { + name: "custom service network", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + ClusterNetwork: &capiv1.ClusterNetwork{ + Services: &capiv1.NetworkRanges{ + CIDRBlocks: []string{"172.16.0.0/24", "172.16.1.0/24"}, }, }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "172.16.0.0/24", "172.16.1.0/24", "kubernetes", - "kubernetes.default", ".svc", ".svc.cluster.local", - }, }, - { - name: "custom servicedomain", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - ClusterNetwork: &capiv1.ClusterNetwork{ - ServiceDomain: "foo.bar", - }, + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "172.16.0.0/24", "172.16.1.0/24", "kubernetes", + "kubernetes.default", ".svc", ".svc.cluster.local", + }, + }, { + name: "custom servicedomain", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + ClusterNetwork: &capiv1.ClusterNetwork{ + ServiceDomain: "foo.bar", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", - ".svc", ".svc.foo.bar", - }, }, - { - name: "all options", - cluster: &capiv1.Cluster{ - Spec: capiv1.ClusterSpec{ - ClusterNetwork: &capiv1.ClusterNetwork{ - Pods: &capiv1.NetworkRanges{ - CIDRBlocks: []string{"10.10.0.0/16"}, - }, - Services: &capiv1.NetworkRanges{ - CIDRBlocks: []string{"172.16.0.0/16"}, - }, - ServiceDomain: "foo.bar", + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "kubernetes", "kubernetes.default", + ".svc", ".svc.foo.bar", + }, + }, { + name: "all options", + cluster: &capiv1.Cluster{ + Spec: capiv1.ClusterSpec{ + ClusterNetwork: &capiv1.ClusterNetwork{ + Pods: &capiv1.NetworkRanges{ + CIDRBlocks: []string{"10.10.0.0/16"}, + }, + Services: &capiv1.NetworkRanges{ + CIDRBlocks: []string{"172.16.0.0/16"}, }, + ServiceDomain: "foo.bar", }, }, - expectedNoProxy: []string{ - "localhost", "127.0.0.1", "10.10.0.0/16", "172.16.0.0/16", "kubernetes", - "kubernetes.default", ".svc", ".svc.foo.bar", - }, }, - } + expectedNoProxy: []string{ + "localhost", "127.0.0.1", "10.10.0.0/16", "172.16.0.0/16", "kubernetes", + "kubernetes.default", ".svc", ".svc.foo.bar", + }, + }} + + for idx := range testCases { + tt := testCases[idx] + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + g := gomega.NewWithT(t) - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(tt *testing.T) { - tt.Parallel() - g.Expect(generateNoProxy(tc.cluster)).To(Equal(tc.expectedNoProxy)) + g.Expect(generateNoProxy(tt.cluster)).To(gomega.Equal(tt.expectedNoProxy)) }) } } From f3541d1580e09ea32f417e194fffdc76df477f5a Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Tue, 19 Sep 2023 15:58:52 +0100 Subject: [PATCH 5/5] fixup! refactor: Rename cluster context file --- .../clustertopology/handlers/{cluster_context.go => helpers.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename common/pkg/capi/clustertopology/handlers/{cluster_context.go => helpers.go} (100%) diff --git a/common/pkg/capi/clustertopology/handlers/cluster_context.go b/common/pkg/capi/clustertopology/handlers/helpers.go similarity index 100% rename from common/pkg/capi/clustertopology/handlers/cluster_context.go rename to common/pkg/capi/clustertopology/handlers/helpers.go