OCPBUGS-84652: Add InternalReleaseImage CA to OSImageStream#5892
OCPBUGS-84652: Add InternalReleaseImage CA to OSImageStream#5892bfournie wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@bfournie: This pull request references Jira Issue OCPBUGS-84652, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
WalkthroughWhen building a minimal controller config for OSImageStream, the operator checks that OSImageStream is enabled and the FeatureGateNoRegistryClusterInstall gate is set; it then attempts to read the InternalReleaseImage TLS secret and, if present with non-empty Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant FeatureGate
participant KubeAPI as Kubernetes API (Secret)
participant ControllerConfig
Operator->>FeatureGate: check FeatureGateNoRegistryClusterInstall
alt feature gate enabled and OSImageStream enabled
Operator->>KubeAPI: GET Secret `InternalReleaseImage` in ctrlcommon.MCONamespace
alt Secret found
KubeAPI-->>Operator: Secret data
Operator->>Operator: extract `tls.crt`
alt tls.crt present and non-empty
Operator->>ControllerConfig: set cc.Spec.RootCAData = tls.crt
else tls.crt missing/empty
Operator-->>Operator: log warning (do not set RootCAData)
end
else Secret not found
KubeAPI-->>Operator: NotFound
Operator-->>Operator: log warning (continue)
else other error
KubeAPI-->>Operator: error
Operator-->>Operator: fail build / return error
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bfournie 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bfournie: This pull request references Jira Issue OCPBUGS-84652, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
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. |
|
@bfournie: This pull request references Jira Issue OCPBUGS-84652, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/osimagestream_ocp.go (1)
298-298: Prefercorev1.TLSCertKeyover hard-coded"tls.crt".Using the Kubernetes constant avoids typo drift and keeps this aligned with TLS secret conventions.
Suggested refactor
- if tlsCert, ok := iriTLSSecret.Data["tls.crt"]; ok && len(tlsCert) > 0 { + if tlsCert, ok := iriTLSSecret.Data[corev1.TLSCertKey]; ok && len(tlsCert) > 0 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/osimagestream_ocp.go` at line 298, Replace the hard-coded "tls.crt" key with the Kubernetes constant corev1.TLSCertKey when accessing iriTLSSecret.Data (e.g., change iriTLSSecret.Data["tls.crt"] to iriTLSSecret.Data[corev1.TLSCertKey]); add the corev1 import if missing and ensure any references to the Data map use the constant to avoid typos and follow TLS secret conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/osimagestream_ocp.go`:
- Around line 291-293: The NotFound branch currently only logs a warning for the
InternalReleaseImage TLS secret; change it to fail fast when
FeatureGateNoRegistryClusterInstall is enabled. In the apierrors.IsNotFound(err)
block (the TLS secret handling for InternalReleaseImage in
osimagestream_ocp.go), check the FeatureGateNoRegistryClusterInstall feature
gate and, if enabled, return a descriptive error (instead of just klog.Warningf)
so the caller can stop; if the feature gate is not enabled, retain the current
warning behavior.
---
Nitpick comments:
In `@pkg/operator/osimagestream_ocp.go`:
- Line 298: Replace the hard-coded "tls.crt" key with the Kubernetes constant
corev1.TLSCertKey when accessing iriTLSSecret.Data (e.g., change
iriTLSSecret.Data["tls.crt"] to iriTLSSecret.Data[corev1.TLSCertKey]); add the
corev1 import if missing and ensure any references to the Data map use the
constant to avoid typos and follow TLS secret conventions.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c599b0d2-fc52-40dd-9722-2055be03b19d
📒 Files selected for processing (1)
pkg/operator/osimagestream_ocp.go
When the OSImageStream feature is enabled with the Agent-Based Installer and ISONoRegistry, MCO fails to build the OSImageStream with the error "unable to retrieve any OSImageStream from the configured sources". This occurs because MCO cannot trust the self-signed TLS certificates used by the InternalReleaseImage mirrored registries. This fix sets the RootCA used for OSImageStream to the InternalReleaseImage CA.
bfournie
force-pushed
the
osimagestream-cert
branch
from
e6e209c to
fce0501
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/osimagestream_ocp.go (1)
286-305: Add regression coverage for the new secret-handling branches.This block now decides whether OSImageStream can trust the mirrored registry during bootstrap. Please add focused tests for the three intended cases here: secret missing, secret present with empty
tls.crt, and secret present with a valid cert populatingRootCAData. That would lock in the warn-and-retry behavior discussed on this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/osimagestream_ocp.go` around lines 286 - 305, Add unit tests covering the three secret-handling branches in the OSImageStream code: when the InternalReleaseImage TLS secret is missing, when it exists but corev1.TLSCertKey is empty, and when it exists with a valid cert that sets cc.Spec.RootCAData. Write tests that exercise the branch guarded by osimagestream.IsFeatureEnabled(optr.fgHandler) && optr.fgHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) by faking optr.mcoSecretLister.Secrets(...).Get to return (a) an apierrors.IsNotFound error, (b) a Secret with Data[corev1.TLSCertKey] = []byte{} and (c) a Secret with Data[corev1.TLSCertKey] containing a sample cert; then assert the expected log/return behavior for cases (a) and (b) and that cc.Spec.RootCAData is populated for case (c) (use the function/logic around InternalReleaseImageTLSSecretName, iriTLSSecret, and cc.Spec.RootCAData to locate the code under test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/osimagestream_ocp.go`:
- Around line 286-305: Add unit tests covering the three secret-handling
branches in the OSImageStream code: when the InternalReleaseImage TLS secret is
missing, when it exists but corev1.TLSCertKey is empty, and when it exists with
a valid cert that sets cc.Spec.RootCAData. Write tests that exercise the branch
guarded by osimagestream.IsFeatureEnabled(optr.fgHandler) &&
optr.fgHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) by faking
optr.mcoSecretLister.Secrets(...).Get to return (a) an apierrors.IsNotFound
error, (b) a Secret with Data[corev1.TLSCertKey] = []byte{} and (c) a Secret
with Data[corev1.TLSCertKey] containing a sample cert; then assert the expected
log/return behavior for cases (a) and (b) and that cc.Spec.RootCAData is
populated for case (c) (use the function/logic around
InternalReleaseImageTLSSecretName, iriTLSSecret, and cc.Spec.RootCAData to
locate the code under test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9888875b-04a7-4aaf-b607-fbcfc67c84b0
📒 Files selected for processing (1)
pkg/operator/osimagestream_ocp.go
|
@bfournie: all tests passed! 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. |
| } else { | ||
| // Extract the TLS certificate from the secret | ||
| if tlsCert, ok := iriTLSSecret.Data[corev1.TLSCertKey]; ok && len(tlsCert) > 0 { | ||
| cc.Spec.RootCAData = tlsCert |
There was a problem hiding this comment.
I don't think this is correct, and moreover the clusterconfig is managed elsewhere
There was a problem hiding this comment.
Sorry I saw after it's the controllerconfig created just before. In any case RootCAData is meant to store the Root CA field, not the (leaf) server cert (which is valid only for the IRI registry)
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Closing as this is covered by #5896 |
|
@bfournie: Closed this PR. DetailsIn response to this:
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. |
|
@bfournie: This pull request references Jira Issue OCPBUGS-84652. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. DetailsIn response to this:
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. |
3 participants
When the OSImageStream feature is enabled with the Agent-Based Installer and ISONoRegistry, MCO fails to build the OSImageStream with the error "unable to retrieve any OSImageStream from the configured sources". This occurs because MCO cannot trust the self-signed TLS certificates used by the InternalReleaseImage mirrored registries.
This fix sets the RootCA used for OSImageStream to the InternalReleaseImage CA.
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit