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

Extensible Kafka Auth Config #1152

Merged
merged 6 commits into from
Nov 16, 2023
Merged

Conversation

stvnksslr
Copy link
Contributor

@stvnksslr stvnksslr commented Nov 7, 2023

Current kafka sink does not support authentication which makes it difficult to adopt or use in many situations. This PR allows the kafka sink to behave as it is but to have an auth block added to easily cater to most if not all the supported authentications schemas of the python kafka package.

Tested With

  • Apache kafka
  • AWS MSK

Not Tested

  • Confluent kafka
  sinksConfig:
  - kafka_sink:
      name: kafka_sink
      kafka_url: "localhost:9096"
      topic: "robusta-playbooks"
      auth:
          sasl_mechanism: SCRAM-SHA-512
          security_protocol: SASL_SSL
          sasl_plain_username: robusta
          sasl_plain_password: password

additionally updated skaffold configs to the latest version, i can easily roll this back if its not wanted or needed.

@CLAassistant
Copy link

CLAassistant commented Nov 7, 2023

CLA assistant check
All committers have signed the CLA.

docs/configuration/sinks/kafka.rst Show resolved Hide resolved
skaffold.yaml Outdated Show resolved Hide resolved
…eft blank behavior will be unchanged, if an auth block is used credentials will be unwrapped into the connection arguments.
…ractices, apple-m1 profile can likely be removed at a later date as skaffold will match the architecture of the k8s cluster or runner.
…d best practices, apple-m1 profile can likely be removed at a later date as skaffold will match the architecture of the k8s cluster or runner."

This reverts commit 060e8e1.
@stvnksslr
Copy link
Contributor Author

stvnksslr commented Nov 8, 2023

Found an issue with my implementation where you couldnt use the {{ .env.SOME_SECRET }} env substitution because the dict wasnt being checked. I have two solutions here, the one ive included in the latest commit extends replace_env_vars_values to be able to recursively search any dictionaries too.

elif isinstance(value, dict):
    env_var_value = replace_env_vars_values(value)
    if env_var_value:
        values[key] = env_var_value

but this change in the root validator might be more encompassing than the maintainers want, Option B would be to extend just the kafka_sink_params to do a recursive search.

     Possible Option B.
     @validator("auth", pre=True, always=True)
     def auth_env_values_validation(cls, value: Dict):
         updated = replace_env_vars_values(value)
         return updated

@LeaveMyYard let me know what you think when you have time.

Copy link
Contributor

@LeaveMyYard LeaveMyYard left a comment

Choose a reason for hiding this comment

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

I think the first solution could be usefull for such cases in a future, so we can keep it

@pavangudiwada pavangudiwada added the needs-review A community contribution for Robusta team to review label Nov 16, 2023
@LeaveMyYard LeaveMyYard merged commit 45954c0 into robusta-dev:master Nov 16, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review A community contribution for Robusta team to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants