Add azure managed identity to azure package in blockstorage #1615

leuyentran · 2022-08-31T19:18:40Z

Change Overview

This PR add :

Support for Azure Manage Identity by adding MSI authorizer
Add an Authenticate() interface to check validity of Azure creds. Azure Managed Identity is the first type to be supported for this interface . Similar to other Azure interfaces, this interface is not united tested because creating a new Azure provider involves calling Instance Azure Metadata which will results in context timeout
Add utilities functions to provide appropriate abstractions levels for point 1 and 2 + unit tests

Pull request type

Please check the type of change your PR introduces:

Test Plan

💪 Manual
⚡ Unit test
💚 E2E

github-actions · 2022-08-31T19:18:55Z

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.

leuyentran · 2022-08-31T19:19:50Z

pkg/blockstorage/azure/azuredisk.go

@@ -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) {


This interface is not united tested because creating a new Azure provider involves calling Azure Instance Metadata which will results in context timeout

leuyentran · 2022-08-31T19:20:28Z

pkg/blockstorage/azure/auth.go

+// authenticate with MSI creds
+type msiAuthenticator struct{}
+
+func (m *msiAuthenticator) authenticate(creds map[string]string) (CredsValidity, error) {


This interface is not united tested because it involves network calls which will results in context timeout

to be added for utilities Signed-off-by: Le Tran <[email protected]>

to be added fro authenticate Signed-off-by: Le Tran <[email protected]>

Signed-off-by: Le Tran <[email protected]>

pkg/blockstorage/azure/auth.go

pkg/blockstorage/azure/client.go

pkg/blockstorage/azure/auth.go

pkg/blockstorage/azure/client.go

pkg/blockstorage/azure/auth.go

Signed-off-by: Le Tran <[email protected]>

…error Signed-off-by: Le Tran <[email protected]>

ihcsim · 2022-09-01T22:20:08Z

pkg/blockstorage/azure/auth.go

+import (
+	"context"
+
+	"github.com/Azure/go-autorest/autorest/adal"


Looks like this package will be deprecated next year. Can we look into using azidentity instead?

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

Can you create a GH issue to track the migration task for both the adal and auth packages? Thanks.

#1621 fyi @ihcsim

ihcsim

Code change LGTM. I will leave it to @onkarbhat and @bathina2 to approve.

ihcsim · 2022-09-01T22:29:59Z

pkg/blockstorage/azure/auth.go

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


TIOLI: Consider just return spt.Refresh(), since Refresh() already returns comprehensive error messages. Same with clientSecretAuthenticator.

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.

pkg/blockstorage/azure/auth_tests.go

pkg/blockstorage/azure/azuredisk.go

ihcsim · 2022-09-01T22:47:24Z

pkg/blockstorage/azure/auth.go

+import (
+	"context"
+
+	"github.com/Azure/go-autorest/autorest/adal"


Can you create a GH issue to track the migration task for both the adal and auth packages? Thanks.

pkg/blockstorage/azure/azuredisk.go

bathina2 · 2022-09-01T23:51:55Z

The changes look good. However, I do have some design opinions. From the caller side I would expect something like -

authenticator, err := GetAuthenticator
if err != nil {
    //spit out err
}
err = authenticator.Authenticate()

Let me know if you have time to discuss tomorrow

Signed-off-by: Le Tran <[email protected]>

…e-in-blockstorage

bathina2

Looks good, Minor nits about naming. Might be good to be more explicit, but not a big deal

bathina2 · 2022-09-02T23:49:22Z

pkg/blockstorage/azure/auth.go

+}
+
+// authenticate with MSI creds
+type msiAuthenticator struct{}


nit: consider renaming to msiAzureAuthenticator

bathina2 · 2022-09-02T23:50:08Z

pkg/blockstorage/azure/auth.go

+}
+
+// authenticate with client secret creds
+type clientSecretAuthenticator struct{}


nit: consider renaming to clientSecretAzureAuthenitcator.

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

onkarbhat

LGTM . Posting it after merge.

onkarbhat · 2022-09-07T00:10:32Z

pkg/blockstorage/azure/auth.go

+	Authenticate(creds map[string]string) error
+}
+
+func NewAzureAutheticator(config map[string]string) (AzureAuthenticator, error) {


Please add comments for exported methods/structs in this PR.

leuyentran commented Aug 31, 2022

View reviewed changes

leuyentran requested a review from bathina2 August 31, 2022 19:20

leuyentran force-pushed the Add-Azure-Managed-Identity-to-azure-package-in-blockstorage branch from cc9e986 to e39e7ad Compare August 31, 2022 20:16

Le Tran added 4 commits August 31, 2022 16:23

Add utilities functions for Azure authentication + unit test

7e4f2f6

to be added for utilities Signed-off-by: Le Tran <[email protected]>

Add an interface for Azure blockstorage to Authenticate credentials

1250b28

to be added fro authenticate Signed-off-by: Le Tran <[email protected]>

Add an option to use MSI Authorizer for Azure

9401cb7

Signed-off-by: Le Tran <[email protected]>

Add guarding for nil + clean up comment

8daf339

Signed-off-by: Le Tran <[email protected]>

leuyentran force-pushed the Add-Azure-Managed-Identity-to-azure-package-in-blockstorage branch from e39e7ad to 8daf339 Compare August 31, 2022 20:24

bathina2 reviewed Aug 31, 2022

View reviewed changes

pkg/blockstorage/azure/auth.go Outdated Show resolved Hide resolved

onkarbhat reviewed Sep 1, 2022

View reviewed changes

Le Tran added 5 commits September 1, 2022 16:32

Update gomod and gosum files

f867abc

Signed-off-by: Le Tran <[email protected]>

Expose functions checking for creds

f38bbab

Signed-off-by: Le Tran <[email protected]>

Add authenticator for client secret credentials type

3055210

Signed-off-by: Le Tran <[email protected]>

Clean up according to PR reviews

5e0819a

Signed-off-by: Le Tran <[email protected]>

Remove CredAvailibility, authentication status will be inferred from …

1d425f4

…error Signed-off-by: Le Tran <[email protected]>

leuyentran force-pushed the Add-Azure-Managed-Identity-to-azure-package-in-blockstorage branch from a4dbac2 to 1d425f4 Compare September 1, 2022 20:33

leuyentran mentioned this pull request Sep 1, 2022

Fixed typos #1617

Merged

10 tasks

leuyentran marked this pull request as ready for review September 1, 2022 20:34

ihcsim reviewed Sep 1, 2022

View reviewed changes

bathina2 reviewed Sep 1, 2022

View reviewed changes

pkg/blockstorage/azure/azuredisk.go Outdated Show resolved Hide resolved

Le Tran and others added 3 commits September 2, 2022 16:36

Address review comments

b0d1bbc

Signed-off-by: Le Tran <[email protected]>

Make Azure authenticator a public interface that can be used directly

183b99e

Signed-off-by: Le Tran <[email protected]>

Merge branch 'master' into Add-Azure-Managed-Identity-to-azure-packag…

867878e

…e-in-blockstorage

bathina2 approved these changes Sep 2, 2022

View reviewed changes

leuyentran added the kueue label Sep 5, 2022

Merge branch 'master' into Add-Azure-Managed-Identity-to-azure-packag…

60a3d63

…e-in-blockstorage

mergify bot merged commit 09018c8 into master Sep 5, 2022

mergify bot deleted the Add-Azure-Managed-Identity-to-azure-package-in-blockstorage branch September 5, 2022 21:18

onkarbhat reviewed Sep 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add azure managed identity to azure package in blockstorage #1615

Add azure managed identity to azure package in blockstorage #1615

leuyentran commented Aug 31, 2022

github-actions bot commented Aug 31, 2022

leuyentran Aug 31, 2022

leuyentran Aug 31, 2022

ihcsim Sep 1, 2022

leuyentran Sep 1, 2022

ihcsim Sep 1, 2022

leuyentran Sep 2, 2022 •

edited

Loading

ihcsim left a comment

ihcsim Sep 1, 2022

leuyentran Sep 2, 2022

ihcsim Sep 1, 2022

bathina2 commented Sep 1, 2022

bathina2 left a comment

bathina2 Sep 2, 2022

bathina2 Sep 2, 2022

leuyentran Sep 5, 2022 •

edited

Loading

onkarbhat left a comment

onkarbhat Sep 7, 2022

Add azure managed identity to azure package in blockstorage #1615

Add azure managed identity to azure package in blockstorage #1615

Conversation

leuyentran commented Aug 31, 2022

Change Overview

Pull request type

Test Plan

github-actions bot commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leuyentran Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

ihcsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bathina2 commented Sep 1, 2022

bathina2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leuyentran Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

onkarbhat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leuyentran Sep 2, 2022 •

edited

Loading

leuyentran Sep 5, 2022 •

edited

Loading