IR-350: add tls security profile configuration for the image registry operator - #8011
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ConfigMap named Sequence Diagram(s)sequenceDiagram
participant HCP as HostedControlPlane
participant Adapter as adaptControllerConfig
participant CM as ConfigMap (image-registry-controller-config)
participant K8s as Kubernetes API
participant Operator as cluster-image-registry-operator
HCP->>Adapter: provide WorkloadContext (TLS profile)
Adapter->>CM: populate/modify Data["config.yaml"] (servingInfo, TLS settings)
Adapter-->>HCP: return success/error
CM->>K8s: apply ConfigMap manifest
K8s->>Operator: mount ConfigMap into Pod (/var/run/configmaps/...)
K8s->>Operator: start container with --config and --files args
Operator->>CM: read mounted config.yaml at startup
Operator-->>K8s: serve using provided servingInfo/TLS settings
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
6a2b484 to
ecb7bea
Compare
|
/retest |
ricardomaraschini
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ricardomaraschini: This pull request references IR-350 which is a valid jira issue. 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. |
| return fmt.Errorf("failed to json unmarshal config: %w", err) | ||
| } | ||
|
|
||
| asMap["apiVersion"] = configv1.GroupVersion.String() |
There was a problem hiding this comment.
Oh, configv1.GenericControllerConfig does not define metav1.TypeMeta field. Compared to GenericOperatorConfig.
There was a problem hiding this comment.
You are right. I guess we could simply not set it out then.
There was a problem hiding this comment.
It needs to be set so the CVO knows which kind it is. Otherwise, it's a blind guess. Also, no field to validate the kind and version against.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, ricardomaraschini The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
ecb7bea to
9ba1a81
Compare
|
@ricardomaraschini: This pull request references IR-350 which is a valid jira issue. 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.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/configmap_test.go (1)
124-141: Type assertions without checks could improve debuggability.The type assertions on lines 124 and 137 will panic if the YAML structure is unexpected, which is acceptable for test code (panic = test failure). However, using Gomega's type-safe matchers could provide clearer failure messages.
♻️ Optional: Use Gomega type-safe extraction for clearer failure messages
- servingInfo := controllerConfig["servingInfo"].(map[string]any) - g.Expect(servingInfo).To(HaveKeyWithValue("bindAddress", ":60000")) + g.Expect(controllerConfig).To(HaveKey("servingInfo")) + servingInfo, ok := controllerConfig["servingInfo"].(map[string]any) + g.Expect(ok).To(BeTrue(), "servingInfo should be a map") + g.Expect(servingInfo).To(HaveKeyWithValue("bindAddress", ":60000"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/configmap_test.go` around lines 124 - 141, Replace unsafe direct type assertions on controllerConfig["servingInfo"] and servingInfo["cipherSuites"] with Gomega type-safe expectations: first assert the presence and type of "servingInfo" (e.g., g.Expect(controllerConfig).To(HaveKey("servingInfo")); g.Expect(controllerConfig["servingInfo"]).To(BeAssignableToTypeOf(map[string]any{}))) then safely cast to map[string]any; similarly assert the presence and type of "cipherSuites" on the servingInfo map (e.g., g.Expect(servingInfo).To(HaveKey("cipherSuites")); g.Expect(servingInfo["cipherSuites"]).To(BeAssignableToTypeOf([]any{}))) before converting to []any and then to []string so failures produce clear Gomega messages rather than panics in the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/configmap_test.go`:
- Around line 124-141: Replace unsafe direct type assertions on
controllerConfig["servingInfo"] and servingInfo["cipherSuites"] with Gomega
type-safe expectations: first assert the presence and type of "servingInfo"
(e.g., g.Expect(controllerConfig).To(HaveKey("servingInfo"));
g.Expect(controllerConfig["servingInfo"]).To(BeAssignableToTypeOf(map[string]any{})))
then safely cast to map[string]any; similarly assert the presence and type of
"cipherSuites" on the servingInfo map (e.g.,
g.Expect(servingInfo).To(HaveKey("cipherSuites"));
g.Expect(servingInfo["cipherSuites"]).To(BeAssignableToTypeOf([]any{}))) before
converting to []any and then to []string so failures produce clear Gomega
messages rather than panics in the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: da592756-f282-4662-b716-f3719be2ae7f
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/AROSwift/zz_fixture_TestControlPlaneComponents_image_registry_controller_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/GCP/zz_fixture_TestControlPlaneComponents_image_registry_controller_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_image_registry_controller_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_image_registry_controller_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/zz_fixture_TestControlPlaneComponents_cluster_image_registry_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-image-registry-operator/zz_fixture_TestControlPlaneComponents_image_registry_controller_config_configmap.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-image-registry-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-image-registry-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/configmap.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/configmap_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-image-registry-operator/controller-config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/component.go
- control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/configmap.go
|
/lgtm |
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 2
|
|
/verify @gangwgr |
|
/verified by @gangwgr |
openshift-ci-robot
added
the
verified
|
@gangwgr: This PR has been marked as verified by 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test e2e-aks |
|
will unhold pr when pre-merge manual testing completed |
|
/retest |
openshift-ci-robot
removed
the
verified
|
@ricardomaraschini: This pull request references IR-350 which is a valid jira issue. 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. |
we have changed the image registry deployment and added a new config map to the equation. we need to regenerate the testdata to incorporate these changes.
15ad845 to
d4c717a
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by @gangwgr |
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
the
verified
|
@gangwgr: This PR has been marked as verified by 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. |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-aks |
|
/test e2e-azure-self-managed |
|
/retest |
|
@ricardomaraschini: 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. |
7 participants
What this PR does / why we need it:
Configure the
cluster-image-registry-operatorto use the TLS security profile settings from the HostedCluster resource. This ensures the operator's serving endpoint uses ciphers and minimum TLS version that match the cluster's security requirements.Implementation:
configv1.GenericControllerConfigwith TLS settings derived fromhcp.spec.configuration.apiServer.tlsSecurityProfile.--configflag on the operator deployment.config.CipherSuites()andconfig.minTLSVersion()helper functions for consistency with other control plane components.Special notes for your reviewer:
This PR is expected to fail as it depends on openshift/cluster-image-registry-operator#1297 being merged.
Checklist:
Summary by CodeRabbit