Skip to content
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

[WIP] Consul resources: consul_agent_service, consul_catalog_entry, consul_node, consul_service #3687

Closed
wants to merge 0 commits into from

Conversation

maxenglander
Copy link
Contributor

@maxenglander maxenglander commented Oct 29, 2015

Resolves #2087.

consul_agent_service and consul_service correspond to the /agent/service/{deregister,register} API calls.
consul_catalog_entry and consul_node correspond to the /catalog/{register,deregister} API calls.

Here are some sample configurations:

    "consul_agent_service": {
        "rds": {
            "address": "${element(split(\":\", aws_db_instance.rds.endpoint), 0)}"
          , "name": "rds"
          , "port": "${aws_db_instance.rds.port}"
          , "tags": ["rds", "mysql"]
        }
  }

  , "consul_catalog_entry": {
        "test": {
            "address": "192.168.1.1"
          , "node": "consul0"
          , "service": [
                {
                    "id": "foo1"
                  , "name": "foo"
                  , "port": 8081
                  , "address": "api.foo.com"
                  , "tags": ["foo","bar"]
                }
            ]
        }
    }

Hopefully will get documentation and tests finished up this weekend. In the meantime, could use some feedback as well as help with the following issue.

When I make any change to a consul_catalog_entry resource, I run into the "diffs don't match" issue. This seems to be an issue with the tags list. The only way I've found around it so far is to make sure that there is always at least one tag in a service definition in consul_catalog_entry. Here's a gist of a TF_LOG that will hopefully shed some light on the issue.

@maxenglander maxenglander changed the title Consul resources: consul_agent_service and consul_catalog_entry` Consul resources: consul_agent_service and consul_catalog_entry Oct 29, 2015
@apparentlymart
Copy link
Contributor

Thanks for working on this! I look forward to being able to use it. 😀

It seems like the pair of resources here is representing an important distinction, but perhaps the subtle difference is lost a bit in the names. Because the first one has "service" in it, it seems like the more attractive choice for someone who knows a little about consul and understands they want to make a service, but I think in many cases it will actually be "catalog entry" that they want.

One important use-case for registering services from Terraform would be to register "external services" for managed resources like ElastiCache clusters and RDS instances. That is, resources that internally manage their own HA and thus only change during terraform apply; therefore they don't need to (and in fact, can't) be represented by a live Consul agent. Assuming that "external services" are the primary use-case for interactive with the catalog API, perhaps your consul_catalog_entry would be more intuitively named consul_external_service.

The consul_agent_service resource is interesting in that you'd presumably use it against the agent on a particular client node, rather than against the cluster servers as with the rest of these services. In this case you're using the resource as an alternative to logging in to the host and running Consul agent commands locally. If we accept the idea of using Terraform to configure client Consul agents, there are a couple other interesting use-cases that could be added by separate patches later:

  • consul_agent_registration: resource wrapper around the agent/join/... and agent/force-leave/... APIs, as an alternative to using a provisioner to log in and run consul join .... (I'd love this one, personally! Running consul join via remote-exec provisioner is awkward.)
  • consul_agent_check: resource wrapper around agent/check/register and agent/check/deregister.

I like the idea of exposing parts of the local agent API to Terraform, and hopefully the consul_agent_ prefix is enough to help users understand the distinction between interacting with the consul cluster as a whole (consul_keys, consul_external_service) and interacting with the agent on a particular node (consul_agent_*).

@maxenglander
Copy link
Contributor Author

Hi @apparentlymart thanks for the feedback!

One objection I might raise to the name consul_external_service is that, as it stands, consul_catalog_entry is not only for registering services: it can also register a plain node with a configuration such as:

"consul_catalog_entry": {
    "my_external_node": {
        "node": "my_external_node"
      , "address": "my.external.node"
    }
}

I don't personally have a good use case for doing this, but since the consul API allows this (via the same /catalog/register call), I made it so that the consul_catalog_entry allows it as well. Also, the word "external" might be a bit blurry for some people. External to what? My team? My datacenter? My organization? The Consul documentation you linked to uses the notion of externality to motivate a use case, but the word "external" isn't strictly part of Consul. Introducing it into Terraform would impose it upon Consul.

Another idea is to just have consul_node, consul_service, and consul_check resources, all of which default to using /catalog/register. Then, consul_service and consul_check could be assigned to either the catalog or a specific agent, and default to "agent": false. However, I'm a little bit at a loss as to what a sensible, non-confusing configuration would look like. Here's an example. For instance, in a catalog context, a consul_service would require a "node" attribute, but not so in the agent context. In the catalog context, a service can have two addresses - the node address and the service address. Finally, a check can accept a "script" in the agent context, but not the catalog context.

These are just my 2 cents - I'm happy to go in whatever direction you and others think makes most sense.

@samdunne
Copy link
Contributor

Is there any further updates on this?

@phinze
Copy link
Contributor

phinze commented Nov 30, 2015

This is looking great so far! Thanks for all the work you have put into this, @maxenglander!

Here's my two cents on the resource modeling questions:

I think that consul_node, consul_service, and consul_check are worth fleshing out as top level resources. This way each can have rich attributes and validation even if internally they all use /catalog/register for create and /catalog/unregister to delete.

It seems to me that the /agent/ API endpoints have distinct enough semantics and behavior that they're worth their own consul_agent_* resources. And theoretically we can split those into a separate PR if you only wanted to tackle the consul_* ones here.

@phinze
Copy link
Contributor

phinze commented Nov 30, 2015

Ah and the tags "diffs don't match" error was an ordering problem. In the API Tags are an unordered but in the implementation you were using a TypeList which has significant ordering. So subsequent calls would get randomly ordered tags and cause the mismatch.

By splitting out separate top level resources for consul_service you can make tags into a TypeSet which should behave properly.

@samdunne
Copy link
Contributor

Any updates on this?

@maxenglander
Copy link
Contributor Author

Thanks for the feedback @phinze! I worked around the "diffs don't match" issue by changing the "tags" attribute in consul_catalog_entry from a list to a set, as you suggested.

I'm able to get the new tests working by changing the Consul endpoint from "demo.consul.io" to my own Consul server. Seems when I run the tests locally I don't have permission to add/remove services on "demo.consul.io", although the Travis CI test seems to have passed.

One thing @phinze: the Consul API allows you to specify an ID when defining a service. Unfortunately for consul_agent_service, it seems that I'm unable to allow a user-defined ID. I guess this is a reserved, non-user-definable Consul keyword? If that's the case, what's the recommended way to allow users to define the ID of a Consul service? Expose an attribute such as "service_id" which maps to ID in Consul?

@phinze @samdunne I haven't had a chance to get started on consul_node, consul_check and consul_service, so I've finished up consul_agent_service and consul_catalog_entry for now. Added tests and docs. Hopefully these will be useful to someone until the remaining pieces are finished.

@tecnobrat
Copy link

@maxenglander thanks for your work on this!

This is something I'm specifically wanting to use, this looks amazing.

Wondering if this is something that could get merged as-is and the other features added later?

@phinze phinze changed the title Consul resources: consul_agent_service and consul_catalog_entry [WIP] Consul resources: consul_agent_service and consul_catalog_entry Feb 9, 2016
@phinze phinze removed the wip label Feb 9, 2016
@maxenglander
Copy link
Contributor Author

Hey @phinze I've created consul_node and consul_service resources. Under the hood, consul_node is using /catalog/register and consul_service is using /agent/service/register.

I haven't gotten to consul_check yet. I can do that in this PR, but, since several people seem interested in using consul_service sooner rather than later, I'd also be happy to let this one go to review and merging, opening a new PR for consul_check.

Looking forward to your feedback.

@sstarcher
Copy link

@phinze It would be great if we could get consul_node and consul_service in. Anything else that needs done to get this merged?

@maxenglander maxenglander changed the title [WIP] Consul resources: consul_agent_service and consul_catalog_entry [WIP] Consul resources: consul_agent_service, consul_catalog_entry, consul_node, consul_service Jun 4, 2016
@ocxo
Copy link

ocxo commented Jul 6, 2016

Did this get closed in favor of another PR?

@maxenglander
Copy link
Contributor Author

Sorry @fromonesrc and everyone else who is subscribed to this PR. I irreversibly deleted the previous PR by force-pushing the branch that was the basis of the PR. Luckily, I had the commits available on another branch, so I created a new PR (#7508) from those commits.

@ghost
Copy link

ghost commented Apr 24, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: consul_service resource
8 participants