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.

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

Work around an azure HTTP/2 bug #183

Merged
merged 4 commits into from Sep 5, 2023
Merged

Conversation

sgmiller
Copy link
Collaborator

@sgmiller sgmiller commented Sep 1, 2023

Azure's client by default doesn't health check HTTP/2 keepalive conns, and instead lets the os close them after inactivity if they become unhealthy or are closed unexpectedly. This can take 15 minutes. If too many idle conns get hung in this state the client becomes unusable. This workaround was provided by Microsoft as they investigate the issue.

Issue: Azure/azure-sdk-for-go#21346
Workaround is frrom that issue but backported to the older Azure client.

@sgmiller sgmiller requested review from jefferai and a team as code owners September 1, 2023 14:32
@sgmiller sgmiller requested review from tcrayford, blueberry-pi and a team September 1, 2023 14:33
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

It appears you forgot to add your unit test commits to the PR. Would you mind adding them?

@sgmiller
Copy link
Collaborator Author

sgmiller commented Sep 1, 2023

Unfortunately this can't be unit tested. It relies on HTTP/2 connections being dropped on the Azure side, which HCP has observed very intermittently. The rest of the change is covered by the acceptance tests that existed already.

FWIW:
image

Copy link
Contributor

@victorr victorr left a comment

Choose a reason for hiding this comment

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

👍

@sgmiller sgmiller dismissed jimlambrt’s stale review September 1, 2023 20:28

Responded, but proceeding due to the time sensitive nature of next weeks release.

go mod tidy, so the tests stop failing
This change allows us to write some unit tests that use
a self-signed cert using httptest.
@jimlambrt
Copy link
Collaborator

I'm a bit skeptical that the existing vault unit tests asserted anything about the custom transport you've added.

I've pushed some unit tests which do some minimal assertions about the fix.

Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Obviously this mostly relies on MSFT being correct in their given workaround but seems fine... any chance you have figured out a repeated way to trigger it to see if this actually helps?

@sgmiller
Copy link
Collaborator Author

sgmiller commented Sep 5, 2023

Unfortunately no. I might be able to do it with iptables but not great for unit testing.

@sgmiller
Copy link
Collaborator Author

sgmiller commented Sep 5, 2023

I'm a bit skeptical that the existing vault unit tests asserted anything about the custom transport you've added.

I've pushed some unit tests which do some minimal assertions about the fix.

Okay, that's a useful thing to verify. Thanks.

@sgmiller sgmiller merged commit a8fa5b8 into main Sep 5, 2023
29 checks passed
@sgmiller sgmiller deleted the sgm/azure-http2conn-workaround branch September 5, 2023 15:07
sgmiller added a commit that referenced this pull request Sep 5, 2023
Backport of the Azure HTTP/2 workaround to vault 1.12.x maintenance branch.
---------

Co-authored-by: Jim <jlambert@hashicorp.com>
sgmiller added a commit that referenced this pull request Sep 5, 2023
* Work around an azure HTTP/2 butg

* fix/chore (azurevault): simple fix for go.sum

go mod tidy, so the tests stop failing

* test (azurevault): add withCertPool to getKeyVaultClient(...)

This change allows us to write some unit tests that use
a self-signed cert using httptest.

* test (azurevault): add unit tests for getKeyVaultClient(...)

---------

Co-authored-by: Jim <jlambert@hashicorp.com>
cldmstr added a commit to Xovis/go-kms-wrapping that referenced this pull request Sep 6, 2023
jimlambrt pushed a commit that referenced this pull request Sep 6, 2023
* Add support for azure workload identity authentication

Use the Azure Workload Identity federated identity magic when it is available to authenticate against
Azure keyvault. This allows vault to run in an AKS cluster with service accounts federated to
service principals or managed identites.

This also requires the azure libraries to be migrated to the newest version as the autorest libraries are
being depricated.

* use passed context in setup

* Add Work around an azure HTTP/2 bug (#183)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants