diff --git a/.coverage-baseline b/.coverage-baseline index f5bfc226..d916ff4b 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -77.8 +77.9 diff --git a/internal/controller/live_message_migration_test.go b/internal/controller/live_message_migration_test.go index 8fe44eaf..9d2b36dc 100644 --- a/internal/controller/live_message_migration_test.go +++ b/internal/controller/live_message_migration_test.go @@ -4,9 +4,9 @@ package controller import ( "context" - "strings" + "os" + "path/filepath" "testing" - "time" "github.com/stretchr/testify/require" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -16,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/yaml" api "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) @@ -33,9 +34,37 @@ func applyIgnoringUnknownFields(ctx context.Context, c client.Client, obj *unstr &client.PatchOptions{Raw: &metav1.PatchOptions{FieldValidation: "Ignore"}}) } -func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { - ctx := context.Background() - env := &envtest.Environment{CRDDirectoryPaths: []string{"../../config/crd/bases"}, ErrorIfCRDPathMissing: true} +const crdBaseDir = "../../config/crd/bases" + +// legacyGitTarget is the shape a GitTarget had before liveTemplate replaced the two +// per-event templates: the documents this migration has to keep working for. +func legacyGitTarget(name string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "configbutler.ai/v1alpha3", "kind": "GitTarget", + "metadata": map[string]any{"name": name, "namespace": "default"}, + "spec": map[string]any{ + "gitProviderRef": map[string]any{"name": "provider"}, + "branch": "main", + "path": name, + "commit": map[string]any{ + "message": map[string]any{"eventTemplate": "old event", "groupTemplate": "old group"}, + }, + }, + }} +} + +// startEnv brings up an envtest apiserver holding exactly the given CRDs and returns a client. +// Passing the CRDs as objects rather than a directory is what lets a caller install a MODIFIED +// schema, without ever editing one that is already being served. +func startEnv(t *testing.T, crds ...*apiextv1.CustomResourceDefinition) client.Client { + t.Helper() + env := &envtest.Environment{} + if len(crds) == 0 { + env.CRDDirectoryPaths = []string{crdBaseDir} + env.ErrorIfCRDPathMissing = true + } else { + env.CRDs = crds + } cfg, err := env.Start() require.NoError(t, err) t.Cleanup(func() { require.NoError(t, env.Stop()) }) @@ -44,40 +73,83 @@ func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { require.NoError(t, apiextv1.AddToScheme(sch)) c, err := client.New(cfg, client.Options{Scheme: sch}) require.NoError(t, err) + + return c +} + +// gitTargetCRDWithoutMessageValidation reads the shipped GitTarget CRD and returns it with the +// retirement rules on spec.commit.message removed. +// +// The point is WHEN this happens: before the apiserver has ever seen the CRD, so the relaxed +// schema is the only one it ever compiles. Editing a live CRD instead means asking it to rebuild +// a serving handler, and under CPU pressure that rebuild is sometimes missed outright -- the new +// schema reads back correctly while the previously compiled one goes on validating, with nothing +// to retrigger it. Doing the edit here removes the rebuild from the test rather than waiting on +// one, which is not something a timeout can fix: measured under load, every run that converged +// did so within 6.3s and every run that failed used its entire budget, whether that was 10 +// seconds or 60. +func gitTargetCRDWithoutMessageValidation(t *testing.T) *apiextv1.CustomResourceDefinition { + t.Helper() + raw, err := os.ReadFile(filepath.Join(crdBaseDir, "configbutler.ai_gittargets.yaml")) + require.NoError(t, err) var crd apiextv1.CustomResourceDefinition - require.NoError(t, c.Get(ctx, client.ObjectKey{Name: "gittargets.configbutler.ai"}, &crd)) - current := crd.DeepCopy() - message := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["commit"].Properties["message"] - message.XValidations = nil - crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["commit"].Properties["message"] = message - require.NoError(t, c.Update(ctx, &crd)) - legacy := func(name string) *unstructured.Unstructured { - return &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "configbutler.ai/v1alpha3", "kind": "GitTarget", - "metadata": map[string]any{"name": name, "namespace": "default"}, - "spec": map[string]any{ - "gitProviderRef": map[string]any{"name": "provider"}, - "branch": "main", - "path": name, - "commit": map[string]any{ - "message": map[string]any{"eventTemplate": "old event", "groupTemplate": "old group"}, - }, - }, - }} + require.NoError(t, yaml.Unmarshal(raw, &crd)) + for i := range crd.Spec.Versions { + root := crd.Spec.Versions[i].Schema.OpenAPIV3Schema + spec := root.Properties["spec"] + commit := spec.Properties["commit"] + msg := commit.Properties["message"] + require.NotEmpty(t, msg.XValidations, "shipped CRD no longer carries the rules this test relaxes") + msg.XValidations = nil + commit.Properties["message"] = msg + spec.Properties["commit"] = commit + root.Properties["spec"] = spec } + + return &crd +} + +// TestLiveMessageValidation_RejectsLegacyAcceptsMigrated pins what the SHIPPED schema does: +// it refuses the pre-migration shape and accepts the migrated one. +// +// It runs against the CRD exactly as installed, and never edits it. That is the point. This +// assertion used to live at the end of the migration test below, which got there by stripping +// the validation and then putting it back -- and waiting for the restore to take effect is a +// race the apiserver loses often enough to matter. The apiextensions apiserver rebuilds a CRD's +// serving handler asynchronously after a spec write, and under CPU pressure that rebuild is +// sometimes missed outright: the new schema reads back correctly while the previously compiled +// one goes on validating, and nothing retriggers it. Measured under load: 571 polls across 60s +// with the restored rules present in the live CRD and still unenforced. A longer timeout does +// not help, because the wait is not slow, it is stuck -- every run that passed did so within +// 6.3s, and every run that failed used its whole budget. Asserting against an unmodified CRD +// removes the rebuild from the picture rather than waiting on it. +func TestLiveMessageValidation_RejectsLegacyAcceptsMigrated(t *testing.T) { + ctx := context.Background() + c := startEnv(t) + + err := applyIgnoringUnknownFields(ctx, c, legacyGitTarget("reject-legacy")) + require.Error(t, err) + require.Contains(t, err.Error(), "liveTemplate") + + migrated := legacyGitTarget("accept-migrated") + require.NoError(t, unstructured.SetNestedMap(migrated.Object, + map[string]any{"liveTemplate": "chore: sync {{.Count}}"}, "spec", "commit", "message")) + require.NoError(t, applyIgnoringUnknownFields(ctx, c, migrated)) +} + +func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { + ctx := context.Background() + legacy := legacyGitTarget + + // Storing the pre-migration shape means getting it past the validation that forbids it. That + // relaxation is applied to the CRD BEFORE it is installed, so the apiserver compiles it once + // and nothing here ever waits on a schema change. Proving the SHIPPED schema still rejects + // this shape is TestLiveMessageValidation_RejectsLegacyAcceptsMigrated's job, against a CRD + // that is never modified at all. + c := startEnv(t, gitTargetCRDWithoutMessageValidation(t)) for _, name := range []string{"migrate-apply", "migrate-patch"} { - obj := legacy(name) - require.Eventually(t, func() bool { - return applyIgnoringUnknownFields(ctx, c, obj) == nil - }, 10*time.Second, 100*time.Millisecond) + require.NoError(t, applyIgnoringUnknownFields(ctx, c, legacy(name))) } - require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(&crd), &crd)) - crd.Spec = current.Spec - require.NoError(t, c.Update(ctx, &crd)) - require.Eventually(t, func() bool { - err := applyIgnoringUnknownFields(ctx, c, legacy("reject-new")) - return err != nil && strings.Contains(err.Error(), "liveTemplate") - }, 10*time.Second, 100*time.Millisecond) for _, name := range []string{"migrate-apply", "migrate-patch"} { var target api.GitTarget key := client.ObjectKey{Name: name, Namespace: "default"}