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

http: Fix double call to stop() in http::client #18304

Merged

Conversation

graphcareful
Copy link
Contributor

@graphcareful graphcareful commented May 8, 2024

http::client::stop() can be called twice in the event the client is used via the with_client method.

The method had unconditionally called stop() in a finally clause with the intention to have users not have to manually do this and forget to call stop.

However stop() can also be called within certain exception handlers within methods invoked by http::client, ones that handle tls::verification_error exceptions.

This patch skips the call to stop() in the event a tls::verification_error exception was encountered.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

Bug Fixes

  • Fixes a bug in the http client where a crash may occur in the event certain tls verification errors are observed

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented May 8, 2024

new failures in https://buildkite.com/redpanda/redpanda/builds/48827#018f58a1-ea7e-45e9-81fe-4ce763ff34a1:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48827#018f58b2-3f1f-471b-8dcf-0a8fce322c66:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_abs_cloud_validation.update_str=ABS_VM_INSTANCE_METADATA"
"rptest.tests.audit_log_test.AuditLogTestOauth.test_kafka_oauth.authz_match=AuthorizationMatch.RBAC"
"rptest.tests.e2e_iam_role_test.AWSRoleFetchTests.test_write"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTest.test_admin_revoke"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTlsTest.test_admin_revoke"
"rptest.tests.cluster_config_test.ClusterConfigTest.test_cloud_validation"
"rptest.tests.redpanda_oauth_test.OIDCLicenseTest.test_license_nag.authn_config=.http_authentication.OIDC.BASIC"

new failures in https://buildkite.com/redpanda/redpanda/builds/48827#018f58b2-3f17-44fe-8ea4-4f14c2f045b9:

"rptest.tests.e2e_iam_role_test.STSRoleFetchTests.test_write"
"rptest.tests.cloud_storage_scrubber_test.CloudStorageScrubberTest.test_scrubber.cloud_storage_type=CloudStorageType.ABS"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTest.test_admin_whoami"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTlsTest.test_admin_whoami"
"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"
"rptest.tests.cluster_config_test.ClusterConfigTest.test_valid_settings"
"rptest.tests.redpanda_oauth_test.OIDCLicenseTest.test_license_nag.authn_config=.sasl_mechanisms.OAUTHBEARER.SCRAM"

new failures in https://buildkite.com/redpanda/redpanda/builds/48827#018f58b2-3f1a-4b5f-bc05-169518a15653:

"rptest.tests.audit_log_test.AuditLogTestOauth.test_admin_oauth"
"rptest.tests.e2e_iam_role_test.ShortLivedCredentialsTests.test_short_lived_credentials"
"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.redpanda_oauth_test.OIDCReauthTest.test_oidc_reauth"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTest.test_init"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTlsTest.test_init"

new failures in https://buildkite.com/redpanda/redpanda/builds/48827#018f58b2-3f1d-4865-8855-a9cf5067b543:

"rptest.tests.audit_log_test.AuditLogTestOauth.test_kafka_oauth.authz_match=AuthorizationMatch.ACL"
"rptest.tests.redpanda_oauth_test.JavaClientOIDCTest.test_java_client"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTest.test_admin_invalidate_keys"
"rptest.tests.redpanda_oauth_test.RedpandaOIDCTlsTest.test_admin_invalidate_keys"

new failures in https://buildkite.com/redpanda/redpanda/builds/48827#018f58a1-ea7c-48cd-99f2-fa992d9bd444:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

- http::client::stop() can be called twice in the event the client is
used via the `with_client` method.

- The method had unconditionally called stop() in a finally clause with
the intention to have users not have to manually do this and forget to
call stop.

- However stop() can also be called within certain exception handlers
within methods invoked by http::client, ones that handle
tls::verification_error exceptions.

- This patch adds a boolean to our http client so that stop() can early
exit if it has already been called.
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented May 8, 2024

@graphcareful
Copy link
Contributor Author

@michael-redpanda michael-redpanda merged commit 18a4197 into redpanda-data:dev May 13, 2024
17 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18304-v23.3.x-823 remotes/upstream/v23.3.x
git cherry-pick -x 2ed203deceffa1d560cdee64131b88f302069318

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18304-v23.2.x-934 remotes/upstream/v23.2.x
git cherry-pick -x 2ed203deceffa1d560cdee64131b88f302069318

Workflow run logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants