feat(eventrecorder): add outputs name - #5393
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughEvent recorder outputs now require validated configured names. Type-qualified names are used for metrics, logs, output construction, duplicate detection, and configuration equality across file, webhook, Kafka, and stdout destinations. ChangesEvent recorder output naming
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change updates output names used in logs and metrics, and the supplied evidence does not identify a current correctness or security defect; no actionable merge-blocking risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
we actually need to fix this for other outputs too, for example the secret url from webhook leaks into metrics and logs. |
3d8503f to
7953a47
Compare
siavashs
changed the title
| f, err := openAppend(fo.path) | ||
| if err != nil { | ||
| fo.logger.Error("Failed to reopen event recorder file", "path", fo.path, "err", err) | ||
| fo.logger.Error("Failed to reopen event recorder file", "output", fo.name) |
There was a problem hiding this comment.
I would like to have the file path also in this error log.
There was a problem hiding this comment.
I guess for files it is fine to keep the path, since they are not technically a secret.
| } | ||
| fo.logger.Error("fsnotify error on event recorder directory", "err", err) | ||
| _ = err | ||
| fo.logger.Error("fsnotify error on event recorder directory", "output", fo.name) |
There was a problem hiding this comment.
filepath (or its dirname) would also be helpful in this error log
7953a47 to
da274a4
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: 3
🧹 Nitpick comments (1)
config/config_test.go (1)
56-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the configured name is parsed.
Since this change adds the required
namefield, also verify its value; otherwise the test does not protect the new configuration contract.Proposed test assertion
require.Len(t, cfg.EventRecorder.WebhookOutputs, 1) + require.Equal(t, "pipelines", cfg.EventRecorder.WebhookOutputs[0].Name) require.True(t, cfg.EventRecorder.WebhookOutputs[0].Batch)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/config_test.go` around lines 56 - 57, Update the configuration parsing test in config_test.go to assert that the parsed pipeline name equals the configured “pipelines” value, alongside the existing URL assertions, so the required name field is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eventrecorder/recorder.go`:
- Line 182: Update the file event recorder initialization error log in the
relevant recorder flow to remove the raw `"path"` field and `"err"` value,
retaining only the type-qualified identifier from safeOutputIdentifier("file",
fc.Name), consistent with webhook and Kafka logging.
In `@eventrecorder/webhook_test.go`:
- Around line 494-500: Update TestWebhookOutputConfig_MalformedURLDoesNotLeak to
assert independently that the malformed-URL error omits each sensitive
component, including the token value, alongside the existing secret and password
checks.
- Around line 503-506: The test TestNewWebhookOutput_ValidatesProgrammaticConfig
currently omits only the URL, so it does not verify empty-name validation. Add a
separate case using a valid URL with Name empty, and assert that
NewWebhookOutput returns an error.
---
Nitpick comments:
In `@config/config_test.go`:
- Around line 56-57: Update the configuration parsing test in config_test.go to
assert that the parsed pipeline name equals the configured “pipelines” value,
alongside the existing URL assertions, so the required name field is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3af1292-600c-4ecb-b436-38c906632484
📒 Files selected for processing (14)
CHANGELOG.mdconfig/config_test.godocs/configuration.mdeventrecorder/config.goeventrecorder/file.goeventrecorder/file_test.goeventrecorder/kafka.goeventrecorder/kafka_test.goeventrecorder/recorder.goeventrecorder/recorder_test.goeventrecorder/stdout.goeventrecorder/stdout_test.goeventrecorder/webhook.goeventrecorder/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- CHANGELOG.md
- docs/configuration.md
- eventrecorder/config.go
- eventrecorder/recorder_test.go
- eventrecorder/stdout.go
- eventrecorder/file_test.go
- eventrecorder/kafka.go
- eventrecorder/stdout_test.go
- eventrecorder/kafka_test.go
- eventrecorder/webhook.go
- eventrecorder/file.go
Require every output to have a safe, unique name and use only the type-qualified name in metrics and logs. Avoid exposing webhook URLs, Kafka brokers, topics, and raw transport errors. Signed-off-by: Siavash Safi <siavash@cloudflare.com>
da274a4 to
6d83ebe
Compare
| "fmt" | ||
| ) | ||
|
|
||
| const maxOutputNameLength = 128 |
There was a problem hiding this comment.
is this limit really necessary? Do we enforce limits on the lengths of other names in the config?
There was a problem hiding this comment.
If i remember correctly the limit is the safe one for label values.
This is configurable on prometheus for example, but 128 characters should be enough to generate unique names.
There was a problem hiding this comment.
Ah, I see, because the name is exported on a metric. This seems fine to me then.
There was a problem hiding this comment.
Actually Prometheus does not have a universal 128-character safe limit for label values.
Label values may contain arbitrary valid UTF-8, and Prometheus’s label_value_length_limit is a per-scrape setting whose default is 0—unlimited.
So my initial comment was me confusing our internal Prometheus config with upstream defaults.
But we should probably keep the safe limit here or make it configurable across all Alertmanager metric label values maybe.
| for _, r := range name { | ||
| if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && (r < '0' || r > '9') && r != '-' && r != '_' && r != '.' { | ||
| return "", fmt.Errorf("event_recorder %s output name must contain only letters, digits, hyphens, underscores, and periods", kind) | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess I have the same question here - is there a reason we need to constrain the valid names here?
There was a problem hiding this comment.
I think validation was added for label values.
There was a problem hiding this comment.
I think we still have the Prometheus label parser in the source tree - could we use that for validation instead? I'm worried that this will drift from whatever the Prometheus implementation is.
There was a problem hiding this comment.
So the label parser only validates label names not label values. the only validation for label values is to check if they are valid UTF-8 strings.
So we can choose to either drop this custom validation or keep it.
3 participants
Require every output to have a safe, unique name and use only the type-qualified name in metrics and logs. Avoid exposing webhook URLs, Kafka brokers, topics, and raw transport errors.
Which user-facing changes does this PR introduce?