OSAC-1628: Make externalHostname and internalHostname mandatory

[jhernand]

Summary

• Make externalHostname and internalHostname required Helm values instead of falling back to
  auto-generated Kubernetes service hostnames. The defaults only worked in integration tests and
  cannot work in real deployments because TLS certificates must include the correct host names.
• Replace fallback logic with Helm's required function for immediate, clear failures.
• Remove conditionals guarding the internal API Route/TLSRoute since the value is always present.

Test plan

• Verify helm template fails with a clear error when either hostname is omitted.
• Verify helm template succeeds when both hostnames are provided.
• Run integration tests (ginkgo run it) to confirm they still pass (they already set both values explicitly).

Related: https://redhat.atlassian.net/browse/OSAC-1628

Summary by CodeRabbit

• Documentation

  • Updated configuration guidance to state that both external and internal hostname values must be correct to generate TLS certificates.
  • Added clearer “note on hostnames” messaging and emphasized network isolation as defense-in-depth for internal APIs.

• Chores

  • External and internal hostname settings are now required at deployment time.
  • Internal and public routing/certificate generation now consistently use the configured hostname values across variants.

• Tests

  • Updated manifest rendering tests to include both hostname values.

[openshift-ci-robot]

@jhernand: This pull request references OSAC-1628 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

• Make externalHostname and internalHostname required Helm values instead of falling back to
  auto-generated Kubernetes service hostnames. The defaults only worked in integration tests and
  cannot work in real deployments because TLS certificates must include the correct host names.
• Replace fallback logic with Helm's required function for immediate, clear failures.
• Remove conditionals guarding the internal API Route/TLSRoute since the value is always present.

Test plan

• Verify helm template fails with a clear error when either hostname is omitted.
• Verify helm template succeeds when both hostnames are provided.
• Run integration tests (ginkgo run it) to confirm they still pass (they already set both values explicitly).

Related: https://redhat.atlassian.net/browse/OSAC-1628

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 80ab0488-f8e7-42be-aa02-ca53846d1929

📥 Commits

Reviewing files that changed from the base of the PR and between c0d2bcf and 694ee01.

📒 Files selected for processing (6)

• charts/service/README.md
• charts/service/templates/_helpers.tpl
• charts/service/templates/ingress-proxy/certificate.yaml
• charts/service/templates/route.yaml
• charts/service/values.yaml
• internal/auth/auth_rules_test.go

---

Walkthrough

Both externalHostname and internalHostname Helm values are now mandatory. Fallback cluster-DNS defaults are removed from hostname helpers via required; tokenIssuerUrl port logic simplifies to a single variant check; internalHostname-gated conditionals are removed from route and certificate templates; and all documentation and tests are updated to enforce this requirement.

Changes

Hostname Required Enforcement

Layer / File(s)	Summary
Hostname helpers made required; tokenIssuerUrl simplified charts/service/templates/_helpers.tpl	fulfillment-api.hostname and fulfillment-internal-api.hostname now use required to fail at render time when values are absent. tokenIssuerUrl port selection is simplified to check only variant == "openshift", dropping the prior conditional dependency on whether externalHostname was set.
Certificate and Route templates consume required helpers unconditionally charts/service/templates/ingress-proxy/certificate.yaml, charts/service/templates/route.yaml	dnsNames in certificate.yaml switches from conditional .Values.* lookups to direct include calls. Both the OpenShift Route and kind TLSRoute for fulfillment-internal-api in route.yaml are rendered without the former .Values.internalHostname guard.
values.yaml, README, and test setup updated for required hostnames charts/service/values.yaml, charts/service/README.md, internal/auth/auth_rules_test.go	Inline comments reframe both hostname fields around TLS certificate correctness and internal-network scope; README marks both fields Required in the config table. Test setup injects both hostname values into the generated manifest to reflect the new mandatory requirement.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• CrystalChun

Poem

🔒 No more optional hosts left behind,
required now guards what callers must mind.
The Route and TLSRoute spring forth every time,
And cert DNS names flow in logical rhyme.
Both hostnames demanded — no fallback, no fuss! 🎉

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: making two Helm values mandatory rather than optional, which is precisely what the changeset implements across documentation, templates, and tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets found. All mentions of 'secret', 'password', 'token' in modified files are configuration parameters, comments, or external Kubernetes references. Test values use clearly marked...
No-Weak-Crypto	✅ Passed	PR makes Helm configuration values required and updates documentation; contains no cryptographic code, weak algorithms, or secret comparisons.
No-Injection-Vectors	✅ Passed	No injection vectors found. Changes are Helm configuration and test updates; no SQL concat, shell injection, unsafe eval/exec, unsafe YAML loading, or other dangerous patterns detected.
Container-Privileges	✅ Passed	PR does not introduce privileged containers, hostPID/Network/IPC, SYS_ADMIN capability, root user, or allowPrivilegeEscalation:true. New container specs include proper hardening: allowPrivilegeEsca...
No-Sensitive-Data-In-Logs	✅ Passed	No logging of sensitive data. PR only uses hostnames in Helm template resource specs (Certificate, Routes) and test setup, not in any logging/print statements.
Ai-Attribution	✅ Passed	AI tool (Cursor) was used and properly attributed with "Assisted-by: Cursor" trailer in the commit message, following Red Hat attribution guidelines.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@charts/service/README.md`:
- Around line 30-31: In the service configuration table in the README.md file,
update the description for the `internalHostname` parameter to clarify that it
specifically refers to the internal API service rather than being ambiguous
about which APIs it accesses. Change the current description "Hostname used to
access both the public and private APIs (see note below)" to "Hostname used to
access the internal API (see note below)" to directly align with the actual
route template naming (fulfillment-internal-api) and eliminate confusion about
service routing. This makes the documentation consistent with the
externalHostname description pattern and the actual architecture implementation.

In `@charts/service/values.yaml`:
- Around line 39-45: The documentation comment for `internalHostname` contains
contradictory language where line 39-40 states the hostname is used to access
APIs "from outside the cluster" while lines 41-45 correctly clarify it should be
restricted to the administrator's internal network. Reword the opening
description (lines 39-40) to directly state this is the hostname used to access
the internal API endpoint and remove the misleading phrase "from outside the
cluster" to align with the network-restriction guidance in the subsequent lines.
Keep the clarification in lines 42-45 about authentication, authorization, and
the recommendation for network-level access restrictions as it correctly
describes the intended usage.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 683fbb95-e877-494d-914b-c535f1af4f22

📥 Commits

Reviewing files that changed from the base of the PR and between d6ce803 and a75bca4.

📒 Files selected for processing (5)

• charts/service/README.md
• charts/service/templates/_helpers.tpl
• charts/service/templates/ingress-proxy/certificate.yaml
• charts/service/templates/route.yaml
• charts/service/values.yaml

[ygalblum]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, jhernand, ygalblum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [adriengentil,jhernand,ygalblum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ygalblum]

/retest

[adriengentil]

/retest

[openshift-ci]

@jhernand: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-vmaas	694ee01	link	unknown	/test e2e-vmaas

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.