Add topic prefix support for multi-tenant isolation (HYPERFLEET-283) #22
Conversation
|
Skipping CI for Draft Pull Request. |
WalkthroughThis pull request introduces a configurable broker topic value exposed as a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (1)internal/config/config_test.go (1)
🪛 LanguageTooldocs/running-sentinel.md[grammar] ~317-~317: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) 🔇 Additional comments (5)
Comment |
rafabene
force-pushed
the
HYPERFLEET-283-topic-prefix
branch
9 times, most recently
from
eb61aa5 to
38a0d3a
Compare
Implement BROKER_TOPIC_PREFIX environment variable and topic_prefix config
option to enable isolation between different environments or tenants sharing
the same message broker.
Changes:
- Add TopicPrefix field to SentinelConfig with env var override
- Update Sentinel to prefix topics: {prefix}-{resourceKind}
- Add Helm chart support for broker.topicPrefix
- Update documentation with naming conventions for namespace, image tags,
and topic prefixes following architecture repo conventions
- Remove AWS SQS reference (only RabbitMQ and Google Pub/Sub supported)
When BROKER_TOPIC_PREFIX is set to "hyperfleet-dev-rafael", topics become
"hyperfleet-dev-rafael-Cluster" instead of just "Cluster".
Empty prefix maintains backwards compatibility (no prefix added).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
rafabene
force-pushed
the
HYPERFLEET-283-topic-prefix
branch
from
38a0d3a to
5bd4444
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (1)
176-182: Add blank lines around the Topic Naming table for markdownlintThe Topic Naming /
BROKER_TOPIC_PREFIXdocs look correct and aligned with the implementation. To satisfy MD058 (“blanks-around-tables”), add a blank line before and after the table:-**Topic Naming (Multi-tenant Isolation):** -| Variable | Description | Example | +**Topic Naming (Multi-tenant Isolation):** + +| Variable | Description | Example | ... -| `BROKER_TOPIC_PREFIX` | Prefix for topic names (optional) | `hyperfleet-dev` | - -When `BROKER_TOPIC_PREFIX` is set, topics are named `{prefix}-{resourceKind}` ... +| `BROKER_TOPIC_PREFIX` | Prefix for topic names (optional) | `hyperfleet-dev` | + +When `BROKER_TOPIC_PREFIX` is set, topics are named `{prefix}-{resourceKind}` ...internal/sentinel/sentinel.go (1)
121-125: Topic prefix application is correct; consider logging the topicThe topic construction logic correctly falls back to
resource.Kindand uses{TopicPrefix}-{resource.Kind}when a prefix is set, preserving existing behavior.For easier debugging of multi-tenant setups, consider including the resolved
topicin the publish log message:- s.logger.Infof(ctx, "Published event resource_id=%s phase=%s reason=%s", - resource.ID, resource.Status.Phase, decision.Reason) + s.logger.Infof(ctx, "Published event resource_id=%s phase=%s reason=%s topic=%s", + resource.ID, resource.Status.Phase, decision.Reason, topic)deployments/helm/sentinel/values.yaml (1)
107-111: broker.topicPrefix default and docs look goodThe
topicPrefixfield and comments accurately describe the{topicPrefix}-{resourceKind}behavior and defaulting to “no prefix” when empty. This aligns with the Deployment template and config loader.If you want to avoid any confusion, you could clarify that
{{ .Release.Namespace }}is an example value to pass via--set broker.topicPrefix=..., not a template evaluated insidevalues.yaml.internal/config/config.go (1)
32-33: Clarify env override semantics for TopicPrefix (and optionally support clearing)The
TopicPrefixfield and BROKER_TOPIC_PREFIX override are wired correctly and match the rest of the feature.Two small follow-ups to consider:
- The comment says “Override … from environment variable if set”, but the code only overrides when the value is non-empty. You could clarify this as “if non-empty” to avoid confusion.
- If you ever need to clear a
topic_prefixfrom config via env, the currentos.Getenvcheck won’t allow that. Usingos.LookupEnvwould let you treat “present but empty” as an explicit override:- // Override topic_prefix from environment variable if set - // Environment variable takes precedence over config file - if prefix := os.Getenv("BROKER_TOPIC_PREFIX"); prefix != "" { - cfg.TopicPrefix = prefix - } + // Override topic_prefix from environment variable if explicitly provided + // Environment variable takes precedence over config file (including empty value) + if prefix, ok := os.LookupEnv("BROKER_TOPIC_PREFIX"); ok { + cfg.TopicPrefix = prefix + }This keeps precedence guarantees while allowing explicit “no prefix” via env if needed.
Also applies to: 95-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)configs/dev-example.yaml(1 hunks)deployments/helm/sentinel/templates/deployment.yaml(1 hunks)deployments/helm/sentinel/values.yaml(1 hunks)docs/running-sentinel.md(10 hunks)docs/testcontainers.md(0 hunks)internal/config/config.go(3 hunks)internal/config/config_test.go(1 hunks)internal/sentinel/sentinel.go(1 hunks)
💤 Files with no reviewable changes (1)
- docs/testcontainers.md
🧰 Additional context used
🧬 Code graph analysis (1)
internal/config/config_test.go (1)
internal/config/config.go (1)
LoadConfig(74-114)
🪛 LanguageTool
docs/running-sentinel.md
[grammar] ~314-~314: Ensure spelling is correct
Context: ...rep hyperfleet_sentinel ``` #### Check PodMonitoring Status List PodMonitoring resources: ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
177-177: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔇 Additional comments (6)
deployments/helm/sentinel/templates/deployment.yaml (1)
63-67: BROKER_TOPIC_PREFIX wiring in Deployment looks correctConditional emission on
.Values.broker.topicPrefixwith a quoted value is consistent with the config loader and keeps the env var optional. No issues found.configs/dev-example.yaml (1)
13-15: Dev example topic-prefix docs are consistent with behaviorThe BROKER_TOPIC_PREFIX example and resulting topic names match the implementation and higher-level docs. Looks good.
internal/config/config_test.go (1)
411-494: TopicPrefix config loader tests are comprehensiveThe new tests cover all key cases (env-only, env-overrides-config, config-only, and empty/default) and align with the current LoadConfig semantics. Environment setup/teardown is handled explicitly, so there’s no hidden cross-test coupling. Looks solid.
docs/running-sentinel.md (3)
18-24: GKE section restructuring and ToC anchors are coherentThe updated GKE flow (env vars → connect cluster → build → push → Helm → verify → cleanup) is clear, and the ToC anchors correctly match the new headings. This makes the doc much easier to follow for end-to-end testing.
Also applies to: 195-224
62-67: Local broker + topic prefix guidance matches implementationThe Pub/Sub emulator setup and the new “Set Topic Prefix (Optional)” section are consistent with the actual config behavior:
PUBSUB_EMULATOR_HOSTusage aligns with typical emulator workflows.BROKER_TOPIC_PREFIX=hyperfleet-dev-${USER}and the example topic names{prefix}-{resourceKind}match the logic ininternal/sentinel/sentinel.go.No issues from a correctness standpoint.
Also applies to: 89-123
238-277: GKE deployment flags correctly wire topicPrefix and metrics verificationThe Helm command wiring:
--set image.repository=gcr.io/${GCP_PROJECT}/sentinel--set image.tag=${IMAGE_TAG}--set broker.googlepubsub.projectId=${GCP_PROJECT}--set broker.topicPrefix=${NAMESPACE}matches the chart’s values and the Deployment template, ensuring
BROKER_TOPIC_PREFIXis set and topics are namespaced per developer. The subsequent port-forward, metrics, and PodMonitoring instructions are consistent with the earlier configuration.Looks accurate and actionable.
Also applies to: 295-346, 361-386
| value: /etc/sentinel/broker.yaml | ||
| # Topic prefix for multi-tenant isolation | ||
| {{- if .Values.broker.topicPrefix }} | ||
| - name: BROKER_TOPIC_PREFIX |
There was a problem hiding this comment.
Could we set directly the TOPIC_NAME and not build it internally within the code?
I think it makes it more explicit to have it in the configuration for the Sentinel what topic it is writing to. Just describe the pod and you find the name of the topic being used.
Of course, this doesn't apply if one Sentinel is writing to different topics, which is not the current design.
One thing with the current approach is that we end up with topic names like amarin-Cluster notice the uppercase C
We could have something already pre-set like:
- name: BROKER_TOPIC
value: {{ .Release.Namespace}}-cluster
There was a problem hiding this comment.
Done. PR for architecture needs to be merge to have this aligned: https://github.com/openshift-hyperfleet/architecture/pull/42/files
Changes based on PR openshift-hyperfleet#22 review comments: - Replace BROKER_TOPIC_PREFIX with BROKER_TOPIC (full topic name) - Move topic construction to Helm: {namespace}-{resourceType} - Default in values.yaml: {{ .Release.Namespace }}-{{ .Values.config.resourceType }} - Use os.LookupEnv to allow empty env var to clear config value - Add topic to log message for debugging - Fix lint errors in tests using t.Setenv - Update documentation with new examples Examples: - clusters in hyperfleet-dev-rafael -> hyperfleet-dev-rafael-clusters - nodepools in hyperfleet-dev-rafael -> hyperfleet-dev-rafael-nodepools
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene, rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Summary
BROKER_TOPIC_PREFIXenvironment variable andtopic_prefixconfig option for multi-tenant isolation{prefix}-{resourceKind}(e.g.,hyperfleet-dev-rafael-Cluster)Test plan
make test🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.