Fix configuration merging for implicit tproxy upstreams.#16000
Merged
hashi-derek merged 4 commits intomainfrom Jan 18, 2023
Merged
Fix configuration merging for implicit tproxy upstreams.#16000hashi-derek merged 4 commits intomainfrom
hashi-derek merged 4 commits intomainfrom
Conversation
Change the merging logic so that the wildcard upstream has correct proxy-defaults and service-defaults values combined into it. It did not previously merge all fields, and the wildcard upstream did not exist unless service-defaults existed (it ignored proxy-defaults, essentially). Change the way we fetch upstream configuration in the xDS layer so that it falls back to the wildcard when no matching upstream is found. This is what allows implicit peer upstreams to have the correct "merged" config. Change proxycfg to always watch local mesh gateway endpoints whenever a peer upstream is found. This simplifies the logic so that we do not have to inspect the "merged" configuration on peer upstreams to extract the mesh gateway mode.
ndhanushkodi
approved these changes
Jan 18, 2023
Contributor
ndhanushkodi
left a comment
There was a problem hiding this comment.
Awesome!!! I was confused about just the last sentence in the changelog so I just wanted to see if its possible to reword that!
hashi-derek
added a commit
that referenced
this pull request
Jan 18, 2023
Fix configuration merging for implicit tproxy upstreams. Change the merging logic so that the wildcard upstream has correct proxy-defaults and service-defaults values combined into it. It did not previously merge all fields, and the wildcard upstream did not exist unless service-defaults existed (it ignored proxy-defaults, essentially). Change the way we fetch upstream configuration in the xDS layer so that it falls back to the wildcard when no matching upstream is found. This is what allows implicit peer upstreams to have the correct "merged" config. Change proxycfg to always watch local mesh gateway endpoints whenever a peer upstream is found. This simplifies the logic so that we do not have to inspect the "merged" configuration on peer upstreams to extract the mesh gateway mode.
hashi-derek
added a commit
that referenced
this pull request
Jan 18, 2023
Fix configuration merging for implicit tproxy upstreams. Change the merging logic so that the wildcard upstream has correct proxy-defaults and service-defaults values combined into it. It did not previously merge all fields, and the wildcard upstream did not exist unless service-defaults existed (it ignored proxy-defaults, essentially). Change the way we fetch upstream configuration in the xDS layer so that it falls back to the wildcard when no matching upstream is found. This is what allows implicit peer upstreams to have the correct "merged" config. Change proxycfg to always watch local mesh gateway endpoints whenever a peer upstream is found. This simplifies the logic so that we do not have to inspect the "merged" configuration on peer upstreams to extract the mesh gateway mode.
hashi-derek
added a commit
that referenced
this pull request
Jan 19, 2023
…6007) Fix configuration merging for implicit tproxy upstreams. Change the merging logic so that the wildcard upstream has correct proxy-defaults and service-defaults values combined into it. It did not previously merge all fields, and the wildcard upstream did not exist unless service-defaults existed (it ignored proxy-defaults, essentially). Change the way we fetch upstream configuration in the xDS layer so that it falls back to the wildcard when no matching upstream is found. This is what allows implicit peer upstreams to have the correct "merged" config. Change proxycfg to always watch local mesh gateway endpoints whenever a peer upstream is found. This simplifies the logic so that we do not have to inspect the "merged" configuration on peer upstreams to extract the mesh gateway mode.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where tproxy would not correctly use service-defaults and proxy-defaults for implicit upstreams. Normally, we handle this by taking configuration from an auto-generated wildcard
*upstream, but that was not done in all cases -- notably peer upstreams.The changes here are essentially:
It's worth noting that another large issue still needs to be resolved in a different PR (#15956), where the
ServiceDefaults.UpstreamConfig.Overrides[].Peerfield does NOT exist. This manifests in two ways after these changes:agent/configentry/merge_service_config.goandagent/configentry/resolve.goare oblivious to the concept of a peer.default/default/mysvc?peer=cluster-02, but the overrides UID isdefault/default/mysvc. This is technically correct behavior, but it's noted here, because overrides will NOT work for peer upstreams in tproxy mode until theagent/configentrypackage is updated.