Use retryable http client in Azure authz provider #372

bingosummer · 2023-08-07T06:26:51Z

We see the timeout error in the checkaccess requrests sometimes.

Error occured during authorization check. Please retry again. Error: Checkaccess requests have timed out.: context deadline exceeded

This PR moves withRetryableHttpClient from Azure authn provider to utils, and leverage it in Azure authz provider's RefreshToken and CheckAccess.

bingosummer · 2023-08-07T07:03:33Z

@weinong @AzureMarker could you please have a review? Thanks!

authz/providers/azure/azure.go

weinong · 2023-08-09T01:30:21Z

authz/providers/azure/azure.go

@@ -120,6 +124,8 @@ func (s Authorizer) Check(ctx context.Context, request *authzv1.SubjectAccessRev
 		return &authzv1.SubjectAccessReviewStatus{Allowed: true, Reason: rbac.AccessAllowedVerdict}, nil
 	}

+	ctx = azureutils.WithRetryableHttpClient(ctx, s.HttpClientRetryCount)


why do we save the http client in the context?

as far as I can tell, it still uses AccessInfo.client, no?

I followed the comment of WithRetryableHttpClient.

// WithRetryableHttpClient sets the oauth2.HTTPClient key of the context to an // *http.Client made from makeRetryableHttpClient. // Some of the libraries we use will take the client out of the context via // oauth2.HTTPClient and use it, so this way we can add retries to external code.

Both RefreshToken and sendCheckAccessRequest are using the library "net/http".

@AzureMarker to confirm whether "net/http" respects the http client in the context?

net/http is a standard library, it doesn't reference third party code so there's no way it uses that context key.

This key is used in go-oidc:

guard/vendor/github.com/coreos/go-oidc/oidc.go

Line 60 in e0840f9

if c, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok {

The other HTTP client usages hard-code to go.kubeguard.dev/guard/util/httpclient's DefaultHTTPClient, which is just a normal HTTP client.

I made this change 57e588e. Could you please review it?

@AzureMarker gentle ping

Added some comments

AzureMarker · 2023-09-10T20:56:08Z

auth/providers/azure/graph/obo_tokenprovider.go

@@ -74,6 +75,9 @@ func (u *oboTokenProvider) Acquire(ctx context.Context, token string) (AuthRespo
 		klog.V(10).Infoln(cmd)
 	}

+	if c, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok {
+		u.client = c


This shouldn't modify the token provider (u.client). Store a reference to the HTTP client in a variable and modify it there.

The issue is if this is called twice (first with a context that holds an HTTP client, then with a context that has no HTTP client), the second call will use the HTTP client of the first.

removed this part to make the pr focus on authz.

AzureMarker · 2023-09-10T20:56:56Z

authz/providers/azure/rbac/rbac.go

@@ -374,6 +375,10 @@ func (a *AccessInfo) CheckAccess(request *authzv1.SubjectAccessReviewSpec) (*aut
 }

 func (a *AccessInfo) sendCheckAccessRequest(ctx context.Context, checkAccessURL url.URL, checkAccessBody *CheckAccessRequest, ch chan *authzv1.SubjectAccessReviewStatus) error {
+	if c, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok {
+		a.client = c


Same comment here, don't modify a.client, store it in a local variable.

fixed. Could you please check whether the fix meets the requirement? Thanks

bingosummer · 2023-11-21T08:06:21Z

@AzureMarker Could you please have a review again? Thanks

bingosummer · 2024-01-18T05:40:12Z

@weinong could you please have a review? Thanks

bingosummer · 2024-03-07T02:44:03Z

The PR is rebased. @weinong could you please have a review? Please let me know if anything is missing. Thanks!

bcho · 2024-03-07T21:48:51Z

authz/providers/azure/rbac/rbac.go

@@ -373,6 +377,11 @@ func (a *AccessInfo) CheckAccess(request *authzv1.SubjectAccessReviewSpec) (*aut
 }

 func (a *AccessInfo) sendCheckAccessRequest(ctx context.Context, checkAccessUsername string, checkAccessURL url.URL, checkAccessBody *CheckAccessRequest, ch chan *authzv1.SubjectAccessReviewStatus) error {
+	var clientInCtx *http.Client
+	if c, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok {


nit: can we move this client loader to util/azure so we can hide the context key from caller?

bcho · 2024-03-07T21:52:11Z

authz/providers/azure/azure_test.go

@@ -37,7 +37,8 @@ import (
 )

 const (
-	loginResp = `{ "token_type": "Bearer", "expires_in": 8459, "access_token": "%v"}`
+	loginResp            = `{ "token_type": "Bearer", "expires_in": 8459, "access_token": "%v"}`
+	httpClientRetryCount = 2


can we add a unit test for covering the retry behavior?

julienstroheker · 2024-03-07T23:38:41Z

authz/providers/azure/azure_test.go

+		ClientID:             "client_id",
+		ClientSecret:         "client_secret",
+		TenantID:             "tenant_id",
+		HttpClientRetryCount: httpClientRetryCount,


Can we assert that it try 2 times ?

Signed-off-by: Bin Xia <binxi@microsoft.com>

bingosummer · 2024-04-01T16:21:21Z

authz/providers/azure/azure_test.go

@@ -171,6 +174,7 @@ func TestCheck(t *testing.T) {
 		assert.Nilf(t, resp, "response should be nil")
 		assert.NotNilf(t, err, "should get error")
 		assert.Contains(t, err.Error(), "Error occured during authorization check")
+		assert.Contains(t, err.Error(), fmt.Sprintf("giving up after %d attempt", httpClientRetryCount+1))


@bcho @julienstroheker this line can assert that it gives up after 3 attempts.

could you please have another round of review? Thanks

bcho

LGTM

bingosummer · 2024-04-02T02:32:10Z

@weinong could you please have another round of review? Thanks

weinong · 2024-04-02T03:21:51Z

@julienstroheker PTAL

weinong

approved

bingosummer force-pushed the azure-authz-retrable-http-client branch from 68962f3 to 1cbc986 Compare August 7, 2023 06:31

bingosummer marked this pull request as ready for review August 7, 2023 06:43

bingosummer requested review from a team as code owners August 7, 2023 06:43

bingosummer changed the title ~~Use retrable http client in Azure authz provider~~ Use retryable http client in Azure authz provider Aug 7, 2023

karataliu approved these changes Aug 7, 2023

View reviewed changes

bingosummer force-pushed the azure-authz-retrable-http-client branch from 1cbc986 to ad88d63 Compare August 7, 2023 07:20

weinong reviewed Aug 9, 2023

View reviewed changes

authz/providers/azure/azure.go Outdated Show resolved Hide resolved

weinong reviewed Aug 9, 2023

View reviewed changes

bingosummer force-pushed the azure-authz-retrable-http-client branch from ad88d63 to bc5f06a Compare August 9, 2023 02:34

AzureMarker reviewed Sep 10, 2023

View reviewed changes

bingosummer force-pushed the azure-authz-retrable-http-client branch from 57e588e to 656a1ec Compare November 7, 2023 02:51

AzureMarker approved these changes Nov 27, 2023

View reviewed changes

bingosummer force-pushed the azure-authz-retrable-http-client branch from fd7361f to f30b13d Compare March 7, 2024 02:19

bcho reviewed Mar 7, 2024

View reviewed changes

julienstroheker reviewed Mar 7, 2024

View reviewed changes

bingosummer force-pushed the azure-authz-retrable-http-client branch 6 times, most recently from da82048 to 685ade4 Compare April 1, 2024 16:04

Use retrable http client in Azure authz provider

3e9b748

Signed-off-by: Bin Xia <binxi@microsoft.com>

bingosummer force-pushed the azure-authz-retrable-http-client branch from 685ade4 to 3e9b748 Compare April 1, 2024 16:13

bingosummer commented Apr 1, 2024

View reviewed changes

bcho approved these changes Apr 1, 2024

View reviewed changes

julienstroheker approved these changes Apr 3, 2024

View reviewed changes

weinong approved these changes Apr 3, 2024

View reviewed changes

weinong merged commit a74230a into kubeguard:master Apr 3, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use retryable http client in Azure authz provider #372

Use retryable http client in Azure authz provider #372

bingosummer commented Aug 7, 2023 •

edited

Loading

bingosummer commented Aug 7, 2023

weinong Aug 9, 2023

bingosummer Aug 9, 2023 •

edited

Loading

AzureMarker Aug 9, 2023

bingosummer Aug 29, 2023

bingosummer Sep 6, 2023

AzureMarker Sep 10, 2023

AzureMarker Sep 10, 2023

bingosummer Nov 7, 2023

AzureMarker Sep 10, 2023

bingosummer Nov 7, 2023

bingosummer commented Nov 21, 2023

bingosummer commented Jan 18, 2024

bingosummer commented Mar 7, 2024

bcho Mar 7, 2024

bcho Mar 7, 2024

bingosummer Apr 1, 2024

julienstroheker Mar 7, 2024

bingosummer Apr 1, 2024

bingosummer Apr 1, 2024

bingosummer Apr 1, 2024

bcho left a comment

bingosummer commented Apr 2, 2024

weinong commented Apr 2, 2024

weinong left a comment

Use retryable http client in Azure authz provider #372

Use retryable http client in Azure authz provider #372

Conversation

bingosummer commented Aug 7, 2023 • edited Loading

bingosummer commented Aug 7, 2023

Choose a reason for hiding this comment

bingosummer Aug 9, 2023 • edited Loading

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

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

bingosummer commented Nov 21, 2023

bingosummer commented Jan 18, 2024

bingosummer commented Mar 7, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcho left a comment

Choose a reason for hiding this comment

bingosummer commented Apr 2, 2024

weinong commented Apr 2, 2024

weinong left a comment

Choose a reason for hiding this comment

bingosummer commented Aug 7, 2023 •

edited

Loading

bingosummer Aug 9, 2023 •

edited

Loading