-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Create new types for service-defaults upstream cfg #9872
Conversation
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 if you can rearrange things so the config flattening RPC doesn't need to know about nodes that'll be a big improvement. The only other substantial concern here was about some missing api
and structs
json omitempty
struct tags and some possible merging problems down the line.
This is done because after removing ID and NodeName from ServiceConfigRequest we will no longer know whether a request coming in is for a Consul client earlier than v1.10.
ResolveServiceConfig is called by service manager before the proxy registration is in the catalog. Therefore we should pass proxy registration flags in the request rather than trying to fetch them from the state store (where they may not exist yet).
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
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.
One minor suggestion around readability, but otherwise this looks good.
@@ -329,31 +329,21 @@ func (c *ConfigEntry) ResolveServiceConfig(args *structs.ServiceConfigRequest, r | |||
&reply.QueryMeta, | |||
func(ws memdb.WatchSet, state *state.Store) error { |
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 wonder if it might make sense to split this function out and apart.
With big closures like this can be hard to reason about what is captured from the surrounding context; splitting it into a small closure that calls another function makes that explicit.
There's also quite a bit of code, and a number of separate concerns; for example figuring out the default protocol is spread around a bit.
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.
That is a good point. I added a TODO here that I can take care of for the beta.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/339293. |
This PR expands service-defaults such that most upstream configuration can be configured centrally. There is a map called upstream_configs keyed on
<optional-namespace/>service-name
which provides per-upstream configuration. There is also anupstream_defaults
object for providing configuration that applies across all upstreams of the given service.The fields in both of these were promoted out of the opaque config map in
Upstream
so that they can be validated on config write. Note that calls toResolveServiceConfig
will return them in the existing opaque map so that clients on older versions can receive the new centrally-configured fields.Below is an example config entry:
In general this is the merge order for something like "protocol" for an upstream listener:
proxy-defaults.Config["protocol"]
service-defaults
of the upstreamservice-defaults.upstream_defaults
of the downstreamservice-defaults.upstream_configs
of the downstream for the upstreamProxy.Upstreams[i].Config["protocol"]
)TODO: