OCPBUGS-61038: SSL Medium Strength Cipher Suites Supported for kube-apiserver-check-endpoints #1913
Conversation
openshift-ci-robot
added
jira/severity-important
|
@lance5890: This pull request references Jira Issue OCPBUGS-61038, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
openshift-ci-robot
added
the
jira/invalid-bug
WalkthroughAdds a kube-apiserver check-endpoints ConfigMap with static TLS settings, updates the kube-apiserver static-pod to pass that config to the check-endpoints container, registers the ConfigMap in revision-controlled maps, and applies it during target-config reconciliation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TCC as TargetConfigController
participant Assets as bindata/assets
participant K8s as Kubernetes API
participant Pod as kube-apiserver static pod
Note over TCC: reconcile target config
TCC->>Assets: Read check-endpoints-config-cm.yaml
TCC->>K8s: Apply ConfigMap kube-apiserver-check-endpoints-config
K8s-->>TCC: Created/Updated or error
rect rgba(220,245,255,0.4)
Note right of Pod: On static-pod start
Pod->>Pod: check-endpoints container invoked with\n`--config /etc/kubernetes/static-pod-resources/configmaps/kube-apiserver-check-endpoints-config/config.yaml`
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
needs-ok-to-test
|
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. 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. |
|
/cc @vrutkovs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/operator/starter.go (1)
635-637: Minor: clarify comment; it’s the check-endpoints TLS config.The comment mentions “operator-config” which is misleading here.
Apply:
- // kube-apiserver-operator-config (TLS cipherSuites/minTLSVersion for check-endpoints). + // kube-apiserver-check-endpoints-config (TLS cipherSuites/minTLSVersion for check-endpoints).pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
222-225: Typo in function name; keep naming consistent.Rename manageKubeAPICheckEndpointdConfig → manageKubeAPICheckEndpointsConfig.
Apply:
- _, _, err = manageKubeAPICheckEndpointdConfig(ctx, c.kubeClient.CoreV1(), recorder, operatorSpec) + _, _, err = manageKubeAPICheckEndpointsConfig(ctx, c.kubeClient.CoreV1(), recorder, operatorSpec)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml(1 hunks)bindata/assets/kube-apiserver/pod.yaml(1 hunks)pkg/operator/starter.go(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (3)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceapply/core.go (1)
ApplyConfigMap(83-85)
🔇 Additional comments (2)
bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml (1)
11-18: Verify AES_128 GCM suites against your security policy
No in-repo docs mandate dropping 128-bit GCM. Confirm whether your scanner flags TLS_ECDHE_*_AES_128_GCM_SHA256 as medium strength and requires only AES_256_GCM/CHACHA20; if so, remove the AES_128 entries:@@ bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml Lines 11-18 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 minTLSVersion: VersionTLS12bindata/assets/kube-apiserver/pod.yaml (1)
242-243: Verify check-endpoints supports --config / GenericOperatorConfig
pkg/cmd/checkendpoints/cmd.go relies on library-go’s ControllerCommandConfig to register flags—no explicit --config definition is present here. Manually confirm the built CLI exposes a --config flag and unmarshals its file into a GenericOperatorConfig schema.
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
c0ec1c2 to
dcc1cf1
Compare
lance5890
changed the title
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
dcc1cf1 to
7b75728
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/operator/starter.go (1)
635-637: Fix misleading comment text.The comment mentions “kube-apiserver-operator-config” but the actual resource is kube-apiserver-check-endpoints-config. Align the comment to avoid confusion.
- // kube-apiserver-operator-config (TLS cipherSuites/minTLSVersion for check-endpoints). + // kube-apiserver-check-endpoints-config: TLS cipherSuites/minTLSVersion for check-endpoints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml(1 hunks)bindata/assets/kube-apiserver/pod.yaml(1 hunks)pkg/operator/starter.go(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- bindata/assets/kube-apiserver/pod.yaml
- pkg/operator/targetconfigcontroller/targetconfigcontroller.go
🔇 Additional comments (2)
pkg/operator/starter.go (2)
635-637: LGTM: adding the revisioned CM for check-endpoints TLS config is correct.Including kube-apiserver-check-endpoints-config in RevisionConfigMaps keeps it roll-forward/rollback aware with the rest of the static-pod inputs. Order remains intact, with kube-apiserver-pod first.
635-637: Cross-file wiring verified
CM uses theconfig.yamlkey and the pod’s--config …/configmaps/kube-apiserver-check-endpoints-config/config.yamlmatches it;manageKubeAPICheckEndpointsConfigin the targetconfigcontroller applies this CM.
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
7b75728 to
347c516
Compare
pkg/operator/starter.go
Outdated
| // optional configmap containing the OIDC structured auth config | ||
| {Name: auth.AuthConfigCMName, Optional: true}, | ||
| // kube-apiserver-check-endpoints-config: TLS cipherSuites/minTLSVersion for check-endpoints. | ||
| {Name: "kube-apiserver-check-endpoints-config"}, |
There was a problem hiding this comment.
compared with #1911, this PR use RevisionConfigMaps, I think this way is more compliant with the origin logic
|
/ok-to-test /cc @sanchezl |
openshift-ci
bot
added
ok-to-test
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
347c516 to
1ed850d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
244-247: Placement: ensure the configmap exists before revision/pod reconciliationConsider moving this call earlier (right after
manageKubeAPIServerConfigand beforemanagePods) to avoid a window where the revision controller or pod template references a not-yet-created configmap. Please confirm bootstrap/upgrade paths don’t go degraded when this CM is missing briefly.Run to verify wiring across the repo:
#!/bin/bash set -euo pipefail # 1) Asset basics file="$(fd -a 'check-endpoints-config-cm.yaml' bindata | head -n1)" echo "Asset file: $file" rg -nC2 -e '^kind:\s*ConfigMap' -e '^metadata:' -e 'name:\s*kube-apiserver-check-endpoints-config' -e 'namespace:\s*openshift-kube-apiserver' "$file" # 2) Ensure data key is config.yaml rg -nC2 -e '^data:' -e '^\s*config\.yaml:' "$file" # 3) Ensure revision controller includes this CM rg -nC3 -e 'RevisionConfigMaps' -e 'kube-apiserver-check-endpoints-config' pkg/operator/starter.go # 4) Ensure pod uses the config path for check-endpoints rg -nC3 -e 'kube-apiserver-check-endpoints' -e '--config' -e 'kube-apiserver-check-endpoints-config/config\.yaml' bindata/assets/kube-apiserver/pod\.yaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml(1 hunks)bindata/assets/kube-apiserver/pod.yaml(1 hunks)pkg/operator/starter.go(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml
- bindata/assets/kube-apiserver/pod.yaml
- pkg/operator/starter.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (4)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)pkg/operator/operatorclient/interfaces.go (1)
TargetNamespace(7-7)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceapply/core.go (1)
ApplyConfigMap(83-85)
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
1ed850d to
f439451
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
310-318: Fix compile error and simplify: use ApplyConfigMap directly; normalize namespaceThe helper currently won’t compile (
return _, _, err) and redundantly performs a Get. Replace with a straightApplyConfigMapand force the namespace toTargetNamespaceto guard against asset drift.-func manageKubeAPICheckEndpointsConfig(ctx context.Context, client coreclientv1.ConfigMapsGetter, recorder events.Recorder) (*corev1.ConfigMap, bool, error) { - configMap := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/check-endpoints-config-cm.yaml")) - cmCLient := client.ConfigMaps(operatorclient.TargetNamespace) - _, err := cmCLient.Get(ctx, "kube-apiserver-check-endpoints-config", metav1.GetOptions{}) - if err != nil && apierrors.IsNotFound(err){ - return _, _, err - } - return resourceapply.ApplyConfigMap(ctx, client, recorder, configMap) -} +func manageKubeAPICheckEndpointsConfig(ctx context.Context, client coreclientv1.ConfigMapsGetter, recorder events.Recorder) (*corev1.ConfigMap, bool, error) { + required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/check-endpoints-config-cm.yaml")) + // Defensively normalize namespace in case the asset drifts. + required.Namespace = operatorclient.TargetNamespace + return resourceapply.ApplyConfigMap(ctx, client, recorder, required) +}
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
244-247: Consider applying the CM before updating the pod template to avoid first-rollout mount stalls
kube-apiserver-podlikely mounts this ConfigMap; applying it aftermanagePodscan transiently reference a non-existent CM on initial revisions. Prefer callingmanageKubeAPICheckEndpointsConfigbeforemanagePods(e.g., right aftermanageKubeAPIServerConfig). If the revision controller already gates on presence, ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml(1 hunks)bindata/assets/kube-apiserver/pod.yaml(1 hunks)pkg/operator/starter.go(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bindata/assets/kube-apiserver/pod.yaml
- pkg/operator/starter.go
- bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (4)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)pkg/operator/operatorclient/interfaces.go (1)
TargetNamespace(7-7)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceapply/core.go (1)
ApplyConfigMap(83-85)
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
f439451 to
f27f4b5
Compare
Signed-off-by: lan.tian <[email protected]>
lance5890
force-pushed
the
support_endpoint_config_revisioned
branch
from
f27f4b5 to
1ce447f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
310-313: Set namespace defensively before apply.Avoid surprises if the asset’s namespace ever drifts; normalize it to TargetNamespace before Apply.
-func manageKubeAPICheckEndpointsConfig(ctx context.Context, client coreclientv1.ConfigMapsGetter, recorder events.Recorder) (*corev1.ConfigMap, bool, error) { - configMap := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/check-endpoints-config-cm.yaml")) - return resourceapply.ApplyConfigMap(ctx, client, recorder, configMap) -} +func manageKubeAPICheckEndpointsConfig(ctx context.Context, client coreclientv1.ConfigMapsGetter, recorder events.Recorder) (*corev1.ConfigMap, bool, error) { + required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/check-endpoints-config-cm.yaml")) + required.Namespace = operatorclient.TargetNamespace + return resourceapply.ApplyConfigMap(ctx, client, recorder, required) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml(1 hunks)bindata/assets/kube-apiserver/pod.yaml(1 hunks)pkg/operator/starter.go(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bindata/assets/kube-apiserver/pod.yaml
- pkg/operator/starter.go
- bindata/assets/kube-apiserver/check-endpoints-config-cm.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (3)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceapply/core.go (1)
ApplyConfigMap(83-85)
🔇 Additional comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (2)
222-225: Good sequencing and error wiring for check-endpoints ConfigMap.Applied before managePods and uses a precise degraded key. This integrates cleanly with the existing error aggregation.
222-225: Resolve: ConfigMap wiring and usage verified
manageKubeAPICheckEndpointsConfig is invoked in createTargetConfig, the ConfigMap asset exists with namekube-apiserver-check-endpoints-configin namespaceopenshift-kube-apiserver, the pod’s args include--config /etc/kubernetes/static-pod-resources/configmaps/kube-apiserver-check-endpoints-config/config.yaml, and the CM is listed (optional) in RevisionConfigMaps.
|
/retest |
|
@lance5890: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Tow different approach for setting more safe Cipher Suites for kube-apiserver-check-endpoints, compared with #1911
Summary by CodeRabbit
New Features
Security
Chores