Fastly improvements

[zimbatm]

Fixes some issues I encountered:

• Fastly backend TLS - Not enough options & ssl_check_cert no longer works #9003
• provider/fastly: service_v1 gets updated every time even when attributes didn't change #8916

Might solve #8868 as well but I didn't test

[zimbatm]

Can I get some love here? The fastly module barely works without these changes. Just rebased to fix the merge conflict.

[hc2p]

It would be lovely to get this merged, as we struggle with exactly those issues

[zimbatm]

In the mean time you can build that providers as an external plugin:

git clone https://github.com/zimbatm/terraform.git
cd terraform
git checkout fastly-improvements
make plugin-dev PLUGIN=provider-fastly
cat <EOF >> ~/.terraformrc
providers {
    fastly = "$(which terraform-provier-fastly)"}
}
EOF

something like that

[jeremylivingston]

Agreed that it would be great to get this merged. I'm in need of some of these fixes as well.

[jeremylivingston]

@zimbatm I'm having issues running the commands you outlined above. It's asking me to set GOPATH. What should that be set to?

[zimbatm]

try something like this (EDIT: fixed per next comment):

mkdir -p $HOME/go/src/github.com/hashicorp/terraform
git clone https://github.com/hashicorp/terraform/ $HOME/go/src/github.com/hashicorp/terraform
export GOPATH=$HOME/go

[jeremylivingston]

@zimbatm Thanks, I was able to get it to work with that pointing me in the right direction. One clarification, the directory structure should be $HOME/go/src/github.com/hashicorp/terraform for the git repo. The above commands check everything into hashicorp, which doesn't work. I also had to make sure I was running the latest Go.

It would be great to get this merged so it is part of the next release. There are some pretty critical pieces left out of the Fastly provider and this helps push it in the right direction. What do you think @stack72?

[jeremylivingston]

Ping, @stack72. Any chance that you or someone else can take a look at this one? Thank you!

[stack72]

Hi @jeremylivingston

Will get to this this week! Sorry it has taken so long

Paul

[stack72]

ok, the only thing this is missing is the update to the documentation - I can take care of that though

[stack72]

