-
Notifications
You must be signed in to change notification settings - Fork 143
add config cm and set safer cipherSuites for operator #449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughIntroduces a new ConfigMap with operator configuration and updates the Deployment to mount this ConfigMap and pass its path via a new --config flag to the operator container. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @lance5890. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lance5890 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
manifests/0000_10_config-operator_02_configmap.yaml (3)
16-22: Consider FIPS-mode clusters.CHACHA20-Poly1305 is non-FIPS; it’s fine to include, but in strict FIPS it will be ignored. If you want zero noise in FIPS, consider omitting the CHACHA entries or documenting the behavior.
Proposed alternative (AES-GCM only):
cipherSuites: - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 - TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 - - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 - - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
16-23: Optional: make cert/key explicit in config to avoid ambiguity.If the binary reads certs from config, set certFile/keyFile to the mounted service-serving-cert paths.
servingInfo: + certFile: /var/run/secrets/serving-cert/tls.crt + keyFile: /var/run/secrets/serving-cert/tls.key cipherSuites:
23-23: TLS 1.2 minimum is OK; consider TLS 1.3 if supported.If the operator and clients support TLS 1.3, you could raise the floor. Cipher list is ignored for TLS 1.3.
- minTLSVersion: VersionTLS12 + minTLSVersion: VersionTLS13manifests/0000_10_config-operator_07_deployment.yaml (2)
43-45: Fail fast on missing ConfigMap.Mark the volume as non-optional to avoid silently starting without config.
- name: config configMap: name: openshift-config-operator-config + optional: false
111-112: Mount config read-only.Tighten container FS perms; ConfigMaps don’t need write access.
- - mountPath: /var/run/configmaps/config - name: config + - mountPath: /var/run/configmaps/config + name: config + readOnly: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
manifests/0000_10_config-operator_02_configmap.yaml(1 hunks)manifests/0000_10_config-operator_07_deployment.yaml(3 hunks)
🔇 Additional comments (5)
manifests/0000_10_config-operator_02_configmap.yaml (3)
6-10: Annotations look correct for payload inclusion.No issues spotted with include.release.* annotations.
16-22: Good: CBC suites removed; modern cipher set only.List aligns with safer AES-GCM and CHACHA20-Poly1305 suites.
13-15: Confirm operator binary actually consumes GenericOperatorConfig via --config
- manifests/0000_10_config-operator_07_deployment.yaml passes --config=/var/run/configmaps/config/config.yaml and manifests/0000_10_config-operator_02_configmap.yaml contains apiVersion: operator.openshift.io/v1alpha1, kind: GenericOperatorConfig (servingInfo: minTLSVersion/cipherSuites).
- Repo search shows no local parsing of GenericOperatorConfig — verify controllercmd.NewControllerCommandConfig (github.com/openshift/library-go/pkg/controller/controllercmd) actually reads/parses that file and applies servingInfo; if it doesn't, the ConfigMap will be ineffective.
manifests/0000_10_config-operator_07_deployment.yaml (2)
43-45: ConfigMap volume wiring LGTM.Name/namespace match the ConfigMap; good.
83-83: Confirmed: binary accepts --config and library-go will load servingInfo.The operator builds its Cobra command via controllercmd.NewControllerCommandConfig(...).NewCommand() and thus inherits library-go's ControllerFlags/ControllerCommandConfig behavior (reads the provided config file and populates ControllerContext.Server/servingInfo) — so the manifest arg --config=/var/run/configmaps/config/config.yaml is supported. (pkg.go.dev)
Files checked: pkg/cmd/operator/cmd.go, manifests/0000_10_config-operator_07_deployment.yaml:83.
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
As the openshift-config-operator log shows it uses
insecure ciphersuites: