Skip to content

feat(helm): add global.ingress defaults shared across charts#129

Merged
bebosudo merged 6 commits intochiusole/improve-handling-and-description-of-platform-service-address-portfrom
feat/global-ingress-defaults
May 5, 2026
Merged

feat(helm): add global.ingress defaults shared across charts#129
bebosudo merged 6 commits intochiusole/improve-handling-and-description-of-platform-service-address-portfrom
feat/global-ingress-defaults

Conversation

@gavinelder
Copy link
Copy Markdown
Contributor

@gavinelder gavinelder commented Apr 30, 2026

Enhancement

Allow global configuration of Ingress options across the platform parent chart and all subcharts via a new global.ingress block. Set once at the parent (enabled, path, defaultPathType, ingressClassName, annotations, extraLabels, tls) and every subchart's Ingress picks it up — no need to repeat controller-wide config per subchart.

Why a global block

In practice, users who expose one of these services via Ingress almost always expose them all the same way: same ingress controller, same class name, same annotations (cert-manager, ALB scheme, NGINX body-size limits, etc.). The previous shape forced the same values to be repeated under platform.ingress.*, studios.ingress.*, wave.ingress.*, and so on — verbose, drift-prone, and easy to get inconsistent across subcharts.

Lifting these controller-wide concerns into global.ingress matches the way users actually configure ingress: one decision per cluster, applied uniformly. Per-chart overrides remain available for the rare case where one service genuinely needs different routing.

Why the Prefix default change

