Document the allow-snippet-annotations prerequisite for the redirect chain

[henzigo]

Closes #1

What

Documents the configuration-snippet redirect prerequisite and makes the redirect style
configurable so locked-down controllers have a first-class fallback.

Why

The built-in redirect annotations (from-to-www-redirect, app-root) perform their redirect
before the HTTPS upgrade, producing an insecure http → http → https chain —
kubernetes/ingress-nginx#6340, unresolved and the ingress-nginx repository is archived. The
snippet is the only way to guarantee the https-first chain, but since ingress-nginx 1.9
snippets are disabled by default and enabling them is a controller-wide, Critical-risk
capability with multi-tenant trust implications.

Changes

• ingress.redirectStyle (new value): snippet (default — https-first chain, requires
  allow-snippet-annotations: true) or native (built-in from-to-www-redirect plus an
  explicit nginx.ingress.kubernetes.io/ssl-redirect, at the cost of the insecure www hop;
  the explicit counterpart-host rule is dropped in this mode because ingress-nginx omits the
  redirect server when the counterpart host has a rule; removes the redirect snippet only —
  security.cloudflare.enabled still emits a server-snippet)
• README requirement incl. the trust boundary warning (Ingress authors must be trusted;
  dedicated controller or restricted Ingress RBAC on shared clusters)
• Migration guide: self-contained "Cluster requirements" section with the same warning and
  the alternative
• Template comment explaining why the snippet must not be blindly "simplified"
• [proposal] Migration off archived ingress-nginx (Gateway API / successor) (investigation) #45 created as the long-term follow-up (migration off archived ingress-nginx)

Default rendering is unchanged (golden snapshots untouched); the native mode is covered by a
new unit test.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Documents ingress-nginx configuration required for secure HTTPS-first redirects.

Changes:

• Adds controller prerequisites to deployment documentation.
• Explains why snippet-based redirects must remain.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
README.md	Adds ingress-nginx requirements.
docs/migrating-from-shopsys-deployment.md	Adds cluster migration prerequisites.
charts/shopsys-app/templates/ingress-domains.yaml	Documents redirect rationale.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[henzigo]

Fair point — addressed in 5b9ff5c: the README requirement now states the trust boundary explicitly (controller-wide Critical-risk capability, Ingress authors must be trusted, dedicated controller / restricted Ingress RBAC on shared clusters). Additionally, the requirement is no longer unconditional: a new ingress.redirectStyle: native values option uses the built-in from-to-www-redirect for locked-down controllers — documented with its own caveat (the www redirect happens before the HTTPS upgrade, kubernetes/ingress-nginx#6340). The secure https-first snippet stays the default.

[henzigo]

Added in 5b9ff5c — the migration guide carries the same trust-boundary warning (controller-wide capability, upstream secret-exposure warning, dedicated controller / restricted RBAC recommendation) plus the ingress.redirectStyle: native alternative, so the guide is self-contained without the README.

[henzigo]

Follow-up created: #45 tracks the migration off archived ingress-nginx (Gateway API / successor investigation, incl. the feature-mapping constraints — https-first redirects per #6340 and basic auth with IP bypass). The migration guide now links it (5b9ff5c), so closing #1 via this PR no longer drops the long-term requirement.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (6)

docs/migrating-from-shopsys-deployment.md:58

• This says the migration is already tracked and directs readers to a named issue, but no link or issue reference is provided, while the linked requirement only calls for opening a separate issue when relevant. Link the actual tracking issue if one exists; otherwise describe this as a future investigation.

- Long-term: ingress-nginx is archived — the migration to Gateway API / a successor
  controller is tracked separately (see the "Gateway API migration" issue).

README.md:40

• native removes only the redirect's configuration-snippet. When security.cloudflare.enabled is true, this template still emits a server-snippet (lines 43–45), which is also rejected when snippet annotations are disabled. Qualify this fallback so operators do not switch styles and still encounter admission failures.

  a dedicated controller or tightly restricted Ingress RBAC, or switch to
  `ingress.redirectStyle: native` (built-in `from-to-www-redirect`; no snippets needed, but
  the www redirect happens before the HTTPS upgrade — an insecure `http → http → https` hop)

docs/migrating-from-shopsys-deployment.md:56

• This alternative does not eliminate the controller prerequisite when Cloudflare handling is enabled: ingress-domains.yaml still adds nginx.ingress.kubernetes.io/server-snippet, and allow-snippet-annotations: false rejects all *-snippet annotations. State that native only removes the redirect snippet and that other snippet-producing features must be disabled.

  Alternatively set `ingress.redirectStyle: native` to use the built-in
  `from-to-www-redirect` (no snippets required, at the cost of an insecure
  `http → http → https` hop for the www redirect).

charts/shopsys-app/values.yaml:33

• native is not sufficient by itself for a locked-down controller. With security.cloudflare.enabled, the chart still renders a server-snippet, so admission still requires snippet annotations to be enabled. Describe this as removing the redirect snippet rather than generally working on locked-down controllers.

  #   native - the built-in from-to-www-redirect annotation; works on locked-down
  #     controllers, but the www redirect happens BEFORE the HTTPS upgrade, producing an
  #     insecure http->http->https hop (kubernetes/ingress-nginx#6340)

charts/shopsys-app/templates/ingress-domains.yaml:25

• Calling native a fallback for locked-down controllers is too broad because this same manifest can still emit the Cloudflare server-snippet below. Clarify that it only removes the redirect snippet; otherwise maintainers may assume the complete Ingress is accepted with snippets disabled.

           It requires allow-snippet-annotations: true on the controller; the "native"
           style is the fallback for locked-down controllers. */}}

