Skip to content

HYPERFLEET-1057 - fix: Update sentinel release name to match e2e name#56

Open
ma-hill wants to merge 1 commit into
openshift-hyperfleet:mainfrom
ma-hill:HYPERFLEET-1057-3
Open

HYPERFLEET-1057 - fix: Update sentinel release name to match e2e name#56
ma-hill wants to merge 1 commit into
openshift-hyperfleet:mainfrom
ma-hill:HYPERFLEET-1057-3

Conversation

@ma-hill

@ma-hill ma-hill commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Update helmfile.yaml.gotmpl to match e2e testing of sentinel names, based on what's defined here:
https://github.com/openshift-hyperfleet/hyperfleet-e2e/blob/main/pkg/helper/constants.go#L14

Test Plan

  • Run E2E tier 0, tier 1 and tier2 tests

Notes:

@openshift-ci openshift-ci Bot requested review from crizzo71 and jsell-rh June 16, 2026 21:53
@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tirthct for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

In helmfile/helmfile.yaml.gotmpl, the Helm release name field generated for each entry in .Values.sentinels is changed from {{ .name }} to sentinel-{{ .name }}. This prefixes every sentinel release name with the literal string sentinel-.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes


Security note — CWE-807 (Reliance on Untrusted Inputs in a Security Decision):

.Values.sentinels[*].name is still fully caller-controlled. The sentinel- prefix does not sanitize the value. If .name is sourced from an untrusted values file or an external values override (--set, --values), an attacker who controls that input can still produce arbitrary release names such as sentinel-../../../something or inject characters that affect Helm's release scoping. Validate and allowlist .name values at the values-injection boundary, not at template render time.

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: prefixing sentinel release names with 'sentinel-' to align with e2e testing conventions.
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.
Sec-02: Secrets In Log Output ✅ Passed No log statements (slog, logr, zap, fmt.Print*) found in modified helmfile.yaml.gotmpl or related configuration files. Change is a Helm template name prefix update with no logging code.
No Hardcoded Secrets ✅ Passed The helmfile.yaml.gotmpl change prefixes sentinel release names with "sentinel-" (line 84: - name: sentinel-{{ .name }}). No hardcoded secrets, API keys, tokens, passwords, private keys, credenti...
No Weak Cryptography ✅ Passed PR changes only Helmfile template syntax (adding "sentinel-" prefix to release names). No cryptographic code, imports, or operations present.
No Injection Vectors ✅ Passed Change applies hardcoded string prefix to sentinel name from static YAML config. No user-controlled input, template injection, or injection vectors present (CWE-78/79/89/502).
No Privileged Containers ✅ Passed PR modifies only the sentinel release name prefix in helmfile.yaml.gotmpl. No privileged containers, elevated capabilities, or privilege escalation configurations present in any Helm templates or v...
No Pii Or Sensitive Data In Logs ✅ Passed helmfile.yaml.gotmpl is a configuration file with no logging statements; no slog, logr, zap, log, or fmt.Print* calls present to expose PII/sensitive data.
Description check ✅ Passed The PR description directly addresses the changeset by referencing the specific file being modified (helmfile.yaml.gotmpl) and explains the naming convention alignment with e2e testing constants.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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


{{ range .Values.sentinels }}
- name: {{ .name }}
- name: sentinel-{{ .name }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @ma-hill,
I was thinking about potential impact on (production) environments, if any, that already have used this helmfile.
Is the thinking that we are instructing end-users/partners to destroy the existing sentinels anyway when upgrading to 1.0.0? (openshift-hyperfleet/architecture#157).
So that is why we don't care about this renaming leaving the old resources orphaned and creating new ones?


{{ range .Values.sentinels }}
- name: {{ .name }}
- name: sentinel-{{ .name }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tip

nit — non-blocking suggestion

Category: Pattern

This renames the Helm release for all environments (e2e + kind + gcp), which means existing deployments will get orphaned releases under the old names. Consider adding a note under [Unreleased] in CHANGELOG.md:

### Changed

- Sentinel Helm release names now use `sentinel-` prefix (`sentinel-clusters`, `sentinel-nodepools`) to match e2e naming conventions — existing releases under the old names must be manually removed

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