search

cloud-gov / cf-cdn-service-broker

A Cloud Foundry service broker for CloudFront and Let's Encrypt
Other
10 stars 13 forks source link

Forward host header for default origin. #65

Closed jmcarp closed 7 years ago

jmcarp commented 7 years ago
jmcarp commented 7 years ago

To explain a little more: this patch adds a default origin to the broker configuration, which would point to the cloud.gov apps load balancer. Users who want to add a domain to cloud.gov wouldn't pass the origin option--they'd add the domain via the cf cli. Users who want to point a domain elsewhere, like to an S3 bucket, will still set the origin option. The magical thing that I don't like is that we forward the host header if the origin is unset or set to the default origin, but not if the user specifies a non-default origin.

We could do better by allowing users to pass a list of headers to forward. If we do that, we can either default the list to ["Host"] (and allow users to default to an empty list for external origins) or default the list to [] (and allow users to pass ["Host"] or ["*"] for the default origin). I was going to do this originally, but I was thinking it might be confusing to users, and to us when we're doing support. WDYT @cnelson @jcscottiii ?

cnelson commented 7 years ago

The magical thing that I don't like is that we forward the host header if the origin is unset or set to the default origin, but not if the user specifies a non-default origin.

I think this is fine -- having the broker configure the distribution to work with cloud.gov when using that as an origin doesn't seem magic to me, it seems like the right thing to do.

I was going to do this originally, but I was thinking it might be confusing to users

I agree, I'd vote for not exposing that level of complexity until we have users demanding it.

jmcarp commented 7 years ago

Tests passing on staging, so I'm dropping the [WIP]. If @jcscottiii and @cnelson, lmk if you have more comments, and I'll address them all together.

jmcarp commented 7 years ago

Also, thoughts on whether we should automatically create and destroy private domains on provision / unprovision?

cnelson commented 7 years ago

Also, thoughts on whether we should automatically create and destroy private domains on provision / unprovision?

I vote no; less magic is better. But WDYT about failing with messaging telling the user they need to create it if they specify a domain that CF doesn't know about?

jmcarp commented 7 years ago

I like that--will make that change once https://github.com/cloudfoundry-community/go-cfclient/pull/55 lands.

jmcarp commented 7 years ago

Updated! @cnelson, can you review the latest changes when you get a chance?

jmcarp commented 7 years ago

@cnelson: I fetched the org name when domains don't exist like you suggested, and acceptance tests haven't broken. I think it's ready!

cnelson commented 7 years ago

As soon as https://github.com/18F/cg-site/pull/752 gets merged :shipit: !

jcscottiii commented 7 years ago

@jmcarp one more question, the rails apps were having trouble with the X-Forwarded- header. will this allow users to modify that? (i see getHeaders returns either Host or empty slice).

I think that the empty slice will work but that will only work for apps passing in the non default origin, right?

jmcarp commented 7 years ago

@jcscottiii: the issue was that we weren't forwarding the original header, so that the header became the cloud.gov domain instead of the cloudfront domain. We could either forward the origin host header, or set x-forwarded-host. We wound up forwarding based on reasons in https://github.com/18F/cf-cdn-service-broker/issues/64.