Skip to content

Commit a6ce8a2

Browse files
fix(gitops): refuse a binding ID that cannot name a Kubernetes object, and clear the delivery path
Two defects found reviewing this branch, both of which reached the cluster or the publish rather than failing at the render. A binding ID becomes a ConfigMap name. bindingID checked only that its parts were non-empty, free of "./ ", and short enough, so a workspace named My_Workspace produced solution-host-binding-My_Workspace.prod.crm — not a legal Kubernetes object name. Nothing downstream refused it: core's bindingPattern deliberately admits uppercase and underscores so a ULID or a UUID can be a binding ID, and ValidateRenderedTree accepts the tree. The first refusal was ArgoCD at sync, which is the late, opaque failure solutionNamespace exists to prevent for namespaces. The route alias already had exactly the right guard; the ID, which is the half that names an object, did not. Both now share one rfc1123Label rule. RenderOwnedTree wrote SolutionHostBindingPath onto the caller's options only when a binding had been written, and never cleared it. A render that declared none left a previous render's value standing, and the inventory then claimed a delivery path the tree did not have. That is not a cosmetic inaccuracy: generateArgoBootstrap and snapshotAuthority both kustomize-build "<path>/overlays/<environment>", so a stale value refuses the entire publication. The field is now derived unconditionally from what was written, so no code path can leave a stale one. Each fix has a test that was verified to fail with only that fix reverted. Also from the same review: - The bindings component moves from sync wave 2 to the module-resources wave. Argo starts a wave only once the previous is healthy, so wave 2 withheld the declaration of desired state from a solution whose workload was unhealthy — exactly when a host needs it. The digests are over rendered bytes in the delivery repository and exist whatever the cluster is doing, so nothing required the workloads to be healthy first. - docs: removal is not a passive absence. Rendering no binding emits no ApplicationSet element, and dropping an element deletes its Application, whose template carries the Argo resources finalizer with prune enabled — so the delivered ConfigMaps are cascade-deleted. A reconciler must hold a binding across its document's disappearance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fba987c commit a6ce8a2

5 files changed

Lines changed: 169 additions & 24 deletions

File tree

‎docs/solution-host-bindings.md‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,21 @@ both and clears the dev deployments.
176176

177177
## Removal
178178

179-
Not implemented. An instance that leaves the composition currently leaves the
179+
No tombstone is rendered. An instance that leaves the composition leaves the
180180
tree, and core is explicit that removal is a generation and never an absence —
181-
so a host keeps the binding until a tombstone arrives. That is core's design
182-
failing safe rather than a host deleting what it can no longer see, but a
183-
solution genuinely withdrawn is not yet withdrawable through delivery.
181+
so a host must keep the binding until a tombstone arrives, and a solution
182+
genuinely withdrawn is not yet withdrawable through delivery.
183+
184+
**The document does not merely stop being refreshed — Argo deletes it.** When no
185+
binding is rendered the inventory records no binding path, so the promotion
186+
emits no ApplicationSet element for it; dropping an element deletes its
187+
Application, and the Application template carries
188+
`finalizers: [resources-finalizer.argocd.argoproj.io]` with
189+
`syncPolicy.automated.prune: true`. The delivered ConfigMaps are cascade-deleted
190+
from the cluster.
191+
192+
So a reconciler cannot treat "the document is gone" as "nothing to do". It has
193+
to hold the binding across the document's disappearance, or removing a `host`
194+
declaration — or a module ceasing to be a solution — silently withdraws every
195+
binding it delivered. That is a requirement delivery places on the host, not
196+
something the host can infer from what it observes.

‎pkg/gitops/argo.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,19 @@ const (
8989
moduleResourcesWave = "-1"
9090
bootstrapUnitWave = "0"
9191
consumerUnitWave = "1"
92-
// Declared presence lands last: a SolutionHostBinding pins the digests of
93-
// the artifacts this generation rendered, so declaring it before those
94-
// artifacts are healthy would declare a presence the delivery has not yet
95-
// produced.
96-
solutionHostBindingWave = "2"
92+
// Declared presence lands with the module's own resources, before any unit.
93+
//
94+
// It was briefly placed last, on the reasoning that a binding pins the
95+
// digests of this generation's artifacts and should not be declared before
96+
// they are healthy. That reasoning is wrong twice over. The digests are
97+
// taken over rendered bytes in the delivery repository, which exist at
98+
// delivery time whatever the cluster is doing, so nothing about them needs
99+
// the workload running first. And Argo only starts a wave once the previous
100+
// one is healthy, so a solution whose Deployment crashloops would never
101+
// have its binding applied — withholding the declaration of desired state
102+
// exactly when actual state has diverged from it, which is when a host most
103+
// needs it.
104+
solutionHostBindingWave = moduleResourcesWave
97105
)
98106

