Use Envoy Aggregated Discovery Service via the gRPC API

[mihaitodor]

Hey @relistan, this is the initial implementation of the new Envoy gRPC API that we discussed about. I also switched to go mod, which probably messes-up #43 (in case you want to resurrect it), sorry about that.

For now, it has no tests and I duplicated a bunch of code from the REST API. Please have a look at the changes and let me know if the design makes sense and if you'd like anything changed / done differently. Happy to do so before I proceed to deduplicate code and write some tests. I also need to update the Readme / docs. There are quite a few TODO items that I left in the code which would be great if you can review and leave some feedback.

If you have some time, I'd appreciate it if you can test out these changes in a dev cluster and let me know if you see any issues.

For some background on the implementation, please see this blog post, the v2 API Overview and the xDS REST and gRPC protocol details (especially the part about the Aggregated Discovery Service).

Note that the current implementation, while it is eventually consistent (I hope!), can lead to transient errors because old listeners / clusters might be removed before new ones are added when Envoy receives an update. See the Eventual consistency considerations and On eventually consistent service discovery topics in the docs. They offer an incremental xDS which can be used to address such issues, but it's not yet implemented in the go control plane. Somebody is currently working on it, though (see here). More details here, here and here.

Also, for a large cluster, this implementation is inefficient, because, for each state change, we recreate the listener and cluster definitions and send them to Envoy, so I think using the incremental xDS in the future makes sense.

To validate this implementation, I have created the following setup on my Macbook using a custom Envoy config (see below) which has ADS enabled via gRPC:

> SIDECAR_LOGGING_LEVEL=info HAPROXY_BIND_IP=0.0.0.0 ENVOY_BIND_IP=0.0.0.0 ENVOY_USE_GRPC_API=true HAPROXY_DISABLE=true SIDECAR_CLUSTER_NAME=dev ./sidecar
> # Run the latest official Envoy with a custom configuration
> docker run --rm -p9901:9901 -p10080:10080 -p10081:10081 -v $(pwd)/envoy.yaml:/etc/envoy/envoy.yaml -l SidecarDiscover=false envoyproxy/envoy:v1.12.2 --service-cluster dev --service-node brainiac.local --config-path /etc/envoy/envoy.yaml
> # Run two whoami endpoints for the `whoami-http` service
> docker run --rm -p80 --name whoami-http1 -l ServiceName=whoami-http -l SidecarDiscover=true -l ServicePort_80=10080 -l HealthCheck="HttpGet" -l HealthCheckArgs="http://{{ host }}:{{ tcp 10080 }}/health" containous/whoami
> docker run --rm -p80 --name whoami-http2 -l ServiceName=whoami-http -l SidecarDiscover=true -l ServicePort_80=10080 -l HealthCheck="HttpGet" -l HealthCheckArgs="http://{{ host }}:{{ tcp 10080 }}/health" containous/whoami
> # Run one whoami endpoint for the `whoami-tcp` service
> docker run --rm -p80 --name whoami-tcp -l ProxyMode=tcp -l ServiceName=whoami-tcp -l SidecarDiscover=true -l ServicePort_80=10081 -l HealthCheck="HttpGet" -l HealthCheckArgs="http://{{ host }}:{{ tcp 10081 }}/health" containous/whoami

envoy.yaml:

admin:
  access_log_path: /dev/null
  address:
    socket_address:
      address: 0.0.0.0
      port_value: 9901

dynamic_resources:
  ads_config:
    api_type: GRPC
    grpc_services:
      envoy_grpc:
        cluster_name: xds_cluster
  lds_config: { ads: {} }
  cds_config: { ads: {} }

static_resources:
  clusters:
    - name: xds_cluster
      connect_timeout: 0.25s
      type: STATIC
      lb_policy: ROUND_ROBIN
      http2_protocol_options: {}
      upstream_connection_options:
        # configure a TCP keep-alive to detect and reconnect to the admin
        # server in the event of a TCP socket half open connection
        tcp_keepalive: {}
      load_assignment:
        cluster_name: xds_cluster
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: 192.168.65.2
                      port_value: 7776

Note that the http2_protocol_options stuff seems to be needed for the gRPC protocol and 192.168.65.2 is the host IP address because I ran this using Docker for Mac.

Given the setup above, I have performed the following curl tests:

> curl -v localhost:10080 # Returns either one of the two `Hostname`s, which should mean that it performs round-robin on the backend endpoints, as expected
> curl -v localhost:10081 # Always returns the same `Hostname` and no `X-...` headers are injected by Envoy, so it looks like a plain TCP proxy, as expected