% make testacc TEST=./builtin/providers/fastly                                                            ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/06 08:21:54 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/fastly -v  -timeout 120m
=== RUN   TestAccFastlyIPRanges
--- PASS: TestAccFastlyIPRanges (0.92s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccFastlyServiceV1CacheSetting_basic
--- PASS: TestAccFastlyServiceV1CacheSetting_basic (38.78s)
=== RUN   TestAccFastlyServiceV1_conditional_basic
--- PASS: TestAccFastlyServiceV1_conditional_basic (16.81s)
=== RUN   TestFastlyServiceV1_FlattenGzips
--- PASS: TestFastlyServiceV1_FlattenGzips (0.00s)
=== RUN   TestAccFastlyServiceV1_gzips_basic
--- PASS: TestAccFastlyServiceV1_gzips_basic (39.52s)
=== RUN   TestFastlyServiceV1_BuildHeaders
--- PASS: TestFastlyServiceV1_BuildHeaders (0.00s)
=== RUN   TestAccFastlyServiceV1_headers_basic
--- PASS: TestAccFastlyServiceV1_headers_basic (37.32s)
=== RUN   TestAccFastlyServiceV1RequestSetting_basic
--- PASS: TestAccFastlyServiceV1RequestSetting_basic (15.24s)
=== RUN   TestAccFastlyServiceV1_s3logging_basic
--- PASS: TestAccFastlyServiceV1_s3logging_basic (33.96s)
=== RUN   TestAccFastlyServiceV1_s3logging_s3_env
--- PASS: TestAccFastlyServiceV1_s3logging_s3_env (15.98s)
=== RUN   TestResourceFastlyFlattenDomains
--- PASS: TestResourceFastlyFlattenDomains (0.00s)
=== RUN   TestResourceFastlyFlattenBackend
--- PASS: TestResourceFastlyFlattenBackend (0.00s)
=== RUN   TestAccFastlyServiceV1_updateDomain
--- PASS: TestAccFastlyServiceV1_updateDomain (35.64s)
=== RUN   TestAccFastlyServiceV1_updateBackend
--- PASS: TestAccFastlyServiceV1_updateBackend (38.66s)
=== RUN   TestAccFastlyServiceV1_basic
--- PASS: TestAccFastlyServiceV1_basic (15.30s)
=== RUN   TestAccFastlyServiceV1_disappears
--- PASS: TestAccFastlyServiceV1_disappears (7.87s)
=== RUN   TestAccFastlyServiceV1_VCL_basic
--- PASS: TestAccFastlyServiceV1_VCL_basic (37.61s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/fastly	333.631s

Manually merging to include the documentation

[jeremylivingston]

Great, thank you @stack72! When does a change like this typically get tagged and/or rolled into the latest stable?

[stack72]

Hi @jeremylivingston

This will be part of the 0.8-rc3 release and thus on the 0.8 stable release in the next couple of weeks

P.

[jeremylivingston]

Good news. Thanks again for the help!

[jeremylivingston]

@zimbatm I think that there may be a bug with this code. I recently ran into an issue where my backends had a TLS hostname set, but pushing a value to it through Terraform cleared it out. Have you run into this issue?

[jeremylivingston]

I've confirmed that this indeed does not work as intended, whether you are adding or updating a backend. The code allows you to get through to the resource creation (which it didn't do previously), but nothing shows up in the backend's "Certificate hostname" field, which causes 503 errors when Fastly hits your origin server.

Is there any way that we could get this fixed in a minor version update? This is pretty much a show-stopper for my HTTPS site, as negotiating with origin will always fail.

Let me know if you have any questions. Thanks!

[zimbatm]

Can you please paste the code with clearly redacted sections if you have to redact something?

[jeremylivingston]

@zimbatm Sure.

resource "fastly_service_v1" "my_service" {
  name = "Example.com"

  domain {
    name = "www.example.com"
  }

  backend {
    address               = "123.12.123.12"
    name                  = "Backend 1 - No SSL"
    port                  = 80
    auto_loadbalance      = true
    connect_timeout       = 5000
    first_byte_timeout    = 90000
    between_bytes_timeout = 20000
    max_conn              = 500
  }

  backend {
    address               = "123.12.123.12"
    name                  = "Backend 1 - SSL"
    port                  = 443
    auto_loadbalance      = false
    connect_timeout       = 5000
    first_byte_timeout    = 90000
    between_bytes_timeout = 20000
    max_conn              = 500
    ssl_hostname          = "cert.example.com"
  }

  backend {
    address               = 123.12.123.13"
    name                  = "Backend 2 - No SSL"
    port                  = 80
    auto_loadbalance      = true
    connect_timeout       = 5000
    first_byte_timeout    = 90000
    between_bytes_timeout = 20000
    max_conn              = 500
  }

  backend {
    address               = "123.12.123.123"
    name                  = "Backend 2 - SSL"
    port                  = 443
    auto_loadbalance      = false
    connect_timeout       = 5000
    first_byte_timeout    = 90000
    between_bytes_timeout = 20000
    max_conn              = 500
    ssl_hostname          = "cert.example.com"
  }

  gzip {
    name          = "Default file extensions and content types"
    extensions    = ["css", "js", "html", "eot", "ico", "otf", "ttf", "json"]
    content_types = ["text/html", "application/x-javascript", "text/css", "application/javascript", "text/javascript", "application/json", "application/vnd.ms-fontobject", "application/x-font-opentype", "application/x-font-truetype", "application/x-font-ttf", "application/xml", "font/eot", "font/opentype", "font/otf", "image/svg+xml", "image/vnd.microsoft.icon", "text/plain", "text/xml"]
  }

  vcl {
    name    = "main.vcl"
    content = "${file("vcl/main.vcl")}"
    main    = true
  }

  default_ttl = 3600
}

When I do this, the ssl_hostname field does end up showing up in my state file for the port 443 backends. However, when I look in my newly activated version, I see the backends with the "Certificate hostname" field blank.

Let me know if you need anything else. Thanks!

[zimbatm]

Can you try to set ssl_check_cert = true in the backend?

Fastly tend to change or remove values that were set through API calls. They are trying too hard to be helpful which results in these weird cases where a value is set but then appears as null or something different when reading the API back. If I remember correctly, if ssl_check_cert is set to false they also remove associated settings like the ssl_hostname.

[jeremylivingston]

Unfortunately that did not work. If I recall correctly, ssl_check_cert defaults to true. I tried toggling it to false, applying, then toggling it back to true. My SSL hostname field is still blank.

[jeremylivingston]

@zimbatm Were you able to confirm if you could reproduce this issue?

[zimbatm]

Sorry I don't have the time to work on this at the moment. If you want to try to debug this try to run terraform with TF_LOG=trace

…

On Mon, 2 Jan 2017, 22:04 Jeremy Livingston, ***@***.***> wrote: @zimbatm <https://github.com/zimbatm> Were you able to confirm if you could reproduce this issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9629 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMsE6lvUq1kCbKCJJKCz-gEfoPWH_Rks5rOXR0gaJpZM4KhSkw> .

[msuterski]

FYI, the part of this "fix" broke imports of backends which have options set to false.

2017/03/07 21:38:00 [DEBUG] plugin: terraform: fastly-provider (internal) 2017/03/07 21:38:00 [WARN] Error setting Backends for (service ID): backend.0.auto_loadbalance: '' expected type 'bool', got unconvertible type '*fastly.Compatibool'

@zimbatm what was the reasoning for wrapping those booleans in gofastly.CBool ?

e.g.

gofastly.CBool(b.AutoLoadbalance)

[msuterski]

@zimbatm I read your comment on the commit. If the false value was mishandled while setting values in Fastly, your changes are probably fine, but only for parts were TF is updating values in Fastly. The code where TF is reading the values, the types are mis-matched. So, all the "flattening" functions should not include the type change probably.

[jeremylivingston]

Also, @zimbatm, I was able to do some testing directly with the Fastly API and think I found the issue with the SSL certificate.

It appears that when I use the ssl_cert_hostname field, the certificate hostname populates properly. However, when using ssl_hostname, the certificate does not populate.

From the docs, it says ssl_cert_hostname:

Overrides ssl_hostname, but only for cert verification. Does not affect SNI at all.

This is confusing because when I use ssl_hostname, nothing shows up in either the Certificate Hostname or SNI Hostname fields. When I use the individual ssl_cert_hostname and ssl_sni_hostname fields, my value shows up. I'm not sure if this is by design, but it makes me wonder if there is a bug in Fastly's API for ssl_hostname.

Let me know if you have any thoughts. My initial reaction is that we should add both ssl_cert_hostname and ssl_sni_hostname to Terraform.

[zimbatm]

@msuterski the reason to use gofastly.CBool is because we need three different states: true, false and unset (nil pointer). Otherwise not all configurations are transmittable to Fastly.

@jeremylivingston yes I think adding the other options is a good idea.

I'm not using Fastly anymore so I'm not highly motivated to work on this

[msuterski]

@jeremylivingston the ssl_hostname that @zimbatm added works as expected. We're using it with all our backends.

If you have problems with it make sure that:

1. the hostname that you're putting there is handled by the SSL Cert on your origin
2. you override the Host header on your request before it is passed to your origin (it should have your origin's hostname)

You can verify your SSL Cert by curling your origin over https with the -v option.

[msuterski]

@stack72 I was wondering if you'd have any suggestions regarding how to handle the types conversions in the TF's schema parser.

The parser clearly expects these values as boolean but with this implementation we're giving it a pointer to an object.

This actual problem is also causing TF to fail updating the state files while we're running TF's plan and apply. This was something I was going to file a bug about, but now we know why it's happening.

The service creation and updates "work" and set expected states only because we are passing already modified schema object to the resourceServiceV1Read (seems like Read function is meant to override the values from the original schema).

My first suggestion would be to remove the type conversion on the functions that pull data from Fastly. I think @zimbatm was correct with the type conversion when we're pushing data to Fastly.

I was incorrect before, this is broken for all the boolean values returned by Fastly, not only ones with the false value.

[jeremylivingston]

@msuterski Thanks for the insight. I didn't realize that you had to override the origin hostname when calling the server. I haven't had to do this when I use the ssl_cert_hostname option...is that because that option is reserved for the initial verification but not for communication?

Side note, it does seem odd that settings on ssl_hostname don't show up in the Fastly interface. I would think that it would show up under both the Certification and SNI fields. It doesn't show up on either for me. That doesn't seem right...

[msuterski]

@jeremylivingston yup, Fastly's web console has a couple of bugs like that.

I don't think we've ever used ssl_cert_hostname option, so I can't comment on it.

[jeremylivingston]

@msuterski Got it.

From their API docs, it looks like:

ssl_hostname - Used for both SNI during the TLS handshake and to validate the cert.
ssl_cert_hostname - Overrides ssl_hostname, but only for cert verification. Does not affect SNI at all.
ssl_sni_hostname - Overrides ssl_hostname, but only for SNI in the handshake. Does not affect cert validation at all.

So judging by what you've described, is overriding the hostname when using the SNI override required? This is surprising, because it would seem that this is precisely the purpose of the ssl_hostname and ssl_sni_hostname settings.

For instance, if I my site lived at www.example.com and my origin was at origin.example.com (with an origin.example.com certificate), is the expectation that I have to override the hostname and set it to origin.example.com in order to have TLS work?

Maybe a question better suited for Fastly support, but I figured you might know. Thanks!

[msuterski]

@jeremylivingston I asked Fastly support about the ssl_*_hostname options and the Web console (and missing values).

Apparently ssl_hostname was "deprecated" and a use of ssl_cert_hostname and ssl_sni_hostname is now the way to go (Fastly's docs do not reflect those changes yet). So, I'd say you were doing it right and we'll need to add those new fields to TF. And I'll need to update all of our services...

[jeremylivingston]

@msuterski Thanks for the info! I opened a PR to add these fields at #12578. Hopefully you don't have too many services to update. 😄

[ghost]

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.