Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 23, 2025
Merged

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

merged 1 commit into from
Sep 23, 2025
+8 −8

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

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 13, 2025
@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.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 13, 2025
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 13, 2025
@gouthampacha
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 13, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2025
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2025
@gouthampacha
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 19, 2025
@stephenfin
[manila-csi-plugin] make auth more tolerant
7887eca 
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.
@k8s-ci-robot k8s-ci-robot removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2025
@stephenfin
Copy link
Member

I stumbled into this today and can confirm it's a real issue (I think it was the same root cause also: I had an errant username field set with application credentials).

We will get good error messages from gophercloud if it's unable to authenticate due to missing fields, so I don't think we lose anything here. I think we should get this in.

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[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

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 added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 23, 2025
@k8s-ci-robot k8s-ci-robot merged commit 8875163 into kubernetes:master Sep 23, 2025
4 checks passed
@stephenfin stephenfin deleted the manila-creds branch September 24, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulek dulek Awaiting requested review from dulek

@zetaab zetaab Awaiting requested review from zetaab

Assignees

@stephenfin stephenfin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Manila CSI unnecessarily picky about openstack creds
6 participants
@mandre @kayrus @k8s-triage-robot @gouthampacha @stephenfin @k8s-ci-robot