-
Notifications
You must be signed in to change notification settings - Fork 690
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
Refactor internal/envoy/v3 package helper functions #5523
Refactor internal/envoy/v3 package helper functions #5523
Conversation
we could use a "singleton" pattern for this type of thing, specifically modifying the |
another idea would be to have a post-processing step after the |
I think this makes sense -- you want to store some state/configuration that informs exactly how the envoy config is generated, so a new struct is a sensible place to do it. Like you say, we could probably use this for some of the global cluster/listener/etc. settings as well, that are currently piped through the DAG, but really don't need to be in the DAG and could just be applied universally when we generate the Envoy config. That would simplify the code paths for those and make it harder to make implementation mistakes with them.
Yeah, if it was just ConfigSource, then maybe something like this would make sense, but I do think we could use this pattern for more things, in which case I'd rather have a struct.
Definitely an option as well, but I feel like this would end up with a lot of similar code to the config generation, since we'd have to walk all the various configs, look into per-filter configs, etc. |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
4017da6
to
c4d8757
Compare
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
@sunjayBhatia going through this today. |
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.
The implementation LGTM. +1 to the idea of having a struct to store the global config state which can then be propagated downwards.
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
Take two of projectcontour#5523 That introduces a struct `NewEnvoysGen` that allows us to modify the behaviour of Envoy config generation. This will help with projectcontour#6806 On the debate on `singleton` vs `struct` approach. I thought this again and I realized that `struct` is probably worth the big diff here. For the following reason: * It allows us to test the behaviour much easier. With a singleton it would be impossible for higher level tests like tests in `internal/featuretests` to modify the behaviour of the Envoy config generation. With a struct, we can just pass the config options to the struct and test the behaviour. Signed-off-by: Sotiris Nanopoulos <sotiris.nanopoulos@reddit.com>
Take two of #5523 That introduces a struct `NewEnvoysGen` that allows us to modify the behaviour of Envoy config generation. This will help with #6806 On the debate on `singleton` vs `struct` approach. I thought this again and I realized that `struct` is probably worth the big diff here. For the following reason: * It allows us to test the behaviour much easier. With a singleton it would be impossible for higher level tests like tests in `internal/featuretests` to modify the behaviour of the Envoy config generation. With a struct, we can just pass the config options to the struct and test the behaviour. Signed-off-by: Sotiris Nanopoulos <sotiris.nanopoulos@reddit.com>
This is a precursor to making changes for adding the ability to configure Contour/Envoy to use ADS (Aggregated Discovery Service). That change requires changing all of the
ConfigSource
references to specify an ADS source, rather than what we have today, which is the gRPC config source. As the code stands today, changing theenvoy_v3.ConfigSource()
method would need a new parameter or conditional logic around the call everywhere it is currently called, which is a lot of places in just implementation code, not to mention tests.The current approach taken in this PR is to create a new type to hold common configuration (like details about
ConfigSource
parameters that are common to all xDS resources we generate) to hang helper methods off of. This way, consumers like theListener/ClusterCache
do not need to know details about theConfigSource
we want to program, theenvoy/v3
packageConfigGenerator
knows and we can limit the testing/blast radius of common configuration changes to that object.Another hope I have is that we can start to reduce the usage of implementation code in test assertions, this sort of change will force us to reevaluate how we set up our tests a bit.
First commit is the main implementation changes, second and beyond for fixing tests, adding new ones, further changes.
This isn't complete, but wanted to get feedback before I went too too far down.