Skip to content
This repository has been archived by the owner on Jan 11, 2018. It is now read-only.

Config rework #3

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

brendangibat
Copy link

No description provided.

@brendangibat
Copy link
Author

This PR is for #2

@brendangibat
Copy link
Author

Alright this has been redone to pull in what's going on with the fleetctl tool.

// etcd_key_prefix can be ommited to use the default value
etcd_key_prefix = "/_some/_weird/etcd/prefix"
// connection_retries defaults to 12
connection_retries = 9000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems connection_retries no longer exists?

@bitglue
Copy link
Contributor

bitglue commented Nov 5, 2015

I haven't forgotten about these PRs. I'll build and test them locally tomorrow. The code looks pretty good so I don't foresee any issues. Thanks so much for the patch!

edit: I totally did not mean to make that a question.

How should this PR be handled along with #1? Is #1 redundant with these changes?

@brendangibat
Copy link
Author

This one is based off of #1, but has the changes that we were talking about for leveraging the fleetctl process included. I split it up so it's still between multiple PR's but if you merge this one then you also merge the other in implicitly. I've been using this fork to provision successfully since I put it together so it's been working well for my terraform uses in a few different environments.

@brendangibat
Copy link
Author

Oh yeah, another thing I wanted to send a PR on was to change this:

option {
    name = "TimeoutStartSec"
    value = "0"
}

and instead make it do this:

option {
    "TimeoutStartSec" = "0"
}

That's just a Schema.TypeMap with an Elem of &schema.Schema{Type:TypeString} (I'm going off the top of my head - I'm doing maps like that in my k8s provider so I know it can work)

It'll help clean up how many lines are being used and makes it a little easier to translate from a fleet unit file to this terraform provider.

@brendangibat
Copy link
Author

Also have you thought about getting this merged back in to the mainline terraform as a default provided provider?

@bitglue
Copy link
Contributor

bitglue commented Nov 6, 2015

That's just a Schema.TypeMap with an Elem of &schema.Schema{Type:TypeString} (I'm going off the top of my head - I'm doing maps like that in my k8s provider so I know it can work)

I was thinking about just having it accept a unit file directly, like

resource "fleet_unit" "test" {
    content = <<EOF
[service]
ExecStart=/bin/echo test
EOF
}

Then the unit files could even be stored separately, brought in with the file() function or the template_file resource.

Also have you thought about getting this merged back in to the mainline terraform as a default provided provider?

I haven't pursued that yet, but I could.

},
"ca_file": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terraform is asking me to enter values for ca_file and cert_file when I terraform plan with this configuration:

provider "fleet" {
    tunnel = "54.152.209.190"
}

Maybe just need to add Default: ""?

@bitglue
Copy link
Contributor

bitglue commented Nov 6, 2015

I took my old configuration:

provider "fleet" {
    tunnel_address = "54.152.209.190"
}

and changed it to:

provider "fleet" {
    tunnel = "54.152.209.190"
}

And now this is what I get:

$ terraform plan
provider.fleet.ca_file
  Location of TLS CA file used to secure communication with the fleet API or etcd

  Enter a value: 

provider.fleet.cert_file
  Location of TLS cert file used to secure communication with the fleet API or etcd

  Enter a value: 

Refreshing Terraform state prior to plan...

Error refreshing state: 1 error(s) occurred:

* failed initializing SSH client: timed out while initiating SSH connection

SSH seems to be working: at least ssh [email protected] connects and gives me a shell. Still investigating exactly why it's not working.

- ssh-timeout
* Description: Amount of time in seconds to allow for SSH connection initialization before failing.
* Default: 10.0
- request-timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like these options need the - replaced with _

@bitglue
Copy link
Contributor

bitglue commented Nov 6, 2015

So running with TF_LOG=debug terraform plan yields a clue:

$ TF_LOG=debug terraform plan
[...]
2015/11/06 13:28:57 [DEBUG] terraform-provider-fleet: The authenticity of host '54.152.209.190' can't be established.
2015/11/06 13:28:57 [DEBUG] terraform-provider-fleet: RSA key fingerprint is 0a:87:7a:8c:9d:fc:e1:55:7d:7e:48:66:6f:ff:fe:86.
2015/11/06 13:29:07 [ERROR] root: eval: *terraform.EvalConfigProvider, err: failed initializing SSH client: timed out while initiating SSH connection
2015/11/06 13:29:07 [ERROR] root: eval: *terraform.EvalSequence, err: failed initializing SSH client: timed out while initiating SSH connection
2015/11/06 13:29:07 [ERROR] root: eval: *terraform.EvalOpFilter, err: failed initializing SSH client: timed out while initiating SSH connection
2015/11/06 13:29:07 [ERROR] root: eval: *terraform.EvalSequence, err: failed initializing SSH client: timed out while initiating SSH connection

