Skip to content

Fix volume snapshotter usage of AAD URI#256

Open
dylanbossie-rise8 wants to merge 2 commits intovelero-io:mainfrom
dylanbossie-rise8:main
Open

Fix volume snapshotter usage of AAD URI#256
dylanbossie-rise8 wants to merge 2 commits intovelero-io:mainfrom
dylanbossie-rise8:main

Conversation

@dylanbossie-rise8
Copy link
Copy Markdown

Currently AAD URI will throw an error with volume snapshotter, and in practice will cause volume snapshots to be attempted using DefaultAzureCredential when used with Azure, which causes failures in certain contexts. This should resolve that issue.

dylanbossie-rise8 and others added 2 commits October 22, 2024 13:24
volume snapshotting currently does not work with Azure workload identities due to ValidateVolumeSnapshotterConfig throwing an error, since the AAD URI is not recognized in that method as a valid key. this will resolve the issue.

Signed-off-by: Dylan Bossie <dylanbossie@gmail.com>
Signed-off-by: Dylan Bossie <dylanbossie@gmail.com>
@Jeremy-Boyle
Copy link
Copy Markdown
Contributor

@shubham-pampattiwar @ywk253100

Can you please review this if you have time, this is something that was overlooked in one of my previous commits :)

@kaovilai
Copy link
Copy Markdown
Collaborator

I don't see it being referenced in code other than docs. can you clarify where the value of this config is used?

Copy link
Copy Markdown
Contributor

@Jeremy-Boyle Jeremy-Boyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make these changes

Comment thread velero-plugin-for-microsoft-azure/volume_snapshotter.go
@@ -0,0 +1 @@
Update volume_snapshotter to correctly use AAD URI which fixes behavior for Azure Workload Identities No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file @dylanbossie-rise8 , its automatically generated via commits, when a release is cut.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelogs/unreleased/ is not automatically generated at least for all other velero repos.

I suggests keeping the changelog here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelogs/unreleased/ is not automatically generated at least for all other velero repos.

I suggests keeping the changelog here.

Ah thanks ! I didn't notice that i just looked at some other commits and MRs good job @dylanbossie-rise8 for noticing that. Side, note did you have any further questions for above ?

Copy link
Copy Markdown
Collaborator

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@shubham-pampattiwar
Copy link
Copy Markdown
Collaborator

/cc @anshulahuja98

@anshulahuja98
Copy link
Copy Markdown
Collaborator

@shubham-pampattiwar / @kaovilai I don't seem to have access to trigger the CI
Do you folks have that?
CC: @reasonerjt

@kaovilai
Copy link
Copy Markdown
Collaborator

kaovilai commented Feb 6, 2025

Not me.
image

@reasonerjt ?

@shubham-pampattiwar
Copy link
Copy Markdown
Collaborator

yeah me too. same access as @kaovilai

@Jeremy-Boyle
Copy link
Copy Markdown
Contributor

@ywk253100 @sseago

Should be able to run the ci

@Jeremy-Boyle
Copy link
Copy Markdown
Contributor

I just approved aswell, there is the pending requests for review , @ywk253100 @sseago when you get a chance could we have a Quick Look at this small but needed change in order for this to work?

@jcamu
Copy link
Copy Markdown

jcamu commented Feb 13, 2026

Hello.

Any news on this subject ?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants