fix(fluentbit): render namespaced rewrite_tag filter as YAML when configFileFormat=yaml - #2019
Conversation
…figFileFormat=yaml Motivation: When a FluentBitConfig's ClusterFluentBitConfig sets spec.configFileFormat: yaml, and a namespaced Filter/FluentBitConfig triggers the operator's auto-generated `rewrite_tag` filter (used to tag records emitted from a given namespace), the operator always rendered that filter as a hand-written classic TOML snippet (`[Filter]\n Name rewrite_tag\n...`) and spliced it directly into the otherwise-YAML fluent-bit.yaml secret. The result is a config file that mixes TOML and YAML syntax, which fluent-bit fails to parse at startup. This only affects the yaml config format combined with the namespaced rewrite-tag scenario; the classic/TOML format and non-namespaced setups are unaffected, and the operator's reconcile loop itself completes without error since it has no visibility into the malformed content it writes. Fixes fluent#1689 Approach: Instead of hand-building a format-specific string, generateRewriteTagConfig now constructs the existing filter.RewriteTag plugin struct (same Rule/EmitterName/ EmitterStorageType/EmitterMemBufLimit values as before) wrapped in a synthetic ClusterFilterList, and renders it through the same Load()/LoadAsYaml() methods already used for user-defined Filter and ClusterFilter resources. A new configFileFormat parameter is threaded from ClusterFluentBitConfig.Spec.ConfigFileFormat through processNamespacedFluentBitCfgs into generateRewriteTagConfig to pick the right renderer. The classic TOML output is unchanged apart from a harmless reordering of the Emitter_* keys (order doesn't matter in fluent-bit's classic format). Validation: - go build ./... - go test ./controllers/... ./apis/fluentbit/v1alpha2/... (all packages pass, including a new TestGenerateRewriteTagConfigYaml covering both the yaml and classic output paths) - go vet ./... ```release-note Fixed a bug where namespaced Filter resources combined with `configFileFormat: yaml` produced an invalid fluent-bit.yaml config (TOML text mixed into YAML) for the auto-generated rewrite_tag filter, causing fluent-bit to fail to parse its configuration. ``` Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
…y goconst lint Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
The "Run linter" check was failing on a |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
controllers/consts.go:17
configFileFormatYamlis a fixed string constant but is declared in avarblock, so it can be mutated at runtime. Declaring it as aconstmakes the intent explicit and avoids accidental modification.
fluentdAgentMode = "agent"
configFileFormatYaml = "yaml"
)
| filterList := fluentbitv1alpha2.ClusterFilterList{ | ||
| Items: []fluentbitv1alpha2.ClusterFilter{ | ||
| { | ||
| Spec: fluentbitv1alpha2.FilterSpec{ | ||
| Match: tag, | ||
| FilterItems: []fluentbitv1alpha2.FilterItem{{RewriteTag: rewriteTag}}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| sl := plugins.NewSecretLoader(nil, "") | ||
| if configFileFormat != nil && *configFileFormat == configFileFormatYaml { | ||
| return filterList.LoadAsYaml(sl, 1) | ||
| } | ||
| return buf.String() | ||
| return filterList.Load(sl) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
controllers/fluentbitconfig_controller.go:521
- In the YAML path, this renders the rewrite_tag snippet via ClusterFilterList.LoadAsYaml(...), which always includes a
filters:header (see apis/fluentbit/v1alpha2/clusterfilter_types.go:166-213). But the full YAML config renderer (ClusterFluentBitConfig.RenderMainConfigInYaml) later appends the cluster filter sections, which also start withfilters:(cluster filters) or writes its ownfilters:header (when only namespaced filters exist). That will produce duplicatefilters:keys underpipeline:in common cases (e.g., when any cluster filters or any namespaced filters are present), which is invalid YAML / may cause fluent-bit to ignore one section.
Suggested direction: make rewrite_tag output be filter list items only (no filters: header) and update RenderMainConfigInYaml to emit a single filters: header when any of {cluster filters, rewrite_tag, namespaced filters} exist, then append rewrite_tag entries into that section; or alternatively merge the synthetic rewrite_tag filter into the cluster filter list before calling filters.LoadAsYaml so only one header is emitted. Please also extend the new test to cover assembly into the full YAML main config so this doesn’t regress.
sl := plugins.NewSecretLoader(nil, "")
if configFileFormat != nil && *configFileFormat == configFileFormatYaml {
return filterList.LoadAsYaml(sl, 1)
}
return filterList.Load(sl)
|
This is rebased on master and green — happy to make any changes that would help move review along, just let me know. |
|
@pujitha24 Hi, thanks for the contribution? Could you please address or comment on #2019 (review)? Thanks! |
generateRewriteTagConfig's YAML branch rendered the synthetic rewrite_tag filter with its own "filters:" header via ClusterFilterList.LoadAsYaml, which RenderMainConfigInYaml then wrote alongside the main config's own "filters:" header, producing two "filters:" keys under "pipeline:" in the same YAML map whenever cluster or namespaced filters were also present. Strip the header in generateRewriteTagConfig so the fragment is a headerless list of filter items (matching the existing convention used for namespaced filters), and have RenderMainConfigInYaml merge it into the single filters: section instead of writing it separately. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
controllers/consts.go:16
configFileFormatYamlis an immutable string; it should be declared as aconst(not in avarblock) to make intent explicit and prevent accidental mutation.
fluentbitApiGVStr = fluentbitv1alpha2.SchemeGroupVersion.String()
fluentdApiGVStr = fluentdv1alpha1.SchemeGroupVersion.String()
fluentdAgentMode = "agent"
configFileFormatYaml = "yaml"
controllers/fluentbitconfig_controller.go:528
strings.TrimPrefixwill silently do nothing ifrendereddoesn’t start with the exactheader(e.g., ifLoadAsYamloutput format/indentation changes), causing afilters:header to leak into the merged YAML and potentially reintroduce the duplicate-key/invalid-config issue. Consider validatingstrings.HasPrefix(rendered, header)and returning an error if it doesn’t match (or remove the firstfilters:line using a more resilient approach), so failures are detected instead of writing malformed config.
sl := plugins.NewSecretLoader(nil, "")
if configFileFormat != nil && *configFileFormat == configFileFormatYaml {
rendered, err := filterList.LoadAsYaml(sl, 1)
if err != nil {
return "", err
}
// Strip the "filters:" header so callers can merge this into the
// single "filters:" section of the main YAML config instead of
// emitting a second, duplicate key (see RenderMainConfigInYaml).
header := fmt.Sprintf("%sfilters:\n", utils.YamlIndent(1))
return strings.TrimPrefix(rendered, header), nil
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
controllers/fluentbitconfig_controller.go:505
- This changes behavior vs the previous implementation:
Emitter_Storage.typeandEmitter_Mem_Buf_Limitused to be emitted only when non-empty, but now the fields are always assigned (possibly to empty strings). If the renderer does notomitemptythese fields, this can produce config entries with empty values (and/or change the classic output unexpectedly). To preserve prior behavior, only setEmitterStorageType/EmitterMemBufLimitwhen the corresponding spec fields are non-empty (or ensure the underlying struct tags omit empty values in both YAML and classic renderers).
if cfg.Spec.Service != nil {
if cfg.Spec.Service.EmitterName != "" {
rewriteTag.EmitterName = cfg.Spec.Service.EmitterName
} else {
rewriteTag.EmitterName = fmt.Sprintf("re_emitted_%x", md5.Sum([]byte(cfg.Namespace)))
}
rewriteTag.EmitterStorageType = cfg.Spec.Service.EmitterStorageType
rewriteTag.EmitterMemBufLimit = cfg.Spec.Service.EmitterMemBufLimit
}
controllers/fluentbitconfig_controller.go:528
- Stripping the YAML header via an exact
TrimPrefixmatch is brittle: it assumes the renderer will always produce exactlyYamlIndent(1) + \"filters:\\n\"(line endings, indentation, or formatting changes can break this silently and reintroduce duplicatefilters:keys). A more robust approach is to have the YAML renderer support emitting a list without the parent key (preferred), or to unmarshal the rendered YAML and re-marshal only thefilterssequence items so the merge logic does not depend on string formatting.
sl := plugins.NewSecretLoader(nil, "")
if configFileFormat != nil && *configFileFormat == configFileFormatYaml {
rendered, err := filterList.LoadAsYaml(sl, 1)
if err != nil {
return "", err
}
// Strip the "filters:" header so callers can merge this into the
// single "filters:" section of the main YAML config instead of
// emitting a second, duplicate key (see RenderMainConfigInYaml).
header := fmt.Sprintf("%sfilters:\n", utils.YamlIndent(1))
return strings.TrimPrefix(rendered, header), nil
controllers/consts.go:16
configFileFormatYamlis a constant value but is declared in avarblock. Consider making it aconst(either in a separateconstblock or by converting this block if feasible) to prevent accidental runtime mutation and clarify intent.
fluentbitApiGVStr = fluentbitv1alpha2.SchemeGroupVersion.String()
fluentdApiGVStr = fluentdv1alpha1.SchemeGroupVersion.String()
fluentdAgentMode = "agent"
configFileFormatYaml = "yaml"
Motivation:
When a FluentBitConfig's ClusterFluentBitConfig sets
spec.configFileFormat: yaml, and a namespaced Filter/FluentBitConfig
triggers the operator's auto-generated
rewrite_tagfilter (used totag records emitted from a given namespace), the operator always
rendered that filter as a hand-written classic TOML snippet
(
[Filter]\n Name rewrite_tag\n...) and spliced it directly intothe otherwise-YAML fluent-bit.yaml secret. The result is a config file
that mixes TOML and YAML syntax, which fluent-bit fails to parse at
startup. This only affects the yaml config format combined with the
namespaced rewrite-tag scenario; the classic/TOML format and
non-namespaced setups are unaffected, and the operator's reconcile
loop itself completes without error since it has no visibility into
the malformed content it writes.
Fixes #1689
Approach:
Instead of hand-building a format-specific string,
generateRewriteTagConfig now constructs the existing
filter.RewriteTag plugin struct (same Rule/EmitterName/
EmitterStorageType/EmitterMemBufLimit values as before) wrapped in a
synthetic ClusterFilterList, and renders it through the same
Load()/LoadAsYaml() methods already used for user-defined Filter and
ClusterFilter resources. A new configFileFormat parameter is threaded
from ClusterFluentBitConfig.Spec.ConfigFileFormat through
processNamespacedFluentBitCfgs into generateRewriteTagConfig to pick
the right renderer. The classic TOML output is unchanged apart from a
harmless reordering of the Emitter_* keys (order doesn't matter in
fluent-bit's classic format).
Validation:
(all packages pass, including a new TestGenerateRewriteTagConfigYaml
covering both the yaml and classic output paths)
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com