From ef0f8d9abf5020d0678290930a6288d2dcf5d7cb Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 20 Nov 2023 11:54:38 +0100 Subject: [PATCH 1/8] chore: refactor code to decrease complexity Signed-off-by: odubajDT --- apis/core/v1beta1/featureflagsource_types.go | 2 +- common/common.go | 48 ++++---- common/constant/configuration.go | 17 --- common/constant/errors.go | 6 - common/flagdinjector/flagdinjector.go | 85 +++++++------- common/flagdinjector/flagdinjector_test.go | 70 +++--------- common/flagdproxy/flagdproxy.go | 4 +- common/flagdproxy/flagdproxy_test.go | 12 +- common/types/envconfig.go | 24 ++++ .../{source-config.go => sourceconfig.go} | 0 common/utils/utils.go | 26 ----- common/utils/utils_test.go | 10 ++ .../core/featureflagsource/controller.go | 12 +- .../core/featureflagsource/controller_test.go | 3 +- main.go | 106 ++++++++++-------- webhooks/common.go | 8 +- webhooks/common_test.go | 8 +- webhooks/pod_webhook.go | 12 +- webhooks/pod_webhook_test.go | 58 +++++----- 19 files changed, 235 insertions(+), 276 deletions(-) delete mode 100644 common/constant/configuration.go delete mode 100644 common/constant/errors.go create mode 100644 common/types/envconfig.go rename common/types/{source-config.go => sourceconfig.go} (100%) diff --git a/apis/core/v1beta1/featureflagsource_types.go b/apis/core/v1beta1/featureflagsource_types.go index b095616b1..c9507ea4e 100644 --- a/apis/core/v1beta1/featureflagsource_types.go +++ b/apis/core/v1beta1/featureflagsource_types.go @@ -194,7 +194,7 @@ func (fc *FeatureFlagSourceSpec) Merge(new *FeatureFlagSourceSpec) { if len(new.EnvVars) != 0 { fc.EnvVars = append(fc.EnvVars, new.EnvVars...) } - if new.SyncProviderArgs != nil && len(new.SyncProviderArgs) > 0 { + if len(new.SyncProviderArgs) != 0 { fc.SyncProviderArgs = append(fc.SyncProviderArgs, new.SyncProviderArgs...) } if new.EnvVarPrefix != "" { diff --git a/common/common.go b/common/common.go index 7a308db83..16521428e 100644 --- a/common/common.go +++ b/common/common.go @@ -2,46 +2,38 @@ package common import ( "context" + "errors" "fmt" "time" api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" appsV1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) const ( - ReconcileErrorInterval = 10 * time.Second - ReconcileSuccessInterval = 120 * time.Second - FinalizerName = "featureflag.core.openfeature.dev/finalizer" - OpenFeatureAnnotationPath = "spec.template.metadata.annotations.openfeature.dev/openfeature.dev" - FeatureFlagSourceAnnotation = "featureflagsource" - OpenFeatureAnnotationRoot = "openfeature.dev" + ReconcileErrorInterval = 10 * time.Second + ReconcileSuccessInterval = 120 * time.Second + FinalizerName = "featureflag.core.openfeature.dev/finalizer" + OpenFeatureAnnotationPath = "spec.template.metadata.annotations.openfeature.dev/openfeature.dev" + OpenFeatureAnnotationRoot = "openfeature.dev" + FlagDImagePullPolicy corev1.PullPolicy = "Always" + ClusterRoleBindingName string = "open-feature-operator-flagd-kubernetes-sync" + AllowKubernetesSyncAnnotation = "allowkubernetessync" + OpenFeatureAnnotationPrefix = "openfeature.dev" + PodOpenFeatureAnnotationPath = "metadata.annotations.openfeature.dev" + SourceConfigParam = "--sources" + ProbeReadiness = "/readyz" + ProbeLiveness = "/healthz" + ProbeInitialDelay = 5 + FeatureFlagSourceAnnotation = "featureflagsource" + EnabledAnnotation = "enabled" ) -type EnvConfig struct { - PodNamespace string `envconfig:"POD_NAMESPACE" default:"open-feature-operator-system"` - FlagdProxyImage string `envconfig:"FLAGD_PROXY_IMAGE" default:"ghcr.io/open-feature/flagd-proxy"` - // renovate: datasource=github-tags depName=open-feature/flagd/flagd-proxy - FlagdProxyTag string `envconfig:"FLAGD_PROXY_TAG" default:"v0.3.0"` - FlagdProxyPort int `envconfig:"FLAGD_PROXY_PORT" default:"8015"` - FlagdProxyManagementPort int `envconfig:"FLAGD_PROXY_MANAGEMENT_PORT" default:"8016"` - FlagdProxyDebugLogging bool `envconfig:"FLAGD_PROXY_DEBUG_LOGGING" default:"false"` - - SidecarEnvVarPrefix string `envconfig:"SIDECAR_ENV_VAR_PREFIX" default:"FLAGD"` - SidecarManagementPort int `envconfig:"SIDECAR_MANAGEMENT_PORT" default:"8014"` - SidecarPort int `envconfig:"SIDECAR_PORT" default:"8013"` - SidecarImage string `envconfig:"SIDECAR_IMAGE" default:"ghcr.io/open-feature/flagd"` - // renovate: datasource=github-tags depName=open-feature/flagd/flagd-proxy - SidecarTag string `envconfig:"SIDECAR_TAG" default:"v0.7.0"` - SidecarSocketPath string `envconfig:"SIDECAR_SOCKET_PATH" default:""` - SidecarEvaluator string `envconfig:"SIDECAR_EVALUATOR" default:"json"` - SidecarProviderArgs string `envconfig:"SIDECAR_PROVIDER_ARGS" default:""` - SidecarSyncProvider string `envconfig:"SIDECAR_SYNC_PROVIDER" default:"kubernetes"` - SidecarLogFormat string `envconfig:"SIDECAR_LOG_FORMAT" default:"json"` - SidecarProbesEnabled bool `envconfig:"SIDECAR_PROBES_ENABLED" default:"true"` -} +var ErrFlagdProxyNotReady = errors.New("flagd-proxy is not ready, deferring pod admission") +var ErrUnrecognizedSyncProvider = errors.New("unrecognized sync provider") func FeatureFlagSourceIndex(o client.Object) []string { deployment, ok := o.(*appsV1.Deployment) diff --git a/common/constant/configuration.go b/common/constant/configuration.go deleted file mode 100644 index b3115d4ea..000000000 --- a/common/constant/configuration.go +++ /dev/null @@ -1,17 +0,0 @@ -package constant - -import corev1 "k8s.io/api/core/v1" - -const ( - FlagDImagePullPolicy corev1.PullPolicy = "Always" - ClusterRoleBindingName string = "open-feature-operator-flagd-kubernetes-sync" - AllowKubernetesSyncAnnotation = "allowkubernetessync" - OpenFeatureAnnotationPrefix = "openfeature.dev" - OpenFeatureAnnotationPath = "metadata.annotations.openfeature.dev" - SourceConfigParam = "--sources" - ProbeReadiness = "/readyz" - ProbeLiveness = "/healthz" - ProbeInitialDelay = 5 - FeatureFlagSourceAnnotation = "featureflagsource" - EnabledAnnotation = "enabled" -) diff --git a/common/constant/errors.go b/common/constant/errors.go deleted file mode 100644 index 2975b6a42..000000000 --- a/common/constant/errors.go +++ /dev/null @@ -1,6 +0,0 @@ -package constant - -import "errors" - -var ErrFlagdProxyNotReady = errors.New("flagd-proxy is not ready, deferring pod admission") -var ErrUnrecognizedSyncProvider = errors.New("unrecognized sync provider") diff --git a/common/flagdinjector/flagdinjector.go b/common/flagdinjector/flagdinjector.go index 0dc2ed505..0d67a3bbb 100644 --- a/common/flagdinjector/flagdinjector.go +++ b/common/flagdinjector/flagdinjector.go @@ -10,7 +10,6 @@ import ( api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" apicommon "github.com/open-feature/open-feature-operator/apis/core/v1beta1/common" "github.com/open-feature/open-feature-operator/common" - "github.com/open-feature/open-feature-operator/common/constant" "github.com/open-feature/open-feature-operator/common/flagdproxy" "github.com/open-feature/open-feature-operator/common/types" "github.com/open-feature/open-feature-operator/common/utils" @@ -63,8 +62,8 @@ func (fi *FlagdContainerInjector) InjectFlagd( // Enable probes if flagSourceConfig.ProbesEnabled != nil && *flagSourceConfig.ProbesEnabled { - flagdContainer.LivenessProbe = buildProbe(constant.ProbeLiveness, int(flagSourceConfig.ManagementPort)) - flagdContainer.ReadinessProbe = buildProbe(constant.ProbeReadiness, int(flagSourceConfig.ManagementPort)) + flagdContainer.LivenessProbe = buildProbe(common.ProbeLiveness, int(flagSourceConfig.ManagementPort)) + flagdContainer.ReadinessProbe = buildProbe(common.ProbeReadiness, int(flagSourceConfig.ManagementPort)) } if err := fi.handleSidecarSources(ctx, objectMeta, podSpec, flagSourceConfig, &flagdContainer); err != nil { @@ -141,11 +140,11 @@ func (fi *FlagdContainerInjector) EnableClusterRoleBinding(ctx context.Context, fi.Logger.V(1).Info(fmt.Sprintf("ServiceAccount not found: %s/%s", serviceAccount.Namespace, serviceAccount.Name)) return err } - fi.Logger.V(1).Info(fmt.Sprintf("Fetching clusterrolebinding: %s", constant.ClusterRoleBindingName)) + fi.Logger.V(1).Info(fmt.Sprintf("Fetching clusterrolebinding: %s", common.ClusterRoleBindingName)) // Fetch service account if it exists crb := rbacv1.ClusterRoleBinding{} - if err := fi.Client.Get(ctx, client.ObjectKey{Name: constant.ClusterRoleBindingName}, &crb); errors.IsNotFound(err) { - fi.Logger.V(1).Info(fmt.Sprintf("ClusterRoleBinding not found: %s", constant.ClusterRoleBindingName)) + if err := fi.Client.Get(ctx, client.ObjectKey{Name: common.ClusterRoleBindingName}, &crb); errors.IsNotFound(err) { + fi.Logger.V(1).Info(fmt.Sprintf("ClusterRoleBinding not found: %s", common.ClusterRoleBindingName)) return err } found := false @@ -186,7 +185,6 @@ func (fi *FlagdContainerInjector) handleSidecarSources(ctx context.Context, obje return nil } -//nolint:gocyclo func (fi *FlagdContainerInjector) buildSources(ctx context.Context, objectMeta *metav1.ObjectMeta, flagSourceConfig *api.FeatureFlagSourceSpec, podSpec *corev1.PodSpec, sidecar *corev1.Container) ([]types.SourceConfig, error) { var sourceCfgCollection []types.SourceConfig @@ -195,40 +193,49 @@ func (fi *FlagdContainerInjector) buildSources(ctx context.Context, objectMeta * source.Provider = flagSourceConfig.DefaultSyncProvider } - var sourceCfg types.SourceConfig - var err error - - switch { - case source.Provider.IsKubernetes(): - sourceCfg, err = fi.toKubernetesProviderConfig(ctx, objectMeta, podSpec, source) - if err != nil { - return []types.SourceConfig{}, err - } - case source.Provider.IsFilepath(): - sourceCfg, err = fi.toFilepathProviderConfig(ctx, objectMeta, podSpec, sidecar, source) - if err != nil { - return []types.SourceConfig{}, err - } - case source.Provider.IsHttp(): - sourceCfg = fi.toHttpProviderConfig(source) - case source.Provider.IsGrpc(): - sourceCfg = fi.toGrpcProviderConfig(source) - case source.Provider.IsFlagdProxy(): - sourceCfg, err = fi.toFlagdProxyConfig(ctx, objectMeta, source) - if err != nil { - return []types.SourceConfig{}, err - } - default: - return []types.SourceConfig{}, fmt.Errorf("could not add provider %s: %w", source.Provider, constant.ErrUnrecognizedSyncProvider) + sourceCfg, err := fi.createSourceConfig(ctx, source, objectMeta, podSpec, sidecar) + if err != nil { + return []types.SourceConfig{}, err } - sourceCfgCollection = append(sourceCfgCollection, sourceCfg) + sourceCfgCollection = append(sourceCfgCollection, *sourceCfg) } return sourceCfgCollection, nil } +func (fi *FlagdContainerInjector) createSourceConfig(ctx context.Context, source api.Source, objectMeta *metav1.ObjectMeta, podSpec *corev1.PodSpec, sidecar *corev1.Container) (*types.SourceConfig, error) { + var sourceCfg types.SourceConfig + var err error + + switch { + case source.Provider.IsKubernetes(): + sourceCfg, err = fi.toKubernetesProviderConfig(ctx, objectMeta, podSpec, source) + if err != nil { + return nil, err + } + case source.Provider.IsFilepath(): + sourceCfg, err = fi.toFilepathProviderConfig(ctx, objectMeta, podSpec, sidecar, source) + if err != nil { + return nil, err + } + case source.Provider.IsHttp(): + sourceCfg = fi.toHttpProviderConfig(source) + case source.Provider.IsGrpc(): + sourceCfg = fi.toGrpcProviderConfig(source) + case source.Provider.IsFlagdProxy(): + sourceCfg, err = fi.toFlagdProxyConfig(ctx, objectMeta, source) + if err != nil { + return nil, err + } + default: + return nil, fmt.Errorf("could not add provider %s: %w", source.Provider, common.ErrUnrecognizedSyncProvider) + } + + return &sourceCfg, nil +} + func (fi *FlagdContainerInjector) toFilepathProviderConfig(ctx context.Context, objectMeta *metav1.ObjectMeta, podSpec *corev1.PodSpec, sidecar *corev1.Container, source api.Source) (types.SourceConfig, error) { // create config map ns, n := utils.ParseAnnotation(source.Source, objectMeta.Namespace) @@ -312,7 +319,7 @@ func (fi *FlagdContainerInjector) toFlagdProxyConfig(ctx context.Context, object return types.SourceConfig{}, err } if !exists || (exists && !ready) { - return types.SourceConfig{}, constant.ErrFlagdProxyNotReady + return types.SourceConfig{}, common.ErrFlagdProxyNotReady } ns, n := utils.ParseAnnotation(source.Source, objectMeta.Namespace) return types.SourceConfig{ @@ -339,7 +346,7 @@ func (fi *FlagdContainerInjector) isFlagdProxyReady(ctx context.Context) (bool, return true, false, fmt.Errorf( "flagd-proxy not ready after 3 minutes, was created at %s: %w", d.CreationTimestamp.Time.String(), - constant.ErrFlagdProxyNotReady, + common.ErrFlagdProxyNotReady, ) } return true, false, nil @@ -365,7 +372,7 @@ func (fi *FlagdContainerInjector) toKubernetesProviderConfig(ctx context.Context if objectMeta.Annotations == nil { objectMeta.Annotations = map[string]string{} } - objectMeta.Annotations[fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation)] = "true" + objectMeta.Annotations[fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation)] = "true" // build K8s config return types.SourceConfig{ @@ -381,7 +388,7 @@ func (fi *FlagdContainerInjector) generateBasicFlagdContainer(flagSourceConfig * Args: []string{ "start", }, - ImagePullPolicy: constant.FlagDImagePullPolicy, + ImagePullPolicy: common.FlagDImagePullPolicy, VolumeMounts: []corev1.VolumeMount{}, Env: []corev1.EnvVar{}, Ports: []corev1.ContainerPort{ @@ -437,7 +444,7 @@ func appendSources(sources []types.SourceConfig, sidecar *corev1.Container) erro return err } - sidecar.Args = append(sidecar.Args, constant.SourceConfigParam, string(bytes)) + sidecar.Args = append(sidecar.Args, common.SourceConfigParam, string(bytes)) return nil } @@ -479,6 +486,6 @@ func buildProbe(path string, port int) *corev1.Probe { ProbeHandler: corev1.ProbeHandler{ HTTPGet: httpGetAction, }, - InitialDelaySeconds: constant.ProbeInitialDelay, + InitialDelaySeconds: common.ProbeInitialDelay, } } diff --git a/common/flagdinjector/flagdinjector_test.go b/common/flagdinjector/flagdinjector_test.go index 7518f2196..4f9200e28 100644 --- a/common/flagdinjector/flagdinjector_test.go +++ b/common/flagdinjector/flagdinjector_test.go @@ -9,7 +9,7 @@ import ( "github.com/go-logr/logr/testr" api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" apicommon "github.com/open-feature/open-feature-operator/apis/core/v1beta1/common" - "github.com/open-feature/open-feature-operator/common/constant" + "github.com/open-feature/open-feature-operator/common" "github.com/open-feature/open-feature-operator/common/flagdproxy" "github.com/open-feature/open-feature-operator/common/utils" "github.com/stretchr/testify/require" @@ -285,7 +285,7 @@ func TestFlagdContainerInjector_InjectFlagdKubernetesSource(t *testing.T) { // verify the update of the ClusterRoleBinding cbr := &rbacv1.ClusterRoleBinding{} - err = fakeClient.Get(context.Background(), client.ObjectKey{Name: constant.ClusterRoleBindingName}, cbr) + err = fakeClient.Get(context.Background(), client.ObjectKey{Name: common.ClusterRoleBindingName}, cbr) require.Nil(t, err) @@ -580,7 +580,7 @@ func TestFlagdContainerInjector_InjectProxySource_ProxyNotAvailable(t *testing.T // expect an error here because we do not have a flagd proxy in our cluster require.NotNil(t, err) - require.ErrorIs(t, err, constant.ErrFlagdProxyNotReady) + require.ErrorIs(t, err, common.ErrFlagdProxyNotReady) } func TestFlagdContainerInjector_InjectProxySource_ProxyNotReady(t *testing.T) { @@ -621,7 +621,7 @@ func TestFlagdContainerInjector_InjectProxySource_ProxyNotReady(t *testing.T) { err = fi.InjectFlagd(context.Background(), &deployment.ObjectMeta, &deployment.Spec.Template.Spec, flagSourceConfig) require.NotNil(t, err) - require.ErrorIs(t, err, constant.ErrFlagdProxyNotReady) + require.ErrorIs(t, err, common.ErrFlagdProxyNotReady) } func TestFlagdContainerInjector_InjectProxySource_ProxyIsReady(t *testing.T) { @@ -753,7 +753,7 @@ func TestFlagdContainerInjector_InjectUnknownSyncProvider(t *testing.T) { err := fi.InjectFlagd(context.Background(), &deployment.ObjectMeta, &deployment.Spec.Template.Spec, flagSourceConfig) require.NotNil(t, err) - require.ErrorIs(t, err, constant.ErrUnrecognizedSyncProvider) + require.ErrorIs(t, err, common.ErrUnrecognizedSyncProvider) } func TestFlagdContainerInjector_createConfigMap(t *testing.T) { @@ -843,7 +843,7 @@ func initContainerInjectionTestEnv() (string, client.WithWatch) { cbr := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: constant.ClusterRoleBindingName, + Name: common.ClusterRoleBindingName, }, } @@ -1035,67 +1035,27 @@ func Test_getSecurityContext(t *testing.T) { } } -//nolint:dupl func TestFlagdContainerInjector_EnableClusterRoleBinding_AddDefaultServiceAccountName(t *testing.T) { - - namespace, fakeClient := initEnableClusterroleBindingTestEnv() - - serviceAccount := &v1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: namespace, - }, - } - - crb := &rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: constant.ClusterRoleBindingName, - }, - } - - err := fakeClient.Create(context.Background(), serviceAccount) - require.Nil(t, err) - - err = fakeClient.Create(context.Background(), crb) - require.Nil(t, err) - - fi := &FlagdContainerInjector{ - Client: fakeClient, - Logger: testr.New(t), - FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), - Image: testImage, - Tag: testTag, - } - - err = fi.EnableClusterRoleBinding(context.Background(), namespace, "") - require.Nil(t, err) - - updatedCrb := &rbacv1.ClusterRoleBinding{} - err = fakeClient.Get(context.Background(), client.ObjectKey{Name: crb.Name}, updatedCrb) - - require.Nil(t, err) - - require.Len(t, updatedCrb.Subjects, 1) - require.Equal(t, "default", updatedCrb.Subjects[0].Name) - require.Equal(t, namespace, updatedCrb.Subjects[0].Namespace) + enableClusterRoleBindingTest(t, "default", "") } -//nolint:dupl func TestFlagdContainerInjector_EnableClusterRoleBinding_ServiceAccountName(t *testing.T) { + enableClusterRoleBindingTest(t, "my-serviceaccount", "my-serviceaccount") +} +func enableClusterRoleBindingTest(t *testing.T, name string, input string) { namespace, fakeClient := initEnableClusterroleBindingTestEnv() serviceAccount := &v1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-serviceaccount", + Name: name, Namespace: namespace, }, } crb := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: constant.ClusterRoleBindingName, + Name: common.ClusterRoleBindingName, }, } @@ -1114,7 +1074,7 @@ func TestFlagdContainerInjector_EnableClusterRoleBinding_ServiceAccountName(t *t Tag: testTag, } - err = fi.EnableClusterRoleBinding(context.Background(), namespace, "my-serviceaccount") + err = fi.EnableClusterRoleBinding(context.Background(), namespace, input) require.Nil(t, err) updatedCrb := &rbacv1.ClusterRoleBinding{} @@ -1123,7 +1083,7 @@ func TestFlagdContainerInjector_EnableClusterRoleBinding_ServiceAccountName(t *t require.Nil(t, err) require.Len(t, updatedCrb.Subjects, 1) - require.Equal(t, "my-serviceaccount", updatedCrb.Subjects[0].Name) + require.Equal(t, name, updatedCrb.Subjects[0].Name) require.Equal(t, namespace, updatedCrb.Subjects[0].Namespace) } @@ -1140,7 +1100,7 @@ func TestFlagdContainerInjector_EnableClusterRoleBinding_ServiceAccountAlreadyIn crb := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: constant.ClusterRoleBindingName, + Name: common.ClusterRoleBindingName, }, Subjects: []rbacv1.Subject{ { diff --git a/common/flagdproxy/flagdproxy.go b/common/flagdproxy/flagdproxy.go index 7f27e78ff..fbb809935 100644 --- a/common/flagdproxy/flagdproxy.go +++ b/common/flagdproxy/flagdproxy.go @@ -5,7 +5,7 @@ import ( "fmt" "github.com/go-logr/logr" - "github.com/open-feature/open-feature-operator/common" + "github.com/open-feature/open-feature-operator/common/types" appsV1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -38,7 +38,7 @@ type FlagdProxyConfiguration struct { OperatorDeploymentName string } -func NewFlagdProxyConfiguration(env common.EnvConfig) *FlagdProxyConfiguration { +func NewFlagdProxyConfiguration(env types.EnvConfig) *FlagdProxyConfiguration { return &FlagdProxyConfiguration{ Image: env.FlagdProxyImage, Tag: env.FlagdProxyTag, diff --git a/common/flagdproxy/flagdproxy_test.go b/common/flagdproxy/flagdproxy_test.go index cc497f065..f8278697d 100644 --- a/common/flagdproxy/flagdproxy_test.go +++ b/common/flagdproxy/flagdproxy_test.go @@ -5,7 +5,7 @@ import ( "testing" "github.com/go-logr/logr/testr" - "github.com/open-feature/open-feature-operator/common" + "github.com/open-feature/open-feature-operator/common/types" "github.com/stretchr/testify/require" v1 "k8s.io/api/apps/v1" v12 "k8s.io/api/core/v1" @@ -15,7 +15,7 @@ import ( ) func TestNewFlagdProxyConfiguration(t *testing.T) { - kpConfig := NewFlagdProxyConfiguration(common.EnvConfig{ + kpConfig := NewFlagdProxyConfiguration(types.EnvConfig{ FlagdProxyPort: 8015, FlagdProxyManagementPort: 8016, }) @@ -30,7 +30,7 @@ func TestNewFlagdProxyConfiguration(t *testing.T) { } func TestNewFlagdProxyConfiguration_OverrideEnvVars(t *testing.T) { - env := common.EnvConfig{ + env := types.EnvConfig{ FlagdProxyImage: "my-image", FlagdProxyTag: "my-tag", PodNamespace: "my-namespace", @@ -54,7 +54,7 @@ func TestNewFlagdProxyConfiguration_OverrideEnvVars(t *testing.T) { } func TestNewFlagdProxyHandler(t *testing.T) { - kpConfig := NewFlagdProxyConfiguration(common.EnvConfig{}) + kpConfig := NewFlagdProxyConfiguration(types.EnvConfig{}) require.NotNil(t, kpConfig) @@ -68,7 +68,7 @@ func TestNewFlagdProxyHandler(t *testing.T) { } func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExists(t *testing.T) { - env := common.EnvConfig{ + env := types.EnvConfig{ PodNamespace: "ns", } kpConfig := NewFlagdProxyConfiguration(env) @@ -115,7 +115,7 @@ func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExists(t *testing.T) { } func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { - env := common.EnvConfig{ + env := types.EnvConfig{ PodNamespace: "ns", } kpConfig := NewFlagdProxyConfiguration(env) diff --git a/common/types/envconfig.go b/common/types/envconfig.go new file mode 100644 index 000000000..0d1b4624e --- /dev/null +++ b/common/types/envconfig.go @@ -0,0 +1,24 @@ +package types + +type EnvConfig struct { + PodNamespace string `envconfig:"POD_NAMESPACE" default:"open-feature-operator-system"` + FlagdProxyImage string `envconfig:"FLAGD_PROXY_IMAGE" default:"ghcr.io/open-feature/flagd-proxy"` + // renovate: datasource=github-tags depName=open-feature/flagd/flagd-proxy + FlagdProxyTag string `envconfig:"FLAGD_PROXY_TAG" default:"v0.3.0"` + FlagdProxyPort int `envconfig:"FLAGD_PROXY_PORT" default:"8015"` + FlagdProxyManagementPort int `envconfig:"FLAGD_PROXY_MANAGEMENT_PORT" default:"8016"` + FlagdProxyDebugLogging bool `envconfig:"FLAGD_PROXY_DEBUG_LOGGING" default:"false"` + + SidecarEnvVarPrefix string `envconfig:"SIDECAR_ENV_VAR_PREFIX" default:"FLAGD"` + SidecarManagementPort int `envconfig:"SIDECAR_MANAGEMENT_PORT" default:"8014"` + SidecarPort int `envconfig:"SIDECAR_PORT" default:"8013"` + SidecarImage string `envconfig:"SIDECAR_IMAGE" default:"ghcr.io/open-feature/flagd"` + // renovate: datasource=github-tags depName=open-feature/flagd/flagd-proxy + SidecarTag string `envconfig:"SIDECAR_TAG" default:"v0.7.0"` + SidecarSocketPath string `envconfig:"SIDECAR_SOCKET_PATH" default:""` + SidecarEvaluator string `envconfig:"SIDECAR_EVALUATOR" default:"json"` + SidecarProviderArgs string `envconfig:"SIDECAR_PROVIDER_ARGS" default:""` + SidecarSyncProvider string `envconfig:"SIDECAR_SYNC_PROVIDER" default:"kubernetes"` + SidecarLogFormat string `envconfig:"SIDECAR_LOG_FORMAT" default:"json"` + SidecarProbesEnabled bool `envconfig:"SIDECAR_PROBES_ENABLED" default:"true"` +} diff --git a/common/types/source-config.go b/common/types/sourceconfig.go similarity index 100% rename from common/types/source-config.go rename to common/types/sourceconfig.go diff --git a/common/utils/utils.go b/common/utils/utils.go index a9b530b85..52515c4b4 100644 --- a/common/utils/utils.go +++ b/common/utils/utils.go @@ -2,8 +2,6 @@ package utils import ( "fmt" - "os" - "strconv" "strings" ) @@ -34,30 +32,6 @@ func ParseAnnotation(s string, defaultNs string) (string, string) { return defaultNs, s } -func GetIntEnvVar(key string, defaultVal int) (int, error) { - val, ok := os.LookupEnv(key) - if !ok { - return defaultVal, nil - } - valInt, err := strconv.Atoi(val) - if err != nil { - return 0, fmt.Errorf("could not parse %s env var to int: %w", key, err) - } - return valInt, nil -} - -func GetBoolEnvVar(key string, defaultVal bool) (bool, error) { - val, ok := os.LookupEnv(key) - if !ok { - return defaultVal, nil - } - valBool, err := strconv.ParseBool(val) - if err != nil { - return false, fmt.Errorf("could not parse %s env var to bool: %w", key, err) - } - return valBool, nil -} - // unique string used to create unique volume mount and file name func FeatureFlagId(namespace, name string) string { return fmt.Sprintf("%s_%s", namespace, name) diff --git a/common/utils/utils_test.go b/common/utils/utils_test.go index 441bf1d45..5f7517321 100644 --- a/common/utils/utils_test.go +++ b/common/utils/utils_test.go @@ -29,3 +29,13 @@ func Test_ContainsString(t *testing.T) { require.True(t, ContainsString(slice, "str1")) require.False(t, ContainsString(slice, "some")) } + +func Test_ParseAnnotations(t *testing.T) { + s1, s2 := ParseAnnotation("some/anno", "default") + require.Equal(t, "some", s1) + require.Equal(t, "anno", s2) + + s1, s2 = ParseAnnotation("anno", "default") + require.Equal(t, "default", s1) + require.Equal(t, "anno", s2) +} diff --git a/controllers/core/featureflagsource/controller.go b/controllers/core/featureflagsource/controller.go index 93c62e87c..469c86836 100644 --- a/controllers/core/featureflagsource/controller.go +++ b/controllers/core/featureflagsource/controller.go @@ -55,8 +55,6 @@ type FeatureFlagSourceReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile -// -//nolint:gocyclo func (r *FeatureFlagSourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { r.Log.Info("Searching for FeatureFlagSource") @@ -86,6 +84,12 @@ func (r *FeatureFlagSourceReconciler) Reconcile(ctx context.Context, req ctrl.Re return r.finishReconcile(nil, false) } + err := r.handleDeploymentUpdate(ctx, fsConfig) + + return r.finishReconcile(err, false) +} + +func (r *FeatureFlagSourceReconciler) handleDeploymentUpdate(ctx context.Context, fsConfig *api.FeatureFlagSource) error { // Object has been updated, so, we can restart any deployments that are using this annotation // => we know there has been an update because we are using the GenerationChangedPredicate filter // and our resource exists within the cluster @@ -94,7 +98,7 @@ func (r *FeatureFlagSourceReconciler) Reconcile(ctx context.Context, req ctrl.Re fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation): "true", }); err != nil { r.Log.Error(err, fmt.Sprintf("Failed to get the pods with annotation %s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation)) - return r.finishReconcile(err, false) + return err } // Loop through all deployments containing the openfeature.dev/featureflagsource annotation @@ -115,7 +119,7 @@ func (r *FeatureFlagSourceReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - return r.finishReconcile(nil, false) + return nil } func (r *FeatureFlagSourceReconciler) isUsingConfiguration(namespace string, name string, deploymentNamespace string, annotation string) bool { diff --git a/controllers/core/featureflagsource/controller_test.go b/controllers/core/featureflagsource/controller_test.go index 53a6803b9..06c4b50c2 100644 --- a/controllers/core/featureflagsource/controller_test.go +++ b/controllers/core/featureflagsource/controller_test.go @@ -10,6 +10,7 @@ import ( apicommon "github.com/open-feature/open-feature-operator/apis/core/v1beta1/common" "github.com/open-feature/open-feature-operator/common" "github.com/open-feature/open-feature-operator/common/flagdproxy" + commontypes "github.com/open-feature/open-feature-operator/common/types" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -88,7 +89,7 @@ func TestFeatureFlagSourceReconciler_Reconcile(t *testing.T) { } else { fakeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tt.fsConfig).WithIndex(&appsv1.Deployment{}, fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), common.FeatureFlagSourceIndex).Build() } - kpConfig := flagdproxy.NewFlagdProxyConfiguration(common.EnvConfig{ + kpConfig := flagdproxy.NewFlagdProxyConfiguration(commontypes.EnvConfig{ FlagdProxyImage: "ghcr.io/open-feature/flagd-proxy", FlagdProxyTag: "v0.3.0", }) diff --git a/main.go b/main.go index be3475686..fe808c155 100644 --- a/main.go +++ b/main.go @@ -26,9 +26,9 @@ import ( "github.com/kelseyhightower/envconfig" corev1beta1 "github.com/open-feature/open-feature-operator/apis/core/v1beta1" "github.com/open-feature/open-feature-operator/common" - "github.com/open-feature/open-feature-operator/common/constant" "github.com/open-feature/open-feature-operator/common/flagdinjector" "github.com/open-feature/open-feature-operator/common/flagdproxy" + "github.com/open-feature/open-feature-operator/common/types" "github.com/open-feature/open-feature-operator/controllers/core/featureflagsource" webhooks "github.com/open-feature/open-feature-operator/webhooks" "go.uber.org/zap/zapcore" @@ -78,9 +78,9 @@ func init() { //+kubebuilder:scaffold:scheme } -//nolint:funlen,gocognit,gocyclo +//nolint:funlen,gocyclo func main() { - var env common.EnvConfig + var env types.EnvConfig if err := envconfig.Process("", &env); err != nil { log.Fatalf("Failed to process env var: %s", err) } @@ -98,6 +98,8 @@ func main() { flag.StringVar(&sidecarCpuRequest, sidecarCpuRequestFlagName, sidecarCpuRequestDefault, "sidecar CPU minimum, in cores. (500m = .5 cores)") flag.StringVar(&sidecarRamRequest, sidecarRamRequestFlagName, sidecarRamRequestDefault, "sidecar memory minimum, in bytes. (500Gi = 500GiB = 500 * 1024 * 1024 * 1024)") + flag.Parse() + level := zapcore.InfoLevel if verbose { level = zapcore.DebugLevel @@ -107,37 +109,11 @@ func main() { Level: level, } opts.BindFlags(flag.CommandLine) - flag.Parse() ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) - cpuLimitResource, err := resource.ParseQuantity(sidecarCpuLimit) + resources, err := processResources() if err != nil { - setupLog.Error(err, "parse sidecar cpu limit", sidecarCpuLimitFlagName, sidecarCpuLimit) - os.Exit(1) - } - - ramLimitResource, err := resource.ParseQuantity(sidecarRamLimit) - if err != nil { - setupLog.Error(err, "parse sidecar ram limit", sidecarRamLimitFlagName, sidecarRamLimit) - os.Exit(1) - } - - cpuRequestResource, err := resource.ParseQuantity(sidecarCpuRequest) - if err != nil { - setupLog.Error(err, "parse sidecar cpu request", sidecarCpuRequestFlagName, sidecarCpuRequest) - os.Exit(1) - } - - ramRequestResource, err := resource.ParseQuantity(sidecarRamRequest) - if err != nil { - setupLog.Error(err, "parse sidecar ram request", sidecarRamRequestFlagName, sidecarRamRequest) - os.Exit(1) - } - - if cpuRequestResource.Value() > cpuLimitResource.Value() || - ramRequestResource.Value() > ramLimitResource.Value() { - setupLog.Error(err, "sidecar resource request is higher than the resource maximum") os.Exit(1) } @@ -161,14 +137,14 @@ func main() { if err := mgr.GetFieldIndexer().IndexField( context.Background(), &corev1.Pod{}, - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPath, constant.AllowKubernetesSyncAnnotation), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.AllowKubernetesSyncAnnotation), webhooks.OpenFeatureEnabledAnnotationIndex, ); err != nil { setupLog.Error( err, "unable to create indexer", "webhook", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPath, constant.AllowKubernetesSyncAnnotation), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.AllowKubernetesSyncAnnotation), ) os.Exit(1) } @@ -183,7 +159,7 @@ func main() { err, "unable to create indexer", "webhook", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPath, common.FeatureFlagSourceAnnotation), ) os.Exit(1) } @@ -213,21 +189,12 @@ func main() { FlagdProxyConfig: kph.Config(), Env: env, FlagdInjector: &flagdinjector.FlagdContainerInjector{ - Client: mgr.GetClient(), - Logger: ctrl.Log.WithName("flagd-container injector"), - FlagdProxyConfig: kph.Config(), - FlagDResourceRequirements: corev1.ResourceRequirements{ - Limits: map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceCPU: cpuLimitResource, - corev1.ResourceMemory: ramLimitResource, - }, - Requests: map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceCPU: cpuRequestResource, - corev1.ResourceMemory: ramRequestResource, - }, - }, - Image: env.SidecarImage, - Tag: env.SidecarTag, + Client: mgr.GetClient(), + Logger: ctrl.Log.WithName("flagd-container injector"), + FlagdProxyConfig: kph.Config(), + FlagDResourceRequirements: *resources, + Image: env.SidecarImage, + Tag: env.SidecarTag, }, } hookServer.Register("/mutate-v1-pod", &webhook.Admission{Handler: podMutator}) @@ -263,3 +230,46 @@ func main() { os.Exit(1) } } + +func processResources() (*corev1.ResourceRequirements, error) { + cpuLimitResource, err := resource.ParseQuantity(sidecarCpuLimit) + if err != nil { + setupLog.Error(err, "parse sidecar cpu limit", sidecarCpuLimitFlagName, sidecarCpuLimit) + return nil, err + } + + ramLimitResource, err := resource.ParseQuantity(sidecarRamLimit) + if err != nil { + setupLog.Error(err, "parse sidecar ram limit", sidecarRamLimitFlagName, sidecarRamLimit) + return nil, err + } + + cpuRequestResource, err := resource.ParseQuantity(sidecarCpuRequest) + if err != nil { + setupLog.Error(err, "parse sidecar cpu request", sidecarCpuRequestFlagName, sidecarCpuRequest) + return nil, err + } + + ramRequestResource, err := resource.ParseQuantity(sidecarRamRequest) + if err != nil { + setupLog.Error(err, "parse sidecar ram request", sidecarRamRequestFlagName, sidecarRamRequest) + return nil, err + } + + if cpuRequestResource.Value() > cpuLimitResource.Value() || + ramRequestResource.Value() > ramLimitResource.Value() { + setupLog.Error(err, "sidecar resource request is higher than the resource maximum") + return nil, err + } + + return &corev1.ResourceRequirements{ + Limits: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: cpuLimitResource, + corev1.ResourceMemory: ramLimitResource, + }, + Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: cpuRequestResource, + corev1.ResourceMemory: ramRequestResource, + }, + }, nil +} diff --git a/webhooks/common.go b/webhooks/common.go index 9a997746e..a5ac2eef1 100644 --- a/webhooks/common.go +++ b/webhooks/common.go @@ -7,7 +7,7 @@ import ( api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" apicommon "github.com/open-feature/open-feature-operator/apis/core/v1beta1/common" "github.com/open-feature/open-feature-operator/common" - "github.com/open-feature/open-feature-operator/common/constant" + "github.com/open-feature/open-feature-operator/common/types" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -22,7 +22,7 @@ func OpenFeatureEnabledAnnotationIndex(o client.Object) []string { "false", } } - val, ok := pod.ObjectMeta.Annotations[fmt.Sprintf("openfeature.dev/%s", constant.AllowKubernetesSyncAnnotation)] + val, ok := pod.ObjectMeta.Annotations[fmt.Sprintf("openfeature.dev/%s", common.AllowKubernetesSyncAnnotation)] if ok && val == "true" { return []string{ "true", @@ -55,11 +55,11 @@ func containsK8sProvider(sources []api.Source) bool { } func checkOFEnabled(annotations map[string]string) bool { - val, ok := annotations[fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation)] + val, ok := annotations[fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation)] return ok && val == "true" } -func NewFeatureFlagSourceSpec(env common.EnvConfig) *api.FeatureFlagSourceSpec { +func NewFeatureFlagSourceSpec(env types.EnvConfig) *api.FeatureFlagSourceSpec { f := false args := strings.Split(env.SidecarProviderArgs, ",") // use empty array when arguments are not set diff --git a/webhooks/common_test.go b/webhooks/common_test.go index 6e58ce92b..d94cfbac8 100644 --- a/webhooks/common_test.go +++ b/webhooks/common_test.go @@ -8,7 +8,7 @@ import ( api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" apicommon "github.com/open-feature/open-feature-operator/apis/core/v1beta1/common" "github.com/open-feature/open-feature-operator/common" - "github.com/open-feature/open-feature-operator/common/constant" + "github.com/open-feature/open-feature-operator/common/types" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -58,11 +58,11 @@ func TestPodMutator_checkOFEnabled(t *testing.T) { }{ { name: "enabled", - annotations: map[string]string{fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true"}, + annotations: map[string]string{fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true"}, want: true, }, { name: "disabled", - annotations: map[string]string{fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "false"}, + annotations: map[string]string{fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "false"}, want: false, }, } @@ -144,7 +144,7 @@ func TestPodMutator_containsK8sProvider(t *testing.T) { } func Test_NewFeatureFlagSourceSpec(t *testing.T) { - env := common.EnvConfig{ + env := types.EnvConfig{ SidecarManagementPort: 80, SidecarPort: 88, SidecarSocketPath: "socket-path", diff --git a/webhooks/pod_webhook.go b/webhooks/pod_webhook.go index 250e36ac1..9d02bc64e 100644 --- a/webhooks/pod_webhook.go +++ b/webhooks/pod_webhook.go @@ -11,9 +11,9 @@ import ( "github.com/go-logr/logr" api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" "github.com/open-feature/open-feature-operator/common" - "github.com/open-feature/open-feature-operator/common/constant" "github.com/open-feature/open-feature-operator/common/flagdinjector" "github.com/open-feature/open-feature-operator/common/flagdproxy" + "github.com/open-feature/open-feature-operator/common/types" "github.com/open-feature/open-feature-operator/common/utils" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -37,7 +37,7 @@ type PodMutator struct { ready bool FlagdProxyConfig *flagdproxy.FlagdProxyConfiguration FlagdInjector flagdinjector.IFlagdContainerInjector - Env common.EnvConfig + Env types.EnvConfig } // Handle injects the flagd sidecar (if the prerequisites are all met) @@ -81,7 +81,7 @@ func (m *PodMutator) Handle(ctx context.Context, req admission.Request) admissio } if err := m.FlagdInjector.InjectFlagd(ctx, &pod.ObjectMeta, &pod.Spec, featureFlagSourceSpec); err != nil { - if errors.Is(err, constant.ErrFlagdProxyNotReady) { + if errors.Is(err, common.ErrFlagdProxyNotReady) { return admission.Denied(err.Error()) } //test @@ -100,7 +100,7 @@ func (m *PodMutator) Handle(ctx context.Context, req admission.Request) admissio func (m *PodMutator) createFSConfigSpec(ctx context.Context, req admission.Request, annotations map[string]string, pod *corev1.Pod) (*api.FeatureFlagSourceSpec, int32, error) { // Check configuration fscNames := []string{} - val, ok := annotations[fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation)] + val, ok := annotations[fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation)] if ok { fscNames = parseList(val) } @@ -130,7 +130,7 @@ func (m *PodMutator) BackfillPermissions(ctx context.Context) error { // fetch all pods with the fmt.Sprintf("%s/%s", OpenFeatureAnnotationPath, EnabledAnnotation) annotation set to "true" podList := &corev1.PodList{} err := m.Client.List(ctx, podList, client.MatchingFields{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPath, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.PodOpenFeatureAnnotationPath, common.AllowKubernetesSyncAnnotation): "true", }) if err != nil { if !errors.Is(err, &cache.ErrCacheNotStarted{}) { @@ -148,7 +148,7 @@ func (m *PodMutator) BackfillPermissions(ctx context.Context) error { err, fmt.Sprintf("unable backfill permissions for pod %s/%s", pod.Namespace, pod.Name), "webhook", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPath, constant.AllowKubernetesSyncAnnotation), + fmt.Sprintf("%s/%s", common.PodOpenFeatureAnnotationPath, common.AllowKubernetesSyncAnnotation), ) } } diff --git a/webhooks/pod_webhook_test.go b/webhooks/pod_webhook_test.go index ed3e2b38c..ec3413257 100644 --- a/webhooks/pod_webhook_test.go +++ b/webhooks/pod_webhook_test.go @@ -13,7 +13,7 @@ import ( "github.com/golang/mock/gomock" api "github.com/open-feature/open-feature-operator/apis/core/v1beta1" apicommon "github.com/open-feature/open-feature-operator/apis/core/v1beta1/common" - "github.com/open-feature/open-feature-operator/common/constant" + "github.com/open-feature/open-feature-operator/common" flagdinjectorfake "github.com/open-feature/open-feature-operator/common/flagdinjector/fake" "github.com/stretchr/testify/require" admissionv1 "k8s.io/api/admission/v1" @@ -68,9 +68,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: pod, Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, }, ), @@ -94,9 +94,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: pod + "-1", Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, }, &corev1.Pod{ @@ -104,9 +104,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: pod + "-2", Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, }, ), @@ -132,9 +132,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: pod, Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, Spec: corev1.PodSpec{ServiceAccountName: "my-service-account"}, }, @@ -143,9 +143,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: name, Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, }, &rbac.ClusterRoleBinding{ @@ -170,9 +170,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: pod, Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, }, &corev1.ServiceAccount{ @@ -180,9 +180,9 @@ func TestPodMutator_BackfillPermissions(t *testing.T) { Name: name, Namespace: ns, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation): "true", }}, }, &rbac.ClusterRoleBinding{ @@ -237,8 +237,8 @@ func TestPodMutator_Handle(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "myAnnotatedPod", Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), }, }, }) @@ -249,8 +249,8 @@ func TestPodMutator_Handle(t *testing.T) { Name: "myAnnotatedPod", Namespace: mutatePodNamespace, Annotations: map[string]string{ - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.EnabledAnnotation): "true", - fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.EnabledAnnotation): "true", + fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.FeatureFlagSourceAnnotation): fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagSourceName), }, OwnerReferences: []metav1.OwnerReference{{UID: "123"}}, }, @@ -379,7 +379,7 @@ func TestPodMutator_Handle(t *testing.T) { gomock.AssignableToTypeOf(&antPod.ObjectMeta), gomock.AssignableToTypeOf(&antPod.Spec), gomock.AssignableToTypeOf(&api.FeatureFlagSourceSpec{}), - ).Return(constant.ErrFlagdProxyNotReady).Times(1) + ).Return(common.ErrFlagdProxyNotReady).Times(1) }, wantCode: http.StatusForbidden, }, @@ -414,7 +414,7 @@ func TestPodMutator_Handle(t *testing.T) { }, }, &rbac.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{Name: constant.ClusterRoleBindingName}, + ObjectMeta: metav1.ObjectMeta{Name: common.ClusterRoleBindingName}, Subjects: nil, RoleRef: rbac.RoleRef{}, }, @@ -498,7 +498,7 @@ func NewClient(withIndexes bool, objs ...client.Object) client.Client { utilruntime.Must(api.AddToScheme(scheme.Scheme)) annotationsSyncIndexer := func(obj client.Object) []string { - res := obj.GetAnnotations()[fmt.Sprintf("%s/%s", constant.OpenFeatureAnnotationPrefix, constant.AllowKubernetesSyncAnnotation)] + res := obj.GetAnnotations()[fmt.Sprintf("%s/%s", common.OpenFeatureAnnotationPrefix, common.AllowKubernetesSyncAnnotation)] return []string{res} } From 3b904aab1bb1a2c62f2d65177ab5562e3926a728 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 20 Nov 2023 13:43:04 +0100 Subject: [PATCH 2/8] pr review Signed-off-by: odubajDT --- common/flagdinjector/flagdinjector.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/common/flagdinjector/flagdinjector.go b/common/flagdinjector/flagdinjector.go index 0d67a3bbb..680af8096 100644 --- a/common/flagdinjector/flagdinjector.go +++ b/common/flagdinjector/flagdinjector.go @@ -193,7 +193,7 @@ func (fi *FlagdContainerInjector) buildSources(ctx context.Context, objectMeta * source.Provider = flagSourceConfig.DefaultSyncProvider } - sourceCfg, err := fi.createSourceConfig(ctx, source, objectMeta, podSpec, sidecar) + sourceCfg, err := fi.newSourceConfig(ctx, source, objectMeta, podSpec, sidecar) if err != nil { return []types.SourceConfig{}, err } @@ -205,35 +205,26 @@ func (fi *FlagdContainerInjector) buildSources(ctx context.Context, objectMeta * return sourceCfgCollection, nil } -func (fi *FlagdContainerInjector) createSourceConfig(ctx context.Context, source api.Source, objectMeta *metav1.ObjectMeta, podSpec *corev1.PodSpec, sidecar *corev1.Container) (*types.SourceConfig, error) { - var sourceCfg types.SourceConfig - var err error +func (fi *FlagdContainerInjector) newSourceConfig(ctx context.Context, source api.Source, objectMeta *metav1.ObjectMeta, podSpec *corev1.PodSpec, sidecar *corev1.Container) (*types.SourceConfig, error) { + sourceCfg := types.SourceConfig{} + var err error = nil switch { case source.Provider.IsKubernetes(): sourceCfg, err = fi.toKubernetesProviderConfig(ctx, objectMeta, podSpec, source) - if err != nil { - return nil, err - } case source.Provider.IsFilepath(): sourceCfg, err = fi.toFilepathProviderConfig(ctx, objectMeta, podSpec, sidecar, source) - if err != nil { - return nil, err - } case source.Provider.IsHttp(): sourceCfg = fi.toHttpProviderConfig(source) case source.Provider.IsGrpc(): sourceCfg = fi.toGrpcProviderConfig(source) case source.Provider.IsFlagdProxy(): sourceCfg, err = fi.toFlagdProxyConfig(ctx, objectMeta, source) - if err != nil { - return nil, err - } default: - return nil, fmt.Errorf("could not add provider %s: %w", source.Provider, common.ErrUnrecognizedSyncProvider) + err = fmt.Errorf("could not add provider %s: %w", source.Provider, common.ErrUnrecognizedSyncProvider) } - return &sourceCfg, nil + return &sourceCfg, err } func (fi *FlagdContainerInjector) toFilepathProviderConfig(ctx context.Context, objectMeta *metav1.ObjectMeta, podSpec *corev1.PodSpec, sidecar *corev1.Container, source api.Source) (types.SourceConfig, error) { From 13ca04b50c0714b0666dfce1cdd188183bb8b145 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 20 Nov 2023 15:27:19 +0100 Subject: [PATCH 3/8] add flagdproxy tests Signed-off-by: odubajDT --- common/flagdproxy/flagdproxy_test.go | 171 ++++++++++++++++++++++++++- 1 file changed, 167 insertions(+), 4 deletions(-) diff --git a/common/flagdproxy/flagdproxy_test.go b/common/flagdproxy/flagdproxy_test.go index f8278697d..ae1d6a88d 100644 --- a/common/flagdproxy/flagdproxy_test.go +++ b/common/flagdproxy/flagdproxy_test.go @@ -2,14 +2,18 @@ package flagdproxy import ( "context" + "fmt" "testing" "github.com/go-logr/logr/testr" "github.com/open-feature/open-feature-operator/common/types" "github.com/stretchr/testify/require" + appsV1 "k8s.io/api/apps/v1" v1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" v12 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -67,6 +71,51 @@ func TestNewFlagdProxyHandler(t *testing.T) { require.Equal(t, kpConfig, ph.Config()) } +func TestDoesFlagdProxyExist(t *testing.T) { + env := types.EnvConfig{ + PodNamespace: "ns", + } + + deployment := &v1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: FlagdProxyDeploymentName, + }, + Spec: v1.DeploymentSpec{ + Template: v12.PodTemplateSpec{ + Spec: v12.PodSpec{ + Containers: []v12.Container{ + { + Name: "my-container", + }, + }, + }, + }, + }, + } + + kpConfig := NewFlagdProxyConfiguration(env) + + require.NotNil(t, kpConfig) + + fakeClient := fake.NewClientBuilder().WithObjects().Build() + + ph := NewFlagdProxyHandler(kpConfig, fakeClient, testr.New(t)) + + require.NotNil(t, ph) + + res, err := ph.doesFlagdProxyExist(context.TODO()) + require.Nil(t, err) + require.False(t, res) + + err = fakeClient.Create(context.TODO(), deployment) + require.Nil(t, err) + + res, err = ph.doesFlagdProxyExist(context.TODO()) + require.Nil(t, err) + require.True(t, res) +} + func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExists(t *testing.T) { env := types.EnvConfig{ PodNamespace: "ns", @@ -116,7 +165,12 @@ func TestFlagdProxyHandler_HandleFlagdProxy_ProxyExists(t *testing.T) { func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { env := types.EnvConfig{ - PodNamespace: "ns", + PodNamespace: "ns", + FlagdProxyImage: "image", + FlagdProxyTag: "tag", + FlagdProxyPort: 88, + FlagdProxyManagementPort: 90, + FlagdProxyDebugLogging: true, } kpConfig := NewFlagdProxyConfiguration(env) @@ -128,11 +182,21 @@ func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { require.NotNil(t, ph) - err := ph.HandleFlagdProxy(context.Background()) + // proxy does not exist + deployment := &v1.Deployment{} + err := fakeClient.Get(context.Background(), client.ObjectKey{ + Namespace: env.PodNamespace, + Name: FlagdProxyDeploymentName, + }, deployment) + + require.NotNil(t, err) + + err = ph.HandleFlagdProxy(context.Background()) require.Nil(t, err) - deployment := &v1.Deployment{} + // proxy should exist + deployment = &v1.Deployment{} err = fakeClient.Get(context.Background(), client.ObjectKey{ Namespace: env.PodNamespace, Name: FlagdProxyDeploymentName, @@ -141,5 +205,104 @@ func TestFlagdProxyHandler_HandleFlagdProxy_CreateProxy(t *testing.T) { require.Nil(t, err) require.NotNil(t, deployment) - require.Equal(t, FlagdProxyDeploymentName, deployment.Spec.Template.Spec.Containers[0].Name) + replicas := int32(1) + args := []string{ + "start", + "--metrics-port", + fmt.Sprintf("%d", 90), + "--debug", + } + + expectedDeployment := &appsV1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: FlagdProxyDeploymentName, + Namespace: "ns", + Labels: map[string]string{ + "app": FlagdProxyDeploymentName, + "app.kubernetes.io/managed-by": ManagedByAnnotationValue, + "app.kubernetes.io/version": "tag", + }, + ResourceVersion: "1", + }, + Spec: appsV1.DeploymentSpec{ + Replicas: &replicas, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": FlagdProxyDeploymentName, + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "app": FlagdProxyDeploymentName, + "app.kubernetes.io/name": FlagdProxyDeploymentName, + "app.kubernetes.io/managed-by": ManagedByAnnotationValue, + "app.kubernetes.io/version": "tag", + }, + }, + Spec: corev1.PodSpec{ + ServiceAccountName: FlagdProxyServiceAccountName, + Containers: []corev1.Container{ + { + Image: "image:tag", + Name: FlagdProxyDeploymentName, + Ports: []corev1.ContainerPort{ + { + Name: "port", + ContainerPort: int32(88), + }, + { + Name: "metrics-port", + ContainerPort: int32(90), + }, + }, + Args: args, + }, + }, + }, + }, + }, + } + + require.Equal(t, expectedDeployment, deployment) + + service := &corev1.Service{} + err = fakeClient.Get(context.Background(), client.ObjectKey{ + Namespace: env.PodNamespace, + Name: FlagdProxyServiceName, + }, service) + + require.Nil(t, err) + require.NotNil(t, service) + + expectedService := &corev1.Service{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Service", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: FlagdProxyServiceName, + Namespace: "ns", + ResourceVersion: "1", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "app.kubernetes.io/name": FlagdProxyDeploymentName, + "app.kubernetes.io/managed-by": ManagedByAnnotationValue, + }, + Ports: []corev1.ServicePort{ + { + Name: "flagd-proxy", + Port: int32(88), + TargetPort: intstr.FromInt(88), + }, + }, + }, + } + + require.Equal(t, expectedService, service) } From c3b92b3a40b4754adc8d94b8d5ba618cc5c3c9f1 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Tue, 21 Nov 2023 07:26:06 +0100 Subject: [PATCH 4/8] polish variables names Signed-off-by: odubajDT --- common/common.go | 2 +- common/flagdinjector/flagdinjector.go | 6 ++-- common/flagdinjector/flagdinjector_test.go | 38 +++++++++++----------- main.go | 2 +- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/common/common.go b/common/common.go index 16521428e..42ec9d462 100644 --- a/common/common.go +++ b/common/common.go @@ -19,7 +19,7 @@ const ( FinalizerName = "featureflag.core.openfeature.dev/finalizer" OpenFeatureAnnotationPath = "spec.template.metadata.annotations.openfeature.dev/openfeature.dev" OpenFeatureAnnotationRoot = "openfeature.dev" - FlagDImagePullPolicy corev1.PullPolicy = "Always" + FlagdImagePullPolicy corev1.PullPolicy = "Always" ClusterRoleBindingName string = "open-feature-operator-flagd-kubernetes-sync" AllowKubernetesSyncAnnotation = "allowkubernetessync" OpenFeatureAnnotationPrefix = "openfeature.dev" diff --git a/common/flagdinjector/flagdinjector.go b/common/flagdinjector/flagdinjector.go index 680af8096..089966b74 100644 --- a/common/flagdinjector/flagdinjector.go +++ b/common/flagdinjector/flagdinjector.go @@ -45,7 +45,7 @@ type FlagdContainerInjector struct { Client client.Client Logger logr.Logger FlagdProxyConfig *flagdproxy.FlagdProxyConfiguration - FlagDResourceRequirements corev1.ResourceRequirements + FlagdResourceRequirements corev1.ResourceRequirements Image string Tag string } @@ -379,7 +379,7 @@ func (fi *FlagdContainerInjector) generateBasicFlagdContainer(flagSourceConfig * Args: []string{ "start", }, - ImagePullPolicy: common.FlagDImagePullPolicy, + ImagePullPolicy: common.FlagdImagePullPolicy, VolumeMounts: []corev1.VolumeMount{}, Env: []corev1.EnvVar{}, Ports: []corev1.ContainerPort{ @@ -389,7 +389,7 @@ func (fi *FlagdContainerInjector) generateBasicFlagdContainer(flagSourceConfig * }, }, SecurityContext: getSecurityContext(), - Resources: fi.FlagDResourceRequirements, + Resources: fi.FlagdResourceRequirements, } } diff --git a/common/flagdinjector/flagdinjector_test.go b/common/flagdinjector/flagdinjector_test.go index 4f9200e28..e3752c46a 100644 --- a/common/flagdinjector/flagdinjector_test.go +++ b/common/flagdinjector/flagdinjector_test.go @@ -39,7 +39,7 @@ func TestFlagdContainerInjector_InjectDefaultSyncProvider(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -78,7 +78,7 @@ func TestFlagdContainerInjector_InjectDefaultSyncProvider_WithDebugLogging(t *te Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -119,7 +119,7 @@ func TestFlagdContainerInjector_InjectDefaultSyncProvider_WithOtelCollectorUri(t Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -160,7 +160,7 @@ func TestFlagdContainerInjector_InjectDefaultSyncProvider_WithResources(t *testi Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -211,7 +211,7 @@ func TestFlagdContainerInjector_InjectDefaultSyncProvider_WithSyncProviderArgs(t Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -252,7 +252,7 @@ func TestFlagdContainerInjector_InjectFlagdKubernetesSource(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -305,7 +305,7 @@ func TestFlagdContainerInjector_InjectFlagdFilePathSource(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -383,7 +383,7 @@ func TestFlagdContainerInjector_InjectFlagdFilePathSource_UpdateReferencedConfig Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -464,7 +464,7 @@ func TestFlagdContainerInjector_InjectHttpSource(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -508,7 +508,7 @@ func TestFlagdContainerInjector_InjectGrpcSource(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -555,7 +555,7 @@ func TestFlagdContainerInjector_InjectProxySource_ProxyNotAvailable(t *testing.T Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -598,7 +598,7 @@ func TestFlagdContainerInjector_InjectProxySource_ProxyNotReady(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -644,7 +644,7 @@ func TestFlagdContainerInjector_InjectProxySource_ProxyIsReady(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -685,7 +685,7 @@ func TestFlagdContainerInjector_Inject_FlagdContainerAlreadyPresent(t *testing.T Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -729,7 +729,7 @@ func TestFlagdContainerInjector_InjectUnknownSyncProvider(t *testing.T) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -1069,7 +1069,7 @@ func enableClusterRoleBindingTest(t *testing.T, name string, input string) { Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -1121,7 +1121,7 @@ func TestFlagdContainerInjector_EnableClusterRoleBinding_ServiceAccountAlreadyIn Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -1157,7 +1157,7 @@ func TestFlagdContainerInjector_EnableClusterRoleBinding_ClusterRoleBindingNotFo Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), Image: testImage, Tag: testTag, } @@ -1174,7 +1174,7 @@ func TestFlagdContainerInjector_EnableClusterRoleBinding_ServiceAccountNotFound( Client: fakeClient, Logger: testr.New(t), FlagdProxyConfig: getProxyConfig(), - FlagDResourceRequirements: getResourceRequirements(), + FlagdResourceRequirements: getResourceRequirements(), } err := fi.EnableClusterRoleBinding(context.Background(), namespace, "my-serviceaccount") diff --git a/main.go b/main.go index fe808c155..07b8988ea 100644 --- a/main.go +++ b/main.go @@ -192,7 +192,7 @@ func main() { Client: mgr.GetClient(), Logger: ctrl.Log.WithName("flagd-container injector"), FlagdProxyConfig: kph.Config(), - FlagDResourceRequirements: *resources, + FlagdResourceRequirements: *resources, Image: env.SidecarImage, Tag: env.SidecarTag, }, From f58ee3e65fec94b7aea97937d58d5365a2372c97 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Tue, 21 Nov 2023 15:14:41 +0100 Subject: [PATCH 5/8] refactor flagdinjector Signed-off-by: odubajDT --- common/flagdinjector/flagdinjector.go | 53 +++++++++++++++++---------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/common/flagdinjector/flagdinjector.go b/common/flagdinjector/flagdinjector.go index 089966b74..a87ce9c74 100644 --- a/common/flagdinjector/flagdinjector.go +++ b/common/flagdinjector/flagdinjector.go @@ -127,12 +127,10 @@ func (fi *FlagdContainerInjector) InjectFlagd( // service account under the given namespace (required for kubernetes sync provider) func (fi *FlagdContainerInjector) EnableClusterRoleBinding(ctx context.Context, namespace, serviceAccountName string) error { serviceAccount := client.ObjectKey{ - Name: serviceAccountName, + Name: determineServiceAccountName(serviceAccountName), Namespace: namespace, } - if serviceAccountName == "" { - serviceAccount.Name = "default" - } + // Check if the service account exists fi.Logger.V(1).Info(fmt.Sprintf("Fetching serviceAccount: %s/%s", serviceAccount.Namespace, serviceAccount.Name)) sa := corev1.ServiceAccount{} @@ -140,6 +138,7 @@ func (fi *FlagdContainerInjector) EnableClusterRoleBinding(ctx context.Context, fi.Logger.V(1).Info(fmt.Sprintf("ServiceAccount not found: %s/%s", serviceAccount.Namespace, serviceAccount.Name)) return err } + fi.Logger.V(1).Info(fmt.Sprintf("Fetching clusterrolebinding: %s", common.ClusterRoleBindingName)) // Fetch service account if it exists crb := rbacv1.ClusterRoleBinding{} @@ -147,28 +146,44 @@ func (fi *FlagdContainerInjector) EnableClusterRoleBinding(ctx context.Context, fi.Logger.V(1).Info(fmt.Sprintf("ClusterRoleBinding not found: %s", common.ClusterRoleBindingName)) return err } - found := false + + if !fi.isServiceAccountSet(&crb, serviceAccount) { + return fi.updateServiceAccount(ctx, &crb, serviceAccount) + } + + return nil +} + +func determineServiceAccountName(name string) string { + if name == "" { + return "default" + } + return name +} + +func (fi *FlagdContainerInjector) isServiceAccountSet(crb *rbacv1.ClusterRoleBinding, serviceAccount client.ObjectKey) bool { for _, subject := range crb.Subjects { if subject.Kind == "ServiceAccount" && subject.Name == serviceAccount.Name && subject.Namespace == serviceAccount.Namespace { fi.Logger.V(1).Info(fmt.Sprintf("ClusterRoleBinding already exists for service account: %s/%s", serviceAccount.Namespace, serviceAccount.Name)) - found = true + return true } } - if !found { - fi.Logger.V(1).Info(fmt.Sprintf("Updating ClusterRoleBinding %s for service account: %s/%s", crb.Name, - serviceAccount.Namespace, serviceAccount.Name)) - crb.Subjects = append(crb.Subjects, rbacv1.Subject{ - Kind: "ServiceAccount", - Name: serviceAccount.Name, - Namespace: serviceAccount.Namespace, - }) - if err := fi.Client.Update(ctx, &crb); err != nil { - fi.Logger.V(1).Info(fmt.Sprintf("Failed to update ClusterRoleBinding: %s", err.Error())) - return err - } + return false +} + +func (fi *FlagdContainerInjector) updateServiceAccount(ctx context.Context, crb *rbacv1.ClusterRoleBinding, serviceAccount client.ObjectKey) error { + fi.Logger.V(1).Info(fmt.Sprintf("Updating ClusterRoleBinding %s for service account: %s/%s", crb.Name, + serviceAccount.Namespace, serviceAccount.Name)) + crb.Subjects = append(crb.Subjects, rbacv1.Subject{ + Kind: "ServiceAccount", + Name: serviceAccount.Name, + Namespace: serviceAccount.Namespace, + }) + if err := fi.Client.Update(ctx, crb); err != nil { + fi.Logger.V(1).Info(fmt.Sprintf("Failed to update ClusterRoleBinding: %s", err.Error())) + return err } fi.Logger.V(1).Info(fmt.Sprintf("Updated ClusterRoleBinding: %s", crb.Name)) - return nil } From b0392702d36c5bcf604deeb3c80f58eafce2d057 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Tue, 21 Nov 2023 15:38:58 +0100 Subject: [PATCH 6/8] polish controllers Signed-off-by: odubajDT --- controllers/core/featureflagsource/controller.go | 3 +++ controllers/core/featureflagsource/controller_test.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/controllers/core/featureflagsource/controller.go b/controllers/core/featureflagsource/controller.go index 469c86836..a8e2b8d37 100644 --- a/controllers/core/featureflagsource/controller.go +++ b/controllers/core/featureflagsource/controller.go @@ -44,6 +44,9 @@ type FeatureFlagSourceReconciler struct { FlagdProxy *flagdproxy.FlagdProxyHandler } +// renovate: datasource=github-tags depName=open-feature/flagd/flagd-proxy +const flagdProxyTag = "v0.3.0" + //+kubebuilder:rbac:groups=core.openfeature.dev,resources=featureflagsources,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=core.openfeature.dev,resources=featureflagsources/status,verbs=get;update;patch //+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete diff --git a/controllers/core/featureflagsource/controller_test.go b/controllers/core/featureflagsource/controller_test.go index 06c4b50c2..e65c5bc7d 100644 --- a/controllers/core/featureflagsource/controller_test.go +++ b/controllers/core/featureflagsource/controller_test.go @@ -91,7 +91,7 @@ func TestFeatureFlagSourceReconciler_Reconcile(t *testing.T) { } kpConfig := flagdproxy.NewFlagdProxyConfiguration(commontypes.EnvConfig{ FlagdProxyImage: "ghcr.io/open-feature/flagd-proxy", - FlagdProxyTag: "v0.3.0", + FlagdProxyTag: flagdProxyTag, }) kpConfig.Namespace = testNamespace From 4c622c1b305a23214c6d94c13586bd18260a5661 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Tue, 21 Nov 2023 15:55:08 +0100 Subject: [PATCH 7/8] fix Signed-off-by: odubajDT --- controllers/core/featureflagsource/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/core/featureflagsource/controller_test.go b/controllers/core/featureflagsource/controller_test.go index e65c5bc7d..baba1f4c4 100644 --- a/controllers/core/featureflagsource/controller_test.go +++ b/controllers/core/featureflagsource/controller_test.go @@ -138,7 +138,7 @@ func TestFeatureFlagSourceReconciler_Reconcile(t *testing.T) { require.Nil(t, err) require.Equal(t, len(deployment.Spec.Template.Spec.Containers), 1) require.Equal(t, len(deployment.Spec.Template.Spec.Containers[0].Ports), 2) - require.Equal(t, deployment.Spec.Template.Spec.Containers[0].Image, "ghcr.io/open-feature/flagd-proxy:v0.3.0") + require.Equal(t, deployment.Spec.Template.Spec.Containers[0].Image, "ghcr.io/open-feature/flagd-proxy:"+flagdProxyTag) service := &corev1.Service{} err = fakeClient.Get(ctx, types.NamespacedName{Name: flagdproxy.FlagdProxyServiceName, Namespace: testNamespace}, service) From 09ae2ee3a812edc146d4aec5dca698d7eb43f9e1 Mon Sep 17 00:00:00 2001 From: odubajDT <93584209+odubajDT@users.noreply.github.com> Date: Wed, 22 Nov 2023 07:56:29 +0100 Subject: [PATCH 8/8] Update common/types/envconfig.go Co-authored-by: Florian Bacher Signed-off-by: odubajDT <93584209+odubajDT@users.noreply.github.com> --- common/types/envconfig.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/types/envconfig.go b/common/types/envconfig.go index 0d1b4624e..4cd2495af 100644 --- a/common/types/envconfig.go +++ b/common/types/envconfig.go @@ -13,7 +13,7 @@ type EnvConfig struct { SidecarManagementPort int `envconfig:"SIDECAR_MANAGEMENT_PORT" default:"8014"` SidecarPort int `envconfig:"SIDECAR_PORT" default:"8013"` SidecarImage string `envconfig:"SIDECAR_IMAGE" default:"ghcr.io/open-feature/flagd"` - // renovate: datasource=github-tags depName=open-feature/flagd/flagd-proxy + // renovate: datasource=github-tags depName=open-feature/flagd/flagd SidecarTag string `envconfig:"SIDECAR_TAG" default:"v0.7.0"` SidecarSocketPath string `envconfig:"SIDECAR_SOCKET_PATH" default:""` SidecarEvaluator string `envconfig:"SIDECAR_EVALUATOR" default:"json"`