diff --git a/internal/controllers/repo/configmap_values_test.go b/internal/controllers/repo/configmap_values_test.go new file mode 100644 index 0000000..b1c2a2a --- /dev/null +++ b/internal/controllers/repo/configmap_values_test.go @@ -0,0 +1,84 @@ +package repo + +import ( + "context" + "testing" + + commonv1 "github.com/krateoplatformops/provider-runtime/apis/common/v1" + "github.com/krateoplatformops/provider-runtime/pkg/logging" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func newExternalWithConfigMap(t *testing.T, data map[string]string) *external { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "tpl-values", Namespace: "test-system"}, + Data: data, + } + return &external{ + kube: fake.NewClientBuilder().WithScheme(scheme).WithObjects(cm).Build(), + log: logging.NewNopLogger(), + } +} + +func ref(key string) *commonv1.ConfigMapKeySelector { + return &commonv1.ConfigMapKeySelector{ + Reference: commonv1.Reference{Name: "tpl-values", Namespace: "test-system"}, + Key: key, + } +} + +// The quiet trigger. provider-runtime's GetConfigMapValue returns `string(cm.Data[ref.Key])`, +// which for an ABSENT key is "" with a NIL error — so a typo in configMapKeyRef.key does not +// surface as a missing-key error, it surfaces as unparseable JSON. loadValuesFromConfigMap must +// still report that as an error rather than handing back a usable-looking nil map. +func TestLoadValuesFromConfigMapErrorsOnMissingKey(t *testing.T) { + e := newExternalWithConfigMap(t, map[string]string{"values": `{"name":"sock-shop"}`}) + + values, err := e.loadValuesFromConfigMap(context.Background(), ref("valeus")) // typo + + require.Error(t, err, "a missing key must be an error, not an empty value set") + require.Nil(t, values) +} + +// Same requirement for the other realistic operator mistake: YAML written into a key the +// controller parses as JSON. +func TestLoadValuesFromConfigMapErrorsOnNonJSON(t *testing.T) { + e := newExternalWithConfigMap(t, map[string]string{"values": "name: sock-shop\n"}) + + values, err := e.loadValuesFromConfigMap(context.Background(), ref("values")) + + require.Error(t, err) + require.Nil(t, values) +} + +func TestLoadValuesFromConfigMapReturnsValues(t *testing.T) { + e := newExternalWithConfigMap(t, map[string]string{"values": `{"name":"sock-shop","replicas":2}`}) + + values, err := e.loadValuesFromConfigMap(context.Background(), ref("values")) + + require.NoError(t, err) + require.Equal(t, "sock-shop", values["name"]) +} + +// Guards the actual regression: on a load failure `values` is nil, and the templating guard +// further down (`if values != nil`) would install NO engine — so had SyncRepos continued, it +// would have copied, committed and pushed files with their `{{ }}` placeholders intact and then +// reported Available/Synced. This asserts the precondition that made that outcome possible, so a +// future refactor that drops the early return has something to fail against. +func TestLoadFailureYieldsNilValuesWhichWouldSkipTemplating(t *testing.T) { + e := newExternalWithConfigMap(t, map[string]string{"values": `{"name":"sock-shop"}`}) + + values, err := e.loadValuesFromConfigMap(context.Background(), ref("nope")) + + require.Error(t, err) + require.Nil(t, values, "nil values is exactly what makes `if values != nil` skip the engine") +} diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index 7ba0f65..b74a680 100644 --- a/internal/controllers/repo/repo.go +++ b/internal/controllers/repo/repo.go @@ -352,6 +352,18 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM if err != nil { e.log.Warn("Unable to load configmap with template data", "msg", err.Error()) e.rec.Event(cr, plumbingevent.Warning("CannotLoadConfigMap", "Reconciling", fmt.Errorf("Unable to load configmap with template data: %s", err.Error()))) + + // Stop. Continuing here is worse than failing: loadValuesFromConfigMap returns + // (nil, err) on every error path, so `values` is nil, the `if values != nil` + // guard below installs no templating engine, and the copy/commit/push still + // runs — publishing files with their `{{ }}` placeholders intact and then + // reporting Available/Synced. Because TargetCommitId then matches what was + // pushed, the resource is terminal: it never retries and never corrects itself. + // + // A CR that sets configMapKeyRef has asked for substitution. If the values + // cannot be read, the honest outcome is a failed sync, not a green one over + // unrendered output. + return e.failSync(ctx, cr, fmt.Errorf("unable to load configmap with template data: %w", err)) } e.log.Debug("Loaded values from config map",