feat(timeout): Implement customizable timeout for Azure CLI token ret…

[Aricg]

This commit introduces the ability to specify a custom timeout for Azure CLI token retrieval within the kubelogin package. The AzureCLIToken struct now includes a timeout field, allowing users to set a specific timeout duration.

[Aricg]

@microsoft-github-policy-service agree company="AIFI"

[Aricg]

relevant message from a maintainer of azure-sdk-for-go/sdk/azidentity/
Azure/azure-sdk-for-go#21985 (comment)

[weinong]

Looking forward to it!

[Aricg]

@weinong can you run the CI please?

golangci-lint run
pkg/token/federatedIdentity.go:65:15: string `/.default` has 3 occurrences, make it a constant (goconst)
		resource += "/.default"
		            ^
 ```
 
 
 Also, am I meant to fix this with a global const?

[Aricg]

I believe I also fixed the tests.

[Aricg]

linting is now passing my changes locally as well

[Aricg]

I now see the timeout in both of the sub commands, and I am no longer wedging the --timeout into the main kubelogin command.

./kubelogin get-token --timeout 90s
./kubelogin convert-kubeconfig --timeout 90s
however we don't really pass the option to the the convert-kubeconfig subcommand..

pkg/cmd/ ->
convert.go
root.go *modified
token.go *modified

I'm only expecting to see it as an option in the get-token

./kubelogin  get-token --help | grep timeout
      --timeout duration                     Timeout duration for Azure CLI token requests (default 30s)
./kubelogin  convert-kubeconfig --help | grep timeout
      --timeout duration                     Timeout duration for Azure CLI token requests (default 30s)

do you know what I did wrong?

[weinong]

@Aricg , get-token and convert-kubeconfig share identical flags because convert-kubeconfig adds the flags to get-token command. Can you update the logic in convert.go so that it appends --timeout when that flag is used in convert-kubeconfig?

[Aricg]

@Aricg , get-token and convert-kubeconfig share identical flags because convert-kubeconfig adds the flags to get-token command. Can you update the logic in convert.go so that it appends --timeout when that flag is used in convert-kubeconfig?

Hi @weinong

I only need --timeout for
kubelogin get-token
not
kubelogin convert-kubeconfig

I don't see convert-kubeconfig using AzureCLIToken from https://github.com/Azure/azure-sdk-for-go/blob/7d4a3cbaadd5bf9f16322e1e2c673a633f146fb1/sdk/azidentity/azure_cli_credential.go#L105-L110

type AzureCLIToken struct {
	resourceID string
	tenantID   string
	timeout    time.Duration
}

func newAzureCLIToken(resourceID string, tenantID string, timeout time.Duration) (TokenProvider, error) {
 	if resourceID == "" {
 		return nil, errors.New("resourceID cannot be empty")
 	}

 	if timeout <= 0 {
 		timeout = defaultTimeout
 	}

 	return &AzureCLIToken{
 		resourceID: resourceID,
 		tenantID:   tenantID,
 		timeout:    timeout,
 	}, nil
 }

[weinong]

@Aricg convert-kubeconfig doesn't do network calls. It generally copy the flags into a kubecofig using get-token sub-command. It's fine you don't add it, if you don't find it a problem cracking up the kubeconfig to add the flag. Besides using the --timeout, a better usability may be via environment variable so that you don't need to update kubeconfig all the time.

Please fix up the UT, though!

[Aricg]

go test -race -coverprofile=coverage.txt -covermode=atomic ./...
?   	github.com/Azure/kubelogin	[no test files]
?   	github.com/Azure/kubelogin/pkg/cmd	[no test files]
?   	github.com/Azure/kubelogin/pkg/token/mock_token	[no test files]
ok  	github.com/Azure/kubelogin/pkg/converter	1.988s	coverage: 90.0% of statements
ok  	github.com/Azure/kubelogin/pkg/pop	3.279s	coverage: 76.9% of statements
ok  	github.com/Azure/kubelogin/pkg/testutils	1.652s	coverage: 9.2% of statements
ok  	github.com/Azure/kubelogin/pkg/token	34.407s	coverage: 67.6% of statements

--timeout duration Timeout duration for Azure CLI token requests. It may be specified in AZURE_CLI_TIMEOUT environment variable (default 30s)

:)

[codecov-commenter]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (369f19a) 65.02% compared to head (851453c) 65.16%.

Files	Patch %	Lines
pkg/token/options.go	63.63%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   65.02%   65.16%   +0.13%     
==========================================
  Files          23       23              
  Lines        1710     1728      +18     
==========================================
+ Hits         1112     1126      +14     
- Misses        536      539       +3     
- Partials       62       63       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[weinong]

LGTM