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

Fix volume snapshotter usage of AAD URI #256

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dylanbossie-rise8
Copy link

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 <[email protected]>
Signed-off-by: Dylan Bossie <[email protected]>
@Jeremy-Boyle
Copy link
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
Member

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
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

@@ -0,0 +1 @@
Update volume_snapshotter to correctly use AAD URI which fixes behavior for Azure Workload Identities
Copy link
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
Member

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
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
Member

@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
Collaborator

/cc @anshulahuja98

@anshulahuja98
Copy link
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
Member

kaovilai commented Feb 6, 2025

Not me.
image

@reasonerjt ?

@shubham-pampattiwar
Copy link
Collaborator

yeah me too. same access as @kaovilai

@Jeremy-Boyle
Copy link
Contributor

@ywk253100 @sseago

Should be able to run the ci

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.

6 participants