-
Notifications
You must be signed in to change notification settings - Fork 546
Return an empty, valid configuration struct in v1alpha1 conversion webhook #4289
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?
Conversation
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.
Thanks @bderrly , could you please add a changelog entry and does it make sense to add an e2e test too? :)
@CodiumAI-Agent /review |
This change is meant to make the conversion webhook to be more resilient in the face of invalid input. It is rare, though possible, for an opentelemetrycollector custom resource to be stored as a v1alpha1 version and with an invalid configuration. With an older version of the operator running this would simply result in no collector starting due to an invalid configuration. However, once the operator is updated to a new enough version where v1beta1 is the stored version, this invalid configuration will result in the conversion webhook failing. After several minutes the operator will crash. On a production cluster we had this situation result in the resource quota controller failing to initialize after a control-plane upgrade. This resulted in an incident where new jobs could not be created.
f059f52
to
780bc39
Compare
Thanks for taking a look @frzifus. I added the changelog file as well as an e2e test. |
@@ -0,0 +1,19 @@ | |||
apiVersion: opentelemetry.io/v1alpha1 |
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.
does v1beta1 fail on the admision with this 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.
Yes, since Config
changed from string to an object:
Error from server: error when creating "otel-test.yaml": admission webhook "mopentelemetrycollectorbeta.kb.io" denied the request: json: cannot unmarshal string into Go struct field OpenTelemetryCollectorSpec.spec.config of type v1beta1.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.
LGTM, thank you for contributing this fix!
Description: Returns a valid, empty configuration struct in the v1alpha1 conversion webhook rather than returning a failure.
Link to tracking Issue(s): #4288
Testing: Added a few tests with invalid YAML configuration strings to ensure that a valid configuration, and no error, is returned from the webhook.
Documentation: None