Remove runtime resolution of configMap settings

pkg/apis/apis.go

@@ -24,10 +24,9 @@ import (
 
 	"github.com/samber/lo"
 
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/utils/functional"
-	"github.com/aws/karpenter-core/pkg/utils/sets"
 )
 
 var (
@@ -41,7 +40,7 @@ var (
 	Resources = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
 		v1alpha5.SchemeGroupVersion.WithKind("Provisioner"): &v1alpha5.Provisioner{},
 	}
-	Settings = sets.New(settings.Registration)
+	Settings = []settings.Injectable{&settings.Settings{}}
 )
 
 //go:generate controller-gen crd object:headerFile="../../hack/boilerplate.go.txt" paths="./..." output:crd:artifacts:config=crds

pkg/apis/settings/injectable.go

@@ -0,0 +1,13 @@
+package settings
+
+import (
+	"context"
+
+	v1 "k8s.io/api/core/v1"
+)
+
+// Injectable defines a ConfigMap registration to be loaded into context on startup
+type Injectable interface {
+	ConfigMap() string
+	Inject(context.Context, *v1.ConfigMap) (context.Context, error)
+}

pkg/apis/config/settings/settings.go → pkg/apis/settings/settings.go

@@ -24,18 +24,13 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/configmap"
-
-	"github.com/aws/karpenter-core/pkg/apis/config"
 )
 
-var ContextKey = Registration
+type settingsKeyType struct{}
 
-var Registration = &config.Registration{
-	ConfigMapName: "karpenter-global-settings",
-	Constructor:   NewSettingsFromConfigMap,
-}
+var ContextKey = settingsKeyType{}
 
