TritonDataCenter / terraform-provider-triton

Terraform Joyent Triton provider
https://www.terraform.io/docs/providers/triton/
Mozilla Public License 2.0
15 stars 24 forks source link

Introduce packages data source #85

Closed jwreagor closed 6 years ago

jwreagor commented 6 years ago

This PR includes a new data source for pulling Package information out of Triton's CloudAPI. Looking for early feedback before I wrap up.

A Package in Triton specifies resources allocated to a machine instance. In order to run a compute instance a consumer must choose a package by various attributes like memory, disk, and swap size.

The HCL looks like this...

provider "triton" {
    insecure_skip_tls_verify = true
}

data "triton_package" "highcpu" {
    filter {
        name = "highcpu"
        memory = 8192
    }
}

data "triton_image" "image" {
    name = "base-64-lts"
    most_recent = true
}

resource "triton_machine" "test" {
    name = "${format("test-%02d", count.index + 1)}"
    package = "${data.triton_package.highcpu.name}"
    image = "${data.triton_image.image.id}"
}

This currently evaluates to the following execution plan...

  + triton_machine.test
      id:                   <computed>
      created:              <computed>
      dataset:              <computed>
      disk:                 <computed>
      domain_names.#:       <computed>
      firewall_enabled:     "false"
      image:                "390639d4-f146-11e7-9280-37ae5c6d53d4"
      ips.#:                <computed>
      memory:               <computed>
      name:                 "test-01"
      nic.#:                <computed>
      package:              "g4-highcpu-8G"
      primaryip:            <computed>
      root_authorized_keys: <computed>
      type:                 <computed>
      updated:              <computed>

In order to make this easier to use we provide a way to search for a sub string within a Package's name. Otherwise, any other attributes will be forwarded into CloudAPI's backend filtering mechanism.

EDIT: Side note, the reason for the filter block is so we can provide different ways to filter packages besides the static attributes they already have. In the case above we're drawing a distinction between filter name highcpu and the actual name g4-highcpu-8G. In the future it would be nice to provide a short name for sizes such as memory and disk, replacing 8192 with 8G.

jwreagor commented 6 years ago

Completely forgot to commit a test change. Will fix.

jwreagor commented 6 years ago

Tests fixed.

kwilczynski commented 6 years ago

Hi @cheapRoc, thank you so much! This is amazing and looks good to me! 👍

Potentially, we would also need to update README.md file here to include new data source, and the documentation to mention new functionality.

Aside of the sub-string match, we might mention that each of the string values can be a wildcard matches as the CloudAPI seem to support wildcards (if you knew that, then I sincerely apologise for commotion), for example:

(wildcard name)

$ cloudapi '/kwilczynski/packages?name=g4-highcpu*16G'
HTTP/1.1 200 OK
Server: cloudapi/8.3.0
Request-Id: 3edb5b63-52e1-4de0-a569-3c4563d49b05
Content-Type: application/json
Access-Control-Expose-Headers: Api-Version, Request-Id, Response-Time
Strict-Transport-Security: max-age=6570000
Date: Mon, 19 Feb 2018 21:54:39 GMT
Content-MD5: nPhZ+gJDqkEPSk2O0wH4Fw==
Access-Control-Allow-Origin: *
Response-Time: 111
Connection: Keep-Alive
Access-Control-Allow-Headers: Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date, Api-Version, Response-Time
Access-Control-Allow-Methods: GET, HEAD
Api-Version: 8.0.0
Content-Length: 263

[{"name":"g4-highcpu-16G","memory":16384,"disk":409600,"swap":65536,"vcpus":0,"lwps":4000,"default":false,"id":"14b143dc-d0f8-11e5-a839-73e2199aecd4","version":"1.0.3","description":"Compute Optimized 16G RAM - 8 vCPUs - 400 GB Disk","group":"Compute Optimized"}]

(wildcard group)

krzysztof.wilczynski@deupc00156 ~/.../joyent/triton-go (master $)$ cloudapi '/kwilczynski/packages?group=*KVM&vcpus=8'
HTTP/1.1 200 OK
Server: cloudapi/8.3.0
Request-Id: b6fb2c6e-d91d-483b-a65b-e5793ec7264b
Content-Type: application/json
Access-Control-Expose-Headers: Api-Version, Request-Id, Response-Time
Strict-Transport-Security: max-age=6570000
Date: Mon, 19 Feb 2018 22:09:22 GMT
Content-MD5: YYUZ401VF3dv6aAHvVs+fA==
Access-Control-Allow-Origin: *
Response-Time: 175
Connection: Keep-Alive
Access-Control-Allow-Headers: Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date, Api-Version, Response-Time
Access-Control-Allow-Methods: GET, HEAD
Api-Version: 8.0.0
Content-Length: 1404

