fix: preserve valid config on failed reload

Author: immanuwell

pkg/identity/keystone/keystone.go

@@ -171,16 +171,16 @@ func (k *Auth) processNextItem() bool {
 	return true
 }
 
-func (k *Auth) updatePolicies(cm *apiv1.ConfigMap, key string) {
+func (k *Auth) updatePolicies(cm *apiv1.ConfigMap, key string) error {
 	klog.Info("ConfigMap created or updated, will update the authorization policy.")
 
 	var policy policyList
 	if err := json.Unmarshal([]byte(cm.Data["policies"]), &policy); err != nil {
-		runtimeutil.HandleError(fmt.Errorf("failed to parse policies defined in the configmap %s: %v", key, err))
+		return fmt.Errorf("failed to parse policies defined in the configmap %s: %w", key, err)
 	}
 	if len(policy) > 0 {
 		if _, err := json.MarshalIndent(policy, "", "  "); err != nil {
-			runtimeutil.HandleError(fmt.Errorf("failed to parse policies defined in the configmap %s: %v", key, err))
+			return fmt.Errorf("failed to parse policies defined in the configmap %s: %w", key, err)
 		}
 	}
 
@@ -189,23 +189,26 @@ func (k *Auth) updatePolicies(cm *apiv1.ConfigMap, key string) {
 	k.authz.mu.Unlock()
 
 	klog.Infof("Authorization policy updated.")
+	return nil
 }
 
-func (k *Auth) updateSyncConfig(cm *apiv1.ConfigMap, key string) {
+func (k *Auth) updateSyncConfig(cm *apiv1.ConfigMap, key string) error {
 	klog.Info("ConfigMap created or updated, will update the sync configuration.")
 
-	var sc *syncConfig
-	newConfig := newSyncConfig()
-	sc = &newConfig
-	if err := yaml.Unmarshal([]byte(cm.Data["syncConfig"]), sc); err != nil {
-		runtimeutil.HandleError(fmt.Errorf("failed to parse sync config defined in the configmap %s: %v", key, err))
+	sc := newSyncConfig()
+	if err := yaml.Unmarshal([]byte(cm.Data["syncConfig"]), &sc); err != nil {
+		return fmt.Errorf("failed to parse sync config defined in the configmap %s: %w", key, err)
+	}
+	if err := sc.validate(); err != nil {
+		return fmt.Errorf("sync config defined in the configmap %s is invalid: %w", key, err)
 	}
 
 	k.syncer.mu.Lock()
-	k.syncer.syncConfig = sc
+	k.syncer.syncConfig = &sc
 	k.syncer.mu.Unlock()
 
 	klog.Infof("Sync configuration updated.")
+	return nil
 }
 
 func (k *Auth) processItem(key string) error {
@@ -234,10 +237,14 @@ func (k *Auth) processItem(key string) error {
 		return fmt.Errorf("error fetching object with key %s: %v", key, err)
 	default:
 		if name == k.config.PolicyConfigMapName {
-			k.updatePolicies(cm, key)
+			if err := k.updatePolicies(cm, key); err != nil {
+				return err
+			}
 		}
 		if name == k.config.SyncConfigMapName {
-			k.updateSyncConfig(cm, key)
+			if err := k.updateSyncConfig(cm, key); err != nil {
+				return err
+			}
 		}
 	}
 

pkg/identity/keystone/keystone_test.go

@@ -27,6 +27,7 @@ import (
 	"testing"
 
 	"github.com/spf13/pflag"
+	apiv1 "k8s.io/api/core/v1"
 )
 
 func TestUserAgentFlag(t *testing.T) {
@@ -152,3 +153,68 @@ func TestWebhookRouting(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdatePolicies(t *testing.T) {
+	existingPolicy := &policy{}
+	auth := &Auth{authz: &Authorizer{pl: policyList{existingPolicy}}}
+
+	err := auth.updatePolicies(&apiv1.ConfigMap{Data: map[string]string{"policies": "{"}}, "kube-system/policy")
+	if err == nil {
+		t.Fatal("expected an invalid policy update to fail")
+	}
+	if len(auth.authz.pl) != 1 || auth.authz.pl[0] != existingPolicy {
+		t.Fatal("invalid policy update replaced the existing policy")
+	}
+
+	err = auth.updatePolicies(&apiv1.ConfigMap{Data: map[string]string{"policies": "[{}]"}}, "kube-system/policy")
+	if err != nil {
+		t.Fatalf("expected a valid policy update to succeed: %v", err)
+	}
+	if len(auth.authz.pl) != 1 || auth.authz.pl[0] == existingPolicy {
+		t.Fatal("valid policy update did not replace the existing policy")
+	}
+}
+
+func TestUpdateSyncConfig(t *testing.T) {
+	tests := []struct {
+		name       string
+		syncConfig string
+	}{
+		{
+			name:       "malformed config",
+			syncConfig: "data-types-to-sync: [projects",
+		},
+		{
+			name:       "invalid config",
+			syncConfig: "data-types-to-sync: [unsupported]",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			existingConfig := &syncConfig{DataTypesToSync: []string{Projects}, NamespaceFormat: "%i"}
+			auth := &Auth{syncer: &Syncer{syncConfig: existingConfig}}
+
+			err := auth.updateSyncConfig(&apiv1.ConfigMap{Data: map[string]string{"syncConfig": tt.syncConfig}}, "kube-system/sync")
+			if err == nil {
+				t.Fatal("expected an invalid sync config update to fail")
+			}
+			if auth.syncer.syncConfig != existingConfig {
+				t.Fatal("invalid sync config update replaced the existing config")
+			}
+		})
+	}
+
+	auth := &Auth{syncer: &Syncer{syncConfig: &syncConfig{}}}
+	err := auth.updateSyncConfig(&apiv1.ConfigMap{Data: map[string]string{
+		"syncConfig": "data-types-to-sync: [projects]\nnamespace-format: prefix-%i"},
+	}, "kube-system/sync")
+	if err != nil {
+		t.Fatalf("expected a valid sync config update to succeed: %v", err)
+	}
+	if auth.syncer.syncConfig.NamespaceFormat != "prefix-%i" ||
+		len(auth.syncer.syncConfig.DataTypesToSync) != 1 ||
+		auth.syncer.syncConfig.DataTypesToSync[0] != Projects {
+		t.Fatalf("valid sync config update was not applied: %#v", auth.syncer.syncConfig)
+	}
+}