Skip to content
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

No errors logged when failing to reconcile with webhooks disabled #1569

Closed
andrewdinunzio opened this issue Mar 10, 2023 · 3 comments · Fixed by #1570
Closed

No errors logged when failing to reconcile with webhooks disabled #1569

andrewdinunzio opened this issue Mar 10, 2023 · 3 comments · Fixed by #1570

Comments

@andrewdinunzio
Copy link
Contributor

andrewdinunzio commented Mar 10, 2023

We've temporarily disabled webhooks due to #1514 and noticed that if we don't fully specify the OpenTelemetryCollector, then it doesn't create/update the collector Deployment. At the same time, the opentelemetry operator doesn't log any errors.

I assume this is because there's some defaulting webhook that isn't being run, and there's some validation failing as a result when it gets reconciled. But it's failing silently. Or maybe it isn't failing but there's some "switch"-like statement that causes it to do nothing because the field isn't set? I haven't had a chance to dig into the code. I guess it's not typical to run without webhooks enabled, but if that's a supported configuration, it would help find problems faster with a bit more logging here.

As soon as I re-enabled the webhooks, it worked fine again. It would be nice to log any errors in the operator that would normally cause a validation failure with the admission webhooks enabled (if it would cause Reconcile to fail).

We disabled the webhooks by setting this in the helm chart:

admissionWebhooks:
  create: false
manager:
  env:
    ENABLE_WEBHOOKS: "false"
@pavolloffay
Copy link
Member

Running the operator without webhooks is not supported.

That said we should definitely add a warning log if the webhook is disabled.

@pavolloffay
Copy link
Member

See #1570

@andrewdinunzio
Copy link
Contributor Author

andrewdinunzio commented Mar 10, 2023

Thank you. (Edit* reopened as the PR will resolve)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants