hashicorp / consul-template

Template rendering, notifier, and supervisor for @HashiCorp Consul and Vault data.
https://www.hashicorp.com/
Mozilla Public License 2.0
4.76k stars 781 forks source link

index global function returns error when called on service slice #189

Closed cvarnerin closed 9 years ago

cvarnerin commented 9 years ago

Calling the index global function in consul-template returns errors when called on a non-empty service slice ([]*service), but works on maps returned by byTag.

Reproduce issue with index function on []*service:

$ echo '{{ index (service "webapp") 0 }}' > file.template
$ CONSUL_TEMPLATE_LOG=debug consul-template -template=file.template:file.out -once
2015/02/10 21:51:45 [INFO] (runner) creating new runner (dry: false, once: true)
2015/02/10 21:51:45 [INFO] (runner) creating consul/api client
2015/02/10 21:51:45 [INFO] (runner) creating Watcher
2015/02/10 21:51:45 [INFO] (runner) starting
2015/02/10 21:51:45 [DEBUG] (runner) running initial templates
2015/02/10 21:51:45 [INFO] (runner) running
2015/02/10 21:51:45 [DEBUG] (runner) checking template file.template
2015/02/10 21:51:45 [INFO] (brain) getting data for service "webapp [passing]"
Consul Template returned errors:
template: template: file.template:1:3: executing "file.template" at <index (service "weba...>: error calling index: index out of range: 0

Whereas index function working with maps:

$ echo '{{ index (service "webapp" | byTag) "mytag" }}' > file.template
$ CONSUL_TEMPLATE_LOG=debug consul-template -template=file.template:file.out -once
2015/02/10 21:54:10 [INFO] (runner) creating new runner (dry: false, once: true)
2015/02/10 21:54:10 [INFO] (runner) creating consul/api client
2015/02/10 21:54:10 [INFO] (runner) creating Watcher
2015/02/10 21:54:10 [INFO] (runner) starting
2015/02/10 21:54:10 [DEBUG] (runner) running initial templates
2015/02/10 21:54:10 [INFO] (runner) running
2015/02/10 21:54:10 [DEBUG] (runner) checking template file.template
2015/02/10 21:54:10 [INFO] (brain) getting data for service "webapp [passing]"
2015/02/10 21:54:10 [INFO] (runner) was not watching 1 dependencies
2015/02/10 21:54:10 [INFO] (watcher) adding service "webapp [passing]"
2015/02/10 21:54:10 [DEBUG] (watcher) service "webapp [passing]" starting
2015/02/10 21:54:10 [INFO] (runner) updating dependencies
2015/02/10 21:54:10 [DEBUG] (view) service "webapp [passing]" starting fetch
2015/02/10 21:54:10 [DEBUG] (service "webapp [passing]") querying Consul with &{Datacenter: AllowStale:false RequireConsistent:false WaitIndex:0 WaitTime:1m0s Token:}
2015/02/10 21:54:10 [DEBUG] (service "webapp [passing]") Consul returned 8 services
2015/02/10 21:54:10 [DEBUG] (service "webapp [passing]") 8 services after health check status filtering
2015/02/10 21:54:10 [INFO] (view) service "webapp [passing]" received data from consul
2015/02/10 21:54:10 [DEBUG] (runner) receiving dependency service "webapp [passing]"
2015/02/10 21:54:10 [INFO] (brain) remembering service "webapp [passing]"
2015/02/10 21:54:10 [INFO] (runner) running
2015/02/10 21:54:10 [DEBUG] (runner) checking template file.template
2015/02/10 21:54:10 [INFO] (brain) getting data for service "webapp [passing]"
2015/02/10 21:54:10 [INFO] (brain) checking if service "webapp [passing]" has data
2015/02/10 21:54:10 [DEBUG] (brain) service "webapp [passing]" had data
2015/02/10 21:54:10 [DEBUG] (runner) checking ctemplate &{Source:file.template Destination:file.out Command:}
2015/02/10 21:54:10 [DEBUG] (runner) rendered: true
2015/02/10 21:54:10 [INFO] (runner) updating dependencies
2015/02/10 21:54:10 [DEBUG] (runner) checking if service "webapp [passing]" still needed
2015/02/10 21:54:10 [DEBUG] (runner) service "webapp [passing]" is still needed
2015/02/10 21:54:10 [INFO] (runner) once mode and all templates rendered
2015/02/10 21:54:10 [INFO] (runner) stopping
2015/02/10 21:54:10 [INFO] (watcher) stopping all views
2015/02/10 21:54:10 [DEBUG] (watcher) stopping service "webapp [passing]"
$ cat file.out
[0xc208044a80 0xc208044cc0 0xc208044c00 0xc208044ae0 0xc208044ba0 0xc208044d20 0xc208044c60 0xc208044b40]

Which returns a []*service as expected.

sethvargo commented 9 years ago

Hrm... I think this is because we are returning a slice that has 0 elements in it like:

result := make([]*dep.HealthService, 0)

But I think you should be able to get the index of that item... The problem is that we can't give that slice a size or else it won't work during nested queries.

The first time CT runs, it doesn't have data for the services, so it just returns an empty slice. It looks like Go's index function does not like that...

ryanuber commented 9 years ago

@cvarnerin While it does seem strange at first that you get an out-of-range error, if you think about it, it does make sense. The thing you are trying to index into (in this case, services), may or may not have data at any given time, not only at consul-template's startup. This means that providing an explicit, non-dynamic index will always have potential to cause this error, since the index is dependent on the received data from consul. The map works because accessing a non-existent map key just returns nilrather than raising an error.

Because of this, to me it seems it would be a mistake to hard-code an index this way for any type of data from consul. If you need to use known indexes, I would recommend using the range $index, $element := pipeline syntax, and check the index.

sethvargo commented 9 years ago

@cvarnerin to follow up on what @ryanuber said, even if we provided our own index function that permitted a 0-index and returned nil if the resource didn't exist (like in the map case), you would immediately get a nil error when you try to call any properties or functions on the resource.

{{ with index (service "webapp") 0 }}
  {{.Name}} // This will error because the service definition is nil
{{ end }}

The reason the range + index works is because range won't iterate over an empty slice.

cvarnerin commented 9 years ago

@sethvargo and @ryanuber - I agree it is wrong to hard-code a static index as provided in the first example, but there are plenty of reasonable uses for the index function. e.g.

{{ if $webapp := service "webapp" }}
{{ index $webapp (index function) }}
// do something
{{ end }} 

Where (index function) is something like (modulo $random (len $webapp)). Of course, the same could be accomplished with:

{{ $index, $element := range service "webapp" }}
{{ if (eq $index (index function)) }}
// do something
{{ end }}{{end}}

Both forms seem similar in complexity and readability, so is there any reason not to support the index global function? If it will remain unsupported, where would this behavior be best documented? Is there a CT project IRC channel? Thanks for your responses!

sethvargo commented 9 years ago

@cvarnerin I don't think we are explaining clearly enough. We aren't choosing not to support the index function - we can't support it. Go does not permit you to access the 0th element in an empty slice. For an empty slice, (modulo $random (len $webapp)) would still return 0, thus returning an error. While part of @ryanuber's argument was "you shouldn't be doing that anyway", it should not be misconstrued with "we won't do it". Does that makes sense? We are not intentionally limiting any functionality to discourage this use case - index is a native Go template function and this is the behavior of slices in Go.

As for "random", you might be interested in my response to this ticket as to why accessing a random element in the slice would be terribly troublesome.

You can use #consul on freenode for Consul and related projects such at CT.

ryanuber commented 9 years ago

@sethvargo is right, we can't support unguarded slice access by index. It will be a problem for the initial consul-template start-up, and even if we could get past that, it could case problems later if the slice became empty later on.

@cvarnerin I actually think index should work as expected when it is guarded by a if condition. The problem with using index without the guard is that consul-template needs to try rendering your templates before it has even gotten any data in consul, thus causing the out-of-range errors. With that said, you should definitely take proper precautions when trying to simulate randomly selected nodes or services, as @sethvargo pointed out and linked to.

cvarnerin commented 9 years ago

@sethvargo I think @ryanuber stated my thoughts better than I did, namely:

index should work as expected when it is guarded by an if condition

I tested the "guarded" example provided above and I can confirm the index function works as expected. Thanks for walking me through the first pass edge case.

Interesting thread on "random". I've been thinking about implementation and it's certainly non-trivial. Maybe something like return the hash of a semi-constant token like temp system file, consul-template PID, hostname, etc. This will guarantee a "random" value that doesn't change between passes of CT but shouldn't be the same across all nodes.