Skip to content
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

chore(*) separate proxy template resolvers #2394

Merged
merged 1 commit into from
Jul 25, 2021
Merged

chore(*) separate proxy template resolvers #2394

merged 1 commit into from
Jul 25, 2021

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Jul 19, 2021

Summary

Breaking the dependency between the simple proxy template resolver and
the default fallback lets us specify the fallback as a default proxy
template resolver. This in turn, makes it possible for plugins to wrap
the default resolver so that this can be extended.

Moving the default ProxyTemplate resources out of pkg/xds/template
breaks a circular dependency between this package and xds/generator.

Full changelog

N/A

Issues resolved

N/A

Documentation

N/A

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

@jpeach jpeach changed the title chore(*) separate proxy template resolver chore(*) separate proxy template resolvers Jul 19, 2021
Breaking the dependency between the simple proxy template resolver and
the default fallback lets us specify the fallback as a default proxy
template resolver. This in turn, makes it possible for plugins to wrap
the default resolver so that this can be extended.

Moving the default ProxyTemplate resources out of pkg/xds/template
breaks a circular dependency between this package and xds/generator.

Signed-off-by: James Peach <james.peach@konghq.com>
@jpeach jpeach marked this pull request as ready for review July 19, 2021 08:06
@jpeach jpeach requested a review from a team as a code owner July 19, 2021 08:06
@codecov-commenter
Copy link

Codecov Report

Merging #2394 (3f90c52) into master (a7c748b) will increase coverage by 0.18%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
+ Coverage   52.35%   52.53%   +0.18%     
==========================================
  Files         875      877       +2     
  Lines       47816    47854      +38     
==========================================
+ Hits        25033    25142     +109     
+ Misses      20747    20664      -83     
- Partials     2036     2048      +12     
Impacted Files Coverage Δ
pkg/xds/generator/proxy_template.go 85.71% <60.00%> (ø)
pkg/xds/template/proxy_template_resolver.go 62.50% <88.88%> (+21.32%) ⬆️
pkg/xds/server/v3/components.go 83.07% <100.00%> (+2.71%) ⬆️
pkg/core/resources/manager/cache.go 79.22% <0.00%> (-2.60%) ⬇️
pkg/mads/v1/client/client.go 41.25% <0.00%> (-2.50%) ⬇️
api/observability/v1/mads.pb.go 34.53% <0.00%> (-1.04%) ⬇️
api/internal/util/proto/proto.go 70.00% <0.00%> (ø)
api/internal/util/proto/types.go 100.00% <0.00%> (ø)
api/mesh/v1alpha1/dataplane_helpers.go 80.16% <0.00%> (+1.68%) ⬆️
pkg/kds/client/sink.go 52.27% <0.00%> (+4.54%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7c748b...3f90c52. Read the comment docs.

@lobkovilya
Copy link
Contributor

But if the plugin overrides only DefaultTemplateResolver, does it mean we can't get rid of the calling SimpleProxyTemplateResolver? Probably it's better to define DefaultTemplateResolver like:

var DefaultTemplateResolver template.ProxyTemplateResolver = template.SequentialResolver(
 	&template.SimpleProxyTemplateResolver{
 		ReadOnlyResourceManager: rt.ReadOnlyResourceManager(),
 	},
 	&template.StaticProxyTemplateResolver{
 		Template: &mesh_proto.ProxyTemplate{
 			Conf: &mesh_proto.ProxyTemplate_Conf{
 				Imports: []string{mesh_core.ProfileDefaultProxy},
 			},
 		},
 	},
 )

and DefaultReconciller:

func DefaultReconciler(rt core_runtime.Runtime, xdsContext v3.XdsContext) xds_sync.SnapshotReconciler {
	return &reconciler{
		&templateSnapshotGenerator{
			ResourceSetHooks: rt.XDSHooks().ResourceSetHooks(),
			ProxyTemplateResolver: template.DefaultTemplateResolver,
		},
		&simpleSnapshotCacher{xdsContext.Hasher(), xdsContext.Cache()},
	}
}

WDYT?

@jpeach
Copy link
Contributor Author

jpeach commented Jul 21, 2021

But if the plugin overrides only DefaultTemplateResolver, does it mean we can't get rid of the calling SimpleProxyTemplateResolver?
...
WDYT?

SimpleProxyTemplateResolver resolves the user-specified template, but when the user hasn't made a specific request, you want to be able to fall back to some other policy. This means that a plugin needs to be able to insert a resolver between the SimpleProxyTemplateResolver and whatever resolvers the xDS server falls back on. If both kinds of policy are combined in the DefaultTemplateResolver variable, there's no way for a plugin to insert a resolver into the correct place in the chain.

@jpeach jpeach merged commit 6b6f3c1 into kumahq:master Jul 25, 2021
mergify bot pushed a commit that referenced this pull request Jul 25, 2021
Breaking the dependency between the simple proxy template resolver and
the default fallback lets us specify the fallback as a default proxy
template resolver. This in turn, makes it possible for plugins to wrap
the default resolver so that this can be extended.

Moving the default ProxyTemplate resources out of pkg/xds/template
breaks a circular dependency between this package and xds/generator.

Signed-off-by: James Peach <james.peach@konghq.com>
(cherry picked from commit 6b6f3c1)
@jpeach jpeach deleted the chore/template-resolvers branch July 25, 2021 21:32
jpeach added a commit that referenced this pull request Jul 25, 2021
Breaking the dependency between the simple proxy template resolver and
the default fallback lets us specify the fallback as a default proxy
template resolver. This in turn, makes it possible for plugins to wrap
the default resolver so that this can be extended.

Moving the default ProxyTemplate resources out of pkg/xds/template
breaks a circular dependency between this package and xds/generator.

Signed-off-by: James Peach <james.peach@konghq.com>
(cherry picked from commit 6b6f3c1)

Co-authored-by: James Peach <james.peach@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants