[pkg/stanza]: fix operator field config validation #40728
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A few operators with `entry.Field` in their config were validating their
configuration in their `Build` function via a comparison to
`entry.NewNilField`. This was an inaccuracy; when a field is not
provided during the unmarshal step, nothing ever gets run to actually
construct the underlying struct field, meaning that the `entry.Field`
was functionally set to `entry.Field{}`. This is not the same as
`entry.NewNilField` which actually implements and supplies an interface
value under the hood. This caused bad configs to pass validation and led
to collector panic instead of nice failure messages.
This PR changed `entry.NewNilField` to actually produce the correct
pattern for a "nil field", while leaving the original implementation
intact in `entry.NewNoopField` in case it is useful elsewhere (though at
the moment I only saw it used for nil comparisons, which as far as I can
tell were either never useful or something got refactored without being
caught). I also added functionality to
`operatortest.ConfigUnmarshalTest` to check for validation failures by
expecting a particular error when calling the operator's `Build` method.
Turns out the construction of the `field` config is different in certain scenarios where all resource or all attributes are intended to be removed, meaning the original comparison didn't cover the case properly. This is now added. Also fixed copy operator test configs to use a prefix that is actually recognized so the field is parsed.
|
Out of time to fix the new lint failures, will work on it tomorrow. |
This commit changes to a `Field.IsEmpty` to allow the deletion of the `NoopField` and `NilField` types. It also adds multiple error validation to allow for friendlier validation error messages for the `copy` and `move` operators. Finally, it fixes some tests that were revealed to be broken by relying on the previously faulty `copy` operator config validation.
|
@andrzej-stencel Implemented your suggestions. While doing so I found some tests that relied on the old broken validation so I've now fixed those as well. Should be ready for re-review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A few operators with
entry.Fieldin their config were validating their configuration in theirBuildfunction via a comparison toentry.NewNilField. This was an inaccuracy; when a field is not provided during the unmarshal step, nothing ever gets run to actually construct the underlying struct field, meaning that theentry.Fieldwas functionally set toentry.Field{}. This is not the same asentry.NewNilFieldwhich actually implements and supplies an interface value under the hood. This caused bad configs to pass validation and led to collector panic instead of nice failure messages.This PR changed
entry.NewNilFieldto actually produce the correct pattern for a "nil field", while leaving the original implementation intact inentry.NewNoopFieldin case it is useful elsewhere (though at the moment I only saw it used for nil comparisons, which as far as I can tell were either never useful or something got refactored without being caught). I also added functionality tooperatortest.ConfigUnmarshalTestto check for validation failures by expecting a particular error when calling the operator'sBuildmethod.Link to tracking issue
Addresses a panic discovered by #40398.
Testing
Tests were added to each operator's config validation. I tested manually via a built collector by running the config from the linked issue and confirming that instead of
panicI get an error message.2025-06-15T01:53:10.582Z info service@v0.127.0/service.go:199 Setting up own telemetry... {"resource": {}} 2025-06-15T01:53:10.582Z info builders/builders.go:26 Development component. May change in the future. {"resource": {}, "otelcol.component.id": "debug", "otelcol.component.kind": "exporter", "otelcol.signal": "logs"} Error: failed to build pipelines: failed to create "filelog/my-app-name" receiver for data type "logs": remove: field is empty 2025/06/15 01:53:10 collector server run finished with error: failed to build pipelines: failed to create "filelog/my-app-name" receiver for data type "logs": remove: field is empty