charts/shopsys-app/templates/ingress-domains.yaml:27

• The PR description lists this template change as comment-only with no rendered-output change, but this branch introduces a new user-facing rendering mode (along with a schema value and tests). Default snapshots remain unchanged, yet selecting native materially changes the Ingress. Update the PR description/change list so reviewers and release notes capture this feature.

    {{- if eq $.Values.ingress.redirectStyle "native" }}
    nginx.ingress.kubernetes.io/from-to-www-redirect: "true"

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/migrating-from-shopsys-deployment.md:59

• This references a follow-up “Gateway API migration” issue but doesn’t link it explicitly (and the PR description says the migration guide links #45). Using the issue number makes the doc self-contained and ensures GitHub autolinks the tracker.

- Long-term: ingress-nginx is archived — the migration to Gateway API / a successor
  controller is tracked separately (see the "Gateway API migration" issue).

[henzigo]

Review summary

Reviewed the full diff (README, ingress-domains.yaml template, values + schema, unit test, docs/values.md, migration doc) plus the surrounding files (infra RabbitMQ ingress, MCP ingress, deviations list, project skills) for context.

What I verified

• helm unittest charts/shopsys-app charts/shopsys-infra — 60/60 pass (after helm dependency build).
• ./tests/run-golden-tests.sh — 15/15 pass without snapshot updates, confirming the PR's claim that default rendering is unchanged.
• helm lint charts/shopsys-app -f .../required.yaml — clean.
• helm template --set ingress.redirectStyle=native renders the expected single-rule ingress with from-to-www-redirect and both hosts kept in spec.tls (required by upstream: the certificate must cover both hostnames); --set ingress.redirectStyle=bogus is rejected by the schema with a clear enum error.
• Upstream claims checked against ingress-nginx docs/issues: allow-snippet-annotations defaults to false since v1.9 (post CVE-2021-25742 hardening) and the admission webhook rejects snippet-bearing ingresses; in v1.12 the default annotations-risk-level was lowered from Critical to High while configuration-snippet is classified Critical, so both settings are indeed required on ≥ 1.12 — the README/migration wording is accurate. kubernetes/ingress-nginx#6340 is open/unresolved and the repository was archived (read-only since 2026-03-24), as stated. The http → http → https chain for from-to-www-redirect is confirmed by kubernetes/ingress-nginx#10024/#2043 (the annotation preserves the scheme; ssl-redirect fires afterwards).
• Completeness: the requirement is correctly scoped to the e-shop domain ingresses — the RabbitMQ management ingress and the MCP ingress use no snippet annotations. README, values.yaml comment, values.schema.json, docs/values.md, the migration doc's new "Cluster requirements" section and the template comment are all consistent; follow-up issue #45 exists. The new unit test follows the suite conventions (required.yaml baseline).

Findings (severity-ranked)

1. Medium — in native mode nothing in the rendered Ingress enforces http→https for the base host: the only ssl-redirect annotation uses the ingress.kubernetes.io/ prefix, which ingress-nginx does not read, so the https upgrade silently relies on the controller's global ssl-redirect: true ConfigMap default (inline comment on the template).
2. Low — kubernetes/ingress-nginx#6340 is specifically about app-root; #10024 is the precise citation for the from-to-www-redirect chain (inline comment on values.yaml).
3. Nit — the counterpart-rule template comment says the explicit rule "would conflict" with the generated server; upstream actually omits the redirect in that case, so removal is necessary, not just conflict-avoidance (inline comment).
4. Nit — consider naming CVE-2021-25742 explicitly in the README warning for searchability (inline comment).

No blocking issues: the documented behavior is technically accurate, the template change is correct and covered by a unit test, and the default rendering is provably unchanged. Nice touch keeping both hosts in spec.tls in native mode — that is exactly what from-to-www-redirect needs for the TLS counterpart server.

— Claude Agent (CR)

[henzigo]

Medium: In native mode the rendered Ingress no longer contains anything that enforces the http→https upgrade for the base host. The snippet mode carries its own if ($scheme = http) { return 308 https://... }, but here the only ssl-redirect annotation on the resource is the legacy-prefixed ingress.kubernetes.io/ssl-redirect: "true" (line 17), which ingress-nginx does not parse — it only reads its configured annotation prefix (nginx.ingress.kubernetes.io/). Native mode therefore works only because the controller's global ConfigMap default ssl-redirect: true applies to TLS-bearing ingresses; on a controller where the admin has set ssl-redirect: "false" globally, the site is served over plain http with no redirect at all.

Since native exists precisely for locked-down/shared controllers whose defaults you don't control, consider emitting nginx.ingress.kubernetes.io/ssl-redirect: "true" in this branch (it is a low-risk annotation, allowed even without snippets), or at least documenting the reliance on the controller default in the README/values comment. I know line 17 itself is pre-existing legacy parity — the point is that this new mode is the first configuration whose https enforcement depends on it (or rather, on the controller default).

— Claude Agent (CR)

[henzigo]

Accepted — fixed in f774765. Native mode now emits nginx.ingress.kubernetes.io/ssl-redirect: "true" alongside from-to-www-redirect. You're right that the legacy-prefixed annotation on line 17 is not parsed by ingress-nginx and that native mode would otherwise depend on the controller's global ssl-redirect ConfigMap default — the one thing this mode exists to be independent of. It's a low-risk annotation accepted without snippets, so it doesn't undermine the locked-down-controller use case. The native-mode unit test now asserts it; line 17 itself stays untouched (legacy parity), and default snippet rendering is unchanged (goldens pass without update). Also added a template comment explaining why the explicit annotation is required in this branch.

[henzigo]

Low (citation precision): kubernetes/ingress-nginx#6340 is specifically about app-root redirecting before the SSL redirect. The same behavior for from-to-www-redirect — the annotation preserves the request scheme, producing exactly the http → http → https chain described here — is tracked in kubernetes/ingress-nginx#10024 (and #2043). Since this comment (and the README/migration doc) attribute the hop to from-to-www-redirect, citing #10024 alongside or instead of #6340 would let a reader land on the directly relevant issue. Applies to the same reference in README.md, docs/values.md and the migration doc.

— Claude Agent (CR)

[henzigo]

Accepted — fixed in f774765. Verified #10024 describes exactly this chain (http://example.com → http://www.example.com → https://www.example.com). The template comment, values.yaml, README, docs/values.md and the migration doc now cite #10024 for the from-to-www-redirect hop; #6340 is kept where the claim covers app-root (README/template comment name both with their respective annotations).

[henzigo]

Nit (comment accuracy): Dropping the counterpart rule here is correct — but the reason is slightly different from "would conflict". Per the upstream annotation docs, when the counterpart host is already defined as a rule (in this or any other Ingress), ingress-nginx omits the from-to-www-redirect server entirely, i.e. the redirect would be silently dropped rather than conflicting. So removing the rule is not just conflict-avoidance, it is required for the native redirect to exist at all. Worth sharpening the comment so a future "simplification" doesn't reintroduce the rule thinking the worst case is a benign duplicate.

— Claude Agent (CR)

[henzigo]

Accepted — fixed in f774765. The comment now states that ingress-nginx omits the from-to-www-redirect server entirely when the counterpart host is defined as a rule (in any Ingress), i.e. dropping the explicit rule is required for the redirect to exist at all, not just conflict-avoidance — precisely to stop a future "simplification" from silently killing the redirect.

[henzigo]

Nit: The warning is accurate and appropriately prominent (verified against the upstream annotations-risk docs and the v1.12 default change from Critical to High). For searchability, consider naming CVE-2021-25742 explicitly — it is the incident that made snippets opt-out (secret/token exfiltration via snippet annotations) and the identifier cluster admins will look for when assessing whether to enable allow-snippet-annotations + annotations-risk-level: Critical.

— Claude Agent (CR)

[henzigo]

Accepted — fixed in f774765. The README warning now names CVE-2021-25742 as the incident that made snippets opt-out, so admins evaluating allow-snippet-annotations + annotations-risk-level: Critical can find the exact advisory.

[henzigo]

Addressing Copilot's suppressed comments (from the review bodies of the 2nd and 4th review rounds) in f774765:

• native is not fully snippet-free with Cloudflare enabled (raised 4× against README, migration doc, values.yaml and the template comment) — valid. With security.cloudflare.enabled the ingresses still carry a nginx.ingress.kubernetes.io/server-snippet, which a controller without allow-snippet-annotations rejects just the same. README, the migration guide's Cluster requirements section, the values.yaml comment and the template comment now all describe native as removing the redirect snippet and call out the Cloudflare server-snippet caveat explicitly.
• Link the Gateway API follow-up issue explicitly — done; the migration doc now links shopsys/helm#45 instead of naming the issue by title.
• PR description listing the template change as comment-only — the description was already rewritten in the previous round to present ingress.redirectStyle as a user-facing feature (values + schema + unit test, unchanged default rendering), so no further action.