Refactor internal/envoy/v3 package helper functions

[sunjayBhatia]

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 the envoy_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 the Listener/ClusterCache do not need to know details about the ConfigSource we want to program, the envoy/v3 package ConfigGenerator 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.

[sunjayBhatia]

we could use a "singleton" pattern for this type of thing, specifically modifying the ConfigSource, but that can become unwieldy to test etc. so that approach is not ideal IMO

[sunjayBhatia]

another idea would be to have a post-processing step after the xdscache.Listener|ClusterCache components generate Envoy xDS config to inject common config like ConfigSource, which I didn't explore too deeply (but could maybe also be used to inject other common config as mentioned in #5458, though it seems we would want to apply those global things at a higher level like a new DAG processor or the like)

[skriss]

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.

we could use a "singleton" pattern for this type of thing, specifically modifying the ConfigSource, but that can become unwieldy to test etc. so that approach is not ideal IMO

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.

another idea would be to have a post-processing step after the xdscache.Listener|ClusterCache components generate Envoy xDS config to inject common config like ConfigSource, which I didn't explore too deeply (but could maybe also be used to inject other common config as mentioned in #5458, though it seems we would want to apply those global things at a higher level like a new DAG processor or the like)

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.

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[clayton-gonsalves]

@sunjayBhatia going through this today.

[clayton-gonsalves]

The implementation LGTM. +1 to the idea of having a struct to store the global config state which can then be propagated downwards.

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack