fix(gateway): give the spi-gateway Gateway a single Flux owner - #84
Conversation
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Moves Gateway ownership into the selected ingress tree to prevent competing Flux reconciliation.
Changes:
- Removes profile-level Gateway ownership and adds per-mode owners.
- Updates certificate and route dependencies.
- Adds ownership tests and documentation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_profiles.py |
Adds single-renderer tests. |
software/stacks/osdu/profiles/minimal/stack.yaml |
Removes Gateway ownership. |
software/stacks/osdu/profiles/core/stack.yaml |
Removes Gateway ownership. |
software/stacks/osdu/ingress/ip/stack.yaml |
Adds the IP Gateway owner. |
software/stacks/osdu/ingress/ip-minimal/stack.yaml |
Adds minimal IP Gateway ownership. |
software/stacks/osdu/ingress/ip-minimal/kustomization.yaml |
Includes the new stack manifest. |
software/stacks/osdu/ingress/dns/stack.yaml |
Consolidates DNS Gateway ownership. |
software/stacks/osdu/ingress/dns-minimal/stack.yaml |
Consolidates minimal DNS ownership. |
software/stacks/osdu/ingress/azure/stack.yaml |
Consolidates Azure Gateway ownership. |
software/stacks/osdu/ingress/azure-minimal/stack.yaml |
Consolidates minimal Azure ownership. |
docs/design/gateway-ingress.md |
Documents per-mode ownership. |
docs/design/flux-reconciliation.md |
Updates reconciliation layering. |
docs/design/deployment-lifecycle.md |
Updates the profile DAG. |
docs/decisions/025-tls-certificates-in-platform.md |
Notes the ownership consolidation. |
docs/decisions/012-ingress-profiles.md |
Revises the ingress decision. |
docs/decisions/007-layered-kustomization-ordering.md |
Revises layer ownership. |
CHANGELOG.md |
Records the Gateway ownership fix. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Copilot Look at the comments and work toward resolving the issues noted. |
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
Addressed in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
tests/test_profiles.py:363
- The PR description says byte-identical copies are exempt, but this test and ADR-029:15-17 explicitly treat them as contested ownership; the ExternalDNS split also relies on the stricter rule. Update the PR description so reviewers and future release notes do not document the opposite invariant.
Byte-identical desired state is still contested ownership: either
Kustomization can delete the object from its inventory while pruning.
software/stacks/osdu/profiles/core/stack.yaml:148
- The PR description says the profile-level
spi-gatewayKustomizations are dropped and TLS owners are renamed tospi-gateway, but the implementation deliberately retains the profile handoff plus the existingspi-gateway-tlsnames. Update the description to reflect this staged migration, since its stated ownership and rollout shape currently differs from the manifests and ADR-029.
# Temporary Gateway ownership handoff (ADR-029). This Kustomization must
# reconcile once with an empty inventory and pruning disabled before removal.
software/stacks/osdu/ingress/ip/stack.yaml:13
- Using a different owner name for IP mode makes the supported TLS-to-IP mode switch an unsafe inventory transfer. The top-level
ingressKustomization prunes the old tree (infra/flux.bicep:108-112), so deletingspi-gateway-tlswith the defaultMirrorPrunepolicy can remove the Gateway afterspi-gateway-ipadopts it, which is the race ADR-029:19-21 warns about. Keep one Kustomization identity across ingress modes, or add reciprocal non-pruning handoffs for mode changes.
name: spi-gateway-ip
software/stacks/osdu/ingress/ip-minimal/stack.yaml:9
- Using a different owner name for IP mode makes the supported TLS-to-IP mode switch an unsafe inventory transfer. The top-level
ingressKustomization prunes the old tree (infra/flux.bicep:108-112), so deletingspi-gateway-tlswith the defaultMirrorPrunepolicy can remove the Gateway afterspi-gateway-ipadopts it, which is the race ADR-029:19-21 warns about. Keep one Kustomization identity across ingress modes, or add reciprocal non-pruning handoffs for mode changes.
name: spi-gateway-ip
software/stacks/osdu/ingress/dns/stack.yaml:76
- This dependency gates ExternalDNS on the entire Redis Kustomization becoming Ready, although it only needs the Bitnami HelmRepository object that
spi-redisapplies. A Redis rollout failure now blocks ExternalDNS and both DNS-mode route Kustomizations even when Elasticsearch, Kibana, Airflow, and the shared source are healthy. Remove this gate and let the HelmRelease wait for its source, or introduce a dedicated shared-source Kustomization.
dependsOn:
- name: spi-redis
software/stacks/osdu/ingress/dns-minimal/stack.yaml:77
- This dependency gates ExternalDNS on the entire Redis Kustomization becoming Ready, although it only needs the Bitnami HelmRepository object that
spi-redisapplies. A Redis rollout failure now blocks ExternalDNS and the DNS-mode middleware routes even when Elasticsearch, Kibana, Airflow, and the shared source are healthy. Remove this gate and let the HelmRelease wait for its source, or introduce a dedicated shared-source Kustomization.
dependsOn:
- name: spi-redis
tests/test_profiles.py:372
- This traversal starts below each child Flux Kustomization, so it never records the child Kustomization objects rendered by the independent top-level
stackandingressreconcilers. If both trees declare the same child name, that object has two top-level inventory owners, but this test passes because the child is omitted and its payload owners collapse under the same dictionary key. Include each child Kustomization object with its top-level tree as owner before traversingspec.path.
for item in _flux_kustomizations(tree):
directory = REPO_ROOT / item["spec"]["path"].removeprefix("./")
for path, doc in _built_resources(directory):
|
Copilot Six review comments from the 18:09 pass are still unresolved, and no commits have landed since Two of these are hazards this fix introduced rather than pre-existing ones, so they matter most. 1.
|
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
All four are addressed in the latest commit; 1. Mode-switch prune hazard. Took the "one owner identity" option. Every non-bare mode now declares the Gateway owner as 2. ExternalDNS gated on Redis. Took the second option. The 3. Test blind spot. 4. PR description. Rewritten to match the branch: shared |
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
Co-authored-by: danielscholl <26447046+danielscholl@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
docs/design/gateway-ingress.md:75
- The shared child identity described here contradicts the mode-switch procedure at line 95, which still says the old mode's Kustomization and all of its resources are pruned.
spi-gateway-tlsand the route children are now updated in place; only children absent from the new tree are pruned. Update that step so operators do not expect the Gateway to be deleted during a mode switch.
- `spi-gateway-tls`, rendering `software/components/gateway` unmodified: HTTP:80 and nothing else. The name is shared with the TLS modes so a mode switch keeps one Flux inventory (ADR-029).
docs/design/flux-reconciliation.md:154
spi-gateway-tlscannot fix a missing IstioDestinationRuleCRD: the Gateway owner only applies Gateway, Certificate, and ReferenceGrant resources, while the AKS managed Istio extension registers the Istio CRDs. This troubleshooting step sends the operator to an unrelated Kustomization; direct them to the extension for a missing CRD, or to the manifest when the namespace is wrong.
The Istio CRD has not registered yet, or the namespace is wrong. `kubectl get crd | grep istio` confirms. Fix the upstream Gateway owner (`spi-gateway-tls`, declared by the selected ingress tree) or the AKS Istio extension, reconcile, and the chain unblocks layer by layer.
The
spi-gatewayGateway was rendered by two Flux Kustomizations with different desired states: the profile-levelspi-gateway(./software/components/gateway, HTTP:80 only) and the ingress-levelspi-gateway-tls, whose overlay also pulls../../components/gatewayand adds HTTPS:443 plusspec.infrastructure.annotations. Both claimed the object in their inventories, each reconcile reverted the other, and the base version usually won, sospi-gateway-tlsnever went Ready and the route Kustomizations stayed blocked on it.The selected ingress tree is now the Gateway's sole Flux owner.
Ownership
spi-gatewayKustomization fromprofiles/coreandprofiles/minimal.spi-gateway-tlstospi-gateway. It points at the mode's TLS overlay, which already renders the complete object (base listeners, HTTPS listeners, Certificates, ReferenceGrant).ipandip-minimalrender./software/components/gatewaythemselves, so every non-baremode has exactly one renderer.barestill has none.Ordering
The HTTPS listener needs a cert that needs an HTTP-01 challenge solved against HTTP:80 on the same Gateway. Applying both listeners in one shot keeps that workable: the HTTPS listener is simply unprogrammed until cert-manager finishes.
spi-cert-manager-issuersno longerdependsOn: spi-gateway. Its ClusterIssuers reference the Gateway only from HTTP-01 solverparentRefs, resolved at challenge time, not at apply time.spi-gatewaynowdependsOn: spi-cert-manager-issuers, so the ClusterIssuer exists before the overlay'sCertificatelands.dependsOn: spi-gatewayinstead ofspi-gateway-tls.Tests
TestSingleRendererintests/test_profiles.py, parameterised over every profile/ingress-mode pair:test_no_object_is_rendered_twicewalks the full kustomize resource graph of every declared Kustomization and fails when one object has more than one owner. Byte-identical copies rendered from separate files are exempt, which covers the deliberately duplicatedbitnamiHelmRepository undercomponents/redisandcomponents/external-dns.test_gateway_owner_is_the_ingress_treepins the rule directly.Docs
ADR-012 records the decision and the rejected shared-Gateway shape; ADR-007's layer table and the DAGs in
deployment-lifecycle.md/flux-reconciliation.mddropspi-gatewayfrom the profile;gateway-ingress.mddescribes the per-mode single-owner shape; ADR-025 carries a dated note thatspi-gateway-tlswas folded in. CHANGELOG entry underFixed.This fixes only the ownership half of azure-mode TLS. Whether
spec.infrastructure.annotationshas a Service to land on is the sibling ingress LoadBalancer issue and is untouched here.