Skip to content

Proposal for refactoring mixerclient configuration #344

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

Closed
2 of 5 tasks
ayj opened this issue Feb 1, 2018 · 6 comments
Closed
2 of 5 tasks

Proposal for refactoring mixerclient configuration #344

ayj opened this issue Feb 1, 2018 · 6 comments

Comments

@ayj
Copy link
Contributor

ayj commented Feb 1, 2018

The current mixerclient protos conflates user intent with implementation specific filter config. We ought to decouple them. User facing config is dimensioned by feature (auth, quota, api-spec) and should be organized according to the proposed proto package scheme. Filter config is dimensioned by service (i.e. envoy route).

This will allow better independent evolution of the user-facing APIs from filter/proxy specific implementations, e.g. mixerclient, upstream envoy (auth, ratelimit).

We should also refactor the mixerclient filter configuration to be defined per route (via RDS) instead of per listener (via LDS). This is both more performant, as service config can then be delivered via RDS instead of the more costly LDS, which requires draining listeners, and more consistent with upstream Envoy filters, e.g. here.

  • Introduce a new user-facing AuthenticationPolicy. @diemtvu already started this work (see Add AuthenticationPolicy proto #335). Pilot consumes AuthenticationPolicy and produce the corresponding mtls/auth filter configuration for envoy/mixerclient.

  • Introduce a new user-facing API proto. OpenAPI (v2/v3), gRPC, and generic REST schemes should be considered. Options include a single umbrella type with subtypes per scheme or first-class types for each of the scheme. Pilot consumes these API spec(s) and produces the corresponding normalized HTTPAPISpec for mixerclient filter.

  • Introduce a new user-facing Quota proto. Schema is TBD. Config WG probably has useful input on what this should look like. Pilot consums this new proto and produce corresponding QuotaSpec for mixerclient filter.

  • Binding/Scoping API is only needed for user facing APIs. QuotaSpecBinding, HTTPAPISpecBinding, and EndUserAuthenticationPolicySpecBinding should be removed. Equivalent bindings should be created for the user-facing auth, quota, and api-spec definitions. It would be great if there were some consistency with RouteRule's concepts/terminology when possible. For example, it seems reasonable to bind an API spec to service name (i.e. host) and/or entry point to the mesh (i.e. ingress/gateway).

  • Refactor mixer/v1/config/client/client_config.proto so that per-service configuration can be provided via RDS.

@ayj ayj changed the title Plan for refactoring mixerclient configuration Proposal for refactoring mixerclient configuration Feb 1, 2018
@mandarjog
Copy link
Contributor

As part of adding mixer calls on egress and external services, I am stuffing serviceconfig as it exists today into per route as b64, in addition to where it is today.
The service config should only be present for inbound|* listener filters so it is not torn down that often, so the problem is not as bad for the current use case.

However since I am adding mixer calls for egress / external service it will result in listener reload when a new external service is added, so I am doing the expedient thing of moving it to per route opaque config as is, and still use V1 api.

The other changes on the list are orthogonal.

@mandarjog
Copy link
Contributor

mandarjog commented Feb 2, 2018

One issue right now, is that mixer filter config is unnecessarily being populated in unrelated listeners, so I am actually fixing that too.

@mandarjog
Copy link
Contributor

mandarjog commented Feb 2, 2018

      opaque_config:
              destination.service: httpbin.default.svc.cluster.local
              per_route_config: base64(json(per_route_config))
             per_route_config_sha: sha(binary_encode(per_route_config)))

@ayj
Copy link
Contributor Author

ayj commented Feb 2, 2018

service-config per route is more important for shared proxies (e.g. ingress) and less critical for sidecar proxies. Providing both per-listen and per-route b64 encoded works shorter term, but we should eventually switch over to per-route only. Maybe v2 api is a good time?

@mandarjog
Copy link
Contributor

I have been thinking of that. Just start with V2 for RDS (REST), but then would we rather move to a templatized model?

@ayj
Copy link
Contributor Author

ayj commented Feb 2, 2018

The data object over which the template is applied may need some refactoring when we switch from per-listener to per-route. I think there may be other ongoing pilot config/api refactoring that effects this area. I guess we can get the proxy/mixerclient side working as intended and deal with pilot refactoring independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants