-
Notifications
You must be signed in to change notification settings - Fork 152
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
Add azure managed identity to azure package in blockstorage #1615
Add azure managed identity to azure package in blockstorage #1615
Conversation
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
pkg/blockstorage/azure/azuredisk.go
Outdated
@@ -44,6 +44,22 @@ func (s *AdStorage) Type() blockstorage.Type { | |||
return blockstorage.TypeAD | |||
} | |||
|
|||
// Authenticate check Azure creds if the credType is supported | |||
func (s *AdStorage) Authenticate(ctx context.Context, credType string, creds map[string]string) (CredsValidity, error) { |
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 interface is not united tested because creating a new Azure provider involves calling Azure Instance Metadata
which will results in context timeout
pkg/blockstorage/azure/auth.go
Outdated
// authenticate with MSI creds | ||
type msiAuthenticator struct{} | ||
|
||
func (m *msiAuthenticator) authenticate(creds map[string]string) (CredsValidity, error) { |
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 interface is not united tested because it involves network calls which will results in context timeout
cc9e986
to
e39e7ad
Compare
to be added for utilities Signed-off-by: Le Tran <le.tran@kasten.io>
to be added fro authenticate Signed-off-by: Le Tran <le.tran@kasten.io>
Signed-off-by: Le Tran <le.tran@kasten.io>
Signed-off-by: Le Tran <le.tran@kasten.io>
e39e7ad
to
8daf339
Compare
Signed-off-by: Le Tran <le.tran@kasten.io>
Signed-off-by: Le Tran <le.tran@kasten.io>
Signed-off-by: Le Tran <le.tran@kasten.io>
Signed-off-by: Le Tran <le.tran@kasten.io>
…error Signed-off-by: Le Tran <le.tran@kasten.io>
a4dbac2
to
1d425f4
Compare
import ( | ||
"context" | ||
|
||
"github.com/Azure/go-autorest/autorest/adal" |
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 this package will be deprecated next year. Can we look into using azidentity
instead?
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.
The purpose of interface Authenticate()
is to validate Azure creds as a one-of action. These creds are then passed on to an Authorizer()
that later authenticates and return a token for every Azure API calls that interact with Azure resources.
And for Authorizer()
, we are still using adal
indirectly as part of github.com/Azure/go-autorest/autorest/azure/auth
Furthermore, azidentity
and adal
differs slightly in their credential definitions and workflows. So I'd prefer not to use azidentity
for Authenticate()
and adal
for Authorizer()
in case we don't completely understand the nuances in expectations.
Instead, I had abstracted out the logics as much as I can, which should make APIs swapping easier during migration :)
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.
Can you create a GH issue to track the migration task for both the adal
and auth
packages? Thanks.
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.
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.
Code change LGTM. I will leave it to @onkarbhat and @bathina2 to approve.
// network call to check for token | ||
err = spt.Refresh() | ||
if err != nil { | ||
return errors.Wrap(err, "Failed to refresh token") | ||
} | ||
// creds passed authentication | ||
return nil |
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.
TIOLI: Consider just return spt.Refresh()
, since Refresh()
already returns comprehensive error messages. Same with clientSecretAuthenticator
.
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.
The caller, i.e. Authenticate()
directly return results of these methods. Therefore, the explicitly here is meant for code readability for future developers to understand: err==nil
is considered passing authentication.
import ( | ||
"context" | ||
|
||
"github.com/Azure/go-autorest/autorest/adal" |
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.
Can you create a GH issue to track the migration task for both the adal
and auth
packages? Thanks.
The changes look good. However, I do have some design opinions. From the caller side I would expect something like -
Let me know if you have time to discuss tomorrow |
Signed-off-by: Le Tran <le.tran@kasten.io>
Signed-off-by: Le Tran <le.tran@kasten.io>
…e-in-blockstorage
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 good, Minor nits about naming. Might be good to be more explicit, but not a big deal
} | ||
|
||
// authenticate with MSI creds | ||
type msiAuthenticator struct{} |
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.
nit: consider renaming to msiAzureAuthenticator
} | ||
|
||
// authenticate with client secret creds | ||
type clientSecretAuthenticator struct{} |
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.
nit: consider renaming to clientSecretAzureAuthenitcator.
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.
Since the package name is azure
, I think it is okay to leave the naming as is. If they need to be public for some reason in the future, it would still be azure.ClientSecretAuthenticator
…e-in-blockstorage
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.
LGTM . Posting it after merge.
Authenticate(creds map[string]string) error | ||
} | ||
|
||
func NewAzureAutheticator(config map[string]string) (AzureAuthenticator, error) { |
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.
Please add comments for exported methods/structs in this PR.
Change Overview
This PR add :
Instance Azure Metadata
which will results incontext timeout
Pull request type
Please check the type of change your PR introduces:
Test Plan