-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(eventrecorder): add outputs name #5393
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,12 @@ | |
|
|
||
| package eventrecorder | ||
|
|
||
| import ( | ||
| "fmt" | ||
| ) | ||
|
|
||
| const maxOutputNameLength = 128 | ||
|
|
||
| // Config configures the event recorder feature. | ||
| // | ||
| // Outputs are grouped by type, one list per destination kind, mirroring | ||
|
|
@@ -26,6 +32,75 @@ type Config struct { | |
| StdoutOutputs []StdoutOutputConfig `yaml:"stdout_outputs,omitempty" json:"stdout_outputs,omitempty"` | ||
| } | ||
|
|
||
| // UnmarshalYAML implements the yaml.Unmarshaler interface, validating that | ||
| // each output identifier is unique. | ||
| func (c *Config) UnmarshalYAML(unmarshal func(any) error) error { | ||
| type plain Config | ||
| if err := unmarshal((*plain)(c)); err != nil { | ||
| return err | ||
| } | ||
| return c.validate() | ||
| } | ||
|
|
||
| func (c Config) validate() error { | ||
| seen := make(map[string]struct{}, c.totalOutputs()) | ||
| add := func(kind, name string) error { | ||
| id, err := outputIdentifier(kind, name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, ok := seen[id]; ok { | ||
| return fmt.Errorf("event_recorder output name %q is duplicated for type %s", name, kind) | ||
| } | ||
| seen[id] = struct{}{} | ||
| return nil | ||
| } | ||
| for _, out := range c.FileOutputs { | ||
| if err := add("file", out.Name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, out := range c.WebhookOutputs { | ||
| if err := add("webhook", out.Name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, out := range c.KafkaOutputs { | ||
| if err := add("kafka", out.Name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, out := range c.StdoutOutputs { | ||
| if err := add("stdout", out.Name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func outputIdentifier(kind, name string) (string, error) { | ||
| if name == "" { | ||
| return "", fmt.Errorf("event_recorder %s output requires a name", kind) | ||
| } | ||
| if len(name) > maxOutputNameLength { | ||
| return "", fmt.Errorf("event_recorder %s output name must not exceed %d characters", kind, maxOutputNameLength) | ||
| } | ||
| 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) | ||
| } | ||
| } | ||
|
Comment on lines
+88
to
+92
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I have the same question here - is there a reason we need to constrain the valid names here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think validation was added for label values.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return kind + ":" + name, nil | ||
| } | ||
|
|
||
| func safeOutputIdentifier(kind, name string) string { | ||
| id, err := outputIdentifier(kind, name) | ||
| if err != nil { | ||
| return kind + ":<invalid>" | ||
| } | ||
| return id | ||
| } | ||
|
|
||
| // totalOutputs returns the number of configured outputs across all | ||
| // destination kinds. | ||
| func (c Config) totalOutputs() int { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, because the name is exported on a metric. This seems fine to me then.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_limitis 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.