Skip to content

test(bdd): fix secure multi-region coverage - #1322

Merged
sbaum1994 merged 2 commits into
test/bdd-1292-secure-multiregionfrom
mcamp/fix/pr1310-coderabbit
Aug 28, 2026
Merged

test(bdd): fix secure multi-region coverage#1322
sbaum1994 merged 2 commits into
test/bdd-1292-secure-multiregionfrom
mcamp/fix/pr1310-coderabbit

Conversation

@mikeyrcamp

@mikeyrcamp mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Address the unresolved review findings on #1310 so the secure multi-region BDD scenario validates the real certificate input, canonical router identities, and cluster-local watch alias wiring.

Additional Details

  • Configure the Region B watch and headless SANs through addons.llm.pki.dnsNames, which is the stack field consumed by the request-router certificate.
  • Derive the two expected Region B StatefulSet identities from the grpcurl response and reject dashed pod-IP aliases.
  • Apply the region-b-watch Service and Endpoints alias in both the control-plane and compute contexts, with a non-live behavioral test for both manifests.
  • Suppress only revive's line-length rule around exact feature-command constants, without wrapping or otherwise changing those command values.

Secure multi-region registration: before

flowchart LR
  A["Region B name only in addons.llm.pki.dnsNames"] --> B["QUIC and Stargate certificate SANs"]
  C["Region B name missing from requestRouter.grpcTls.dnsNames"] --> D["Port 50071 Gateway certificate lacks the SAN"]
  B -. "Does not populate the Gateway certificate" .-> D
  E["Ready pods and EndpointSlice targets"] --> F["HTTPS Watch attempt"]
  D --> F
  F --> G["Hostname verification fails"]
Loading

Secure multi-region registration: after

flowchart LR
  A["Region B name in requestRouter.grpcTls.dnsNames"] --> B["Gateway gRPC TLS SANs for port 50071"]
  C["Region B name in addons.llm.pki.dnsNames"] --> D["Region B QUIC and Stargate identity covered"]
  B --> E["Exact Gateway Certificate dnsNames readback"]
  E --> F["HTTPS Watch with verified Region B hostname"]
  G["Ready pods and EndpointSlice targets"] --> F
  F --> H["Exactly two canonical pod identities"]
  H --> I["Dashed IP aliases rejected"]
  D --> J["Both TLS identities cover Region B"]
  I --> J
Loading

This Pull Request is stacked on #1310 and targets its feature branch.

Integration order

The current #1310 base predates merged #1300 and does not yet contain the chart template that consumes addons.llm.requestRouter.grpcTls.dnsNames. The safe integration and validation order is:

  1. Incorporate this companion into test(bdd): cover secure multi-region Pylon registration #1310.
  2. Rebase test(bdd): cover secure multi-region Pylon registration #1310 onto current main, including feat(self-managed): secure LLM gRPC ingress #1300.
  3. Run the live multi-region BDD against that combined stack.

Until those steps are complete, the new Gateway SAN input and Certificate readback are not an effective live validation path. No final live pass is claimed here.

For the Reviewer

Please focus on the Region B identity assertion and the two-context alias application in tests/bdd/scripts/install-llm-region-b.sh.

For QA

Validated on the development VM:

  • go test -short -count=1 ./...
  • focused multi-region wiring and alias behavior tests
  • go vet ./...
  • golangci-lint v2.12.2 with the configured revive rules: 0 issues
  • bash -n tests/bdd/scripts/install-llm-region-b.sh
  • git diff --check

The configured full lint command still reports two pre-existing staticcheck capitalization findings in tests/bdd/dsl/manifests.go; this Pull Request does not modify that file. A live destructive k3d run was not performed for this review-only companion.

Issues

Relates to #1307

Dependencies

No new or updated third-party dependencies. No license or NOTICE changes are required.

Checklist

  • I am familiar with the Contributing Guidelines.
  • I have signed off my commits for Developer Certificate of Origin (DCO) compliance.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Relates to #1307

Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp
mikeyrcamp requested a review from a team as a code owner August 28, 2026 18:41
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c314be46-c090-48b7-921c-ace7b70181a0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Independent review of the companion diff found no Critical, Important, or Minor issues.

The change addresses all four reported classes: certificate SAN configuration, canonical router identity validation with dashed-IP alias rejection, equivalent watch-alias wiring in both cluster contexts, and narrowly scoped revive line-length handling.

Validation passed for the focused wiring/alias tests, the short Go suite, go vet, configured revive lint, shell syntax, and diff hygiene. The live multi-region BDD run remains a separate gate.

Relates to #1307

Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Live BDD follow-up: the first multi-region run reached the Region B HTTPS Watch request and exposed a missing hostname on the port-50071 Gateway Certificate. Commit 26070673 adds region-b-watch.nvcf.svc.cluster.local to addons.llm.requestRouter.grpcTls.dnsNames[1], while retaining the separate QUIC SANs under addons.llm.pki.dnsNames.

The feature now reads the issued Gateway Certificate and requires the Region B SAN before it attempts Watch. Focused wiring, the full short BDD suite, go vet, configured revive lint, and diff hygiene are green. The final live rerun is pending.

@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Integration note from independent review: #1322's current #1310 base predates merged #1300, so it does not yet include the chart template that consumes addons.llm.requestRouter.grpcTls.dnsNames.

Safe order: incorporate #1322 into #1310, rebase #1310 onto current main containing #1300, and then run the live multi-region BDD against the combined stack. Until then, the Gateway SAN input and Certificate readback are not an effective live validation path; no final live pass is claimed.

@mikeyrcamp

mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Independent review of current head 2607067 found no Critical, Important, or Minor issues.

Focused Gateway-SAN wiring/readback checks, multi-region alias and identity tests, the short BDD suite, Go vet, configured revive lint, shell syntax, and diff hygiene passed. The documented SAN precheck is not a final live pass.

CodeRabbit skipped review because this PR targets a stacked, non-default base. Readiness remains gated on incorporating #1322 into #1310, rebasing #1310 onto current main with #1300, and rerunning the live multi-region BDD. No merge was performed.

@sbaum1994
sbaum1994 merged commit 9beff1a into test/bdd-1292-secure-multiregion Aug 28, 2026
3 checks passed
@sbaum1994
sbaum1994 deleted the mcamp/fix/pr1310-coderabbit branch August 28, 2026 20:15
@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Final sanitized live acceptance result for a validation tree combining exact #1321 head 40bdda431fe25ee1e54f2cc8aec302d322ff0cb8, exact #1322 head 26070673a0e2e819ec2f5161a91ae24b5dc55ace, and current main containing #1300:

  • Passed 2/2 scenarios and 63/63 steps in 955.96 seconds.
  • Verified the certificate chain and hostname, ALPN h2, Region B SAN/readback, and HTTPS Watch.
  • Observed 3 canonical Deployment identities and 2 canonical StatefulSet identities through the explicit HTTPS remote seed, with no dashed-IP aliases.
  • Observed registration=5, reverse QUIC=3, backend health and authentication, and a successful authenticated chat.completion with the fixed response and no no_eligible_candidates result.
  • Pylon 0.14.1 used digest prefix sha256:9befb43d30d5660ac; Stargate 0.14.1 used sha256:97c621e4d8c65c636. Both were pinned promoted artifacts, not local sideloads.

Task cleanup is complete and zero task-owned clusters remain. #1322 has been merged into its stacked parent #1310, not into main. #1310 still needs its documented main rebase and normal review, and its pending traceparent-metadata CodeRabbit Minor must be resolved: #1310 (comment). No merge was performed in this publication phase.

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.

2 participants