[{"name":"k4-bigdisk-kvm-63.75G","memory":65280,"disk":5120000,"swap":261120,"vcpus":8,"lwps":4000,"default":false,"id":"14c0992c-d0f8-11e5-bd78-e71dad0f8626","version":"1.0.3","description":"Storage Optimized KVM 63.75G RAM - 8 vCPUs - 5000 GB Disk","group":"Storage Optimized KVM"},{"name":"k4-fastdisk-kvm-63.75G","memory":65280,"disk":1638400,"swap":261120,"vcpus":8,"lwps":4000,"default":false,"id":"14be13c8-d0f8-11e5-b55b-47eb44d4e064","version":"1.0.3","description":"Storage Optimized KVM 63.75G RAM - 8 vCPUs - 1600 GB Disk","group":"Storage Optimized KVM"},{"name":"k4-general-kvm-31.75G","memory":32512,"disk":409600,"swap":130048,"vcpus":8,"lwps":4000,"default":false,"id":"14ad1a32-d0f8-11e5-a465-8f264489308b","version":"1.0.3","description":"General Purpose KVM 31.75G RAM - 8 vCPUs - 400 GB Disk","group":"General Purpose KVM"},{"name":"k4-highcpu-kvm-15.75G","memory":16128,"disk":409600,"swap":64512,"vcpus":8,"lwps":4000,"default":false,"id":"14b783d2-d0f8-11e5-8d93-6ba10192d750","version":"1.0.3","description":"Compute Optimized KVM 15.75G RAM - 8 vCPUs - 400 GB Disk","group":"Compute Optimized KVM"},{"name":"k4-highram-kvm-63.75G","memory":65280,"disk":409600,"swap":261120,"vcpus":8,"lwps":4000,"default":false,"id":"14bb84f0-d0f8-11e5-8014-2fb7b19ccb24","version":"1.0.3","description":"Memory Optimized KVM 63.75G RAM - 8 vCPUs - 400 GB Disk","group":"Memory Optimized KVM"}]

This is a bit counter-intuitive as the documentation is a bit vague, plus the images API in the CloudAPI does not seem to support wildcards, for example:

$ cloudapi '/kwilczynski/images?name=base-*-lts'
HTTP/1.1 200 OK
Server: cloudapi/8.3.0
Request-Id: f7e41c6c-2691-4b07-8689-e7bc1cb2f22d
Content-Type: application/json
Access-Control-Expose-Headers: Api-Version, Request-Id, Response-Time
Strict-Transport-Security: max-age=6570000
Date: Mon, 19 Feb 2018 22:14:51 GMT
Content-MD5: 11FxOYiYfpMxmANj4kGJzg==
Access-Control-Allow-Origin: *
Response-Time: 132
Connection: Keep-Alive
Access-Control-Allow-Headers: Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date, Api-Version, Response-Time
Access-Control-Allow-Methods: GET, HEAD, POST
Api-Version: 8.0.0
Content-Length: 2

[]

Some endpoints throw an error point blank, as per:

$ cloudapi '/kwilczynski/machines?name=t*'
HTTP/1.1 409 Conflict
Server: cloudapi/8.3.0
Request-Id: 6543a78b-c729-4908-a961-3a5f4c925cf2
role-tag: test,test123
Content-Type: application/json
Access-Control-Expose-Headers: Api-Version, Request-Id, Response-Time
Strict-Transport-Security: max-age=6570000
Date: Tue, 20 Feb 2018 10:33:00 GMT
Content-MD5: fNBb5jj9mPyHGdrmIllgfA==
Access-Control-Allow-Origin: *
Response-Time: 893
Connection: Keep-Alive
Access-Control-Allow-Headers: Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date, Api-Version, Response-Time
Access-Control-Allow-Methods: POST, GET, HEAD
Api-Version: 8.0.0
Content-Length: 183

{"code":"ValidationFailed","message":"Invalid Parameters","errors":[{"field":"alias","code":"Invalid","message":"String does not match regexp: /^[a-zA-Z0-9][a-zA-Z0-9\\_\\.\\-]*$/"}]}

(Note: the name of the field is alias rather than name; a bit odd.)

Which would be really helpful to be able to use wildcards there (as in the CloudAPI).

Update: Add examples of errors coming from CloudAPI.

jwreagor commented 6 years ago

Thanks @kwilczynski.

Aside of the sub-string match, we might mention that each of the string values can be a wildcard matches as the CloudAPI seem to support wildcards (if you knew that, then I sincerely apologise for commotion)

This is a bit counter-intuitive as the documentation is a bit vague

It wasn't apparent to me either, from the language of our documentation, that wildcard searches were actually working as we would want in Terraform itself. That's why I actually substring match on the package name within Terraform itself, outside CloudAPI, and pass the remaining package attributes through to CloudAPI.

So you think this really only needs documentation updates? It's been awhile since I've reviewed this myself.

jwreagor commented 6 years ago

Also, I'll post doc updates later today.

kwilczynski commented 6 years ago

Hi @cheapRoc!

It seems that CloudAPI is inconsistent in terms what supports wildcard and what does not, and I only discovered this for packages by accident just by fuzzing inputs for all the API calls that take filters via a query strings.

It is nice to the end-user, but definitely would break the sub-string match.

I think we have the following possible options:

1) Don't mention wildcards and keep strings.Contains; 2) Mention wildcards support for the Packages end-point, and drop string.Contains; 3) Generalise into something that can work here, or can work for other data sources too.

As per (3) we could support wildcards ourselves (since CloudAPI behaviour is inconsistent between different end-points) so that user could do simple glob here and in the triton_image data source, and we could only support a wildcard match using * (and perhaps ?). This is something we could add later, after merging this Pull Request.

What do you think?

kwilczynski commented 6 years ago

An example of supporting only * and ? at https://play.golang.org/p/5bX_mIIfJ17 (based on Russ Cox's linear search algorithm).

jwreagor commented 6 years ago

I concur, we should fix our backend ambiguities at Terraform. triton_package and triton_image sound like good first steps also. And thanks for the sample art.

kwilczynski commented 6 years ago

Hi @cheapRoc!

I know that resolving this client-side (in this provider, that is), so to speak, seem a bit awkward, but it might be quicker than an update to CloudAPI and its various end-points.

Also, only * and ? would be. We don't need to support ranges e.g., [a-z] (and it is hard to justify an use case not fulfilled by * and ? already). Even AWS does keep it simple too, as per Filtering_Resources_CLI.

jwreagor commented 6 years ago

From all the feedback it sounds like it'll be worth pushing this through in its current state and have subsequent optimizations made in the near future, prior to release.