-
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
Feat/large validation #10417
Feat/large validation #10417
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: |
Visit the preview URL for this PR (updated for commit 807a1d1): https://gloo-edge--pr10417-feat-large-validatio-lmx29blq.web.app (expires Mon, 09 Dec 2024 20:20:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
This reverts commit e39fc1e.
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.
looking good!
projects/envoyinit/pkg/runner/run.go
Outdated
|
||
start := time.Now() | ||
err := validateCmd.Run() | ||
logger.Debugf("full envoy validation of %d size completed in %s", len(bootstrapConfig), time.Since(start)) |
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.
I like this! I wonder if we could make this information consumable to create insights on, so users could see if their configuration was growing over time.
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.
I think this might not be the right place but I like the idea. Perhaps higher up in translation we take our final output there to emit a metric?
@@ -1,7 +1,9 @@ | |||
gateway: | |||
validation: | |||
failurePolicy: Fail # For "strict" validation mode, fail the validation if webhook server is not available | |||
allowWarnings: false # For "strict" validation mode, webhook will also reject warnings | |||
allowWarnings: false |
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.
Any reason this is false? I imagined that we could test the fullEnvoyValidation even with allowWarnings=true
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.
We could, this doesnt change the option just removes the comment
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!
Can you update the PR body to just add some context to it?
Note this is entirely ripped from #10296
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: