Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use the request context throughout the code, and add retries & timeouts to more Azure auth provider calls #358

Merged
merged 7 commits into from
Mar 16, 2023
Merged

Use the request context throughout the code, and add retries & timeouts to more Azure auth provider calls #358

merged 7 commits into from
Mar 16, 2023

Conversation

AzureMarker
Copy link
Contributor

@AzureMarker AzureMarker commented Feb 28, 2023 •
edited
Loading

There are two improvements in this PR:

  1. The request context is used. This context gets canceled when the client cancels the request or the connection closes, stopping operations from running longer than needed.
  2. The Azure auth provider uses a retryable HTTP client with request timeouts wherever possible. This should improve the reliability of the provider when network connectivity is unstable.

Tested by deploying to an Azure Resource Bridge and verifying auth calls to Guard still worked.

@AzureMarker
Use the request context to cancel processing if request is canceled
e584812 
Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
@AzureMarker
Use a retrying HTTP client wherever possible in azure auth provider
5b0293a 
Also includes a 3 second timeout per request.

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
@AzureMarker
Reformat
8ab8914 
Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
@AzureMarker AzureMarker marked this pull request as ready for review March 1, 2023 19:10
@AzureMarker AzureMarker requested review from a team as code owners March 1, 2023 19:10
@AzureMarker
Merge branch 'master' into fix/azure-timeouts-retries
ac189f5 
# Conflicts:
#	auth/providers/azure/graph/aks_tokenprovider.go
#	auth/providers/azure/graph/clientcredential_tokenprovider.go
#	auth/providers/azure/graph/obo_tokenprovider.go
#	auth/providers/github/github_test.go
#	auth/providers/google/google_test.go
@AzureMarker
Clarify a comment
c8f6b9c 
Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
@AzureMarker
Merge branch 'master' into fix/azure-timeouts-retries
404f2e4 
# Conflicts:
#	server/server.go
#	util/azure/utils.go
Anumita
Anumita approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@Anumita Anumita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. @AzureMarker should we go ahead and merge?

@AzureMarker
Copy link
Contributor Author

lgtm. @AzureMarker should we go ahead and merge?

Yes, if possible (we need an approval from someone who is authorized to merge).

@krdhruva krdhruva added the automerge Kodiak will auto merge PRs that have this label label Mar 16, 2023
@kodiakhq kodiakhq bot merged commit c67ed7e into kubeguard:master Mar 16, 2023
krdhruva pushed a commit that referenced this pull request Mar 16, 2023
@AzureMarker @krdhruva
Use the request context throughout the code, and add retries & timeou…
7c26eb0 
…ts to more Azure auth provider calls (#358)

* Use the request context to cancel processing if request is canceled

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Use a retrying HTTP client wherever possible in azure auth provider

Also includes a 3 second timeout per request.

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Reformat

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Clarify a comment

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

---------

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
krdhruva pushed a commit that referenced this pull request Mar 16, 2023
@AzureMarker @krdhruva
Use the request context throughout the code, and add retries & timeou…
8e8937e 
…ts to more Azure auth provider calls (#358)

* Use the request context to cancel processing if request is canceled

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Use a retrying HTTP client wherever possible in azure auth provider

Also includes a 3 second timeout per request.

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Reformat

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Clarify a comment

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

---------

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>
kodiakhq bot pushed a commit that referenced this pull request Mar 16, 2023
@krdhruva @dependabot
@asuffield @tamalsaha @Anumita @AzureMarker
Releasing 0.12.2 (#361)
b276cd8 
* Bump golang.org/x/net from 0.0.0-20220725212005-46097bf591d3 to 0.7.0 (#357)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.0.0-20220725212005-46097bf591d3 to 0.7.0.
- [Release notes](https://github.com/golang/net/releases)
- [Commits](https://github.com/golang/net/commits/v0.7.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>

* Update clientauthentication api to v1beta1 (#355)

v1alpha1 is removed in k8s v24, so this needs updating to allow
forward compatibility.

v1 does now exist, but k8s deps will have to be updated to v22 to use
that, and this is a more minimal first step.

Signed-off-by: andrew.suffield <andrew.suffield@robinhood.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>

* Use Go 1.20 and update dependencies (#359)

Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>

* Add support for reconciling discover resources (#354)

* add check for metrics error

Signed-off-by: Anumita <ansheno@microsoft.com>

* add reconciling of discover resources

Signed-off-by: Anumita <ansheno@microsoft.com>

* add lock and map interface

Signed-off-by: Anumita <ansheno@microsoft.com>

* add deepcopy of map

Signed-off-by: Anumita <ansheno@microsoft.com>

* address comments

Signed-off-by: Anumita <ansheno@microsoft.com>

* add tests for reconcile

Signed-off-by: Anumita <ansheno@microsoft.com>

---------

Signed-off-by: Anumita <ansheno@microsoft.com>
Co-authored-by: Krupesh <krdhruva@microsoft.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>

* fix log line for correlation id (#360)

Signed-off-by: Anumita <ansheno@microsoft.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>

* Use the request context throughout the code, and add retries & timeouts to more Azure auth provider calls (#358)

* Use the request context to cancel processing if request is canceled

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Use a retrying HTTP client wherever possible in azure auth provider

Also includes a 3 second timeout per request.

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Reformat

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

* Clarify a comment

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>

---------

Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Krupesh Dhruva <krdhruva@microsoft.com>
Signed-off-by: andrew.suffield <andrew.suffield@robinhood.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Anumita <ansheno@microsoft.com>
Signed-off-by: Mark Drobnak <markdrobnak@microsoft.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Andrew Suffield <andrew.suffield@robinhood.com>
Co-authored-by: Tamal Saha <tamal@appscode.com>
Co-authored-by: Anumita Shenoy <ansheno@microsoft.com>
Co-authored-by: Mark Drobnak <markdrobnak@microsoft.com>
@AzureMarker AzureMarker deleted the fix/azure-timeouts-retries branch March 16, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amanohar amanohar amanohar left review comments

@Anumita Anumita Anumita approved these changes

@krdhruva krdhruva krdhruva approved these changes

Assignees
No one assigned
Labels
automerge Kodiak will auto merge PRs that have this label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AzureMarker @Anumita @amanohar @krdhruva