I have also tested the above in the browser, which Envoy detects and seems to always forward the localhost:10080 request to the same backend which responded to the initial request, or it might be due to some browser caching, not sure.

I tested that clusters are created and removed as expected when backend endpoints go down or come back up.

I made sure that Sidecar can be killed safely and Envoy will continue to happily proxy requests to the last known backends.

Finally, I wanted to compare this implementation to the old Envoy REST API, so I stopped the new Envoy and ran the old one like so:

> docker run --rm -p9901:9901 -p10080:10080 -p10081:10081 -l SidecarDiscover=false gonitro/envoyproxy:latest

If you're using Docker for Mac, you'll have to hack the config in the container to point to the host IP, so I added the following to the s6 run script to make my life easier:

HOST_ADDR=$(getent hosts host.docker.internal | awk '{ print $1 }')

sed -i "s/127\.0\.0\.1/${HOST_ADDR}/g" /etc/envoy/envoy.yaml

Then I fetched the Sidecar services.json and did a config dump in both the old Envoy and the new one, which I am attaching here as reference: json_files.zip. As far as I can tell, everything looks OK in there.

[dselans]

Can't see complete implementation because of missing file(s) - in the meantime, I've read through all the comments between Mihai and @relistan - everything makes sense.

I'll familiarize myself with all the docs that @mihaitodor linked in the PR (thank you btw, very helpful!).

[mihaitodor]

@dselans Thanks for having a look! Actually, there are no missing files. It's just GH being dumb. Sometimes it doesn't expand some files like envoy/server.go.

[mihaitodor]

@relistan @dselans I have made (most of) the requested changes. Can you please have another look and let me know what you think? I left some open conversations above that I think still need to be discussed.

[mihaitodor]

Since this is the only place where ExpireServer is called, I moved the locks to the body of the function, to make it easier to write one of my tests. It needs to lock the state anyway.

[mihaitodor]

There is still some duplication in the functions here and what remained in sidecarhttp/envoy_api.go, but I think deduplicating it will be ugly and I'm thinking that it's probably safe to delete that REST API soon, given that Envoy has a 1-2 year API lifecycle and the V2 API that I just implemented will be deprecated at the end of this year.

I wanted to switch directly to V3, but the go-control-plane doesn't seem to have made the necessary updates yet. It should be a much easier transition, though, since V3 is mostly similar to V2.

[relistan]

I think we can and should use state.EachService() which takes a closure to operate on the state items. This save an allocation of a big blob of memory—which will happen pretty often—and it saves an unnecessary sort as well.

A good way to do that would be to build your own hash as you go (it will be much smaller) and then map it into a list at the very end.

listenerMap := make(map[string]cache.Resource)

state.EachService(func(hostname *string, id *string, svc *service.Service) {
     // inspect things, put them in the hash as needed, overwriting without checking
})

listeners := make([]cache.Resource, len(listenersMap))
// insert items in a numbered way to avoid using append and the associate re-allocs

This is not blocking feedback, but it's a better implementation I think.

[mihaitodor]

Hehe, that's what I get for copy/pasting code 😅This is a great suggestion! Now this API should be much faster than the old REST-based one. Thanks for that!

[relistan]

Some comments about alternate implementation of part of this, and a couple little cleanup thing, but in general it looks very good!

[mihaitodor]

@relistan Thanks a lot for the feedback! I decided to switch to using a simple TimedLooper instead of hooking into the Sidecar Listener functionality because this prevents the spurious updates issue and it ended up being much simpler overall, given your suggestion to check if state.LastChanged has been updated before doing anything and skip if not.

Using your other suggestion of calling state.EachService() instead of state.ByService(), I noticed that I can easily compute both clusters and listeners in one go and I can get away with locking the state only once higher up. Since now I'm only calling state.EachService() once instead of twice, I think holding the lock for it is fine.

After adding a few more tests, I noticed that it makes sense to tombstone services before sending updates in ExpireServer(), since otherwise there is a small chance to skip an update when expiring a server which has only a single service running, so I made this change as well.

Finally, I noticed that we were using the wrong zero value for time.Time, which makes the IsZero() call on such a timestamp return false. This can trip people up when writing tests, but I'm OK to drop this change or move it to another PR if you think it can cause trouble.

[relistan]

I'd rather not make this change. I definitely have outside code that uses this in a few projects.

[relistan]

If we can back this out, I'll merge it!

[mihaitodor]

Done and created #54 to keep a record of this issue in case we want to fix it in the future.

[relistan]

Looks good! 🐐 🐐 🐐

[relistan]

@mihaitodor sorry I approved and thought you'd merge. I'll merge it now.

[mihaitodor]

I don't have permissions to merge. Thanks! :)