Add condition "nodes" that triggers a task everytime a Consul node is updated

[remilapeyre]

No description provided.

[findkim]

Hi @remilapeyre. We are excited about your contribution! @lornasong has caught me up to speed with communication between you two. We rotate community duties and so happy to jump in here and update you on where we are:

After reviewing the changes, the approach and proposed feature look good. It’s great to see how the nodes feature you have added extends our new construct of conditions quite well.

There are a few items that would get this PR and feature ready.

• Can you write up a PR summary to include: a high level summary of the feature + use case, and a code block with a config example setting a task with the node condition?
• Can you add an integration test and an end-to-end test for the node condition template? Here are some examples that might help with that: catalog-services integration, services-regex e2e

We are planning to release 0.3.0 soon, and so your feature could make it into the next release! It shouldn't be too long after 0.3.0 🙂

[findkim]

Would you mind adding a compile check for NodesConditionConfig to satisfy the interface ConditionConfig below the const? It helps us keep all of the condition types standardized.

var _ ConditionConfig = (*NodesConditionConfig)(nil)

[findkim]

I support the idea that the nodes condition doesn't allow setting the services in the task config until we know of a clearer use-case. Though I don't see how this list of services here would be set for this validation during config loading.

If that's the case, can you remove this and add the validation within the TaskConfig.Validate()? There are other cases where the condition specs are checked.

[findkim]

minor nit to avoid copying the pointer here to avoid unintended changes between copies and merges.

Suggested change

	o.Datacenter = n.Datacenter
	o.Datacenter = StringCopy(n.Datacenter)

[findkim]

Adding a couple of test cases for completion

Suggested change

	},
	},
	{
	"datacenter_empty_one",
	& NodesConditionConfig{Datacenter: String("same")},
	& NodesConditionConfig{},
	& NodesConditionConfig{Datacenter: String("same")},
	},
	{
	"datacenter_empty_two",
	& NodesConditionConfig{},
	& NodesConditionConfig{Datacenter: String("same")},
	& NodesConditionConfig{Datacenter: String("same")},
	},

[findkim]

I see that the query parameter Filter is available and supported in the node template, it would be a good feature to expose through the configuration to be used.

I like the omission of the node-meta parameter since it's marked as deprecated for the /v1/catalog/nodes endpoint

[findkim]

The attributes in the variable object look good 👍

We have a larger design choice for CTS to generate input variables as maps instead of lists for the purpose of modules to easily consume it with the Terraform for_each syntax. Would you mind changing it from a list to map?

As for the key of the map, we could use id.node.datacenter for uniqueness. Similar to the ID of the var.services input variable

[findkim]

Although these functions are not public, can you add code comments for them since they contain a lot of broader context that is not immediately noticeable.

You can follow the general format found here

consul-terraform-sync/templates/tftmpl/tmplfunc/catalog_services_registration.go

Lines 15 to 22 in afe4d8d

	// catalogServicesRegistrationFunc returns information on registered Consul
	// services. It queries the Catalog List Services API and supports the query
	// parameters dc, ns, and node-meta. It also adds an additional layer of
	// custom functionality on the API response:
	// - Adds regex filtering on service name option e.g. "regexp=api"
	//
	// Endpoint: /v1/catalog/services
	// Template: {{ catalogServicesRegistration <filter options> ... }}

[findkim]

Adding one more comment that is not an ask.

The content added in templates/tftmpl/tmplfunc/nodes.go and its test file would be great to move to the hashicorp/hcat library. It's where general Consul API handling is done for templates. The library will be also adopted by Consul-Template, and I can imagine that others would benefit from your addition for nodes and filtering support!

@mkam has some hcat work inflight too, so that's why it's not an ask for you to move it there. We can handle that and updating the CTS dependency on the library afterwards.

[mkam]

Hi @remilapeyre, just for awareness, we did a fairly major refactoring of our conditions code (see #385), so your PR is out of date with how conditions are structured now. Please let us know how you'd like to proceed with your changes!

[hashicorp-cla]

All committers have signed the CLA.