fix(security): read FIPS state from HostedCluster on HyperShift#31288
Open
mgencur wants to merge 1 commit into
Open
fix(security): read FIPS state from HostedCluster on HyperShift#31288mgencur wants to merge 1 commit into
mgencur wants to merge 1 commit into
Conversation
On HyperShift hosted clusters, the install-config ConfigMap in kube-system does not reflect the actual FIPS configuration. Instead, FIPS is set via .spec.fips on the HostedCluster resource in the management cluster. Add helpers to discover the HostedCluster and read its FIPS field via the management cluster API. When the management cluster env vars are not set, the test is skipped. Remove unused installConfigName const. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
WalkthroughThis PR extends FIPS security testing in OpenShift to support HyperShift-managed external control planes. It adds discovery helpers to query hosted cluster FIPS settings from a management cluster, and refactors the TestFIPS test to conditionally use those helpers when the control plane topology is external. ChangesHyperShift FIPS External Topology Support
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/security/fips.go (1)
61-68: ⚡ Quick winConsider validating parsed Kubernetes resource names.
The parsed
nsandnamevalues from jsonpath output are used in a subsequent CLI call (line 86-87) without validation. While these should be valid DNS labels from the Kubernetes API, defense-in-depth suggests validating the format before use.🛡️ Add validation for extracted names
for _, line := range strings.Split(strings.TrimSpace(output), "\n") { parts := strings.SplitN(line, ",", 2) if len(parts) == 2 { ns, name := parts[0], parts[1] + // Validate DNS label format (alphanumeric, '-', '.', max 63 chars) + if ns == "" || name == "" || len(ns) > 63 || len(name) > 63 { + continue + } if ns+"-"+name == hcpNS { return name, ns, nil } } }🤖 Prompt for 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. In `@test/extended/security/fips.go` around lines 61 - 68, The parsed ns and name extracted from the jsonpath output are used directly; validate they are valid Kubernetes DNS-1123 labels before using or returning them. Inside the loop that processes parts (the variables ns and name), add a check using the Kubernetes DNS-1123 label rules (e.g., k8s.io/apimachinery/pkg/util/validation.IsDNS1123Label or equivalent regex: lowercase alphanumerics and '-' only, start/end with alphanumeric, max 63 chars) and skip any entries that fail validation; only perform the ns+"-"+name comparison and return name, ns, nil when both ns and name pass validation.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/extended/security/fips.go`:
- Around line 61-68: The parsed ns and name extracted from the jsonpath output
are used directly; validate they are valid Kubernetes DNS-1123 labels before
using or returning them. Inside the loop that processes parts (the variables ns
and name), add a check using the Kubernetes DNS-1123 label rules (e.g.,
k8s.io/apimachinery/pkg/util/validation.IsDNS1123Label or equivalent regex:
lowercase alphanumerics and '-' only, start/end with alphanumeric, max 63 chars)
and skip any entries that fail validation; only perform the ns+"-"+name
comparison and return name, ns, nil when both ns and name pass validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4f2e9246-39f0-4302-9390-b259eb275cdf
📒 Files selected for processing (1)
test/extended/security/fips.go
openshift-ci
Bot
added
the
ready-for-human-review
Contributor
|
Scheduling required tests: |
Author
|
/unhold Locally it's passing for me with these env variables set and hosted cluster being FIPS-compliant: Log: |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
|
@mgencur: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On HyperShift hosted clusters, the install-config ConfigMap in kube-system does not reflect the actual FIPS configuration. Instead, FIPS is set via .spec.fips on the HostedCluster resource in the management cluster.
Add helpers to discover the HostedCluster and read its FIPS field via the management cluster API. When the management cluster env vars are not set, the test is skipped. Remove unused installConfigName const.
Summary by CodeRabbit