OSASINFRA-3731: openstack: Consume CA cert from CCO secret#359
OSASINFRA-3731: openstack: Consume CA cert from CCO secret#359stephenfin wants to merge 9 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@stephenfin: This pull request references OSASINFRA-3731 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. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@stephenfin: This pull request references OSASINFRA-3731 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. |
stephenfin
force-pushed
the
OSASINFRA-3731
branch
3 times, most recently
from
e11d83d to
7aa9bc9
Compare
|
/retest |
|
/unhold openshift/cloud-credential-operator#780 is merged |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@stephenfin: This pull request references OSASINFRA-3731 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. |
|
/retest openshift/cluster-storage-operator#557 has merged |
|
I will investigate these failures tomorrow. Hopefully just a case of the payload not pulling in the CSO change yet 🤞 |
stephenfin
force-pushed
the
OSASINFRA-3731
branch
from
7aa9bc9 to
2ab019e
Compare
|
This is failing because the |
stephenfin
force-pushed
the
OSASINFRA-3731
branch
from
2ab019e to
4cb9b04
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stephenfin 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
|
/retest-required |
stephenfin
force-pushed
the
OSASINFRA-3731
branch
from
4cb9b04 to
0b2d16a
Compare
|
/hold I am investigating the failures |
openshift-ci
Bot
added
the
do-not-merge/hold
stephenfin
force-pushed
the
OSASINFRA-3731
branch
from
0b2d16a to
61c8118
Compare
eshulman2
force-pushed
the
OSASINFRA-3731
branch
from
61c8118 to
e23fecd
Compare
|
/retest |
1 similar comment
|
/retest |
|
/test okd-scos-images |
|
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 |
openshift-ci
Bot
added
the
lifecycle/stale
📝 WalkthroughWalkthroughThis change refactors certificate and credential handling in OpenStack Cinder and Manila CSI drivers. Kubernetes manifests for controllers and DaemonSets are updated to mount cloud credentials and certificates from 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.2.526)assets/overlays/openstack-cinder/patches/controller_add_driver.yaml2026-05-05 17:24:11,166 [MainThread ] [ERROR] Context for Kubernetes runner was not set ... [truncated 1047 characters] ... sets/overlays/openstack-cinder/patches/controller_add_driver.yaml' Comment |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
This at least tells you what the secret is for. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Put it in a more usual place. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This is going to be superseded in a coming change. Rename it in preparation. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
In this change, we modify the assets to start (optionally) mounting the CA cert from the secret in the containers. We leave a fallback in place for the old config map source to allow time for the cloud-credential-operator to update things in an upgrade scenario. This fallback can be removed in 4.20, as noted by the copious TODOs. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
cloud-credential-operator and hypershift-operator now support deploying the CA cert to the credentials secrets they generate, which means we can start consuming them from there rather than from configuration. In this change, we modify the controller to start (optionally) consuming the CA cert from the secret. We leave a fallback in place for the old config map source to allow time for the cloud-credential-operator to update things in an upgrade scenario. This fallback can be removed in 4.22, as noted by the copious TODOs. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This is going to be superseded in a coming change. Rename it in preparation. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Again, do what we already did for openstack-cinder but for openstack-manila. Like the openstack-cinder change, we continue to allow consuming from the old location to ease upgrades. It's worth highlighting that this is a nice little step towards having the Manila CSI driver and controller source their credentials from a 'clouds.yaml' rather than a 'cloud.conf' file, which would let us remove a lot of logic currently found in the operator. Completing that effort is a job best left to another day though so a TODO is included for now. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Do what we previously did for the openstack-cinder controller but for the openstack-manila controller. In effect, we're really just reflecting the changes made in cluster-storage-operator in [1]. However, we do need to add some logic to detect where we are consuming our CA cert from so that we can match forthcoming changes to our assets. While here, we also replace use of the deprecated `ioutil.ReadFile` function in favour of its suggested replacement, `os.ReadFile` [2]. We also replace use of `os.IsNotExist` in favour of its suggested replacement, `errors.Is(err, fs.ErrNotExist)` [3]. [1] github.com/openshift/cluster-storage-operator/pull/557 [2] https://pkg.go.dev/io/ioutil#ReadFile [3] https://pkg.go.dev/os#IsNotExist Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
|
/remove-lifecycle rotten |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
assets/overlays/openstack-manila/patches/controller_add_driver.yaml (2)
118-143:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
cloud-credentialsvolume definitions - second will override first.Two volumes named
cloud-credentialsare defined:
- Lines 119-124: projects
clouds.yamlfrom the secret- Lines 127-135: projects only
cacertasca.crtKubernetes will use only one definition (typically the last), so
clouds.yamlwon't be available in the container. Merge these into a single volume definition with both items.Proposed fix - merge both items into one volume
volumes: - - name: cloud-credentials - secret: - secretName: manila-cloud-credentials - items: - - key: clouds.yaml - path: clouds.yaml - name: socket-dir emptyDir: {} - name: cloud-credentials secret: secretName: manila-cloud-credentials - # TODO(stephenfin): We should also mount the clouds.yaml file - # here and start consuming that rather than converting it into - # the cloud.conf format items: + - key: clouds.yaml + path: clouds.yaml - key: cacert path: ca.crt + optional: true - name: legacy-cacert # TODO(stephenfin): Remove in 4.22 configMap:🤖 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 `@assets/overlays/openstack-manila/patches/controller_add_driver.yaml` around lines 118 - 143, There are two Kubernetes volume entries named "cloud-credentials" so the latter overrides the former and clouds.yaml won't be mounted; remove the duplicate block and merge the items under the single "cloud-credentials" volume (the one using secretName: manila-cloud-credentials) so it includes both the item with key: clouds.yaml path: clouds.yaml and the item with key: cacert path: ca.crt (preserve any comments/optional flags as needed) and delete the extra "cloud-credentials" volume definition.
80-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate
cloud-credentialsvolumeMount entries will cause unpredictable behavior.The
cloud-credentialsvolumeMount appears twice:
- Lines 83-84: mounts to
/etc/openstack- Lines 87-89: also mounts to
/etc/openstackwithreadOnly: trueThis duplication appears to be a merge artifact. Remove the duplicate entry.
Proposed fix
volumeMounts: - name: socket-dir mountPath: /plugin - name: cloud-credentials mountPath: /etc/openstack + readOnly: true - name: legacy-cacert mountPath: /etc/kubernetes/static-pod-resources/configmaps/cloud-config - - name: cloud-credentials - mountPath: /etc/openstack - readOnly: true🤖 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 `@assets/overlays/openstack-manila/patches/controller_add_driver.yaml` around lines 80 - 89, Remove the duplicate volumeMount for the volume named "cloud-credentials" under the volumeMounts list: keep a single entry that mounts to /etc/openstack (preferably the one with readOnly: true) and delete the other duplicate entry that also mounts /etc/openstack; update the volumeMounts block so only one "name: cloud-credentials" mapping to /etc/openstack exists to avoid unpredictable behavior.assets/overlays/openstack-manila/patches/node_add_driver.yaml (2)
96-139:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
cloud-credentialsvolume definitions - same issue as controller.Two volumes named
cloud-credentialsare defined:
- Lines 97-102: projects
clouds.yaml- Lines 123-131: projects only
cacertasca.crtThe second definition will override the first, leaving
clouds.yamlunavailable. Merge into a single volume with both items.Proposed fix
volumes: - - name: cloud-credentials - secret: - secretName: manila-cloud-credentials - items: - - key: clouds.yaml - path: clouds.yaml - name: registration-dir # ... other volumes unchanged ... - name: cloud-credentials secret: secretName: manila-cloud-credentials items: + - key: clouds.yaml + path: clouds.yaml - key: cacert path: ca.crt + optional: true # TODO(stephenfin): Remove in 4.22 - name: legacy-cacert🤖 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 `@assets/overlays/openstack-manila/patches/node_add_driver.yaml` around lines 96 - 139, There are two duplicate volume entries named "cloud-credentials" in the volumes block: one mounting items key "clouds.yaml" and the other mounting item "cacert" -> "ca.crt"; the second overrides the first and drops clouds.yaml. Fix by merging these into a single "cloud-credentials" secret volume that includes both items (key: clouds.yaml -> path: clouds.yaml and key: cacert -> path: ca.crt) so both files are available to the pod; update any comment TODOs if needed and remove the redundant second "cloud-credentials" entry.
61-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate
cloud-credentialsvolumeMount entries - same issue as controller.The
cloud-credentialsvolumeMount appears twice (lines 62-64 and lines 73-75), both mounting to/etc/openstack. This appears to be a merge artifact. Consolidate into a single entry.Proposed fix
volumeMounts: - name: cloud-credentials mountPath: /etc/openstack readOnly: true - name: plugin-dir mountPath: /var/lib/kubelet/plugins/manila.csi.openstack.org - name: fwd-plugin-dir mountPath: /var/lib/kubelet/plugins/csi-nfsplugin - name: etc-selinux mountPath: /etc/selinux - name: sys-fs mountPath: /sys/fs - # credentials and configuration - - name: cloud-credentials - mountPath: /etc/openstack - name: legacy-cacert mountPath: /etc/kubernetes/static-pod-resources/configmaps/cloud-config🤖 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 `@assets/overlays/openstack-manila/patches/node_add_driver.yaml` around lines 61 - 77, The volumeMounts list in node_add_driver.yaml contains a duplicate entry for the volume name "cloud-credentials" (both mounting to /etc/openstack); remove the redundant entry so there is only one volumeMount with name "cloud-credentials" mounting to /etc/openstack and keep the other mounts (plugin-dir, fwd-plugin-dir, etc-selinux, sys-fs, legacy-cacert) unchanged; ensure the remaining "cloud-credentials" entry is the one you want to preserve and that no other references to that mount are duplicated elsewhere in the manifest.
🤖 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.
Inline comments:
In `@assets/overlays/openstack-manila/patches/controller_rename_config_map.yaml`:
- Around line 5-12: In the controller_rename_config_map.yaml patch, the volume
named legacy-cacert incorrectly references configMap.name:
openstack-cloud-config; change that to cloud-provider-config so it matches the
same legacy-cacert volume definition in controller_add_driver.yaml and
node_add_driver.yaml (the volume with items.key ca-bundle.pem and path
ca-bundle.pem should remain unchanged).
In `@pkg/openstack-cinder/config/cloudinfo.go`:
- Around line 90-93: The calls to os.Setenv for OS_CACERT when caFile or
legacyCAFile exist are not checking the returned error; update the logic around
the existing checks (the block referencing caFile and legacyCAFile in
cloudinfo.go) to capture and handle the error returned by os.Setenv, e.g.,
assign the result to a variable and if non-nil log or return the error from the
surrounding function (or wrap it) so the function handles failures from
os.Setenv instead of discarding them.
In `@pkg/openstack-cinder/config/configsync.go`:
- Around line 169-173: The controller currently only wires ConfigMap informers
so changes to the "openstack-cloud-credentials" Secret are not triggering
reconciles; update the informer wiring to also watch Secrets from
c.controlPlaneInformers.InformersFor(c.controlPlaneNamespace).Core().V1().Secrets()
(where secretLister is created) and register an event handler that enqueues a
config sync when the Secret used by hasCACert/openstack-cloud-credentials is
created/updated/deleted, ensuring changes are picked up immediately rather than
waiting for the periodic resync.
- Around line 240-253: getCACertLegacy currently returns only a boolean; change
it to also return the PEM string (e.g., (string, bool, error)) so callers can
publish the legacy "ca-bundle.pem", then update the sync(...) call site to
accept and propagate that PEM when configCACertSource is set to the legacy path
so the destination ConfigMap includes the "ca-bundle.pem" key; ensure
getCACertLegacy still handles missing/empty keys (return empty string + false)
and that sync writes the actual PEM into the generated cloud-config ConfigMap
while the legacy configCACertSource is supported.
In `@pkg/openstack-manila/util/const.go`:
- Around line 14-15: LegacyCertFile constant points to
/etc/openstack-ca/ca-bundle.pem which doesn't match the legacy-cacert volume
mount in the controller/node YAMLs; update the LegacyCertFile constant value to
the actual mounted path used by the YAML patches (the ca-bundle.pem under the
cloud-config mount) and scan usages of LegacyCertFile in functions that perform
cert fallback to ensure they check that updated path and handle a missing file
with proper logging.
---
Outside diff comments:
In `@assets/overlays/openstack-manila/patches/controller_add_driver.yaml`:
- Around line 118-143: There are two Kubernetes volume entries named
"cloud-credentials" so the latter overrides the former and clouds.yaml won't be
mounted; remove the duplicate block and merge the items under the single
"cloud-credentials" volume (the one using secretName: manila-cloud-credentials)
so it includes both the item with key: clouds.yaml path: clouds.yaml and the
item with key: cacert path: ca.crt (preserve any comments/optional flags as
needed) and delete the extra "cloud-credentials" volume definition.
- Around line 80-89: Remove the duplicate volumeMount for the volume named
"cloud-credentials" under the volumeMounts list: keep a single entry that mounts
to /etc/openstack (preferably the one with readOnly: true) and delete the other
duplicate entry that also mounts /etc/openstack; update the volumeMounts block
so only one "name: cloud-credentials" mapping to /etc/openstack exists to avoid
unpredictable behavior.
In `@assets/overlays/openstack-manila/patches/node_add_driver.yaml`:
- Around line 96-139: There are two duplicate volume entries named
"cloud-credentials" in the volumes block: one mounting items key "clouds.yaml"
and the other mounting item "cacert" -> "ca.crt"; the second overrides the first
and drops clouds.yaml. Fix by merging these into a single "cloud-credentials"
secret volume that includes both items (key: clouds.yaml -> path: clouds.yaml
and key: cacert -> path: ca.crt) so both files are available to the pod; update
any comment TODOs if needed and remove the redundant second "cloud-credentials"
entry.
- Around line 61-77: The volumeMounts list in node_add_driver.yaml contains a
duplicate entry for the volume name "cloud-credentials" (both mounting to
/etc/openstack); remove the redundant entry so there is only one volumeMount
with name "cloud-credentials" mounting to /etc/openstack and keep the other
mounts (plugin-dir, fwd-plugin-dir, etc-selinux, sys-fs, legacy-cacert)
unchanged; ensure the remaining "cloud-credentials" entry is the one you want to
preserve and that no other references to that mount are duplicated elsewhere in
the manifest.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 13cd09bd-49e8-40eb-9363-c93c21e2a49d
⛔ Files ignored due to path filters (8)
assets/overlays/openstack-cinder/generated/hypershift/controller.yamlis excluded by!**/generated/**assets/overlays/openstack-cinder/generated/hypershift/node.yamlis excluded by!**/generated/**assets/overlays/openstack-cinder/generated/standalone/controller.yamlis excluded by!**/generated/**assets/overlays/openstack-cinder/generated/standalone/node.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/hypershift/controller.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/hypershift/node.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/controller.yamlis excluded by!**/generated/**assets/overlays/openstack-manila/generated/standalone/node.yamlis excluded by!**/generated/**
📒 Files selected for processing (11)
assets/overlays/openstack-cinder/patches/controller_add_driver.yamlassets/overlays/openstack-cinder/patches/node_add_driver.yamlassets/overlays/openstack-manila/patches/controller_add_driver.yamlassets/overlays/openstack-manila/patches/controller_rename_config_map.yamlassets/overlays/openstack-manila/patches/node_add_driver.yamlpkg/openstack-cinder/config/cloudinfo.gopkg/openstack-cinder/config/configsync.gopkg/openstack-cinder/config/configsync_test.gopkg/openstack-cinder/config/const.gopkg/openstack-manila/client/openstack.gopkg/openstack-manila/util/const.go
| # TODO(stephenfin): Remove in 4.22 | ||
| - name: legacy-cacert | ||
| configMap: | ||
| name: openstack-cloud-config | ||
| items: | ||
| - key: ca-bundle.pem | ||
| path: ca-bundle.pem | ||
| optional: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which ConfigMap names are used across the Manila patches
rg -n "configMap:" -A2 assets/overlays/openstack-manila/patches/Repository: openshift/csi-operator
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Read the full controller_rename_config_map.yaml file
echo "=== controller_rename_config_map.yaml ==="
cat -n assets/overlays/openstack-manila/patches/controller_rename_config_map.yaml
echo -e "\n=== controller_add_driver.yaml (lines 130-150) ==="
sed -n '130,150p' assets/overlays/openstack-manila/patches/controller_add_driver.yaml | cat -n
echo -e "\n=== node_add_driver.yaml (lines 130-145) ==="
sed -n '130,145p' assets/overlays/openstack-manila/patches/node_add_driver.yaml | cat -nRepository: openshift/csi-operator
Length of output: 1563
Fix ConfigMap name inconsistency in legacy-cacert volume.
The legacy-cacert volume in controller_rename_config_map.yaml references openstack-cloud-config, but the same volume in both controller_add_driver.yaml and node_add_driver.yaml references cloud-provider-config. Since all three patches define the same volume with identical items, change this to cloud-provider-config for consistency.
🤖 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 `@assets/overlays/openstack-manila/patches/controller_rename_config_map.yaml`
around lines 5 - 12, In the controller_rename_config_map.yaml patch, the volume
named legacy-cacert incorrectly references configMap.name:
openstack-cloud-config; change that to cloud-provider-config so it matches the
same legacy-cacert volume definition in controller_add_driver.yaml and
node_add_driver.yaml (the volume with items.key ca-bundle.pem and path
ca-bundle.pem should remain unchanged).
| if _, err := os.Stat(caFile); err == nil { | ||
| // NOTE(stephenfin): gophercloud (or rather, the clientconfig package) doesn't currently | ||
| // provide the way to override configuration other than via environment variables | ||
| os.Setenv("OS_CACERT", caFile) | ||
| } else if _, err := os.Stat(legacyCAFile); err == nil { // legacy path | ||
| os.Setenv("OS_CACERT", legacyCAFile) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "cloudinfo.go" -path "*openstack-cinder*" | head -5Repository: openshift/csi-operator
Length of output: 109
🏁 Script executed:
# Read the file around lines 90-93 with context
if [ -f "pkg/openstack-cinder/config/cloudinfo.go" ]; then
cat -n "pkg/openstack-cinder/config/cloudinfo.go" | sed -n '80,105p'
fiRepository: openshift/csi-operator
Length of output: 1418
🏁 Script executed:
# Get the function signature to understand what the function returns
if [ -f "pkg/openstack-cinder/config/cloudinfo.go" ]; then
cat -n "pkg/openstack-cinder/config/cloudinfo.go" | sed -n '1,100p' | grep -E "^[[:space:]]*[0-9]+[[:space:]]+(func |type )" | tail -5
fiRepository: openshift/csi-operator
Length of output: 236
🏁 Script executed:
# Get more context around the function to see the complete function signature and return patterns
cat -n "pkg/openstack-cinder/config/cloudinfo.go" | sed -n '65,120p'Repository: openshift/csi-operator
Length of output: 2272
🏁 Script executed:
# Check if there are any other linting configuration files or if errcheck is configured
find . -name ".golangci.yml" -o -name "golangci.yaml" -o -name ".golintrc" | head -5Repository: openshift/csi-operator
Length of output: 336
🏁 Script executed:
# Let's check what os.Setenv documentation says and whether it can actually return errors
# We can also look for other uses of os.Setenv in the codebase to see patterns
rg "os\.Setenv" --type go -A 2 -B 2 | head -50Repository: openshift/csi-operator
Length of output: 1917
🏁 Script executed:
# Check the root level golangci configuration
cat golangci.yml 2>/dev/null || cat .golangci.yml 2>/dev/null || echo "No config found in root"Repository: openshift/csi-operator
Length of output: 90
🏁 Script executed:
# Let's verify the Go standard library signature for os.Setenv
# and check if there are any comments about this in the code
rg "Setenv" --type go -B 3 -A 3 "pkg/openstack-cinder/config/cloudinfo.go"Repository: openshift/csi-operator
Length of output: 527
🏁 Script executed:
# Check if there's any linter output or test that would show errcheck flagging these lines
find . -name "*.txt" -o -name "*.log" | xargs grep -l "errcheck\|Setenv" 2>/dev/null | head -5Repository: openshift/csi-operator
Length of output: 48
🏁 Script executed:
# Let's check if there's any GitHub Actions workflow or CI config that runs errcheck
find . -name "*.yml" -o -name "*.yaml" | grep -E "(workflow|ci|lint)" | head -10Repository: openshift/csi-operator
Length of output: 716
🏁 Script executed:
# Check the .github/workflows directory if it exists
if [ -d ".github/workflows" ]; then
ls -la .github/workflows/
fiRepository: openshift/csi-operator
Length of output: 48
🏁 Script executed:
# Let's verify the Go documentation about os.Setenv by looking at Go source or any go.mod references
if [ -f "go.mod" ]; then
head -20 go.mod
fiRepository: openshift/csi-operator
Length of output: 884
🏁 Script executed:
# Check for CI configuration that might run linters
cat .ci-operator.yaml 2>/dev/null | head -50Repository: openshift/csi-operator
Length of output: 172
🏁 Script executed:
# Check if there's a Makefile that defines linting targets
if [ -f "Makefile" ]; then
grep -E "(lint|check|errcheck)" Makefile
fiRepository: openshift/csi-operator
Length of output: 48
🏁 Script executed:
# Search for any test or CI scripts that might run linters
find . -maxdepth 2 -name "*.sh" -o -name "Makefile*" | xargs grep -l "errcheck\|golangci-lint" 2>/dev/nullRepository: openshift/csi-operator
Length of output: 50
🏁 Script executed:
# Let's verify the exact signature of os.Setenv by checking Go documentation
# We can look at the Go SDK installed or use a Python script to verify the standard library behavior
python3 << 'EOF'
import subprocess
import json
# Try to get Go's os package documentation
result = subprocess.run(['go', 'doc', 'os.Setenv'], capture_output=True, text=True)
print(result.stdout)
EOFRepository: openshift/csi-operator
Length of output: 237
Handle the os.Setenv errors on lines 91 and 93.
os.Setenv returns an error that must be checked. The unchecked error returns on these lines will trigger errcheck linting failures.
Suggested fix
if _, err := os.Stat(caFile); err == nil {
- os.Setenv("OS_CACERT", caFile)
+ if err := os.Setenv("OS_CACERT", caFile); err != nil {
+ return nil, fmt.Errorf("failed to set OS_CACERT: %w", err)
+ }
} else if _, err := os.Stat(legacyCAFile); err == nil { // legacy path
- os.Setenv("OS_CACERT", legacyCAFile)
+ if err := os.Setenv("OS_CACERT", legacyCAFile); err != nil {
+ return nil, fmt.Errorf("failed to set OS_CACERT: %w", err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err := os.Stat(caFile); err == nil { | |
| // NOTE(stephenfin): gophercloud (or rather, the clientconfig package) doesn't currently | |
| // provide the way to override configuration other than via environment variables | |
| os.Setenv("OS_CACERT", caFile) | |
| } else if _, err := os.Stat(legacyCAFile); err == nil { // legacy path | |
| os.Setenv("OS_CACERT", legacyCAFile) | |
| if _, err := os.Stat(caFile); err == nil { | |
| if err := os.Setenv("OS_CACERT", caFile); err != nil { | |
| return nil, fmt.Errorf("failed to set OS_CACERT: %w", err) | |
| } | |
| } else if _, err := os.Stat(legacyCAFile); err == nil { // legacy path | |
| if err := os.Setenv("OS_CACERT", legacyCAFile); err != nil { | |
| return nil, fmt.Errorf("failed to set OS_CACERT: %w", err) | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.1)
[error] 91-91: Error return value of os.Setenv is not checked
(errcheck)
[error] 93-93: Error return value of os.Setenv is not checked
(errcheck)
🤖 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 `@pkg/openstack-cinder/config/cloudinfo.go` around lines 90 - 93, The calls to
os.Setenv for OS_CACERT when caFile or legacyCAFile exist are not checking the
returned error; update the logic around the existing checks (the block
referencing caFile and legacyCAFile in cloudinfo.go) to capture and handle the
error returned by os.Setenv, e.g., assign the result to a variable and if
non-nil log or return the error from the surrounding function (or wrap it) so
the function handles failures from os.Setenv instead of discarding them.
| secretLister := c.controlPlaneInformers.InformersFor(c.controlPlaneNamespace).Core().V1().Secrets().Lister() | ||
|
|
||
| var caCertSource caCertSource | ||
|
|
||
| hasCACert, err := hasCACert(secretLister, c.controlPlaneNamespace, "openstack-cloud-credentials") |
There was a problem hiding this comment.
Watch the credentials Secret too.
This new CA-source path depends on openstack-cloud-credentials, but the controller is still only wired to ConfigMap informers. Secret creation or rotation will not enqueue a sync until the 20-minute resync, so cloud.conf can stay stale after CCO updates the secret.
🤖 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 `@pkg/openstack-cinder/config/configsync.go` around lines 169 - 173, The
controller currently only wires ConfigMap informers so changes to the
"openstack-cloud-credentials" Secret are not triggering reconciles; update the
informer wiring to also watch Secrets from
c.controlPlaneInformers.InformersFor(c.controlPlaneNamespace).Core().V1().Secrets()
(where secretLister is created) and register an event handler that enqueues a
config sync when the Secret used by hasCACert/openstack-cloud-credentials is
created/updated/deleted, ensuring changes are picked up immediately rather than
waiting for the periodic resync.
| // getCACertLegacy determines whether there is a CA Cert provided in the cloud | ||
| // provider config map by either the Installer or the hypershift-operator so | ||
| // that we can inject it into our config files. This is the legacy path for the | ||
| // transition period as the CA cert is now provided in the credentials secret | ||
| func getCACertLegacy(configMapLister corelisters.ConfigMapLister, ns, name string) (bool, error) { | ||
| cm, err := configMapLister.ConfigMaps(ns).Get(name) | ||
| if err != nil { | ||
| return nil, err | ||
| return false, err | ||
| } | ||
| caCert, ok := cm.Data["ca-bundle.pem"] | ||
| if !ok || len(caCert) == 0 { | ||
| return nil, nil | ||
| return false, nil | ||
| } | ||
| return &caCert, nil | ||
| return true, nil |
There was a problem hiding this comment.
Keep the legacy PEM content until the fallback is removed.
getCACertLegacy now collapses the legacy source down to a boolean, but the new legacy-cacert volumes in assets/overlays/openstack-cinder/patches/controller_add_driver.yaml and assets/overlays/openstack-cinder/patches/node_add_driver.yaml still mount ca-bundle.pem from the generated cloud-conf ConfigMap. On legacy-only clusters this will write ca-file = /etc/kubernetes/static-pod-resources/configmaps/cloud-config/ca-bundle.pem while never publishing that key, so the configured path points at a file that does not exist.
Suggested direction
-func getCACertLegacy(configMapLister corelisters.ConfigMapLister, ns, name string) (bool, error) {
+func getCACertLegacy(configMapLister corelisters.ConfigMapLister, ns, name string) (string, bool, error) {
cm, err := configMapLister.ConfigMaps(ns).Get(name)
if err != nil {
- return false, err
+ return "", false, err
}
caCert, ok := cm.Data["ca-bundle.pem"]
if !ok || len(caCert) == 0 {
- return false, nil
+ return "", false, nil
}
- return true, nil
+ return caCert, true, nil
}Then thread the returned PEM through sync(...) and keep publishing "ca-bundle.pem" into the destination ConfigMap while configCACertSource is still supported.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // getCACertLegacy determines whether there is a CA Cert provided in the cloud | |
| // provider config map by either the Installer or the hypershift-operator so | |
| // that we can inject it into our config files. This is the legacy path for the | |
| // transition period as the CA cert is now provided in the credentials secret | |
| func getCACertLegacy(configMapLister corelisters.ConfigMapLister, ns, name string) (bool, error) { | |
| cm, err := configMapLister.ConfigMaps(ns).Get(name) | |
| if err != nil { | |
| return nil, err | |
| return false, err | |
| } | |
| caCert, ok := cm.Data["ca-bundle.pem"] | |
| if !ok || len(caCert) == 0 { | |
| return nil, nil | |
| return false, nil | |
| } | |
| return &caCert, nil | |
| return true, nil | |
| // getCACertLegacy determines whether there is a CA Cert provided in the cloud | |
| // provider config map by either the Installer or the hypershift-operator so | |
| // that we can inject it into our config files. This is the legacy path for the | |
| // transition period as the CA cert is now provided in the credentials secret | |
| func getCACertLegacy(configMapLister corelisters.ConfigMapLister, ns, name string) (string, bool, error) { | |
| cm, err := configMapLister.ConfigMaps(ns).Get(name) | |
| if err != nil { | |
| return "", false, err | |
| } | |
| caCert, ok := cm.Data["ca-bundle.pem"] | |
| if !ok || len(caCert) == 0 { | |
| return "", false, nil | |
| } | |
| return caCert, true, 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 `@pkg/openstack-cinder/config/configsync.go` around lines 240 - 253,
getCACertLegacy currently returns only a boolean; change it to also return the
PEM string (e.g., (string, bool, error)) so callers can publish the legacy
"ca-bundle.pem", then update the sync(...) call site to accept and propagate
that PEM when configCACertSource is set to the legacy path so the destination
ConfigMap includes the "ca-bundle.pem" key; ensure getCACertLegacy still handles
missing/empty keys (return empty string + false) and that sync writes the actual
PEM into the generated cloud-config ConfigMap while the legacy
configCACertSource is supported.
| CertFile = "/etc/openstack/ca.crt" | ||
| LegacyCertFile = "/etc/openstack-ca/ca-bundle.pem" |
There was a problem hiding this comment.
Path mismatch between LegacyCertFile constant and YAML mount.
LegacyCertFile is set to /etc/openstack-ca/ca-bundle.pem, but in the controller and node YAML patches, the legacy-cacert volume is mounted at /etc/kubernetes/static-pod-resources/configmaps/cloud-config. The file would be at /etc/kubernetes/static-pod-resources/configmaps/cloud-config/ca-bundle.pem, not /etc/openstack-ca/ca-bundle.pem.
This mismatch will cause the fallback to fail silently since the legacy cert path won't exist.
Proposed fix
- LegacyCertFile = "/etc/openstack-ca/ca-bundle.pem"
+ LegacyCertFile = "/etc/kubernetes/static-pod-resources/configmaps/cloud-config/ca-bundle.pem"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CertFile = "/etc/openstack/ca.crt" | |
| LegacyCertFile = "/etc/openstack-ca/ca-bundle.pem" | |
| CertFile = "/etc/openstack/ca.crt" | |
| LegacyCertFile = "/etc/kubernetes/static-pod-resources/configmaps/cloud-config/ca-bundle.pem" |
🤖 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 `@pkg/openstack-manila/util/const.go` around lines 14 - 15, LegacyCertFile
constant points to /etc/openstack-ca/ca-bundle.pem which doesn't match the
legacy-cacert volume mount in the controller/node YAMLs; update the
LegacyCertFile constant value to the actual mounted path used by the YAML
patches (the ca-bundle.pem under the cloud-config mount) and scan usages of
LegacyCertFile in functions that perform cert fallback to ensure they check that
updated path and handle a missing file with proper logging.
|
@stephenfin: 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. |
5 participants
In openshift/cloud-credential-operator/pull/780, we have added the ability for
cloud-credential-operatorto consume a CA cert from the root credentials secret and to include in the credentials secrets it provisions.In openshift/installer/pull/9194, we have modified the Installer to start setting this field where necessary.
In openshift/cluster-storage-operator/pull/557, we modified cluster-storage-operator to pass this CA cert through to the
csi-operatorwhen present.Adapt the assets and controllers for both the openstack-cinder and openstack-manila CSI drivers to start consuming this field, where present. We maintain fallbacks for the previous locations of the cert for now, but these can be removed in the next release.
This needs to wait for the CCO change to be approved before we merge this. It also needs the CSO change to merge first.
Dependencies:
/hold