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

use module to handle redirects for 18F Jobs #410

Merged
merged 5 commits into from
Mar 24, 2020
Merged

use module to handle redirects for 18F Jobs #410

merged 5 commits into from
Mar 24, 2020

Conversation

afeld
Copy link
Contributor

@afeld afeld commented Mar 9, 2020

I've been thinking a bit about how to make setting up a redirect for a TTS site easier, and was wondering if there's an all-AWS way to do it. I found an example of the basics, and then found a module that takes care of everything, including multiple source domains and managing certificates.

Besides creating a CloudFront distribution being slow (not the module's fault), I tried it out from my machine, and it Just Works 🚀

Doing redirects in this way rather than using pages-redirects means that we don't:

  • Have the configuration for each redirect spread across two repositories and cloud.gov services
  • Need to create CDN distributions by hand
  • Have references to mystery CloudFront hostnames in the code

I only converted the one to give a proof of concept, but it wouldn't be hard to do the rest. Let me know what you think!

@afeld afeld requested a review from amirbey March 9, 2020 13:53
Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

looks like an easier solution to our current redirects workflow ... if this works ... we should follow up with a removal of the jobs/join.18f.gov redirects in pages-redirects

@amirbey
Copy link
Contributor

amirbey commented Mar 9, 2020

👀 @davemcorwin

@davemcorwin
Copy link
Contributor

@afeld I know very little about Terraform, and am not a DNS expert, so bear with me if these thoughts are naive. It looks like this approach replaces using CNAMEs and explicit references to Cloudfront distributions with 301 redirects placed in S3 buckets.

While I like not having to explicitly reference the distributions, it is clear exactly where things are pointing.

  1. Will folks be confused by the artifacts in S3?
  2. Will the permanent redirect cause issues down the road? (ex. if we want to change join.18f.gov to point elsewhere in the future, we have to remember to redirect from 18f.gsa.gov/join, probably OK)
  3. Are we ok relying on this 3rd party OS library?

@afeld
Copy link
Contributor Author

afeld commented Mar 9, 2020

it is clear exactly where things are pointing

I disagree: There is no way to know from this repository where that CloudFront distribution lives. Is the record pointing to pages-redirect, or something else? They are disconnected. Here, at least, CloudFront references the S3 bucket in the same account, which is easier to retrieve.

Will folks be confused by the artifacts in S3?

I don't think so. The bucket names are all prefixed with redirect-.

Admittedly, it is moving the cruft from one AWS account (cloud.gov, via the service instances) to another, but at least it's easier to manage this way.

Will the permanent redirect cause issues down the road?

I don't think it makes it worse, and is better for SEO.

if we want to change join.18f.gov to point elsewhere in the future, we have to remember to redirect from 18f.gsa.gov/join

Given folks might be linking to the latter, we would have to do that anyway.

Are we ok relying on this 3rd party OS library?

🤷‍♂Terraform is a third-party library. We can pin the module version if it would make us feel more comfortable.

@afeld
Copy link
Contributor Author

afeld commented Mar 9, 2020

For a little more context: the need to set up redirects is going to come up more and more, especially with the Digital Council looking to shut down sites that aren't maintained.

we should follow up with a removal of the jobs/join.18f.gov redirects in pages-redirects

Yeah, if we want to go all the way with this, we can remove a lot from pages-redirects, and possibly deprecate the whole thing.

@davemcorwin
Copy link
Contributor

Thanks @afeld , sounds fine to me, especially as we'd probably prefer Tech Portfolio taking ownership of this anyways. Regarding the 3rd party library thing, i should have emphasized the word this. Terraform is a thing managed by a pretty well-known company with a ton of history. I can't speak for other folks, but I've never heard of mediapop so I'd be more cautious since this interacts with our infra. Since I'm not all that familiar with Terraform, I have no idea what a malicious user could do if configuration of their choosing is injected into our Terraform config, but at a minimum, they could redirect traffic :)

Locking the version seems like the minimum here. We could even go old school and copy/paste the code into our repo. This amount of caution may seem strange from a Javascript dev (mostly these days), but the worst case scenario here seems bad.

@its-a-lisa-at-work
Copy link
Contributor

@afeld this seems OK to merge, yes?

@its-a-lisa-at-work
Copy link
Contributor

@afeld nudge

@afeld
Copy link
Contributor Author

afeld commented Mar 23, 2020

Locking the version seems like the minimum here. We could even go old school and copy/paste the code into our repo.

That's fair. Not sure of the best way to do it. Asked the Terraform community.

@afeld
Copy link
Contributor Author

afeld commented Mar 23, 2020

Locked the version until we figure out a better way.

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

Successfully merging this pull request may close these issues.

4 participants