-
Notifications
You must be signed in to change notification settings - Fork 46
OSASINFRA-3675: Rework authentication in Manila CSI Driver Operator #373
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
dkokkino
commented
Mar 26, 2025
- Updated manila assets to mount secret and configmap to node/controller
- Modified the secret generator to use different parameters in preparation for change to manila driver
@dkokkino: This pull request references OSASINFRA-3675 which is a valid jira issue. 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. |
data["os-use-clouds"] = []byte(strconv.FormatBool(cloud.UseClouds)) | ||
|
||
if cloud.CloudsFile != "" { | ||
data["os-clouds-file"] = []byte(cloud.CloudsFile) |
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.
This indicates that path that the clouds.yaml
can be found in. We're using this value in the Manila CSI driver, thus the value of this should be static and correspond to the path you are mounting the clouds.yaml
file to.
Looking at your manifests from an earlier commit, that file is currently /etc/kubernetes/secret/clouds.yaml
(though I'm suggesting to change it). You should use the same static path here (with a comment explaining what the path corresponds to)
items: | ||
- key: cloud.conf | ||
path: cloud.conf | ||
- name: secret-manilaplugin |
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.
I know you're aligning with Cinder again here, but would a more descriptive name be helpful? Say, cloud-credentials
? Again, I'm planning on doing the same thing for Cinder.
mountPath: /etc/kubernetes/config | ||
readOnly: true | ||
- name: secret-manilaplugin | ||
mountPath: /etc/kubernetes/secret |
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.
Purely a nice to have, but clouds.yaml
are typically found in /etc/openstack
. I know you're aligning with Cinder here - which is good - but I wonder if we could use that here?
Note: I am planning on changing this in Cinder too
825f7b0
to
f7a25a0
Compare
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.
Looks like you need to run make verify
too
} | ||
data["os-cloud"] = []byte(util.CloudName) | ||
data["os-use-clouds"] = []byte(strconv.FormatBool(true)) | ||
data["os-clouds-file"] = []byte("etc/openstack/clouds.yaml") |
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.
data["os-clouds-file"] = []byte("etc/openstack/clouds.yaml") | |
data["os-clouds-file"] = []byte("/etc/openstack/clouds.yaml") |
You should also leave a comment here indicating what this path means (hint: you defined it in the assets)
data["os-certAuthorityPath"] = []byte(cacertPath) | ||
} | ||
data["os-cloud"] = []byte(util.CloudName) | ||
data["os-use-clouds"] = []byte(strconv.FormatBool(true)) |
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.
We're never expecting to change this so it can be static.
data["os-use-clouds"] = []byte(strconv.FormatBool(true)) | |
data["os-use-clouds"] = []byte("true") |
2a4970a
to
99ce2f5
Compare
54a63e7
to
c83cfb2
Compare
/retest-required |
8dca79f
to
cd2eca5
Compare
/retest-required |
/retest |
This is going to fail until we get the CPO changes from upstream to downstream. For that to happen, we need openshift/release#67348 to merge and openshift/cloud-provider-openstack#334 to be updated to sync from |
/retest |
/test e2e-openstack |
- Adding clouds.yaml as a secret mount volume. This will be used for authentication by the manila driver to match how cinder does authentication.
- Goal is to change how the aunthentication for the manila driver occurs.Changes the fields generated in the secret csi-manila-secrets
- Previously the csi-manila-secrets secret in the openshift-manila-csi-driver namespace required secretsync.go to generate the secret dynamically. Since we are changing how the authentication occurs for the manila driver that process can be replaced with a static secret asset.
/test e2e-openstack |
/retest |
/test e2e-openstack |
/test e2e-openstack-manila-csi |
3 similar comments
/test e2e-openstack-manila-csi |
/test e2e-openstack-manila-csi |
/test e2e-openstack-manila-csi |
/approve I've tested this locally with openshift/hypershift#6683 and HCP deploys just fine. @dkokkino please remove the hold once openshift/hypershift#6683 has merged |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkokkino, stephenfin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/retest Looks like a slow node |
/test hypershift-e2e-openstack-aws-csi-manila |
2 similar comments
/test hypershift-e2e-openstack-aws-csi-manila |
/test hypershift-e2e-openstack-aws-csi-manila |
I am still investigating the hypershift failures. Just leaving notes here for myself. Currently this is failing with:
From the
I am attempting to fix this in openshift/hypershift#6777 but the approach I've taken is wrong (per that LLM-generated warning) so I'll need to take another punt at it. |
Weird. I deployed this locally and things appear to be working as-is. My pod definitions are the same: apiVersion: v1
kind: Pod
metadata:
# ...
name: openstack-manila-csi-controllerplugin-5994b65bf7-q2x8x
namespace: clusters-stephenfin-hcp
# ...
spec:
# ...
volumes:
# ...
- name: cloud-credentials
secret:
defaultMode: 420
items:
- key: clouds.yaml
path: clouds.yaml
secretName: manila-cloud-credentials
- configMap:
defaultMode: 420
items:
- key: ca-bundle.pem
path: ca-bundle.pem
name: openstack-cloud-config
optional: true
# ... I see the credentials in the correct place with the correct key: ❯ oc get -n clusters-stephenfin-hcp secret manila-cloud-credentials -o yaml
apiVersion: v1
data:
cloud.conf: <redacted>
clouds.yaml: <redacted>
kind: Secret
metadata:
annotations:
hypershift.openshift.io/cluster: clusters/stephenfin-hcp
creationTimestamp: "2025-09-16T11:00:30Z"
name: manila-cloud-credentials
namespace: clusters-stephenfin-hcp
resourceVersion: "2080159"
uid: 5ba57428-8af4-4b04-9dae-acb9f55c2bf2
type: Opaque I don't see a ❯ oc get -n clusters-stephenfin-hcp cm openstack-cloud-config -o yaml
apiVersion: v1
data:
cloud.conf: |
<redacted>
kind: ConfigMap
metadata:
creationTimestamp: "2025-09-16T11:04:40Z"
name: openstack-cloud-config
namespace: clusters-stephenfin-hcp
ownerReferences:
- apiVersion: hypershift.openshift.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: HostedControlPlane
name: stephenfin-hcp
uid: 1ab35480-3265-44e9-aed2-d890112f3018
resourceVersion: "2081779"
uid: 72ab196c-85af-49c0-b58a-560d9ee54752 So I'm not really sure what's going on currently 😕 |
/retest e2e-openstack |
@dkokkino: The
The following commands are available to trigger optional jobs:
Use
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 kubernetes-sigs/prow repository. |
/test e2e-openstack |
/test hypershift-e2e-openstack-aws-csi-manila |
@dkokkino: 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. |