From 36467bc7079ca855c717bb6d664de4de34a2a1ec Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 22 Jan 2024 08:11:24 +0100 Subject: [PATCH] pr review Signed-off-by: odubajDT --- chart/open-feature-operator/README.md | 4 +- common/flagdproxy/flagdproxy.go | 64 ++++++---- common/flagdproxy/flagdproxy_test.go | 120 ++++++++++++++++-- .../core/featureflagsource/controller_test.go | 13 +- docs/installation.md | 4 +- docs/v1beta_migration.md | 2 +- 6 files changed, 166 insertions(+), 41 deletions(-) diff --git a/chart/open-feature-operator/README.md b/chart/open-feature-operator/README.md index 39c4345a4..509f58fe2 100644 --- a/chart/open-feature-operator/README.md +++ b/chart/open-feature-operator/README.md @@ -46,9 +46,9 @@ helm upgrade --install open-feature-operator openfeature/open-feature-operator ``` > [!NOTE] -> If you have used `flagd-proxy` provider and upgrading to OFO version `v0.5.4` or higher, +> If you upgrade to OFO `v0.5.4` or higher while using a `flagd-proxy` provider, the instance of `flagd-proxy` will be automatically upgraded to the latest supported version by the `open-feature-operator`. -This upgrade will also consider your current `FeatureFlagSource` configuration and adapt +The upgrade of `flagd-proxy` will also consider your current `FeatureFlagSource` configuration and adapt the `flagd-proxy` Deployment accordingly. If you are upgrading OFO to `v0.5.3` or lower, `flagd-proxy` (if present) won't be upgraded automatically. diff --git a/common/flagdproxy/flagdproxy.go b/common/flagdproxy/flagdproxy.go index 39eda8552..03887a8ab 100644 --- a/common/flagdproxy/flagdproxy.go +++ b/common/flagdproxy/flagdproxy.go @@ -21,6 +21,7 @@ const ( FlagdProxyServiceAccountName = "open-feature-operator-flagd-proxy" FlagdProxyServiceName = "flagd-proxy-svc" operatorDeploymentName = "open-feature-operator-controller-manager" + ofoDeployedProxy = "open-feature-operator-proxy-deployed" ) type FlagdProxyHandler struct { @@ -30,6 +31,7 @@ type FlagdProxyHandler struct { } type CreateUpdateFunc func(ctx context.Context, obj client.Object) error + type FlagdProxyConfiguration struct { Port int ManagementPort int @@ -78,41 +80,47 @@ func (f *FlagdProxyHandler) HandleFlagdProxy(ctx context.Context) error { return err } - ownerReferences := f.getOwnerReferences(ctx) - newDeployment := f.newFlagdProxyManifest(ownerReferences) - newService := f.newFlagdProxyServiceManifest(ownerReferences) + ownerReference, err := f.getOwnerReference(ctx) + if err != nil { + return err + } + newDeployment := f.newFlagdProxyManifest(ownerReference) + newService := f.newFlagdProxyServiceManifest(ownerReference) if !exists { f.Log.Info("flagd-proxy Deployment does not exist, creating") return f.deployFlagdProxy(ctx, f.createObject, newDeployment, newService) } - // flagd-proxy exists, need to check if it's the right version - if !f.isFlagdProxyUpToDate(deployment, newDeployment) { - f.Log.Info("flagd-proxy Deployment changed, updating") + // flagd-proxy exists, need to check if we should update it + if f.shouldUpdateFlagdProxy(deployment, newDeployment) { + f.Log.Info("flagd-proxy Deployment out of sync, updating") return f.deployFlagdProxy(ctx, f.updateObject, newDeployment, newService) } f.Log.Info("flagd-proxy Deployment up-to-date") return nil } -func (f *FlagdProxyHandler) deployFlagdProxy(ctx context.Context, ff CreateUpdateFunc, deployment *appsV1.Deployment, service *corev1.Service) error { +func (f *FlagdProxyHandler) deployFlagdProxy(ctx context.Context, createUpdateFunc CreateUpdateFunc, deployment *appsV1.Deployment, service *corev1.Service) error { f.Log.Info("deploying the flagd-proxy") - if err := ff(ctx, deployment); err != nil && !errors.IsAlreadyExists(err) { + if err := createUpdateFunc(ctx, deployment); err != nil && !errors.IsAlreadyExists(err) { return err } f.Log.Info("deploying the flagd-proxy service") - if err := ff(ctx, service); err != nil && !errors.IsAlreadyExists(err) { + if err := createUpdateFunc(ctx, service); err != nil && !errors.IsAlreadyExists(err) { return err } return nil } -func (f *FlagdProxyHandler) newFlagdProxyServiceManifest(ownerReferences []metav1.OwnerReference) *corev1.Service { +func (f *FlagdProxyHandler) newFlagdProxyServiceManifest(ownerReference *metav1.OwnerReference) *corev1.Service { return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: FlagdProxyServiceName, Namespace: f.config.Namespace, - OwnerReferences: ownerReferences, + OwnerReferences: []metav1.OwnerReference{*ownerReference}, + Labels: map[string]string{ + ofoDeployedProxy: "true", + }, }, Spec: corev1.ServiceSpec{ Selector: map[string]string{ @@ -130,7 +138,7 @@ func (f *FlagdProxyHandler) newFlagdProxyServiceManifest(ownerReferences []metav } } -func (f *FlagdProxyHandler) newFlagdProxyManifest(ownerReferences []metav1.OwnerReference) *appsV1.Deployment { +func (f *FlagdProxyHandler) newFlagdProxyManifest(ownerReference *metav1.OwnerReference) *appsV1.Deployment { replicas := int32(1) args := []string{ "start", @@ -148,8 +156,9 @@ func (f *FlagdProxyHandler) newFlagdProxyManifest(ownerReferences []metav1.Owner "app": FlagdProxyDeploymentName, "app.kubernetes.io/managed-by": ManagedByAnnotationValue, "app.kubernetes.io/version": f.config.Tag, + ofoDeployedProxy: "true", }, - OwnerReferences: ownerReferences, + OwnerReferences: []metav1.OwnerReference{*ownerReference}, }, Spec: appsV1.DeploymentSpec{ Replicas: &replicas, @@ -206,32 +215,35 @@ func (f *FlagdProxyHandler) doesFlagdProxyExist(ctx context.Context) (bool, *app return true, d, nil } -func (f *FlagdProxyHandler) isFlagdProxyUpToDate(old, new *appsV1.Deployment) bool { - return reflect.DeepEqual(old.Spec, new.Spec) +func (f *FlagdProxyHandler) shouldUpdateFlagdProxy(old, new *appsV1.Deployment) bool { + return isDeployedByOFO(old) && !reflect.DeepEqual(old.Spec, new.Spec) } func (f *FlagdProxyHandler) getOperatorDeployment(ctx context.Context) (*appsV1.Deployment, error) { d := &appsV1.Deployment{} if err := f.Client.Get(ctx, client.ObjectKey{Name: f.config.OperatorDeploymentName, Namespace: f.config.Namespace}, d); err != nil { - return nil, fmt.Errorf("unable to fetch operator deployment to create owner reference: %w", err) + return nil, fmt.Errorf("unable to fetch operator deployment: %w", err) } return d, nil } -func (f *FlagdProxyHandler) getOwnerReferences(ctx context.Context) []metav1.OwnerReference { +func (f *FlagdProxyHandler) getOwnerReference(ctx context.Context) (*metav1.OwnerReference, error) { operatorDeployment, err := f.getOperatorDeployment(ctx) if err != nil { f.Log.Error(err, "unable to create owner reference for open-feature-operator") - return []metav1.OwnerReference{} + return nil, err } - return []metav1.OwnerReference{ - { - UID: operatorDeployment.GetUID(), - Name: operatorDeployment.GetName(), - APIVersion: operatorDeployment.APIVersion, - Kind: operatorDeployment.Kind, - }, - } + return &metav1.OwnerReference{ + UID: operatorDeployment.GetUID(), + Name: operatorDeployment.GetName(), + APIVersion: operatorDeployment.APIVersion, + Kind: operatorDeployment.Kind, + }, nil +} + +func isDeployedByOFO(d *appsV1.Deployment) bool { + val, ok := d.Labels[ofoDeployedProxy] + return ok && val == "true" } diff --git a/common/flagdproxy/flagdproxy_test.go b/common/flagdproxy/flagdproxy_test.go index f1d628874..61309f6bb 100644 --- a/common/flagdproxy/flagdproxy_test.go +++ b/common/flagdproxy/flagdproxy_test.go @@ -126,7 +126,66 @@ func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExistsWithBadVersion(t *testing require.NotNil(t, kpConfig) - fakeClient := fake.NewClientBuilder().WithObjects(&v1.Deployment{ + fakeClient := fake.NewClientBuilder().WithObjects(createOFOTestDeployment(env.PodNamespace)).Build() + + ownerRef, err := getTestOFODeploymentOwnerRef(fakeClient, env.PodNamespace) + require.Nil(t, err) + + proxyDeployment := &v1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: kpConfig.Namespace, + Name: FlagdProxyDeploymentName, + OwnerReferences: []metav1.OwnerReference{*ownerRef}, + Labels: map[string]string{ + ofoDeployedProxy: "true", + }, + }, + Spec: v1.DeploymentSpec{ + Template: v12.PodTemplateSpec{ + Spec: v12.PodSpec{ + Containers: []v12.Container{ + { + Name: "my-container", + }, + }, + }, + }, + }, + } + + err = fakeClient.Create(context.TODO(), proxyDeployment) + require.Nil(t, err) + + ph := NewFlagdProxyHandler(kpConfig, fakeClient, testr.New(t)) + + require.NotNil(t, ph) + + err = ph.HandleFlagdProxy(context.Background()) + + require.Nil(t, err) + + deployment := &v1.Deployment{} + err = fakeClient.Get(context.Background(), client.ObjectKey{ + Namespace: env.PodNamespace, + Name: FlagdProxyDeploymentName, + }, deployment) + + require.Nil(t, err) + require.NotNil(t, deployment) + + // verify that the existing deployment has been changed + require.Equal(t, "flagd-proxy", deployment.Spec.Template.Spec.Containers[0].Name) +} + +func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExistsWithoutLabel(t *testing.T) { + env := types.EnvConfig{ + PodNamespace: "ns", + } + kpConfig := NewFlagdProxyConfiguration(env) + + require.NotNil(t, kpConfig) + + proxyDeployment := &v1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Namespace: kpConfig.Namespace, Name: FlagdProxyDeploymentName, @@ -142,7 +201,9 @@ func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExistsWithBadVersion(t *testing }, }, }, - }).Build() + } + + fakeClient := fake.NewClientBuilder().WithObjects(createOFOTestDeployment(env.PodNamespace), proxyDeployment).Build() ph := NewFlagdProxyHandler(kpConfig, fakeClient, testr.New(t)) @@ -161,8 +222,8 @@ func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExistsWithBadVersion(t *testing require.Nil(t, err) require.NotNil(t, deployment) - // verify that the existing deployment has been changed - require.Equal(t, "flagd-proxy", deployment.Spec.Template.Spec.Containers[0].Name) + // verify that the existing deployment has not been changed + require.Equal(t, "my-container", deployment.Spec.Template.Spec.Containers[0].Name) } func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExistsWithNewestVersion(t *testing.T) { @@ -173,15 +234,18 @@ func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExistsWithNewestVersion(t *test require.NotNil(t, kpConfig) - fakeClient := fake.NewClientBuilder().WithObjects().Build() + fakeClient := fake.NewClientBuilder().WithObjects(createOFOTestDeployment(env.PodNamespace)).Build() ph := NewFlagdProxyHandler(kpConfig, fakeClient, testr.New(t)) require.NotNil(t, ph) - proxy := ph.newFlagdProxyManifest([]metav1.OwnerReference{}) + ownerRef, err := getTestOFODeploymentOwnerRef(fakeClient, env.PodNamespace) + require.Nil(t, err) - err := fakeClient.Create(context.TODO(), proxy) + proxy := ph.newFlagdProxyManifest(ownerRef) + + err = fakeClient.Create(context.TODO(), proxy) require.Nil(t, err) err = ph.HandleFlagdProxy(context.Background()) @@ -214,7 +278,7 @@ func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { require.NotNil(t, kpConfig) - fakeClient := fake.NewClientBuilder().Build() + fakeClient := fake.NewClientBuilder().WithObjects(createOFOTestDeployment(env.PodNamespace)).Build() ph := NewFlagdProxyHandler(kpConfig, fakeClient, testr.New(t)) @@ -263,8 +327,16 @@ func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { "app": FlagdProxyDeploymentName, "app.kubernetes.io/managed-by": ManagedByAnnotationValue, "app.kubernetes.io/version": "tag", + ofoDeployedProxy: "true", }, ResourceVersion: "1", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "apps/v1", + Kind: "Deployment", + Name: operatorDeploymentName, + }, + }, }, Spec: appsV1.DeploymentSpec{ Replicas: &replicas, @@ -326,6 +398,16 @@ func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { Name: FlagdProxyServiceName, Namespace: "ns", ResourceVersion: "1", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "apps/v1", + Kind: "Deployment", + Name: operatorDeploymentName, + }, + }, + Labels: map[string]string{ + ofoDeployedProxy: "true", + }, }, Spec: corev1.ServiceSpec{ Selector: map[string]string{ @@ -344,3 +426,25 @@ func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { require.Equal(t, expectedService, service) } + +func createOFOTestDeployment(ns string) *v1.Deployment { + return &v1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: operatorDeploymentName, + }, + } +} + +func getTestOFODeploymentOwnerRef(c client.Client, ns string) (*metav1.OwnerReference, error) { + d := &appsV1.Deployment{} + if err := c.Get(context.TODO(), client.ObjectKey{Name: operatorDeploymentName, Namespace: ns}, d); err != nil { + return nil, err + } + return &metav1.OwnerReference{ + UID: d.GetUID(), + Name: d.GetName(), + APIVersion: d.APIVersion, + Kind: d.Kind, + }, nil +} diff --git a/controllers/core/featureflagsource/controller_test.go b/controllers/core/featureflagsource/controller_test.go index baba1f4c4..ea4b1e821 100644 --- a/controllers/core/featureflagsource/controller_test.go +++ b/controllers/core/featureflagsource/controller_test.go @@ -85,9 +85,9 @@ func TestFeatureFlagSourceReconciler_Reconcile(t *testing.T) { // setting up fake k8s client var fakeClient client.Client if tt.deployment != nil { - fakeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tt.fsConfig, tt.deployment).WithIndex(&appsv1.Deployment{}, fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), common.FeatureFlagSourceIndex).Build() + fakeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(createOFOTestDeployment(testNamespace), tt.fsConfig, tt.deployment).WithIndex(&appsv1.Deployment{}, fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), common.FeatureFlagSourceIndex).Build() } else { - fakeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tt.fsConfig).WithIndex(&appsv1.Deployment{}, fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), common.FeatureFlagSourceIndex).Build() + fakeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(createOFOTestDeployment(testNamespace), tt.fsConfig).WithIndex(&appsv1.Deployment{}, fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), common.FeatureFlagSourceIndex).Build() } kpConfig := flagdproxy.NewFlagdProxyConfiguration(commontypes.EnvConfig{ FlagdProxyImage: "ghcr.io/open-feature/flagd-proxy", @@ -210,3 +210,12 @@ func createTestFSConfig(fsConfigName string, testNamespace string, rollout bool, return fsConfig } + +func createOFOTestDeployment(ns string) *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: "open-feature-operator-controller-manager", + }, + } +} diff --git a/docs/installation.md b/docs/installation.md index 7180f7970..a84a4b3fa 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -29,9 +29,9 @@ helm upgrade --install openfeature openfeature/open-feature-operator ``` > [!NOTE] -> If you have used `flagd-proxy` provider and upgrading to OFO version `v0.5.4` or higher, +> If you upgrade to OFO `v0.5.4` or higher while using a `flagd-proxy` provider, the instance of `flagd-proxy` will be automatically upgraded to the latest supported version by the `open-feature-operator`. -This upgrade will also consider your current `FeatureFlagSource` configuration and adapt +The upgrade of `flagd-proxy` will also consider your current `FeatureFlagSource` configuration and adapt the `flagd-proxy` Deployment accordingly. If you are upgrading OFO to `v0.5.3` or lower, `flagd-proxy` (if present) won't be upgraded automatically. diff --git a/docs/v1beta_migration.md b/docs/v1beta_migration.md index 7ea6f2263..2b1c74705 100644 --- a/docs/v1beta_migration.md +++ b/docs/v1beta_migration.md @@ -80,6 +80,6 @@ If you have used `flagd-proxy` provider, then you have to upgrade the image used For this, please edit the deployment of `flagd-proxy` to version [v0.3.1](https://github.com/open-feature/flagd/pkgs/container/flagd-proxy/152333134?tag=v0.3.1) or above. > [!NOTE] -> Since OFO version `v0.5.4`, `flagd-proxy` pod (if present) will be upgraded automatically to the +> Since OFO version `v0.5.4`, the instance of `flagd-proxy` pod (if used) will be upgraded automatically to the to the latest supported version by `open-feature-operator`. For more information see the [upgrade section](./installation.md#upgrading).