With the previous defaults (defaultPathType: ImplementationSpecific, path: "/"), the same chart and values produced different routing behavior depending on the customer's ingress controller:

  • NGINX treats path: / + ImplementationSpecific as a prefix match
  • AWS ALB treats it as an exact match and needs /* for the same effect
  • GKE applies its own controller-specific interpretation

That's an unpredictable user experience: the chart "works" on one cluster and silently routes differently on another. Switching the default to Prefix is spec-defined and produces consistent prefix-match semantics across NGINX, Traefik, AWS ALB, and most modern controllers. Customers can still override per-controller when needed.

Summary

  • Introduce global.ingress block (enabled, path, defaultPathType, ingressClassName) that the platform parent and every subchart's Ingress falls back to. Set once at the parent and every chart picks it up.
  • BREAKING: default defaultPathType is now Prefix (was ImplementationSpecific). ALB users on the old default can set global.ingress.defaultPathType: ImplementationSpecific once at the parent.
  • Adds a seqera.ingress.host template helper in each chart's _helpers.tpl returning that chart's primary domain. Set it once in global.ingress.annotations and the right hostname renders per chart:
    global:
      ingress:
        annotations:
          external-dns.alpha.kubernetes.io/hostname: '{{ include "seqera.ingress.host" . }}'
  • Adds docs/conventions/ingress.md documenting the Ingress conventions for this repo.
  • Updates examples/ingress-configurations/* to drop redundant defaultPathType: Prefix overrides and showcase global.ingress.ingressClassName in nginx-cert-manager.yaml.

Resolution semantics

Field Local default Global default Resolution
enabled false false OR — either being true enables the Ingress
path "" "/" Local wins when set; otherwise global
defaultPathType "" "Prefix" Local wins when set; otherwise global
ingressClassName "" "" Local wins when set; otherwise global
annotations {} {} Merged; local wins on key collision
extraLabels {} {} Merged; local wins on key collision
tls [] [] Concatenated (supports a single wildcard cert across charts)

Version bumps

  • platform 0.32.3 → 0.33.0
  • studios 1.2.11 → 1.3.0
  • portal-web 0.2.5 → 0.3.0
  • mcp 0.3.2 → 0.4.0
  • wave 0.1.0 → 0.2.0
  • agent-backend 0.4.7 → 0.5.0

Tests

  • All 1279 helm-unittest tests pass (snapshots refreshed for the new default pathType).
  • New tests cover: rendering when only global.ingress.enabled is true; falling back to global.ingress.defaultPathType when local is empty; local defaultPathType overriding global.

Test plan

  • Confirm version bumps are correct (or adjust before merge)
  • Confirm Prefix default is acceptable as a breaking change, or restore ImplementationSpecific and only ship the global plumbing
  • Sanity-check rendered output with each example file: helm template platform charts/platform -f charts/platform/examples/ingress-configurations/<file>
  • Review docs/conventions/ingress.md content/wording

🤖 Generated with Claude Code

@gavinelder gavinelder force-pushed the feat/global-ingress-defaults branch from b07f919 to dc43914 Compare April 30, 2026 13:57
@gavinelder gavinelder changed the title feat(ingress): add global.ingress defaults shared across charts feat(helm): add global.ingress defaults shared across charts Apr 30, 2026
@gavinelder gavinelder force-pushed the feat/global-ingress-defaults branch 2 times, most recently from f8721c7 to b477b56 Compare April 30, 2026 14:36
@gwright99
Copy link
Copy Markdown
Member

FWIW, I would still have to do some minor bespoke annotations in the individual Ingresses still (e.g externaldns entry, unique group numbering since my Ingresses share an ALB for cost savings).

In general, however, I like the idea of being more DRY so there are fewer ways I can shoot myself in the foot as I'm updating various charts 👍

@gavinelder
Copy link
Copy Markdown
Contributor Author

gavinelder commented Apr 30, 2026

@gwright99

For external DNS we have the following

based on your example chart usage the new value would be the following at the top level.

(This is in the examples)

global:
  ingress:
    enabled: true
    ingressClassName: alb
    annotations:
      alb.ingress.kubernetes.io/group.name: cluster-alb
      alb.ingress.kubernetes.io/listen-ports: '[{"HTTP": 80}, {"HTTPS":443}]'
      alb.ingress.kubernetes.io/ssl-redirect: "443"
      external-dns.alpha.kubernetes.io/hostname: '{{ include "seqera.ingress.host" . }}'

If you wish to specific annotations at a deployment level these can still be merged.

wave:
  ingress:
    annotations:
	  alb.ingress.kubernetes.io/group.order: 1 

@gavinelder gavinelder marked this pull request as ready for review April 30, 2026 15:05
@gavinelder gavinelder requested a review from a team as a code owner April 30, 2026 15:05
Copy link
Copy Markdown
Member

@bebosudo bebosudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice addition, only one minor comment

Comment thread charts/platform/templates/_helpers.tpl Outdated
gavinelder and others added 3 commits April 30, 2026 17:10
Introduce a `global.ingress` block (`enabled`, `path`, `defaultPathType`,
`ingressClassName`) that the platform parent and every subchart's Ingress
template falls back to. Setting these once at the parent level propagates to
studios, portal-web, mcp, wave, and agent-backend so users don't have to
repeat controller-wide config per subchart.

Resolution semantics:
- `enabled`: OR — either local or global being `true` enables the Ingress
- `path`, `defaultPathType`, `ingressClassName`: local wins when set,
  otherwise falls back to the global

BREAKING: default `pathType` is now `Prefix` (was `ImplementationSpecific`).
`Prefix` works for nginx, traefik, AWS ALB, and most modern controllers. ALB
users who relied on the old default may need to set
`global.ingress.defaultPathType: ImplementationSpecific`.

Also:
- Add docs/conventions/ingress.md documenting the pattern
- Update examples/ingress-configurations/* — drop redundant
  `defaultPathType: Prefix` lines, showcase `global.ingress.ingressClassName`
  in nginx-cert-manager.yaml, fix the README's Path Types section
- Bump platform 0.32.3→0.33.0, studios 1.2.11→1.3.0, portal-web 0.2.5→0.3.0,
  mcp 0.3.2→0.4.0, wave 0.1.0→0.2.0, agent-backend 0.4.7→0.5.0
- Scope helm-docs pre-commit hook to charts/ and exclude .claude/worktrees so
  agent worktrees don't cause spurious doc regeneration

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@gavinelder gavinelder force-pushed the feat/global-ingress-defaults branch from b477b56 to 6615a59 Compare April 30, 2026 16:12
@bebosudo bebosudo changed the base branch from master to chiusole/improve-handling-and-description-of-platform-service-address-port May 4, 2026 14:13
…ddress-port into feat/global-ingress-defaults
Copy link
Copy Markdown
Member

@bebosudo bebosudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chart versions were updated in the changelog files and readme files, but weren't updated in the chart.yaml files, which automatically rebuilds the readme files upon commit with helm-docs, so changing the readmes was reverted in my latest merge commit - could you make sure prek is installed in your local repo?

Once versions are upgraded, could you run make rebuild-deps-force too to update the Chart.lock file in charts/platform with the new subchart versions?

@bebosudo bebosudo merged commit 90b2851 into chiusole/improve-handling-and-description-of-platform-service-address-port May 5, 2026
2 checks passed
@bebosudo bebosudo deleted the feat/global-ingress-defaults branch May 5, 2026 12:32
bebosudo added a commit that referenced this pull request May 5, 2026
…s to be provided (#131)

* Update all platform subcharts to require platformServiceAddress to be provided

* chore: improve wait-for-redis initcont no-auth-set message (#133)

* chore: improve wait-for-redis initcont no-auth-set message

* Use more explicit check for password set/unset

* feat(helm): add global.ingress defaults shared across charts (#129)

* feat(ingress): add global.ingress defaults shared across charts

Introduce a `global.ingress` block (`enabled`, `path`, `defaultPathType`,
`ingressClassName`) that the platform parent and every subchart's Ingress
template falls back to. Setting these once at the parent level propagates to
studios, portal-web, mcp, wave, and agent-backend so users don't have to
repeat controller-wide config per subchart.

Resolution semantics:
- `enabled`: OR — either local or global being `true` enables the Ingress
- `path`, `defaultPathType`, `ingressClassName`: local wins when set,
  otherwise falls back to the global

BREAKING: default `pathType` is now `Prefix` (was `ImplementationSpecific`).
`Prefix` works for nginx, traefik, AWS ALB, and most modern controllers. ALB
users who relied on the old default may need to set
`global.ingress.defaultPathType: ImplementationSpecific`.

Also:
- Add docs/conventions/ingress.md documenting the pattern
- Update examples/ingress-configurations/* — drop redundant
  `defaultPathType: Prefix` lines, showcase `global.ingress.ingressClassName`
  in nginx-cert-manager.yaml, fix the README's Path Types section
- Bump platform 0.32.3→0.33.0, studios 1.2.11→1.3.0, portal-web 0.2.5→0.3.0,
  mcp 0.3.2→0.4.0, wave 0.1.0→0.2.0, agent-backend 0.4.7→0.5.0
- Scope helm-docs pre-commit hook to charts/ and exclude .claude/worktrees so
  agent worktrees don't cause spurious doc regeneration

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

* fix(ingress): rewrite seqera.ingress.host comment to avoid helmfmt stripping indentation

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* chore: update snapshots after rebase onto master

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* Merge chiusole/improve-handling-and-description-of-platform-service-address-port into feat/global-ingress-defaults

* Bump subcharts minor version for new global.ingress feature, fix changelogs

---------

Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Co-authored-by: Alberto Chiusole <[email protected]>

* feat(ci): run subchart unit tests standalone in addition to parent (#132)

spike(ci): run subchart unit tests standalone in addition to parent

Adds two new make targets to charts/platform/Makefile:

- `unittests-standalone`: invokes `helm unittest` from inside each subchart
  directory (charts/*/  with a tests/ dir) so each renders with its own
  values/helpers — matching how a customer would install it on its own.
- `test-all`: runs both the existing parent-recursive `unittests` AND
  `unittests-standalone`, catching helper/values divergence that the
  parent-only run silently masks.

Wires `test-all` into `.github/scripts/run_chart_tests.py` so CI exercises
both paths. The script falls back to the legacy `tests` target for charts
that haven't adopted `test-all` yet.

Spike findings on running `make -C charts/platform unittests-standalone`
against current master:

- `wave` and `agent-backend` standalone tests fail with a snapshot mismatch
  in the wait-for-redis init container script. These pass in parent-context
  because parent and subcharts ship different versions of `seqera-common`.
  Pre-existing latent divergence — exactly the gap this spike surfaces.

This is intentionally a spike — pre-commit was bypassed to push the draft
PR for review. The standalone failures aren't fixed here; they demonstrate
the value of the per-chart standalone runner and need triage in a follow-up.

Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Co-authored-by: Alberto Chiusole <[email protected]>

* Default to testing both umbrella platform chart, then its subcharts standalone

---------

Co-authored-by: Gavin <[email protected]>
Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants