Update approach to kafka security for clients #17527
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
travisdowns
commented
Apr 1, 2024
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47161#018e9a76-f50c-446a-9af7-32721974565e ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47161#018e9a76-f503-46a1-83a3-699f455933e5 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47413#018eaad5-7437-4be9-9329-2d5a2160e947 |
oleiman
reviewed
Apr 3, 2024
travisdowns
force-pushed
the
td-security-config-update
branch
2 times, most recently
from
fd465d0
to
ef43e16
Compare
|
fd465d0 was a pure rebase on dev. |
|
ef43e16 addressed a comment about a spurious |
oleiman
previously approved these changes
Apr 4, 2024
oleiman
previously approved these changes
Apr 4, 2024
Dataclass is generally preferred over NamedTuple in python versions where it is available. Additionally, type all the members of SaslCredentials.
We manage a lot of kafka client security state (i.e., the auth related stuff needed to connect to the kafka endpoint) across all our client wrappers (and "application" wrappers like OMB) with some inconsistency in the way we apply default configuration from the associated redpanda service and some missing typing. This becomes more important as we introduce mixed cloud tests as now the default security settings are different depending on the underlying environment: TLS and SASL is always enabled in the cloud, but is off by default for vanilla RedpandaService. Introduce KafkaClientSecurity which is a strongly typed class which encapsulates these connection settings (in their simple non-SASL and SASL variations, not stuff like oauth currently). Expose it on RedpandaService and RedpandaServiceCloud. To be consumed further in subsequent commits in this series. Issue redpanda-data/core-internal#1199.
Add a very basic rpk test to cloud self test. This is relevant because rpk (and all clients, generally) need special handling to work in the cloud as the security settings are different.
It also needs to mock kafka_client_security() as this is used in the RpkTool constructor. Add a class docstring.
Use KafkaClientSecurity in rpk tool in the idiomatic way we hope to do for all clients. Basically take the default KafkaClientSecurity object from the redpanda service and then "merge in" any explicitly specified username, password, sasl mechanism or tls enabled settings passed into __init__. This merging does checks to make sure the overridden values form a reasonable set and then exposes the associated properties which we re-expose on RpkTool under the existing names (we no longer store them as attributes on the class, we just store the merged KCS object).
It now type checks at basic level.
Now checks clean in standard mode.
Use KafkaClientSecurity in python_librdkafka wrapper. Basically take the default KafkaClientSecurity object from the redpanda service and then "merge in" any explicitly specified username, password, sasl mechanism or tls enabled settings passed into __init__. This merging does checks to make sure the overridden values form a reasonable set and then store them as attributes on the class, rather we access them just in the merged KCS object.
KafkaServiceAdapter wraps a Kafka install while exposing the attributes that things like RpkTool (which expect a RedpandaService-ish) want. We need to additionally expose kafka_client_security getter on this wrapper as RpkTool requires it.
The default behavior already prints out the message passed to the constructor.
Use KafkaClientSecurity in our kafka cli tools wrapper. Basically take the default KafkaClientSecurity object from the redpanda service and then "merge in" any explicitly specified username, password, sasl mechanism or tls enabled settings passed into __init__. This merging does checks to make sure the overridden values form a reasonable set. We only use KCS in the "non-oauth" scenario.
Types at strict level, except for datapolicy stuff which is obsolete and will be deleted.
Remove this method from KafkaCliTools as it had a single caller which wasn't using its unique feature (ability to specify arbitrary config).
This is a "Protocol" for python typing which has the basic functionality a client like RpkTool or KafkaCliTools requires from RedpandaService and similar classes. This allows us to type the clients.
Now it type checks at pyright strict.
Should be str, not list[str].
After the updates to KafkaCliClient, it already does the right thing when SASL is enabled: taking its credentials from the Redpanda service so there is no need to manually extract and pass the credentials any more.
To satisfy a legacy use-case in SerdeClient which requires it.
After the updates to KafkaCliClient, it already does the right thing when SASL is enabled: taking its credentials from the Redpanda service so there is no need to manually extract and pass the credentials any more. Convert the schema registry tests to take advantage by dropping the explicit configuration. In another place SerdeClient expects a dict of configuration properties to pass to python scripts, to we use the new to_dict method of KafkaClientSecurity to get a dict with the expected values.
After the updates to KafkaCliClient, it already does the right thing when SASL is enabled: taking its credentials from the Redpanda service so there is no need to manually extract and pass the credentials any more. Convert the panda proxy tests to take advantage by dropping the explicit configuration.
After the updates to KafkaCliClient, it already does the right thing when SASL is enabled: taking its credentials from the Redpanda service so there is no need to manually extract and pass the credentials any more. This change converts _create_initial_topics in RedpandaTest to take advantage by dropping the explicit configuration.
In place of the to-be-removed security_config()
Type checks cleanly at pyright strict.
Delete security_config() as there are no remaining callers. Clients of this class should use KafkaClientSecurity instead.
SSL_SECURITY has *no* SASL Co-authored-by: Oren Leiman <mumblemumble777@gmail.com>
Co-authored-by: Oren Leiman <mumblemumble777@gmail.com>
travisdowns
force-pushed
the
td-security-config-update
branch
from
d7840cd
to
8710ae5
Compare
|
Force 8710ae5 to resolve a merge conflict only. |
oleiman
approved these changes
Apr 5, 2024
|
👏 👏 👏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary thrust of this change is to consolidate the way our "client wrappers" (i.e., the python code that wraps a Kafka client) get their security credentials to authenticate to the Kafka endpoint. This change only changes ducktape test framework and test code, and does not touch Redpanda itself.
Specifically we want to:
KafkaClientSecurity)security_config()method (removed in this series) but usingKafkaClientSecurityand supporting more server-side configurationThe primary motivation is that ducktape tests running against the cloud have SASL and TLS on by default. This is the opposite of vanilla ducktape where TLS and SASL are disabled. For existing client wrappers (and hence existing tests) to "just work" when run against the cloud, we need a consistent and expanded way for client wrappers to handle their kafka credentials.
The basic approach that client wrappers will work after this change (before this change the behavior "varied"):
RedpandaService.kafka_security_credentials().KafkaSecurityCredentials.override()for details.Backports Required
Release Notes