diff --git a/go.mod b/go.mod index fc429c3..d0be0ca 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc github.com/go-logr/logr v1.4.3 github.com/gobuffalo/flect v1.0.3 - github.com/krateoplatformops/plumbing v1.7.15 + github.com/krateoplatformops/plumbing v1.10.0 github.com/krateoplatformops/unstructured-runtime v1.1.0 github.com/stretchr/testify v1.11.1 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0 @@ -160,12 +160,13 @@ require ( // Use the braghettos fork of unstructured-runtime: create-pending Observe-before-refuse // recovery (#2), jq-based statusprojection (#3), the shared OTel JSON log handler // NewOTelJSONHandler (#4), AND the OTel metrics-resource attrs + trace pipeline / reconcile -// span (#5) -- unified in tag v1.3.0. +// span (#5, tag v1.3.0). v1.3.1 drops the plumbing/slogs/pretty test dependency so the module +// builds against plumbing's current main line. // The fork keeps the upstream module path, so pin it via replace. -replace github.com/krateoplatformops/unstructured-runtime => github.com/braghettos/unstructured-runtime v1.3.0 +replace github.com/krateoplatformops/unstructured-runtime => github.com/braghettos/unstructured-runtime v1.3.1 -// Source plumbing from the braghettos fork (v1.7.7): carries the jqutil int64/int32 -// gojq-panic fix, crdgen array-default markers, AND the krateo.io/traceparent child-manifest -// post-render stamping for cross-composition trace propagation. (v1.7.x maintenance line — -// the divergent v1.8.x fork line dropped slogs/pretty, which unstructured-runtime tests need.) -replace github.com/krateoplatformops/plumbing => github.com/braghettos/plumbing v1.7.15 \ No newline at end of file +// Source plumbing from the braghettos fork (v1.10.0, main line): carries the jqutil int64/int32 +// gojq-panic fix, crdgen array-default markers, the krateo.io/traceparent child-manifest +// post-render stamping for cross-composition trace propagation, AND the fork-free +// apply-if-changed reconcile with semantic change-detection that this controller drives. +replace github.com/krateoplatformops/plumbing => github.com/braghettos/plumbing v1.10.0 diff --git a/go.sum b/go.sum index 3832fd5..6dab926 100644 --- a/go.sum +++ b/go.sum @@ -28,10 +28,10 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM= github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ= -github.com/braghettos/plumbing v1.7.15 h1:ZR3KN9a+u/hjz3PS8gTocs7027hjDl34dsBF8rxzSUA= -github.com/braghettos/plumbing v1.7.15/go.mod h1:7iHgxAVJWAZeYTnnTV+JgX1U/Sm4ADsu+hlna0L6vqM= -github.com/braghettos/unstructured-runtime v1.3.0 h1:t8QnEN3TIdfJVfO9iNP5OSFIbQlwpR/xIzWMfcDvwHc= -github.com/braghettos/unstructured-runtime v1.3.0/go.mod h1:hkDWf74tgbfYONkwKEjaX24KgQLPe7fUvArz8wqglnA= +github.com/braghettos/plumbing v1.10.0 h1:TZmyD9OtpI83sxFV/B7jBR3cRxi/nA9YQ53IvLKME50= +github.com/braghettos/plumbing v1.10.0/go.mod h1:v5uFPU7mbHGg2Nyt1UFG2c+5tFmqxLJeUVRIX4V+MjE= +github.com/braghettos/unstructured-runtime v1.3.1 h1:ZwuLl1VP7Ysqk/JSrYHIC0GOYVSN7tV0XdiFkZAIa3Q= +github.com/braghettos/unstructured-runtime v1.3.1/go.mod h1:FhrvKaArNeocZaW0TDQuvR2nmPo+fgbFHFNP2l7H/i4= github.com/bshuster-repo/logrus-logstash-hook v1.0.0 h1:e+C0SB5R1pu//O4MQ3f9cFuPGoOVeF2fE4Og9otCc70= github.com/bshuster-repo/logrus-logstash-hook v1.0.0/go.mod h1:zsTqEiSzDgAa/8GZR7E1qaXrhYNDKBYy5/dWPTIflbk= github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= @@ -447,4 +447,4 @@ sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxO sigs.k8s.io/structured-merge-diff/v6 v6.3.0 h1:jTijUJbW353oVOd9oTlifJqOGEkUw2jB/fXCbTiQEco= sigs.k8s.io/structured-merge-diff/v6 v6.3.0/go.mod h1:M3W8sfWvn2HhQDIbGWj3S099YozAsymCo/wrT5ohRUE= sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs= -sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4= \ No newline at end of file +sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4= diff --git a/internal/composition/composition.go b/internal/composition/composition.go index 5df9255..5890aea 100644 --- a/internal/composition/composition.go +++ b/internal/composition/composition.go @@ -51,6 +51,13 @@ import ( var ( krateoNamespace = env.String(krateoNamespaceEnvVar, krateoNamespaceDefault) helmMaxHistory = env.Int(helmMaxHistoryEnvvar, 3) + // pendingOperationGrace: a helm release found in a pending-* status is rolled back ONLY if it + // has been pending LONGER than this — i.e. genuinely stuck (the controller died mid-operation), + // not legitimately in-flight. A large composition's upgrade (hundreds of children + hooks) can + // stay pending for tens of seconds; rolling that back mid-flight caused an Upgrade<->Rollback + // thrash on the 319-resource portal composition. Default 5m (>= helm's default op timeout), so + // only operations that exceeded a real upgrade are treated as stuck. Tunable per deployment. + pendingOperationGrace = env.Duration(pendingGraceEnvvar, 5*time.Minute) ) const ( @@ -64,6 +71,7 @@ const ( // Environment variables helmMaxHistoryEnvvar = "HELM_MAX_HISTORY" krateoNamespaceEnvVar = "KRATEO_NAMESPACE" + pendingGraceEnvvar = "COMPOSITION_CONTROLLER_PENDING_GRACE" // Default namespace for Krateo Installation krateoNamespaceDefault = "krateo-system" @@ -226,8 +234,26 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c } if rel.Status == helmconfig.StatusPendingInstall || rel.Status == helmconfig.StatusPendingUpgrade || rel.Status == helmconfig.StatusPendingRollback { - log.Debug("Composition stuck install or upgrade in progress. Rolling back to previous release before re-attempting.") - // Rollback to previous release + // A pending status means a helm operation is in flight OR its process died mid-flight — helm + // labels both the same and never re-labels a crash. Status alone can't tell them apart, so we + // use how long it has been pending (rel.Updated = helm Info.LastDeployed). A LARGE composition's + // upgrade (hundreds of children + hooks) legitimately stays pending for tens of seconds; the old + // unconditional rollback reverted such in-flight operations mid-flight (even rolling back a + // pending-rollback), which — with the reconcile re-enqueue cadence — produced an Upgrade<->Rollback + // thrash on the 319-resource portal composition. + pendingFor := time.Since(rel.Updated) + if pendingFor < pendingOperationGrace { + // Recent => legitimately in flight. Do NOT roll it back and do NOT start a concurrent + // operation; report up-to-date so the in-flight op settles and the next reconcile proceeds. + log.Debug("Release operation in progress; waiting for it to settle.", + "status", string(rel.Status), "pendingFor", pendingFor.String()) + return controller.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil + } + // Pending longer than any real operation => genuinely stuck (e.g. controller died mid-op). Roll + // back once to clear the stale pending lock so a fresh upgrade can proceed (helm refuses to + // upgrade a release that is stuck pending). + log.Debug("Composition stuck in a pending helm operation past the grace period; rolling back to clear it.", + "status", string(rel.Status), "pendingFor", pendingFor.String(), "grace", pendingOperationGrace.String()) rel, err = hc.Rollback(ctx, releaseName, &helmconfig.RollbackConfig{ MaxHistory: helmMaxHistory, ReleaseVersion: rel.Revision, @@ -339,7 +365,7 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c // Without this, one un-adoptable child 500s the entire reconcile ("cannot be imported // into the current release: invalid ownership metadata") and wedges the platform (D1, // 2026-07-08); with it the release takes ownership, self-healing the conflict. - TakeOwnership: true, + TakeOwnership: true, }, MaxHistory: helmMaxHistory, }) @@ -516,7 +542,7 @@ func (h *handler) Create(ctx context.Context, mg *unstructured.Unstructured) err // non-Helm ownership metadata (out-of-band-created/edited composition instance). Otherwise one // un-adoptable child 500s the entire reconcile and wedges the platform (D1); with it the // release takes ownership and self-heals the conflict. - TakeOwnership: true, + TakeOwnership: true, } // Check if the release already exists before attempting to install, this can happen if the create event is triggered after a failed install @@ -938,4 +964,4 @@ func (h *handler) getHelmLogger(verbose bool) func(format string, v ...interface } } return func(format string, v ...interface{}) {} -} \ No newline at end of file +} diff --git a/internal/tools/rbac/installer.go b/internal/tools/rbac/installer.go index 9359c98..cc76751 100644 --- a/internal/tools/rbac/installer.go +++ b/internal/tools/rbac/installer.go @@ -72,11 +72,6 @@ func (i *RBACInstaller) ApplyRBAC(rbac *RBAC) error { } func (r *RBACInstaller) ApplyNamespace(ctx context.Context, namespace *corev1.Namespace) (*corev1.Namespace, error) { - m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(namespace) - if err != nil { - return nil, fmt.Errorf("failed to convert Namespace to unstructured: %w", err) - } - u := &unstructured.Unstructured{Object: m} cli := r.DynamicClient.Resource( schema.GroupVersionResource{ Group: "", @@ -85,14 +80,18 @@ func (r *RBACInstaller) ApplyNamespace(ctx context.Context, namespace *corev1.Na }, ) - if _, err := cli.Get(ctx, namespace.Name, metav1.GetOptions{}); errors.IsNotFound(err) { - res, err := cli.Create(ctx, u, metav1.CreateOptions{}) + existingU, err := cli.Get(ctx, namespace.Name, metav1.GetOptions{}) + if errors.IsNotFound(err) { + m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(namespace) + if err != nil { + return nil, fmt.Errorf("failed to convert Namespace to unstructured: %w", err) + } + res, err := cli.Create(ctx, &unstructured.Unstructured{Object: m}, metav1.CreateOptions{}) if err != nil { return nil, fmt.Errorf("failed to Create Namespace: %w", err) } namespace = &corev1.Namespace{} - err = runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace) - if err != nil { + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace); err != nil { return nil, fmt.Errorf("failed to convert unstructured to Namespace: %w", err) } return namespace, nil @@ -100,13 +99,53 @@ func (r *RBACInstaller) ApplyNamespace(ctx context.Context, namespace *corev1.Na return nil, fmt.Errorf("failed to Get Namespace: %w", err) } - res, err := cli.Update(ctx, u, metav1.UpdateOptions{}) + // The Namespace already exists. It may be owned by the composition's Helm + // release (e.g. the portal's demo-system, declared as a chart child), in + // which case its metadata carries foreign labels such as the postrenderer's + // krateo.io/composition-* set. A blind full-PUT of our freshly-built object + // would replace metadata.labels and strip those, so helm re-adds them on the + // next reconcile — an endless label tug-of-war that bumps a helm revision + // every cycle. Instead merge our labels/annotations INTO the existing object + // (like ApplyRole does for rules) and only Update when something is missing. + existing := &corev1.Namespace{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(existingU.Object, existing); err != nil { + return nil, fmt.Errorf("failed to convert unstructured to Namespace: %w", err) + } + + modified := false + if len(namespace.Labels) > 0 && existing.Labels == nil { + existing.Labels = map[string]string{} + } + for k, v := range namespace.Labels { + if existing.Labels[k] != v { + existing.Labels[k] = v + modified = true + } + } + if len(namespace.Annotations) > 0 && existing.Annotations == nil { + existing.Annotations = map[string]string{} + } + for k, v := range namespace.Annotations { + if existing.Annotations[k] != v { + existing.Annotations[k] = v + modified = true + } + } + + if !modified { + return existing, nil + } + + m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(existing) + if err != nil { + return nil, fmt.Errorf("failed to convert Namespace to unstructured: %w", err) + } + res, err := cli.Update(ctx, &unstructured.Unstructured{Object: m}, metav1.UpdateOptions{}) if err != nil { return nil, fmt.Errorf("failed to Update Namespace: %w", err) } namespace = &corev1.Namespace{} - err = runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace) - if err != nil { + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace); err != nil { return nil, fmt.Errorf("failed to convert unstructured to Namespace: %w", err) } return namespace, nil diff --git a/internal/tools/rbac/installer_test.go b/internal/tools/rbac/installer_test.go index e396bc5..966dd01 100644 --- a/internal/tools/rbac/installer_test.go +++ b/internal/tools/rbac/installer_test.go @@ -1083,6 +1083,60 @@ func TestApplyNamespace_UpdateExisting(t *testing.T) { testenv.Test(t, f) } +// TestApplyNamespace_PreservesForeignLabels is the regression guard for the +// demo-system label-strip churn: when the Namespace already exists and carries +// labels this installer does not manage (e.g. the postrenderer's +// krateo.io/composition-* set stamped by Helm), ApplyNamespace must merge its +// own labels in WITHOUT clobbering the foreign ones. A blind full-PUT of the +// bare CreateNamespace object used to strip them, and Helm re-added them every +// reconcile — one helm revision per cycle, forever. +func TestApplyNamespace_PreservesForeignLabels(t *testing.T) { + f := features.New("ApplyNamespace_PreservesForeignLabels"). + Setup(e2e.Logger("test")). + Setup(func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { + return ctx + }).Assess("Foreign composition labels survive a bare apply", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { + dyn := dynamic.NewForConfigOrDie(cfg.Client().RESTConfig()) + + installer := &RBACInstaller{DynamicClient: dyn} + + // Simulate the Helm-owned namespace: created by the chart + postrenderer + // with a krateo.io/composition-* label the RBAC installer knows nothing about. + helmOwned := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace-foreign", + Labels: map[string]string{ + "krateo.io/composition-id": "abc123", + }, + }, + } + if _, err := installer.ApplyNamespace(context.Background(), helmOwned); err != nil { + t.Fatalf("expected no error seeding helm-owned namespace, got %v", err) + } + + // The RBAC installer's own view: only the managed-by label (see CreateNamespace). + bare := CreateNamespace("test-namespace-foreign", "some-release", "krateo-system") + + result, err := installer.ApplyNamespace(context.Background(), bare) + if err != nil { + t.Fatalf("expected no error applying bare namespace, got %v", err) + } + + // Our label must be present... + if result.Labels["app.kubernetes.io/managed-by"] != "Helm" { + t.Errorf("expected managed-by=Helm to be merged in, got %q", result.Labels["app.kubernetes.io/managed-by"]) + } + // ...AND the foreign composition label must NOT have been stripped. + if result.Labels["krateo.io/composition-id"] != "abc123" { + t.Errorf("foreign label krateo.io/composition-id was stripped: got %q, want abc123", result.Labels["krateo.io/composition-id"]) + } + + return ctx + }).Feature() + + testenv.Test(t, f) +} + func TestApplyNamespace_InvalidName(t *testing.T) { f := features.New("ApplyNamespace_InvalidName"). Setup(e2e.Logger("test")).