-var defaultSettings = Settings{
+var defaultSettings = &Settings{
 	BatchMaxDuration:  metav1.Duration{Duration: time.Second * 10},
 	BatchIdleDuration: metav1.Duration{Duration: time.Second * 1},
 	DriftEnabled:      false,
@@ -48,23 +43,25 @@ type Settings struct {
 	DriftEnabled bool
 }
 
-// NewSettingsFromConfigMap creates a Settings from the supplied ConfigMap
-func NewSettingsFromConfigMap(cm *v1.ConfigMap) (Settings, error) {
+func (*Settings) ConfigMap() string {
+	return "karpenter-global-settings"
+}
+
+// Inject creates a Settings from the supplied ConfigMap
+func (*Settings) Inject(ctx context.Context, cm *v1.ConfigMap) (context.Context, error) {
 	s := defaultSettings
 
 	if err := configmap.Parse(cm.Data,
 		AsMetaDuration("batchMaxDuration", &s.BatchMaxDuration),
 		AsMetaDuration("batchIdleDuration", &s.BatchIdleDuration),
 		configmap.AsBool("featureGates.driftEnabled", &s.DriftEnabled),
 	); err != nil {
-		// Failing to parse means that there is some error in the Settings, so we should crash
-		panic(fmt.Sprintf("parsing settings, %v", err))
+		return ctx, fmt.Errorf("parsing settings, %w", err)
 	}
 	if err := s.Validate(); err != nil {
-		// Failing to validate means that there is some error in the Settings, so we should crash
-		panic(fmt.Sprintf("validating settings, %v", err))
+		return ctx, fmt.Errorf("validating settings, %w", err)
 	}
-	return s, nil
+	return ToContext(ctx, s), nil
 }
 
 // Validate leverages struct tags with go-playground/validator so you can define a struct with custom
@@ -73,7 +70,7 @@ func NewSettingsFromConfigMap(cm *v1.ConfigMap) (Settings, error) {
 //	type ExampleStruct struct {
 //	    Example  metav1.Duration `json:"example" validate:"required,min=10m"`
 //	}
-func (s Settings) Validate() (err error) {
+func (s *Settings) Validate() (err error) {
 	validate := validator.New()
 	if s.BatchMaxDuration.Duration <= 0 {
 		err = multierr.Append(err, fmt.Errorf("batchMaxDuration cannot be negative"))
@@ -98,15 +95,15 @@ func AsMetaDuration(key string, target *metav1.Duration) configmap.ParseFunc {
 	}
 }
 
-func ToContext(ctx context.Context, s Settings) context.Context {
+func ToContext(ctx context.Context, s *Settings) context.Context {
 	return context.WithValue(ctx, ContextKey, s)
 }
 
-func FromContext(ctx context.Context) Settings {
+func FromContext(ctx context.Context) *Settings {
 	data := ctx.Value(ContextKey)
 	if data == nil {
 		// This is developer error if this happens, so we should panic
 		panic("settings doesn't exist in context")
 	}
-	return data.(Settings)
+	return data.(*Settings)
 }

pkg/apis/config/settings/suite_test.go → pkg/apis/settings/suite_test.go

@@ -24,9 +24,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	. "knative.dev/pkg/logging/testing"
 
-	. "github.com/aws/karpenter-core/pkg/test/expectations"
-
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 )
 
 var ctx context.Context
@@ -42,7 +40,9 @@ var _ = Describe("Validation", func() {
 		cm := &v1.ConfigMap{
 			Data: map[string]string{},
 		}
-		s, _ := settings.NewSettingsFromConfigMap(cm)
+		ctx, err := (&settings.Settings{}).Inject(ctx, cm)
+		Expect(err).ToNot(HaveOccurred())
+		s := settings.FromContext(ctx)
 		Expect(s.BatchMaxDuration.Duration).To(Equal(time.Second * 10))
 		Expect(s.BatchIdleDuration.Duration).To(Equal(time.Second))
 		Expect(s.DriftEnabled).To(BeFalse())
@@ -55,36 +55,38 @@ var _ = Describe("Validation", func() {
 				"featureGates.driftEnabled": "true",
 			},
 		}
-		s, _ := settings.NewSettingsFromConfigMap(cm)
+		ctx, err := (&settings.Settings{}).Inject(ctx, cm)
+		Expect(err).ToNot(HaveOccurred())
+		s := settings.FromContext(ctx)
 		Expect(s.BatchMaxDuration.Duration).To(Equal(time.Second * 30))
 		Expect(s.BatchIdleDuration.Duration).To(Equal(time.Second * 5))
 		Expect(s.DriftEnabled).To(BeTrue())
 	})
 	It("should fail validation with panic when batchMaxDuration is negative", func() {
-		defer ExpectPanic()
 		cm := &v1.ConfigMap{
 			Data: map[string]string{
 				"batchMaxDuration": "-10s",
 			},
 		}
-		_, _ = settings.NewSettingsFromConfigMap(cm)
+		_, err := (&settings.Settings{}).Inject(ctx, cm)
+		Expect(err).To(HaveOccurred())
 	})
 	It("should fail validation with panic when batchIdleDuration is negative", func() {
-		defer ExpectPanic()
 		cm := &v1.ConfigMap{
 			Data: map[string]string{
 				"batchIdleDuration": "-1s",
 			},
 		}
-		_, _ = settings.NewSettingsFromConfigMap(cm)
+		_, err := (&settings.Settings{}).Inject(ctx, cm)
+		Expect(err).To(HaveOccurred())
 	})
 	It("should fail validation with panic when driftEnabled is not a valid boolean value", func() {
-		defer ExpectPanic()
 		cm := &v1.ConfigMap{
 			Data: map[string]string{
 				"featureGates.driftEnabled": "foobar",
 			},
 		}
-		_, _ = settings.NewSettingsFromConfigMap(cm)
+		_, err := (&settings.Settings{}).Inject(ctx, cm)
+		Expect(err).To(HaveOccurred())
 	})
 })

pkg/controllers/controllers.go

@@ -36,7 +36,6 @@ import (
 	"github.com/aws/karpenter-core/pkg/events"
 	"github.com/aws/karpenter-core/pkg/metrics"
 	"github.com/aws/karpenter-core/pkg/operator/controller"
-	"github.com/aws/karpenter-core/pkg/operator/settingsstore"
 )
 
 func init() {
@@ -50,7 +49,6 @@ func NewControllers(
 	kubernetesInterface kubernetes.Interface,
 	cluster *state.Cluster,
 	eventRecorder events.Recorder,
-	settingsStore settingsstore.Store,
 	cloudProvider cloudprovider.CloudProvider,
 ) []controller.Controller {
 	provisioner := provisioning.NewProvisioner(ctx, kubeClient, kubernetesInterface.CoreV1(), eventRecorder, cloudProvider, cluster)

pkg/controllers/deprovisioning/drift.go

@@ -23,7 +23,7 @@ import (
 	"knative.dev/pkg/logging"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/controllers/provisioning"
 	"github.com/aws/karpenter-core/pkg/controllers/state"

pkg/controllers/deprovisioning/suite_test.go

@@ -38,7 +38,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/aws/karpenter-core/pkg/apis"
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/cloudprovider"
 	"github.com/aws/karpenter-core/pkg/cloudprovider/fake"

pkg/controllers/inflightchecks/suite_test.go

@@ -31,7 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/aws/karpenter-core/pkg/apis"
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/cloudprovider/fake"
 	"github.com/aws/karpenter-core/pkg/controllers/inflightchecks"

pkg/controllers/metrics/state/suite_test.go

@@ -28,7 +28,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/aws/karpenter-core/pkg/apis"
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	metricsstate "github.com/aws/karpenter-core/pkg/controllers/metrics/state"
 	"github.com/aws/karpenter-core/pkg/controllers/state/informer"

pkg/controllers/node/controller.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	corecontroller "github.com/aws/karpenter-core/pkg/operator/controller"
 
@@ -85,14 +86,18 @@ func (c *Controller) Reconcile(ctx context.Context, node *v1.Node) (reconcile.Re
 	// Execute Reconcilers
 	var results []reconcile.Result
 	var errs error
-	for _, reconciler := range []interface {
+
+	reconcilers := []interface {
 		Reconcile(context.Context, *v1alpha5.Provisioner, *v1.Node) (reconcile.Result, error)
 	}{
 		c.initialization,
 		c.emptiness,
 		c.finalizer,
-		c.drift,
-	} {
+	}
+	if settings.FromContext(ctx).DriftEnabled {
+		reconcilers = append(reconcilers, c.drift)
+	}
+	for _, reconciler := range reconcilers {
 		res, err := reconciler.Reconcile(ctx, provisioner, node)
 		errs = multierr.Append(errs, err)
 		results = append(results, res)

pkg/controllers/node/drift.go

@@ -20,12 +20,12 @@ import (
 	"time"
 
 	v1 "k8s.io/api/core/v1"
+	"knative.dev/pkg/logging"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/samber/lo"
 
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/cloudprovider"
 	"github.com/aws/karpenter-core/pkg/utils/machine"
@@ -37,10 +37,7 @@ type Drift struct {
 }
 
 func (d *Drift) Reconcile(ctx context.Context, provisioner *v1alpha5.Provisioner, node *v1.Node) (reconcile.Result, error) {
-	if !settings.FromContext(ctx).DriftEnabled {
-		return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil
-	}
-
+	logging.FromContext(ctx).Infof("Drift is enabled!")
 	if _, ok := node.Annotations[v1alpha5.VoluntaryDisruptionAnnotationKey]; ok {
 		return reconcile.Result{}, nil
 	}

pkg/controllers/node/suite_test.go

@@ -33,7 +33,7 @@ import (
 	. "knative.dev/pkg/logging/testing"
 
 	"github.com/aws/karpenter-core/pkg/apis"
-	"github.com/aws/karpenter-core/pkg/apis/config/settings"
+	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/cloudprovider/fake"
 	"github.com/aws/karpenter-core/pkg/operator/controller"
 	"github.com/aws/karpenter-core/pkg/operator/scheme"
@@ -50,7 +50,6 @@ var ctx context.Context
 var nodeController controller.Controller
 var env *test.Environment
 var fakeClock *clock.FakeClock
-var settingsStore test.SettingsStore
 var cp *fake.CloudProvider
 
 func TestAPIs(t *testing.T) {
@@ -79,9 +78,7 @@ var _ = Describe("Controller", func() {
 			ObjectMeta: metav1.ObjectMeta{Name: test.RandomName()},
 			Spec:       v1alpha5.ProvisionerSpec{},
 		}
-		settingsStore = test.SettingsStore{
-			settings.ContextKey: test.Settings(),
-		}
+		ctx = settings.ToContext(ctx, test.Settings(test.SettingsOptions{DriftEnabled: true}))
 	})
 
 	AfterEach(func() {
@@ -92,10 +89,7 @@ var _ = Describe("Controller", func() {
 	Context("Drift", func() {
 		It("should not detect drift if the feature flag is disabled", func() {
 			cp.Drifted = true
-			settingsStore = test.SettingsStore{
-				settings.ContextKey: test.Settings(test.SettingsOptions{DriftEnabled: false}),
-			}
-			ctx = settingsStore.InjectSettings(ctx)
+			ctx = settings.ToContext(ctx, test.Settings(test.SettingsOptions{DriftEnabled: false}))
 			node := test.Node(test.NodeOptions{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{
@@ -111,10 +105,6 @@ var _ = Describe("Controller", func() {
 		})
 		It("should not detect drift if the provisioner does not exist", func() {
 			cp.Drifted = true
-			settingsStore = test.SettingsStore{
-				settings.ContextKey: test.Settings(test.SettingsOptions{DriftEnabled: true}),
-			}
-			ctx = settingsStore.InjectSettings(ctx)
 			node := test.Node(test.NodeOptions{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{
@@ -130,10 +120,6 @@ var _ = Describe("Controller", func() {
 		})
 		It("should annotate the node when it has drifted in the cloud provider", func() {
 			cp.Drifted = true
-			settingsStore = test.SettingsStore{
-				settings.ContextKey: test.Settings(test.SettingsOptions{DriftEnabled: true}),
-			}
-			ctx = settingsStore.InjectSettings(ctx)
 			node := test.Node(test.NodeOptions{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{