99107
func unitWave(unit *InventoryUnit) string {

‎pkg/gitops/render.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,7 @@ func RenderOwnedTree(ctx context.Context, opts *RenderOptions, generate func(con
104104
if err != nil {
105105
return RenderResult{}, err
106106
}
107-
if len(bindings) > 0 {
108-
opts.SolutionHostBindingPath = solutionHostBindingDir
109-
}
107+
opts.SolutionHostBindingPath = deliveredBindingPath(bindings)
110108
manifests, err := validateTree(owned, opts)
111109
if err != nil {
112110
return RenderResult{}, err

‎pkg/gitops/solution_host_binding.go‎

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,20 @@ const (
7878
kindConfigMap = "ConfigMap"
7979
)
8080

81-
// routeAliasPattern is narrower than core's namePattern on purpose. core admits
82-
// a dotted or slashed alias; a host keys its registry on the alias as a single
83-
// URL path segment, and a slashed alias cannot be one. Rendering an alias the
84-
// host would have to map is worse than refusing it here, where the composition
85-
// that authored it is in hand.
86-
var routeAliasPattern = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
81+
// rfc1123Label is one lowercase DNS label. Two different things in this file
82+
// must each be one, for two different reasons, and both are narrower than what
83+
// core accepts:
84+
//
85+
// - A route alias, because a host keys its registry on it as a single URL
86+
// path segment, and core's namePattern admits dots and slashes.
87+
// - Every part of a binding ID, because the ID becomes a Kubernetes object
88+
// name, and core's bindingPattern deliberately admits uppercase and
89+
// underscores so that a ULID or a UUID can be a binding ID.
90+
//
91+
// Refusing here is the whole point: both would otherwise sail through the
92+
// render and the publish and fail only when ArgoCD applies the manifest — the
93+
// late, opaque error solutionNamespace exists to prevent for namespaces.
94+
var rfc1123Label = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
8795

8896
// bindingIDMaxLength bounds a binding ID to one Kubernetes label value, which
8997
// is stricter than core's 128. The ID is stamped as a label so a host can find
@@ -217,7 +225,7 @@ func solutionHostBinding(owned string, opts *RenderOptions, instance *SolutionIn
217225
if err != nil {
218226
return nil, fmt.Errorf("solution %s: %w", instance.Name, err)
219227
}
220-
if !routeAliasPattern.MatchString(instance.Alias) {
228+
if !rfc1123Label.MatchString(instance.Alias) {
221229
return nil, fmt.Errorf(
222230
"solution %s claims the route alias %q; a host keys its registry on the alias as one URL path segment, so it must be lowercase letters, digits and dashes, starting and ending alphanumeric",
223231
instance.Name, instance.Alias)
@@ -295,8 +303,17 @@ func bindingID(workspace, environment, instance string) (string, error) {
295303
if part.value == "" {
296304
return "", fmt.Errorf("solution host binding ID needs the %s name", part.label)
297305
}
298-
if strings.ContainsAny(part.value, "./ ") {
299-
return "", fmt.Errorf("solution host binding ID cannot be built from %s %q: it joins its parts on \".\"", part.label, part.value)
306+
// Each part must be one lowercase DNS label. core's bindingPattern is
307+
// wider — it admits uppercase and underscores so a ULID or UUID can be
308+
// a binding ID — so core's own Validate accepts "My_Workspace.prod.crm"
309+
// and the render would deliver a ConfigMap named
310+
// "solution-host-binding-My_Workspace.prod.crm", which is not a legal
311+
// Kubernetes object name. Nothing downstream refuses that: the tree
312+
// validates, the publish succeeds, and ArgoCD fails at sync.
313+
if !rfc1123Label.MatchString(part.value) {
314+
return "", fmt.Errorf(
315+
"solution host binding ID cannot be built from %s %q: the ID names a Kubernetes object, so each part must be lowercase letters, digits and dashes, starting and ending alphanumeric",
316+
part.label, part.value)
300317
}
301318
}
302319
id := strings.Join([]string{workspace, environment, instance}, ".")
@@ -538,6 +555,23 @@ func writeSolutionHostBindingKustomization(owned, environment string, names []st
538555
return nil
539556
}
540557

558+
// deliveredBindingPath is the render subdirectory the inventory records, derived
559+
// from what this render actually wrote.
560+
//
561+
// It is computed unconditionally, and that is the point. Setting the field only
562+
// when a binding was written leaves a previous render's value in place on a
563+
// reused RenderOptions, and the inventory then claims a delivery path the tree
564+
// does not have. Both readers of that field — generateArgoBootstrap and
565+
// snapshotAuthority — kustomize-build "<path>/overlays/<environment>", so a
566+
// stale value does not degrade to the old behaviour: it refuses the whole
567+
// publication with "build promotion source ...: must build at directory".
568+
func deliveredBindingPath(bindings []DeclaredSolutionHostBinding) string {
569+
if len(bindings) == 0 {
570+
return ""
571+
}
572+
return solutionHostBindingDir
573+
}
574+
541575
// undeclaredSolutions names the solution instances this render delivered
542576
// workloads for without declaring a binding, because the environment names no
543577
// host. An empty SolutionHostBindings list alone cannot say that: a composition

‎pkg/gitops/solution_host_binding_test.go‎

Lines changed: 95 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -558,10 +558,15 @@ func TestDeclaredBindingsReachArgo(t *testing.T) {
558558
if !strings.Contains(set, "overlay: "+overlay) {
559559
t.Fatalf("no Argo Application delivers the declared bindings:\n%s", set)
560560
}
561-
// Last: a binding pins the digests of artifacts this generation rendered,
562-
// so it is declared after they are healthy, never before.
561+
// With the module's own resources, before any unit. Argo starts a wave only
562+
// once the previous is healthy, so a later wave would withhold the
563+
// declaration of desired state from a solution whose workload is unhealthy
564+
// — exactly when a host needs it.
563565
if !strings.Contains(set, "wave: \""+solutionHostBindingWave+"\"") {
564-
t.Fatalf("the bindings do not land in the last wave:\n%s", set)
566+
t.Fatalf("the bindings do not land in the module-resources wave:\n%s", set)
567+
}
568+
if solutionHostBindingWave != moduleResourcesWave {
569+
t.Fatalf("declared presence is gated behind the units it describes (wave %q)", solutionHostBindingWave)
565570
}
566571
cluster, namespaced, err := snapshotAuthority(root, inventory, "prod")
567572
if err != nil {
@@ -704,3 +709,90 @@ func TestRenderingAnotherEnvironmentResetsTheGeneration(t *testing.T) {
704709
t.Fatalf("the render did not report the generation it declared: %+v", result.SolutionHostBindings)
705710
}
706711
}
712+
713+
// TestBindingIDRefusesAPartThatCannotNameAKubernetesObject is the guard whose
714+
// absence let an invalid ConfigMap name reach ArgoCD.
715+
//
716+
// core's bindingPattern deliberately admits uppercase and underscores so a ULID
717+
// or a UUID can be a binding ID, so core's own Validate accepts
718+
// "My_Workspace.prod.crm" — and the render would then deliver a ConfigMap named
719+
// "solution-host-binding-My_Workspace.prod.crm", which Kubernetes refuses as an
720+
// object name. Nothing downstream catches it: the tree validates and the
721+
// publish succeeds, so the first refusal is ArgoCD at sync, far from here.
722+
func TestBindingIDRefusesAPartThatCannotNameAKubernetesObject(t *testing.T) {
723+
for name, part := range map[string]struct{ workspace, environment, instance string }{
724+
"uppercase workspace": {"My_Workspace", "prod", "crm"},
725+
"capitalised": {"Obin", "prod", "crm"},
726+
"underscore": {"obin_prod", "prod", "crm"},
727+
"uppercase instance": {"obin", "prod", "CRM"},
728+
"leading dash": {"obin", "prod", "-crm"},
729+
"trailing dash": {"obin", "prod", "crm-"},
730+
"uppercase environment": {"obin", "Prod", "crm"},
731+
} {
732+
t.Run(name, func(t *testing.T) {
733+
id, err := bindingID(part.workspace, part.environment, part.instance)
734+
if err == nil {
735+
t.Fatalf("binding ID %q was accepted; it would name the ConfigMap %q", id, solutionHostBindingKind+"-"+id)
736+
}
737+
if !strings.Contains(err.Error(), "Kubernetes object") {
738+
t.Fatalf("refused for the wrong reason: %v", err)
739+
}
740+
})
741+
}
742+
if _, err := bindingID("obin", "prod", "crm-eu-1"); err != nil {
743+
t.Fatalf("a legal binding ID was refused: %v", err)
744+
}
745+
}
746+
747+
// TestRenderRefusesAWorkspaceThatCannotNameAKubernetesObject drives the same
748+
// refusal through the render, so the guard is exercised where it matters and
749+
// not only in the helper.
750+
func TestRenderRefusesAWorkspaceThatCannotNameAKubernetesObject(t *testing.T) {
751+
destination := filepath.Join(t.TempDir(), "modules", "crm")
752+
options := solutionRenderOptions(destination)
753+
options.Workspace = "My_Workspace"
754+
_, err := RenderOwnedTree(context.Background(), options, renderWorkload(pinnedDeployment))
755+
if err == nil || !strings.Contains(err.Error(), "Kubernetes object") {
756+
t.Fatalf("the render delivered a document ArgoCD would refuse: %v", err)
757+
}
758+
}
759+
760+
// TestARenderThatDeclaresNothingClearsTheDeliveryPath is the guard whose
761+
// absence refused an entire publication.
762+
//
763+
// The inventory field is what points publish at the binding overlay. Writing it
764+
// only when a binding was written leaves a previous render's value in place on
765+
// a reused RenderOptions, and both readers of the field kustomize-build
766+
// "<path>/overlays/<environment>" — so a stale value is not a cosmetic
767+
// inaccuracy, it fails generateArgoBootstrap and refuses the whole publication.
768+
func TestARenderThatDeclaresNothingClearsTheDeliveryPath(t *testing.T) {
769+
destination := filepath.Join(t.TempDir(), "modules", "crm")
770+
options := solutionRenderOptions(destination)
771+
first, err := RenderOwnedTree(context.Background(), options, renderWorkload(pinnedDeployment))
772+
if err != nil {
773+
t.Fatal(err)
774+
}
775+
if first.Inventory.SolutionHostBindingPath != solutionHostBindingDir {
776+
t.Fatalf("the first render recorded no delivery path: %q", first.Inventory.SolutionHostBindingPath)
777+
}
778+
// The operator removes the host declaration and re-renders with the same
779+
// options, as a caller that holds one render's options would.
780+
options.Host = nil
781+
second, err := RenderOwnedTree(context.Background(), options, renderWorkload(pinnedDeployment))
782+
if err != nil {
783+
t.Fatal(err)
784+
}
785+
if second.Inventory.SolutionHostBindingPath != "" {
786+
t.Fatalf("the inventory still claims %q; the tree has no such directory, and publish builds it",
787+
second.Inventory.SolutionHostBindingPath)
788+
}
789+
// The field and the tree must agree: the directory really is gone, so a
790+
// non-empty field would point publish at nothing. (What publish then does
791+
// with a path that is present is covered by TestDeclaredBindingsReachArgo.)
792+
if _, statErr := os.Stat(filepath.Join(destination, solutionHostBindingDir)); !os.IsNotExist(statErr) {
793+
t.Fatalf("the binding directory survived a render that declared none: %v", statErr)
794+
}
795+
if len(second.UndeclaredSolutions) != 1 || second.UndeclaredSolutions[0] != "crm" {
796+
t.Fatalf("the render did not report what it could not declare: %v", second.UndeclaredSolutions)
797+
}
798+
}

0 commit comments

Comments
 (0)