So it would seem the host key checking behavior of the plugin differs from the previous behavior. But it does seem to be consistent with fleetctl, so maybe that's fine. But we should document this behavior clearly, and see if the error can be made more accurate because I suspect many people will run into this very same confusion.

However, disabling host key checking just yields a different problem. With this configuration:

provider "fleet" {
    tunnel = "54.152.209.190:22"
    strict_host_key_checking = false
}

then I'll get:

$ TF_LOG=debug terraform plan
[...]
2015/11/06 13:32:02 [ERROR] root: eval: *terraform.EvalConfigProvider, err: open /var/run/fleet.sock: no such file or directory
2015/11/06 13:32:02 [ERROR] root: eval: *terraform.EvalSequence, err: open /var/run/fleet.sock: no such file or directory
2015/11/06 13:32:02 [ERROR] root: eval: *terraform.EvalOpFilter, err: open /var/run/fleet.sock: no such file or directory
2015/11/06 13:32:02 [ERROR] root: eval: *terraform.EvalSequence, err: open /var/run/fleet.sock: no such file or directory
Error refreshing state: 1 error(s) occurred:

2015/11/06 13:32:02 [DEBUG] waiting for all plugin processes to complete...
* open /var/run/fleet.sock: no such file or directory

Not sure what's going on here. /var/run/fleet.sock does exist on the host at the remote end of the tunnel, and fleetctl --tunnel=54.152.209.190 --strict-host-key-checking=false list-machines works fine.

* Default:
- known-hosts-file
* Description: File used to store remote machine fingerprints. Ignored if strict host key checking is disabled.
* Default: ssh.DefaultKnownHostsFile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ~/.fleetctl/known_hosts

@brendangibat
Copy link
Author

I was thinking about just having it accept a unit file directly

I was thinking about this as well and talking with another dev about this - at first I was thinking it's nice to be able to selectively apply updates or create / delete based on resource definition changes, but I can't help but think doing updates on everything but when the name changes (which would be the only recreate) is the use case that makes the most sense. Which would mean using a string input, possibly from a template, would be practical at that point - although I do like capturing the state of fields as resources.

@bitglue
Copy link
Contributor

bitglue commented Nov 6, 2015

I've added a few commits to your branch which fix a lot of the issues I mentioned at https://github.com/bitglue/terraform-provider-fleet/tree/config_rework

Unfortunately something seems to be screwy with my github fork (it doesn't know its a fork) so it won't let me open a PR. But you can apply the changes to your branch manually with something like

git pull https://github.com/bitglue/terraform-provider-fleet.git config_rework

@bitglue
Copy link
Contributor

bitglue commented Nov 6, 2015

The Fleet API doesn't seem to offer any capability for updates, so I don't know if there's much value in having individual options as individual Terraform attributes is of much benefit there.

I suppose one reason to keep the structure at the Terraform level would be if some other tool wanted to query Terraform's state -- this would save that tool the trouble of parsing the unit file. But I guess instead it has to parse JSON, and I don't know if that's any easier than parsing an INI-ish file.

@brendangibat
Copy link
Author

Ah, yep you're right - there aren't updates which makes capturing the fine details of the fleet unit as resource definitions not as useful over templates.

edit: querying the state with a terraform remote state is useful, but with the current resource setup it would be difficult in terraform to use specific sections or key/values since terraform doesn't provide conditional statements. But if you had the state file it is easier to parse JSON than their INI format - everything knows JSON format.

@bitglue
Copy link
Contributor

bitglue commented Nov 6, 2015

Perhaps another issue: I'm now getting this error on any terraform operation that refreshes the state:

Error refreshing state: 2 error(s) occurred:

* fleet_unit.statsite: Get http://domain_sock/fleet/v1/units/statsite.service?alt=json: forwarding request denied
* fleet_unit.www: Get http://domain_sock/fleet/v1/units/www.service?alt=json: forwarding request denied

Grepping the fleetctl sources suggests this error originates from attempting to enable SSH agent forwarding, but I've no idea why that should be necessary, or why it's only a problem now. 😖

@brendangibat
Copy link
Author

Phew! A lot of errors and changes. So I said I was using this branch already - I was wrong, I meant the other PR I have against the repo. Also I suggested switching to using a TypeMap for the key / values but I ran in to an issue this weekend with another terraform provider that if the key in a TypeMap has a '.' in it it will see it as a nested object on a parse of the resource state. it would set the base object key/value but doing a Get on the resource data wouldn't have anything. It's due to the fact that they split on '.' in their parsing and do not ignore even escaped dots. If there is a guarantee against a dot in the fleet key name then it would be ok.

@bitglue
Copy link
Contributor

bitglue commented Nov 9, 2015

Let's leave any changes to the unit configuration for a separate PR. For now this seems very close to being merged if you can add the commits I made and the "forwarding request denied" error can be sorted out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants