From b5e7fbed8cffeefea48e00284dbaae72ecf91d80 Mon Sep 17 00:00:00 2001
From: Amine <hilalyamine@gmail.com>
Date: Wed, 9 Oct 2024 11:54:32 -0700
Subject: [PATCH] Improve feature gate handling in config and runtime (#162)

Improved the handling of feature gates in the `config` and `runtime`
packages by updating the validation logic to properly handle errors
when overriding feature gates.

This patch also renams the `ReadOnly` featuregate to
`ReadOnlyResources` to better describe its purpose.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
---
 pkg/config/config.go             |  5 ++++-
 pkg/featuregate/features.go      | 20 ++++++++++++--------
 pkg/featuregate/features_test.go |  6 ++++--
 pkg/runtime/reconciler.go        |  4 ++--
 4 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/pkg/config/config.go b/pkg/config/config.go
index ea82484..5438f46 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -340,7 +340,10 @@ func (cfg *Config) Validate(options ...Option) error {
 	if err != nil {
 		return fmt.Errorf("invalid value for flag '%s': %v", flagFeatureGates, err)
 	}
-	cfg.FeatureGates = featuregate.GetFeatureGatesWithOverrides(featureGatesMap)
+	cfg.FeatureGates, err = featuregate.GetFeatureGatesWithOverrides(featureGatesMap)
+	if err != nil {
+		return fmt.Errorf("error overriding feature gates: %v", err)
+	}
 
 	return nil
 }
diff --git a/pkg/featuregate/features.go b/pkg/featuregate/features.go
index 20b3c9b..79ce3fd 100644
--- a/pkg/featuregate/features.go
+++ b/pkg/featuregate/features.go
@@ -16,9 +16,12 @@
 // optionally overridden.
 package featuregate
 
+import "fmt"
+
 const (
-	ReadOnly = "ReadOnly"
-  
+	// ReadOnlyResources is a feature gate for enabling ReadOnly resources annotation.
+	ReadOnlyResources = "ReadOnlyResources"
+
 	// TeamLevelCARM is a feature gate for enabling CARM for team-level resources.
 	TeamLevelCARM = "TeamLevelCARM"
 
@@ -29,10 +32,9 @@ const (
 // defaultACKFeatureGates is a map of feature names to Feature structs
 // representing the default feature gates for ACK controllers.
 var defaultACKFeatureGates = FeatureGates{
-	// Set feature gates here
-	ReadOnly:         {Stage: Alpha, Enabled: false},
-	TeamLevelCARM:    {Stage: Alpha, Enabled: false},
-	ServiceLevelCARM: {Stage: Alpha, Enabled: false},
+	ReadOnlyResources: {Stage: Alpha, Enabled: false},
+	TeamLevelCARM:     {Stage: Alpha, Enabled: false},
+	ServiceLevelCARM:  {Stage: Alpha, Enabled: false},
 }
 
 // FeatureStage represents the development stage of a feature.
@@ -102,13 +104,15 @@ func GetDefaultFeatureGates() FeatureGates {
 
 // GetFeatureGatesWithOverrides returns a new FeatureGates instance with the default features,
 // but with the provided overrides applied. This allows for runtime configuration of feature gates.
-func GetFeatureGatesWithOverrides(featureGateOverrides map[string]bool) FeatureGates {
+func GetFeatureGatesWithOverrides(featureGateOverrides map[string]bool) (FeatureGates, error) {
 	gates := GetDefaultFeatureGates()
 	for name, enabled := range featureGateOverrides {
 		if feature, ok := gates[name]; ok {
 			feature.Enabled = enabled
 			gates[name] = feature
+		} else {
+			return nil, fmt.Errorf("unknown feature gate: %v", name)
 		}
 	}
-	return gates
+	return gates, nil
 }
diff --git a/pkg/featuregate/features_test.go b/pkg/featuregate/features_test.go
index 818cc25..3156bef 100644
--- a/pkg/featuregate/features_test.go
+++ b/pkg/featuregate/features_test.go
@@ -15,6 +15,8 @@ package featuregate
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestIsEnabled(t *testing.T) {
@@ -108,10 +110,10 @@ func TestGetFeatureGatesWithOverrides(t *testing.T) {
 	overrides := map[string]bool{
 		"feature1": true,
 		"feature2": false,
-		"feature4": true, // This should be ignored as it's not in defaultFeatureGates
 	}
 
-	gates := GetFeatureGatesWithOverrides(overrides)
+	gates, err := GetFeatureGatesWithOverrides(overrides)
+	require.Nil(t, err)
 
 	tests := []struct {
 		name     string
diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go
index 03b84a4..6a4f43a 100644
--- a/pkg/runtime/reconciler.go
+++ b/pkg/runtime/reconciler.go
@@ -313,7 +313,7 @@ func (r *resourceReconciler) reconcile(
 		if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete &&
 			// If the ReadOnly feature gate is enabled, and the resource is read-only,
 			// we don't delete the resource.
-			!(r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnly) && IsReadOnly(res)) {
+			!(r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) && IsReadOnly(res)) {
 			// Resolve references before deleting the resource.
 			// Ignore any errors while resolving the references
 			resolved, _, _ := rm.ResolveReferences(ctx, r.apiReader, res)
@@ -360,7 +360,7 @@ func (r *resourceReconciler) Sync(
 	isAdopted := IsAdopted(desired)
 	rlog.WithValues("is_adopted", isAdopted)
 
-	if r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnly) {
+	if r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) {
 		isReadOnly := IsReadOnly(desired)
 		rlog.WithValues("is_read_only", isReadOnly)