Skip to content

fix(kafka): name the profile and env var when SASL credentials are missing - #95874

Draft
posthog[bot] wants to merge 2 commits into
posthog-self-driving/fixdlq-replay-pass-sasl-credentials-to-9a9246from
posthog-self-driving/fixkafka-fail-loudly-when-a-cluster-c48187
Draft

fix(kafka): name the profile and env var when SASL credentials are missing#95874
posthog[bot] wants to merge 2 commits into
posthog-self-driving/fixdlq-replay-pass-sasl-credentials-to-9a9246from
posthog-self-driving/fixkafka-fail-loudly-when-a-cluster-c48187

Conversation

@posthog

@posthog posthog Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Problem

  • A deployment that misses one SASL credential drops every internal event on that cluster profile, and the operator cannot tell why from the error.
  • Health-check alerts, activity-log internal events, and query usage events all take the same produce_internal_event path, so all three go silent together.
  • _resolve_profile accepts a profile with a SASL protocol and no credentials, and _build_sasl_config hands it to librdkafka anyway.
  • librdkafka answers with _INVALID_ARG: sasl.username and sasl.password must be set, which names no profile and no env var, so the reader goes after the cluster.
  • get_producer caches only on success, so every produce attempt rebuilds the producer and raises the same message again.

Changes

  • A produce against a profile that misses a SASL credential now fails with the profile name and the exact env vars to set, so the operator fixes the config instead of searching the cluster.
Kafka profile 'cyclotron' uses SASL_SSL, but these env vars have no value:
KAFKA_CYCLOTRON_SASL_USER, KAFKA_CYCLOTRON_SASL_PASSWORD.
Set them, or the matching KAFKA_DEFAULT_* vars.
  • The check sits in get_profile_settings, the one place every routed producer and consumer resolves a profile through. Both producer builders now route through it too.
  • The check does not sit in _resolve_profile itself. That runs at settings import for every profile, so a raise there would stop a process from booting over a profile it never produces to.
  • Self-hosted base64 cert mode stays exempt. It forces the protocol to SSL and attaches no SASL credentials, so a missing credential is not a defect there.
  • Mechanical: the DLQ replay activities drop their own copy of the SASL check and reuse this one. KafkaProfileSettings gains uses_sasl and check_sasl_credentials.
  • Nothing user-visible changes. The error reaches logs and error tracking, not a product surface.

Note

A profile that is configured correctly is unaffected: the check runs on every profile resolution but only raises when the protocol needs SASL.

How did you test this code?

  • New cases in posthog/kafka_client/test/test_kafka_settings.py cover the regression: a SASL profile that misses the mechanism, the user, or the password raises an error naming that profile and that env var. No existing test looked at credential completeness.
  • New GetProfileSettingsTest in posthog/kafka_client/test/test_routing.py pins the two router decisions: a SASL profile with no credentials is rejected, and base64 cert mode is exempt.
  • The DLQ replay test now asserts the router's error becomes a non-retryable Temporal error, because a retry cannot fix a config gap.
  • Repo-wide uv run mypy --cache-fine-grained . is clean.
  • Not verified: posthog/kafka_client/test/test_client.py needs a live ClickHouse, which this sandbox has no access to. Those 9 tests error at setup on the same connection refusal before and after the change. No produce ran against a real SASL cluster.

Automatic notifications

  • Publish to changelog?

Docs update

None. No documented workflow or config surface changes.

🤖 Agent context

Autonomy: Fully autonomous

  • Written by Claude Code from a PostHog inbox report about dropped internal events on a Kafka profile. Skills invoked: /writing-pr-descriptions, /writing-simplified-technical-english, /simplify, /writing-tests.
  • Stacked on #94236, which adds hosts_configured and the DLQ replay fail-fast path. This layer moves the SASL half of that check onto the profile itself, so one implementation serves every caller.
  • Considered and rejected: raising inside _resolve_profile. It is the site the report named, but it runs at settings import, so a half-configured profile would take down a whole process rather than the produce path that uses it.
  • Considered and rejected: raising inside _build_sasl_config. It has the credentials but not the profile name, so its message could not name the env var to set.
  • The base64 cert mode exemption exists because _KafkaProducer and the warehouse person-property consumer both force the protocol to SSL in that mode. Without the exemption a working self-hosted deployment would start failing.
  • Public artifact: the diff and this description carry no material from the agent session. No customer data, log, or internal thread content is present.

Created with PostHog Desktop from this inbox report.

…ssing

The router now rejects a cluster profile that uses SASL_PLAINTEXT or SASL_SSL
while a SASL credential is unset. The error names the profile and the
KAFKA_<PROFILE>_SASL_* vars, instead of librdkafka's _INVALID_ARG, which names
neither.

The check runs in `get_profile_settings`, so every producer and consumer that
routes through it reports the same message. Self-hosted base64 cert mode is
exempt, because it forces the protocol to SSL and attaches no credentials.

The DLQ replay activities drop their own copy of the check and reuse this one.

Generated-By: PostHog Desktop
Task-Id: d2888f81-46cf-416c-922d-d3e6a417e1e2
@posthog

posthog Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

🦔 PostHog Review reviewed this pull request

Nothing worth raising this time, so here's a calming picture instead:

Someone relaxing in a sunny garden

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not approved — escalated to a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

This rewires how every Kafka producer and consumer resolves its cluster profile (an event-ingestion path shared by health checks, activity-log, and usage events), and no completed review or approval exists yet — the only discussion comment is an automated review still at step 1 of 6, so there is no independent assurance to rely on.

  • Change touches the shared Kafka producer/consumer profile-resolution path (event ingestion), which is risky territory per policy.
  • No completed human or agent review exists on the current head — the automated PostHog review comment shows it is still in progress (step 1/6), not a finished verdict.
  • No ownership match in owners.yaml/product.yaml, and the author is a machine implementation bot so familiarity/org-membership signals don't apply.
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 69L, 3F substantive, 141L/6F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (141L, 6F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ c66dcae · reviewed head c66dcae

@stamphog stamphog Bot added the reviewhog ($$$) Reviews pull requests before humans do label Sep 7, 2026
…s-to-9a9246' into posthog-self-driving/fixkafka-fail-loudly-when-a-cluster-c48187

Generated-By: PostHog Desktop
Task-Id: d2888f81-46cf-416c-922d-d3e6a417e1e2

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

Touches the Kafka producer/consumer resolution path (event ingestion), but the change is well-scoped, tested (new unit tests for the validation, the base64 exemption, and the DLQ non-retryable wrapping), verified against the fallback env-var chain so it can't false-positive, and the automated PostHog review bot found no concerns.

Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 69L, 3F substantive, 141L/6F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (141L, 6F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ 4540f89 · reviewed head 4540f89

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

Labels

reviewhog ($$$) Reviews pull requests before humans do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants