-
Notifications
You must be signed in to change notification settings - Fork 0
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
Validate config using STDIN and /dev/fd/0 #10296
base: main
Are you sure you want to change the base?
Conversation
…io/gloo into rolds/envoy_large_validation
…io/gloo into rolds/envoy_large_validation
Co-authored-by: Nathan Fudenberg <[email protected]>
…io/gloo into rolds/envoy_large_validation
…io/gloo into rolds/envoy_large_validation
…io/gloo into rolds/envoy_large_validation
Issues linked to changelog: |
1 similar comment
Issues linked to changelog: |
…holder.typicode.com/ (#10296)
I've made the changes that switch the full Envoy validation to being passed in via STDIN and Steps to reproduce the timeout I'm seeing:
Repeatedly running the test will cause the config size validated to grow to the point of taking longer than the timeout:
It looks like in some cases the timeout is a background process running during the tests that also causes a full envoy validation that holds the validating webhook for too long. I'm also seeing OOMKilled after running the tests several time without starting the controller. I have heap dumps from before and after running the tests if they would be useful. Here a heap png output from after the tests have been run 2-3 times. |
This archive contains heap dumps taken w/o validation and w/ validation. |
I'm handing this off to Nathan. I've taken it far as I can (I don't know the internals of Gloo well enough to address the config growth issue) which is blocking the tests from passing. |
Description
To address an issue with large configs passed as arguments causing a
arguments to long
error during validation; This PR adjusts how the config is provided to Envoy. The config is now fed to the process via STDIN and the--config-yaml
is set to the file descriptor for STDIN.Code changes
Context
A customer with a large config reported the error.
Interesting decisions
Initially there were discussions about saving a file inside of the container, but concerns about read-only root filesystems were raised. A volume would address that issue, but is a more complex solution. To avoid the need for a volume in some cases, I opted to use STDIN to provide the config to the program and read the STDIN FD to the config.
Testing steps
I've applied the large config yaml in the test with and without the fix confirming that is was broken and is now fixed.
Notes for reviewers
Checklist: