From 1ac8e14924f52bc621ab21f20085ebcebb8139f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Mon, 28 Oct 2024 08:24:52 +0100 Subject: [PATCH] Set global.platform when not set by the user (#451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change ensures that the `openshift` platform is set correctly even when the `openshift` profile isn't selected via `spec.profile`. Users can now select a profile (e.g. `remote`) and still have the correct platform selected automatically. Signed-off-by: Marko Lukša --- cmd/main.go | 6 +- controllers/istio/istio_controller.go | 7 +- controllers/istio/istio_controller_test.go | 11 +-- controllers/istiocni/istiocni_controller.go | 7 +- .../istiocni/istiocni_controller_test.go | 6 +- .../remoteistio/remoteistio_controller.go | 7 +- .../remoteistio_controller_test.go | 11 +-- pkg/config/platform.go | 1 + pkg/helm/values.go | 13 ++++ pkg/helm/values_test.go | 68 +++++++++++++++++++ pkg/istiovalues/profiles.go | 14 +++- pkg/revision/values.go | 4 +- pkg/revision/values_test.go | 4 +- tests/integration/api/suite_test.go | 7 +- 14 files changed, 136 insertions(+), 30 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index e1555e277..66ccde742 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -137,14 +137,14 @@ func main() { defaultProfile = "default" } - err = istio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, defaultProfile). + err = istio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, platform, defaultProfile). SetupWithManager(mgr) if err != nil { setupLog.Error(err, "unable to create controller", "controller", "Istio") os.Exit(1) } - err = remoteistio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, defaultProfile). + err = remoteistio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, platform, defaultProfile). SetupWithManager(mgr) if err != nil { setupLog.Error(err, "unable to create controller", "controller", "RemoteIstio") @@ -158,7 +158,7 @@ func main() { os.Exit(1) } - err = istiocni.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, chartManager, defaultProfile). + err = istiocni.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, chartManager, platform, defaultProfile). SetupWithManager(mgr) if err != nil { setupLog.Error(err, "unable to create controller", "controller", "IstioCNI") diff --git a/controllers/istio/istio_controller.go b/controllers/istio/istio_controller.go index eecd54586..bc656bfe0 100644 --- a/controllers/istio/istio_controller.go +++ b/controllers/istio/istio_controller.go @@ -24,6 +24,7 @@ import ( "github.com/go-logr/logr" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/errlist" "github.com/istio-ecosystem/sail-operator/pkg/kube" @@ -46,14 +47,16 @@ import ( // Reconciler reconciles an Istio object type Reconciler struct { ResourceDirectory string + Platform config.Platform DefaultProfile string client.Client Scheme *runtime.Scheme } -func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, defaultProfile string) *Reconciler { +func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, platform config.Platform, defaultProfile string) *Reconciler { return &Reconciler{ ResourceDirectory: resourceDir, + Platform: platform, DefaultProfile: defaultProfile, Client: client, Scheme: scheme, @@ -108,7 +111,7 @@ func validate(istio *v1alpha1.Istio) error { func (r *Reconciler) reconcileActiveRevision(ctx context.Context, istio *v1alpha1.Istio) error { values, err := revision.ComputeValues( istio.Spec.Values, istio.Spec.Namespace, istio.Spec.Version, - r.DefaultProfile, istio.Spec.Profile, + r.Platform, r.DefaultProfile, istio.Spec.Profile, r.ResourceDirectory, getActiveRevisionName(istio)) if err != nil { return err diff --git a/controllers/istio/istio_controller_test.go b/controllers/istio/istio_controller_test.go index fe8e199fb..0af1f84dd 100644 --- a/controllers/istio/istio_controller_test.go +++ b/controllers/istio/istio_controller_test.go @@ -23,6 +23,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/scheme" "github.com/istio-ecosystem/sail-operator/pkg/test/testtime" "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" @@ -60,7 +61,7 @@ func TestReconcile(t *testing.T) { cl := newFakeClientBuilder(). WithObjects(istio). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") _, err := reconciler.Reconcile(ctx, istio) if err == nil { @@ -96,7 +97,7 @@ func TestReconcile(t *testing.T) { WithStatusSubresource(&v1alpha1.Istio{}). WithObjects(istio). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "invalid-profile") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "invalid-profile") _, err := reconciler.Reconcile(ctx, istio) if err == nil { @@ -136,7 +137,7 @@ func TestReconcile(t *testing.T) { }, }). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") _, err := reconciler.Reconcile(ctx, istio) if err == nil { @@ -532,7 +533,7 @@ func TestDetermineStatus(t *testing.T) { WithObjects(initObjs...). WithInterceptorFuncs(interceptorFuncs). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") status, err := reconciler.determineStatus(ctx, istio, tc.reconciliationErr) if (err != nil) != tc.wantErr { @@ -733,7 +734,7 @@ func TestUpdateStatus(t *testing.T) { WithObjects(initObjs...). WithInterceptorFuncs(interceptorFuncs). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") err := reconciler.updateStatus(ctx, istio, tc.reconciliationErr) if (err != nil) != tc.wantErr { diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index 04ff6392b..933dfe741 100644 --- a/controllers/istiocni/istiocni_controller.go +++ b/controllers/istiocni/istiocni_controller.go @@ -57,6 +57,7 @@ const ( // Reconciler reconciles an IstioCNI object type Reconciler struct { ResourceDirectory string + Platform config.Platform DefaultProfile string client.Client Scheme *runtime.Scheme @@ -64,10 +65,11 @@ type Reconciler struct { } func NewReconciler( - client client.Client, scheme *runtime.Scheme, resourceDir string, chartManager *helm.ChartManager, defaultProfile string, + client client.Client, scheme *runtime.Scheme, resourceDir string, chartManager *helm.ChartManager, platform config.Platform, defaultProfile string, ) *Reconciler { return &Reconciler{ ResourceDirectory: resourceDir, + Platform: platform, DefaultProfile: defaultProfile, Client: client, Scheme: scheme, @@ -150,7 +152,8 @@ func (r *Reconciler) installHelmChart(ctx context.Context, cni *v1alpha1.IstioCN userValues = applyImageDigests(cni, userValues, config.Config) // apply userValues on top of defaultValues from profiles - mergedHelmValues, err := istiovalues.ApplyProfiles(r.ResourceDirectory, cni.Spec.Version, r.DefaultProfile, cni.Spec.Profile, helm.FromValues(userValues)) + mergedHelmValues, err := istiovalues.ApplyProfilesAndPlatform( + r.ResourceDirectory, cni.Spec.Version, r.Platform, r.DefaultProfile, cni.Spec.Profile, helm.FromValues(userValues)) if err != nil { return fmt.Errorf("failed to apply profile: %w", err) } diff --git a/controllers/istiocni/istiocni_controller_test.go b/controllers/istiocni/istiocni_controller_test.go index a3ae661d3..a20ccf72e 100644 --- a/controllers/istiocni/istiocni_controller_test.go +++ b/controllers/istiocni/istiocni_controller_test.go @@ -107,7 +107,7 @@ func TestValidate(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tc.objects...).Build() - r := NewReconciler(cl, scheme.Scheme, "", nil, "") + r := NewReconciler(cl, scheme.Scheme, "", nil, config.PlatformKubernetes, "") err := r.validate(context.TODO(), tc.cni) if tc.expectErr == "" { @@ -282,7 +282,7 @@ func TestDetermineReadyCondition(t *testing.T) { cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tt.clientObjects...).WithInterceptorFuncs(tt.interceptors).Build() - r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, "") + r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, config.PlatformKubernetes, "") cni := &v1alpha1.IstioCNI{ ObjectMeta: metav1.ObjectMeta{ @@ -464,7 +464,7 @@ func TestDetermineStatus(t *testing.T) { ctx := context.TODO() resourceDir := t.TempDir() cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() - r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, "") + r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, config.PlatformKubernetes, "") for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/controllers/remoteistio/remoteistio_controller.go b/controllers/remoteistio/remoteistio_controller.go index 31c70a51c..61d0ad62a 100644 --- a/controllers/remoteistio/remoteistio_controller.go +++ b/controllers/remoteistio/remoteistio_controller.go @@ -24,6 +24,7 @@ import ( "github.com/go-logr/logr" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/errlist" "github.com/istio-ecosystem/sail-operator/pkg/kube" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" @@ -44,14 +45,16 @@ import ( // Reconciler reconciles a RemoteIstio object type Reconciler struct { ResourceDirectory string + Platform config.Platform DefaultProfile string client.Client Scheme *runtime.Scheme } -func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, defaultProfile string) *Reconciler { +func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, platform config.Platform, defaultProfile string) *Reconciler { return &Reconciler{ ResourceDirectory: resourceDir, + Platform: platform, DefaultProfile: defaultProfile, Client: client, Scheme: scheme, @@ -106,7 +109,7 @@ func validate(istio *v1alpha1.RemoteIstio) error { func (r *Reconciler) reconcileActiveRevision(ctx context.Context, istio *v1alpha1.RemoteIstio) error { values, err := revision.ComputeValues( istio.Spec.Values, istio.Spec.Namespace, istio.Spec.Version, - r.DefaultProfile, istio.Spec.Profile, + r.Platform, r.DefaultProfile, istio.Spec.Profile, r.ResourceDirectory, getActiveRevisionName(istio)) if err != nil { return err diff --git a/controllers/remoteistio/remoteistio_controller_test.go b/controllers/remoteistio/remoteistio_controller_test.go index c9ff33f0a..b3fc01a51 100644 --- a/controllers/remoteistio/remoteistio_controller_test.go +++ b/controllers/remoteistio/remoteistio_controller_test.go @@ -23,6 +23,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/scheme" "github.com/istio-ecosystem/sail-operator/pkg/test/testtime" "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" @@ -60,7 +61,7 @@ func TestReconcile(t *testing.T) { cl := newFakeClientBuilder(). WithObjects(istio). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") _, err := reconciler.Reconcile(ctx, istio) if err == nil { @@ -96,7 +97,7 @@ func TestReconcile(t *testing.T) { WithStatusSubresource(&v1alpha1.RemoteIstio{}). WithObjects(istio). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "invalid-profile") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "invalid-profile") _, err := reconciler.Reconcile(ctx, istio) if err == nil { @@ -136,7 +137,7 @@ func TestReconcile(t *testing.T) { }, }). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") _, err := reconciler.Reconcile(ctx, istio) if err == nil { @@ -532,7 +533,7 @@ func TestDetermineStatus(t *testing.T) { WithObjects(initObjs...). WithInterceptorFuncs(interceptorFuncs). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") status, err := reconciler.determineStatus(ctx, istio, tc.reconciliationErr) if (err != nil) != tc.wantErr { @@ -733,7 +734,7 @@ func TestUpdateStatus(t *testing.T) { WithObjects(initObjs...). WithInterceptorFuncs(interceptorFuncs). Build() - reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "") + reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "") err := reconciler.updateStatus(ctx, istio, tc.reconciliationErr) if (err != nil) != tc.wantErr { diff --git a/pkg/config/platform.go b/pkg/config/platform.go index 521a29ed0..71bb23387 100644 --- a/pkg/config/platform.go +++ b/pkg/config/platform.go @@ -25,6 +25,7 @@ import ( type Platform string const ( + PlatformUndefined Platform = "" PlatformOpenShift Platform = "openshift" PlatformKubernetes Platform = "kubernetes" ) diff --git a/pkg/helm/values.go b/pkg/helm/values.go index 1538e9031..450d10e10 100644 --- a/pkg/helm/values.go +++ b/pkg/helm/values.go @@ -42,6 +42,19 @@ func (h *Values) Set(key string, val any) error { return unstructured.SetNestedField(*h, val, toKeys(key)...) } +// SetIfAbsent sets the value of a nested field to a deep copy of the value +// provided if the field does not exist. +func (h *Values) SetIfAbsent(key string, val any) error { + if _, found, err := unstructured.NestedFieldNoCopy(*h, toKeys(key)...); err != nil { + return fmt.Errorf("failed to get value %s: %w", key, err) + } else if !found { + if err := h.Set(key, val); err != nil { + return fmt.Errorf("failed to set value %s: %w", key, err) + } + } + return nil +} + func toKeys(key string) []string { return strings.Split(key, ".") } diff --git a/pkg/helm/values_test.go b/pkg/helm/values_test.go index d336854f1..b6405320b 100644 --- a/pkg/helm/values_test.go +++ b/pkg/helm/values_test.go @@ -144,3 +144,71 @@ func TestSet(t *testing.T) { }) } } + +func TestSetIfAbsent(t *testing.T) { + tests := []struct { + name string + input Values + key string + val string + expected Values + expectErr bool + }{ + { + name: "Key Exists", + input: Values{ + "foo": map[string]any{ + "bar": "baz", + }, + }, + key: "foo.bar", + val: "newVal", + expected: Values{ + "foo": map[string]any{ + "bar": "baz", + }, + }, + }, + { + name: "New Key", + input: Values{ + "foo": map[string]any{ + "bar": "baz", + }, + }, + key: "foo.baz", + val: "newVal", + expected: Values{ + "foo": map[string]any{ + "bar": "baz", + "baz": "newVal", + }, + }, + }, + { + name: "Key Exists, but is not a map", + input: Values{"foo": "bar"}, + key: "foo.baz", + val: "newVal", + expectErr: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := test.input.SetIfAbsent(test.key, test.val) + if test.expectErr { + if err == nil { + t.Errorf("Expected an error, but got nil") + } + } else { + if err != nil { + t.Errorf("Expected no error, but got an error: %v", err) + } + if !reflect.DeepEqual(test.input, test.expected) { + t.Errorf("Expected %v, but got %v", test.expected, test.input) + } + } + }) + } +} diff --git a/pkg/istiovalues/profiles.go b/pkg/istiovalues/profiles.go index fa59f4708..c211864c9 100644 --- a/pkg/istiovalues/profiles.go +++ b/pkg/istiovalues/profiles.go @@ -19,6 +19,7 @@ import ( "os" "path" + "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" "gopkg.in/yaml.v3" @@ -27,13 +28,22 @@ import ( "istio.io/istio/pkg/util/sets" ) -func ApplyProfiles(resourceDir string, version string, defaultProfile, userProfile string, userValues helm.Values) (helm.Values, error) { +func ApplyProfilesAndPlatform( + resourceDir string, version string, platform config.Platform, defaultProfile, userProfile string, userValues helm.Values, +) (helm.Values, error) { profile := resolve(defaultProfile, userProfile) defaultValues, err := getValuesFromProfiles(path.Join(resourceDir, version, "profiles"), profile) if err != nil { return nil, fmt.Errorf("failed to get values from profile %q: %w", profile, err) } - return mergeOverwrite(defaultValues, userValues), nil + values := helm.Values(mergeOverwrite(defaultValues, userValues)) + + if platform != config.PlatformKubernetes && platform != config.PlatformUndefined { + if err = values.SetIfAbsent("global.platform", string(platform)); err != nil { + return nil, fmt.Errorf("failed to set global.platform: %w", err) + } + } + return values, nil } func resolve(defaultProfile, userProfile string) []string { diff --git a/pkg/revision/values.go b/pkg/revision/values.go index 9666dee2a..c6f72127b 100644 --- a/pkg/revision/values.go +++ b/pkg/revision/values.go @@ -29,14 +29,14 @@ import ( // - applies overrides that are not configurable by the user func ComputeValues( userValues *v1alpha1.Values, namespace string, version string, - defaultProfile, userProfile string, resourceDir string, + platform config.Platform, defaultProfile, userProfile string, resourceDir string, activeRevisionName string, ) (*v1alpha1.Values, error) { // apply image digests from configuration, if not already set by user userValues = istiovalues.ApplyDigests(version, userValues, config.Config) // apply userValues on top of defaultValues from profiles - mergedHelmValues, err := istiovalues.ApplyProfiles(resourceDir, version, defaultProfile, userProfile, helm.FromValues(userValues)) + mergedHelmValues, err := istiovalues.ApplyProfilesAndPlatform(resourceDir, version, platform, defaultProfile, userProfile, helm.FromValues(userValues)) if err != nil { return nil, fmt.Errorf("failed to apply profile: %w", err) } diff --git a/pkg/revision/values_test.go b/pkg/revision/values_test.go index 4f0806fc3..14d224a2b 100644 --- a/pkg/revision/values_test.go +++ b/pkg/revision/values_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + "github.com/istio-ecosystem/sail-operator/pkg/config" "istio.io/istio/pkg/ptr" ) @@ -66,7 +67,7 @@ spec: }, } - result, err := ComputeValues(values, namespace, version, "default", "my-profile", resourceDir, revisionName) + result, err := ComputeValues(values, namespace, version, config.PlatformOpenShift, "default", "my-profile", resourceDir, revisionName) if err != nil { t.Errorf("Expected no error, but got an error: %v", err) } @@ -78,6 +79,7 @@ spec: Image: ptr.Of("from-istio-spec-values"), }, Global: &v1alpha1.GlobalConfig{ + Platform: ptr.Of("openshift"), IstioNamespace: ptr.Of(namespace), // this value is always added/overridden based on IstioRevision.spec.namespace }, Revision: ptr.Of(revisionName), diff --git a/tests/integration/api/suite_test.go b/tests/integration/api/suite_test.go index d060f325d..ebe2a04b5 100644 --- a/tests/integration/api/suite_test.go +++ b/tests/integration/api/suite_test.go @@ -25,6 +25,7 @@ import ( "github.com/istio-ecosystem/sail-operator/controllers/istiocni" "github.com/istio-ecosystem/sail-operator/controllers/istiorevision" "github.com/istio-ecosystem/sail-operator/controllers/remoteistio" + "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/scheme" "github.com/istio-ecosystem/sail-operator/pkg/test" @@ -74,16 +75,16 @@ var _ = BeforeSuite(func() { chartManager := helm.NewChartManager(mgr.GetConfig(), "") resourceDir := path.Join(project.RootDir, "resources") - Expect(istio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, ""). + Expect(istio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, config.PlatformKubernetes, ""). SetupWithManager(mgr)).To(Succeed()) - Expect(remoteistio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, ""). + Expect(remoteistio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, config.PlatformKubernetes, ""). SetupWithManager(mgr)).To(Succeed()) Expect(istiorevision.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, chartManager). SetupWithManager(mgr)).To(Succeed()) - Expect(istiocni.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, chartManager, ""). + Expect(istiocni.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDir, chartManager, config.PlatformKubernetes, ""). SetupWithManager(mgr)).To(Succeed()) // create new cancellable context