Skip to content
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

[manila-csi-plugin] make auth more tolerant #2758

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mandre
Copy link
Contributor

@mandre mandre commented Jan 13, 2025

What this PR does / why we need it: Remove dependency on a couple of fields where the logic was wrong. For example, these fields do not necessarily depend on a password being set, as we could be using application credentials.

This prevents manila driver from entering an error state when it finds unnecessary fields in the clouds.yaml. It now simply ignores them.

Which issue this PR fixes(if applicable):

Fixes #2757

Special notes for reviewers:
The fix is in pkg/client/client.go so presumably affects all binaries?

Release note:

Make authentication more tolerant to unneeded fields

Remove dependency on a couple of fields where the logic was wrong. For
example, these fields do not necessarily depend on a password being set,
as we could be using application credentials.

This prevents manila driver from entering an error state when it finds
unnecessary fields in the clouds.yaml. It now simply ignores them.

Fixes kubernetes#2757.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Jan 13, 2025
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

  • 38b428f [manila-csi-plugin] make auth more tolerant

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 13, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zetaab for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from dulek and zetaab January 13, 2025 16:55
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 13, 2025
@kayrus
Copy link
Contributor

kayrus commented Jan 13, 2025

I never had issues with application credentials and manila CSI. Are you sure this is not related to a cloud.yaml parser code?

@mandre
Copy link
Contributor Author

mandre commented Jan 13, 2025

@kayrus I have provided more context in the related issue. You'll get an error when using application credentials, and leaving username in the auth options. One could argue that the clouds.yaml is invalid and should then be rejected, but openstack SDK and other components in kubernetes happily takes it and ignores the extra options. The fact that the manila driver errors on clouds.yaml is a surprising behavior IMO. I propose to loosen some validations and make the auth parsing more tolerant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manila CSI unnecessarily picky about openstack creds
3 participants