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

CORE-33: Hooked OpenSSL context service into app #17214

Conversation

michael-redpanda
Copy link
Contributor

@michael-redpanda michael-redpanda commented Mar 20, 2024

Construction and start of service is very early in application startup to ensure that any services that require crypto are using the appropriate thread local library context and that the contexts have been initialized appropriately.

Fixes: https://github.com/redpanda-data/core-internal/issues/1191

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
  • v23.3.x
  • v23.2.x

Release Notes

  • None

@michael-redpanda
Copy link
Contributor Author

/dt

1 similar comment
@michael-redpanda
Copy link
Contributor Author

/dt

@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/46513#018e5d9a-e4be-4aa4-bb38-e37205dd29bf:

"rptest.tests.cpu_profiler_admin_api_test.CPUProfilerAdminAPITest.test_get_cpu_profile_with_override"

@michael-redpanda
Copy link
Contributor Author

/dt

@michael-redpanda
Copy link
Contributor Author

/dt

@michael-redpanda
Copy link
Contributor Author

/dt

Cluster fixture tests appear to start multiple Redpanda instances
on the same thread.  This was resulting in an assertion during
service shutdown because the replaced context was not the expected
one (it was replaced by startup of the service on the same thread).

A new environment variable has been added to the CTest environment
when running a fixture test.  The short circuit now detects if the
global default context is the current context and if it is not,
will exit the start up of the service if the environment is an RP
fixture environment, as denoted by the added environmental variable.

Signed-off-by: Michael Boquard <michael@redpanda.com>
Construction and start of service is very early in application
startup to ensure that any services that require crypto are using
the appropriate thread local library context and that the contexts
have been initialized appropriately.

Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
Copy link
Contributor Author

/dt

@michael-redpanda michael-redpanda marked this pull request as ready for review March 25, 2024 12:24
@michael-redpanda michael-redpanda requested review from BenPope and a team as code owners March 25, 2024 12:24
@michael-redpanda michael-redpanda requested review from savex and removed request for a team March 25, 2024 12:24
@michael-redpanda
Copy link
Contributor Author

/dt

@michael-redpanda michael-redpanda changed the title Hooked OpenSSL context service into app CORE-33: Hooked OpenSSL context service into app Mar 25, 2024
Comment on lines +324 to +331
if (in_rp_fixture_test()) {
lg.warn(
"Detected RP Fixture test, not initializing OSSL Context service");
return ss::make_ready_future();
} else {
return _impl->start();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

question: Does this mean that the default context is used in fixture tests? What's the implication of that? What are we not testing, are we sure it's limited to not blocking the reactor during I/O?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean that the default context is used in fixture tests?

yes

What's the implication of that? What are we not testing, are we sure it's limited to not blocking the reactor during I/O?

We wouldn't be able to run OpenSSL in FIPS mode in fixture tests, however CDT tests and unit tests will be able to. I believe I/O, if there is any, will be limited to initialization of the OpenSSL library which would only happen once at the start of the testing application and not in between the different test cases.

src/v/redpanda/application.cc Show resolved Hide resolved
@michael-redpanda michael-redpanda merged commit e0bdc1d into redpanda-data:dev Mar 26, 2024
18 of 19 checks passed
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

4 participants