-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Too many recursive expansions #10617
Comments
@danelson Hey, thanks for the issue! I would like to know more about your use case before deciding on what to do here. What are you trying to accomplish and what alternatives have you tried? |
@mx-psi thanks for the response. My component basically implements its own authentication and I load a bunch of tokens in via environment variables which I validate incoming headers against. I actually have a different flow where I do something similar via nginx so I am thinking that is a better approach that I can leverage. I just need to update some routing in my cluster. I could also load the environment variables based on a convention (prefix) which would allow me to collapse this to a single environment variable. I know there are some auth extensions (although I don't think any serve my use case). I could also explore writing my own. So I know I have have some workarounds. Just wanted to make sure this was an intentional change since it was breaking from before. |
@danelson this is technically an unexpected consequence of making the env vars expanded by the envprovider instead of the expandconverter, but a true fix is probably too much work to be worth it. I think we'll just bump the recursive check constant to a higher number since this is a startup check and not hot-path execution and there are lots of work arounds like you mentioned. |
#### Description Bumps the limit of how much recursion we allow. This check is also gating non-recursive expansions. We probably could separate these concerns, but thats work that isn't really worthwhile, a simple constant bump is simple and will cover most users. For the rest, there are workarounds. <!-- Issue number if applicable --> #### Link to tracking issue Fixes #10617
Describe the bug
After updating to v0.104.0 I receive the following error.
https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/expand.go#L27
I am not actually sure this is a bug but as it seems somewhat intentional based on the above link. It is at least a regression with the previous behavior.
Steps to reproduce
Create a string in the collector config that contains more than 100 environment variables.
What did you expect to see?
I expected it not to error.
What did you see instead?
Collector fails to start with the following error:
What version did you use?
v0.104.0
What config did you use?
Environment
otel/opentelemetry-collector-contrib:0.104.0 in docker
Additional context
The above config is just an example. I have a custom component that loads ~110 environment variables (which will continue to grow) as a common separated list. I could modify this component to work differently but just wanted to make sure this was intended.
Disabling the feature flag
confmap.unifyEnvVarExpansion
allows the above config to be loaded successfully.The text was updated successfully, but these errors were encountered: