From 601aecb5ececd0a9f5ac225bff3cf9fb4ed261a9 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Thu, 10 Sep 2026 11:37:21 +0200 Subject: [PATCH] fix(repo): fail the sync when template values cannot be loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SyncRepos` logged a warning and carried on when `loadValuesFromConfigMap` failed. That function returns `(nil, err)` on every error path, so `values` was nil, the `if values != nil` guard installed no templating engine, and the copy, commit and push all still ran — publishing files with their `{{ }}` placeholders intact and then setting Available/Synced. The resource is then terminal. `TargetCommitId` matches the commit that was just pushed, so Observe sees an up-to-date resource: it never retries and never corrects itself. An operator sees READY=True SYNCED=True over a target repo full of unrendered templates. The trigger does not need to be exotic. 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 fail as a missing key, it reaches `json.Unmarshal([]byte(""))` and fails as "unexpected end of JSON input". Writing YAML into a key the controller parses as JSON does the same, as does a ConfigMap the provider cannot read. 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. The warning log and the CannotLoadConfigMap event are kept; only the silent continue is replaced with failSync. Tests (internal/controllers/repo/configmap_values_test.go), plain unit tests with a fake client — the existing repo_test.go is `//go:build integration` and needs Docker, these do not: - a typo'd key is an error, not an empty value set - YAML in a JSON key is an error - a valid key still returns its values - a load failure yields nil values — the precondition that made the silent skip possible, asserted so a future refactor dropping the early return has something to fail against go build ./..., go vet and go test ./internal/... clean. Found by an adversarial review of the write-path templating; it survived five independent refutation attempts. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv --- .../controllers/repo/configmap_values_test.go | 84 +++++++++++++++++++ internal/controllers/repo/repo.go | 12 +++ 2 files changed, 96 insertions(+) create mode 100644 internal/controllers/repo/configmap_values_test.go 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",