Bugfix: Connect Intentions API, blocking queries

[kainoaseto]

Hi there,

As I've been working on building out a service hoping to take advantage of blocking queries with Consul intentions, I came across the same issue as seen here: #4989

After making the change seen in this PR, a request to a local dev consul succeeds and returns consul headers:

curl -i -X GET 'http://localhost:8500/v1/connect/intentions'

HTTP/1.1 200 OK
Content-Type: application/json
Vary: Accept-Encoding
X-Consul-Index: 1
X-Consul-Knownleader: true
X-Consul-Lastcontact: 0
Date: Sat, 16 Feb 2019 01:02:26 GMT
Content-Length: 3

[]

[hashicorp-cla]

All committers have signed the CLA.

[banks]

Thanks @kainoaseto!

It would be great to have tests that sanity check the header is present just like we do for Get:

consul/agent/intentions_endpoint_test.go

Lines 352 to 355 in b3c7447

	ixn.ID = value.ID
	ixn.RaftIndex = value.RaftIndex
	ixn.CreatedAt, ixn.UpdatedAt = value.CreatedAt, value.UpdatedAt
	assert.Equal(ixn, value)

Ideally we should have tests that blocking actually works as expected here although the plumbing for it is all in place with this PR and we don't actually test that for other endpoints.

The history here is that we never actually block on that listing internally so it wasn't noticed, but we do document that it supports blocking so this is a great fix thanks! https://www.consul.io/api/connect/intentions.html#list-intentions

If you don't have time to add the test as mentioned please say and we can do that before we merge! Thanks again!

---

I'm also interested to know more about what you are doing with Envoy and intentions? Partly out of curiosity, partly because the intentions endpoint may not be ideal for your needs as the rule set scales up. If your not able to share more for now that's totally understood!

[banks]

Never mind, I've just realised that he linked test example simply stubs out the indexes to make the test deterministic and doesn't actually assert anything useful about them being set.

We should probably fix that in our tests, but this PR is not making them any worse! I'll merge it as-is and we can clean those up another time.

[kainoaseto]

Thanks @banks for the review/merge and context around the tests! Maybe I'll get some time to get some tests in around that since it would be useful to have as these endpoints become more critical.

My use case with Envoy and intentions is to utilize this endpoint in an xDS API that uses the results to provide services(Envoy clusters) to Envoy sidecar proxies and front proxies to form a service mesh. The nodes (Envoy endpoints) themselves are being discovered from the catalog API with blocking calls. I would really enjoy having a more in-depth chat about the use case with Envoy if there are scaling concerns that come to mind. Also if there is some place more well suited than this PR for an ongoing discussion about this I'd be happy to chat more!

[banks]

@kainoaseto thanks, sounds interesting. Are you aware of our Connect Service Mesh feature where we export xDS to Envoy directly already? I assume so but want something different which is why i'm interested to talk.

More info on Connect and Envoy available in docs.

Best place to talk more would probably be our mailing list. Feel free to drop a link here to close the loop if you start a thread there!

[kainoaseto]

Hi @banks , Apologies on taking so long to bring this to full circle. I've created a post in the Consul mailing list and hope to discuss this more with you!

https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!topic/consul-tool/C5V4S8a6p5I

[eliatglympse]

@kainoaseto You're the man.