From 168f99ae81051588a6a54ef029a5c943294fe714 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Thu, 11 May 2023 15:55:13 -0700 Subject: [PATCH 1/6] add --scope argument into refresh-token --- cmd/authtoken/main.go | 8 +++++- cmd/authtoken/main_test.go | 31 ++++++++++++++++++++++ pkg/authtoken/providers/azure/azure_msi.go | 8 +++--- 3 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 cmd/authtoken/main_test.go diff --git a/cmd/authtoken/main.go b/cmd/authtoken/main.go index fe743e485..1103a0242 100644 --- a/cmd/authtoken/main.go +++ b/cmd/authtoken/main.go @@ -23,6 +23,10 @@ var ( configPath string ) +const ( + aksAADApplicationId = "6dae42f8-4368-4678-94ff-3960e28e3630" +) + func parseArgs() (interfaces.AuthTokenProvider, error) { var tokenProvider interfaces.AuthTokenProvider rootCmd := &cobra.Command{Use: "refreshtoken", Args: cobra.NoArgs} @@ -50,15 +54,17 @@ func parseArgs() (interfaces.AuthTokenProvider, error) { _ = secretCmd.MarkFlagRequired("namespace") var clientID string + var scope string azureCmd := &cobra.Command{ Use: "azure", Args: cobra.NoArgs, Run: func(_ *cobra.Command, args []string) { - tokenProvider = azure.New(clientID) + tokenProvider = azure.New(clientID, scope) }, } azureCmd.Flags().StringVar(&clientID, "clientid", "", "Azure AAD client ID (required)") + azureCmd.Flags().StringVar(&scope, "scope", aksAADApplicationId, "Azure AAD token scope (optinal)") _ = azureCmd.MarkFlagRequired("clientid") rootCmd.AddCommand(secretCmd, azureCmd) diff --git a/cmd/authtoken/main_test.go b/cmd/authtoken/main_test.go new file mode 100644 index 000000000..04058efd1 --- /dev/null +++ b/cmd/authtoken/main_test.go @@ -0,0 +1,31 @@ +package main + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseArgs(t *testing.T) { + t.Run("all arguments", func(t *testing.T) { + os.Args = []string{"refreshtoken", "azure", "--clientid=test-client-id", "--scope=test-scope"} + t.Cleanup(func() { + os.Args = nil + }) + tokenProvider, err := parseArgs() + assert.NotNil(t, tokenProvider) + assert.Nil(t, err) + + }) + t.Run("no optional arguments", func(t *testing.T) { + os.Args = []string{"refreshtoken", "azure", "--clientid=test-client-id"} + t.Cleanup(func() { + os.Args = nil + }) + tokenProvider, err := parseArgs() + assert.NotNil(t, tokenProvider) + assert.Nil(t, err) + + }) +} diff --git a/pkg/authtoken/providers/azure/azure_msi.go b/pkg/authtoken/providers/azure/azure_msi.go index c01d7f7a1..0bc818ff1 100644 --- a/pkg/authtoken/providers/azure/azure_msi.go +++ b/pkg/authtoken/providers/azure/azure_msi.go @@ -17,15 +17,13 @@ import ( "go.goms.io/fleet/pkg/interfaces" ) -const ( - aksScope = "6dae42f8-4368-4678-94ff-3960e28e3630" -) type azureAuthTokenProvider struct { clientID string + scope string } -func New(clientID string) interfaces.AuthTokenProvider { +func New(clientID, scope string) interfaces.AuthTokenProvider { return &azureAuthTokenProvider{ clientID: clientID, } @@ -48,7 +46,7 @@ func (a *azureAuthTokenProvider) FetchToken(ctx context.Context) (interfaces.Aut }, func() error { klog.V(2).InfoS("GetToken start", "credential", credential) azToken, err = credential.GetToken(ctx, policy.TokenRequestOptions{ - Scopes: []string{aksScope}, + Scopes: []string{a.scope}, }) if err != nil { klog.ErrorS(err, "Failed to GetToken") From 75ad54fd17e756d12bc66643af1fa22e6ece544f Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Thu, 11 May 2023 21:44:37 -0700 Subject: [PATCH 2/6] fix linter --- cmd/authtoken/main.go | 4 ++-- cmd/authtoken/main_test.go | 2 -- pkg/authtoken/providers/azure/azure_msi.go | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/cmd/authtoken/main.go b/cmd/authtoken/main.go index 1103a0242..47bd03b61 100644 --- a/cmd/authtoken/main.go +++ b/cmd/authtoken/main.go @@ -24,7 +24,7 @@ var ( ) const ( - aksAADApplicationId = "6dae42f8-4368-4678-94ff-3960e28e3630" + aksAADApplicationID = "6dae42f8-4368-4678-94ff-3960e28e3630" ) func parseArgs() (interfaces.AuthTokenProvider, error) { @@ -64,7 +64,7 @@ func parseArgs() (interfaces.AuthTokenProvider, error) { } azureCmd.Flags().StringVar(&clientID, "clientid", "", "Azure AAD client ID (required)") - azureCmd.Flags().StringVar(&scope, "scope", aksAADApplicationId, "Azure AAD token scope (optinal)") + azureCmd.Flags().StringVar(&scope, "scope", aksAADApplicationID, "Azure AAD token scope (optinal)") _ = azureCmd.MarkFlagRequired("clientid") rootCmd.AddCommand(secretCmd, azureCmd) diff --git a/cmd/authtoken/main_test.go b/cmd/authtoken/main_test.go index 04058efd1..cc8457ef5 100644 --- a/cmd/authtoken/main_test.go +++ b/cmd/authtoken/main_test.go @@ -16,7 +16,6 @@ func TestParseArgs(t *testing.T) { tokenProvider, err := parseArgs() assert.NotNil(t, tokenProvider) assert.Nil(t, err) - }) t.Run("no optional arguments", func(t *testing.T) { os.Args = []string{"refreshtoken", "azure", "--clientid=test-client-id"} @@ -26,6 +25,5 @@ func TestParseArgs(t *testing.T) { tokenProvider, err := parseArgs() assert.NotNil(t, tokenProvider) assert.Nil(t, err) - }) } diff --git a/pkg/authtoken/providers/azure/azure_msi.go b/pkg/authtoken/providers/azure/azure_msi.go index 0bc818ff1..bd03278fa 100644 --- a/pkg/authtoken/providers/azure/azure_msi.go +++ b/pkg/authtoken/providers/azure/azure_msi.go @@ -17,10 +17,9 @@ import ( "go.goms.io/fleet/pkg/interfaces" ) - type azureAuthTokenProvider struct { clientID string - scope string + scope string } func New(clientID, scope string) interfaces.AuthTokenProvider { From 519162505c9510efd64d734fec83aca9c2a5f7bf Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Fri, 12 May 2023 11:10:14 -0700 Subject: [PATCH 3/6] 1. expose AzureAuthTokenProvider in package so that unit test can verify the type 2. move the default scope logic to pkg/authtoken/providers/azure/azure_msi.go --- cmd/authtoken/main.go | 6 +---- cmd/authtoken/main_test.go | 9 ++++++++ pkg/authtoken/providers/azure/azure_msi.go | 26 ++++++++++++++-------- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cmd/authtoken/main.go b/cmd/authtoken/main.go index 47bd03b61..09d4e616e 100644 --- a/cmd/authtoken/main.go +++ b/cmd/authtoken/main.go @@ -23,10 +23,6 @@ var ( configPath string ) -const ( - aksAADApplicationID = "6dae42f8-4368-4678-94ff-3960e28e3630" -) - func parseArgs() (interfaces.AuthTokenProvider, error) { var tokenProvider interfaces.AuthTokenProvider rootCmd := &cobra.Command{Use: "refreshtoken", Args: cobra.NoArgs} @@ -64,7 +60,7 @@ func parseArgs() (interfaces.AuthTokenProvider, error) { } azureCmd.Flags().StringVar(&clientID, "clientid", "", "Azure AAD client ID (required)") - azureCmd.Flags().StringVar(&scope, "scope", aksAADApplicationID, "Azure AAD token scope (optinal)") + azureCmd.Flags().StringVar(&scope, "scope", "", "Azure AAD token scope (optinal)") _ = azureCmd.MarkFlagRequired("clientid") rootCmd.AddCommand(secretCmd, azureCmd) diff --git a/cmd/authtoken/main_test.go b/cmd/authtoken/main_test.go index cc8457ef5..83f37b6a5 100644 --- a/cmd/authtoken/main_test.go +++ b/cmd/authtoken/main_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.goms.io/fleet/pkg/authtoken/providers/azure" ) func TestParseArgs(t *testing.T) { @@ -16,6 +17,10 @@ func TestParseArgs(t *testing.T) { tokenProvider, err := parseArgs() assert.NotNil(t, tokenProvider) assert.Nil(t, err) + + azTokenProvider, ok := tokenProvider.(*azure.AzureAuthTokenProvider) + assert.Equal(t, true, ok) + assert.Equal(t, azTokenProvider.Scope, "test-scope") }) t.Run("no optional arguments", func(t *testing.T) { os.Args = []string{"refreshtoken", "azure", "--clientid=test-client-id"} @@ -25,5 +30,9 @@ func TestParseArgs(t *testing.T) { tokenProvider, err := parseArgs() assert.NotNil(t, tokenProvider) assert.Nil(t, err) + + azTokenProvider, ok := tokenProvider.(*azure.AzureAuthTokenProvider) + assert.Equal(t, true, ok) + assert.Equal(t, azTokenProvider.Scope, "6dae42f8-4368-4678-94ff-3960e28e3630") }) } diff --git a/pkg/authtoken/providers/azure/azure_msi.go b/pkg/authtoken/providers/azure/azure_msi.go index bd03278fa..c6a9f5161 100644 --- a/pkg/authtoken/providers/azure/azure_msi.go +++ b/pkg/authtoken/providers/azure/azure_msi.go @@ -17,23 +17,31 @@ import ( "go.goms.io/fleet/pkg/interfaces" ) -type azureAuthTokenProvider struct { - clientID string - scope string +const ( + aksScope = "6dae42f8-4368-4678-94ff-3960e28e3630" +) + +type AzureAuthTokenProvider struct { + ClientID string + Scope string } func New(clientID, scope string) interfaces.AuthTokenProvider { - return &azureAuthTokenProvider{ - clientID: clientID, + if scope == "" { + scope = aksScope + } + return &AzureAuthTokenProvider{ + ClientID: clientID, + Scope: scope, } } // FetchToken gets a new token to make request to the associated fleet' hub cluster. -func (a *azureAuthTokenProvider) FetchToken(ctx context.Context) (interfaces.AuthToken, error) { +func (a *AzureAuthTokenProvider) FetchToken(ctx context.Context) (interfaces.AuthToken, error) { token := interfaces.AuthToken{} - opts := &azidentity.ManagedIdentityCredentialOptions{ID: azidentity.ClientID(a.clientID)} + opts := &azidentity.ManagedIdentityCredentialOptions{ID: azidentity.ClientID(a.ClientID)} - klog.V(2).InfoS("FetchToken", "client ID", a.clientID) + klog.V(2).InfoS("FetchToken", "client ID", a.ClientID) credential, err := azidentity.NewManagedIdentityCredential(opts) if err != nil { return token, fmt.Errorf("failed to create managed identity cred: %w", err) @@ -45,7 +53,7 @@ func (a *azureAuthTokenProvider) FetchToken(ctx context.Context) (interfaces.Aut }, func() error { klog.V(2).InfoS("GetToken start", "credential", credential) azToken, err = credential.GetToken(ctx, policy.TokenRequestOptions{ - Scopes: []string{a.scope}, + Scopes: []string{a.Scope}, }) if err != nil { klog.ErrorS(err, "Failed to GetToken") From 718e45983e12521cf4a821e4fbb27939d2eb4644 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Fri, 12 May 2023 11:18:18 -0700 Subject: [PATCH 4/6] swith assert expected and actualy parameters after self review --- cmd/authtoken/main_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/authtoken/main_test.go b/cmd/authtoken/main_test.go index 83f37b6a5..49ac7bb6c 100644 --- a/cmd/authtoken/main_test.go +++ b/cmd/authtoken/main_test.go @@ -20,7 +20,7 @@ func TestParseArgs(t *testing.T) { azTokenProvider, ok := tokenProvider.(*azure.AzureAuthTokenProvider) assert.Equal(t, true, ok) - assert.Equal(t, azTokenProvider.Scope, "test-scope") + assert.Equal(t, "test-scope", azTokenProvider.Scope) }) t.Run("no optional arguments", func(t *testing.T) { os.Args = []string{"refreshtoken", "azure", "--clientid=test-client-id"} @@ -33,6 +33,6 @@ func TestParseArgs(t *testing.T) { azTokenProvider, ok := tokenProvider.(*azure.AzureAuthTokenProvider) assert.Equal(t, true, ok) - assert.Equal(t, azTokenProvider.Scope, "6dae42f8-4368-4678-94ff-3960e28e3630") + assert.Equal(t, "6dae42f8-4368-4678-94ff-3960e28e3630", azTokenProvider.Scope) }) } From dfa8a23a9b2d2d30aeac6fd27f36997e1c9b2a92 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Fri, 12 May 2023 16:09:34 -0700 Subject: [PATCH 5/6] rename AzureAuthTokenProvider -> AuthTokenProvider to fix linter complain --- cmd/authtoken/main_test.go | 5 +++-- pkg/authtoken/providers/azure/azure_msi.go | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cmd/authtoken/main_test.go b/cmd/authtoken/main_test.go index 49ac7bb6c..ad00526d0 100644 --- a/cmd/authtoken/main_test.go +++ b/cmd/authtoken/main_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.goms.io/fleet/pkg/authtoken/providers/azure" ) @@ -18,7 +19,7 @@ func TestParseArgs(t *testing.T) { assert.NotNil(t, tokenProvider) assert.Nil(t, err) - azTokenProvider, ok := tokenProvider.(*azure.AzureAuthTokenProvider) + azTokenProvider, ok := tokenProvider.(*azure.AuthTokenProvider) assert.Equal(t, true, ok) assert.Equal(t, "test-scope", azTokenProvider.Scope) }) @@ -31,7 +32,7 @@ func TestParseArgs(t *testing.T) { assert.NotNil(t, tokenProvider) assert.Nil(t, err) - azTokenProvider, ok := tokenProvider.(*azure.AzureAuthTokenProvider) + azTokenProvider, ok := tokenProvider.(*azure.AuthTokenProvider) assert.Equal(t, true, ok) assert.Equal(t, "6dae42f8-4368-4678-94ff-3960e28e3630", azTokenProvider.Scope) }) diff --git a/pkg/authtoken/providers/azure/azure_msi.go b/pkg/authtoken/providers/azure/azure_msi.go index c6a9f5161..c481ec008 100644 --- a/pkg/authtoken/providers/azure/azure_msi.go +++ b/pkg/authtoken/providers/azure/azure_msi.go @@ -21,7 +21,7 @@ const ( aksScope = "6dae42f8-4368-4678-94ff-3960e28e3630" ) -type AzureAuthTokenProvider struct { +type AuthTokenProvider struct { ClientID string Scope string } @@ -30,14 +30,14 @@ func New(clientID, scope string) interfaces.AuthTokenProvider { if scope == "" { scope = aksScope } - return &AzureAuthTokenProvider{ + return &AuthTokenProvider{ ClientID: clientID, Scope: scope, } } // FetchToken gets a new token to make request to the associated fleet' hub cluster. -func (a *AzureAuthTokenProvider) FetchToken(ctx context.Context) (interfaces.AuthToken, error) { +func (a *AuthTokenProvider) FetchToken(ctx context.Context) (interfaces.AuthToken, error) { token := interfaces.AuthToken{} opts := &azidentity.ManagedIdentityCredentialOptions{ID: azidentity.ClientID(a.ClientID)} From 454d8ddaaf913819b536eaa0eaf4b2e0d00c8e46 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Mon, 15 May 2023 12:54:33 -0700 Subject: [PATCH 6/6] add TODO for --scope argument --- cmd/authtoken/main.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/authtoken/main.go b/cmd/authtoken/main.go index 09d4e616e..c3f94162c 100644 --- a/cmd/authtoken/main.go +++ b/cmd/authtoken/main.go @@ -60,7 +60,9 @@ func parseArgs() (interfaces.AuthTokenProvider, error) { } azureCmd.Flags().StringVar(&clientID, "clientid", "", "Azure AAD client ID (required)") - azureCmd.Flags().StringVar(&scope, "scope", "", "Azure AAD token scope (optinal)") + // TODO: this scope argument is specific for Azure provider. We should allow registering and parsing provider specific argument + // in provider level, instead of global level. + azureCmd.Flags().StringVar(&scope, "scope", "", "Azure AAD token scope (optional)") _ = azureCmd.MarkFlagRequired("clientid") rootCmd.AddCommand(secretCmd, azureCmd)