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

Terraforming Cloudflare for DNS records #3391

Closed

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Jun 20, 2023

Main Changes

  • Added support for Terraform in the repository (setup, remote backend, dependencies, etc)
  • Imported DNS records for nodejs.org
  • Imported DNS records for iojs.org
  • Added dedicated pipeline for Terraform that plan (PR) and apply (merged to main) the changes when changes are done in the terraform folder.
  • Added basic documentation, including examples of how to review PRs
  • Added specific git ignore rules

Context

Notes

Cloudflare API Token

The Cloudflare API Token is stored in the Terraform Cloud workspace. The token is used to authenticate the Terraform Cloud agent to the Cloudflare API. The token is stored in the TF_API_TOKEN environment variable in the Terraform Cloud workspace.

This token currently is in READ ONLY mode so it can not be used to create or update resources in Cloudflare. This is a security measure to prevent accidental changes in the Cloudflare account until we are familiar with Terraform as a team. See: #3370

CI Settings

I need to include a secret TF_API_TOKEN in the github actions settings in this repository. Not sure how to do that as I am not an admin in this repository.

Captura de pantalla 2023-06-20 a las 14 56 30

Changelog

@ovflowd
Copy link
Member

ovflowd commented Jun 20, 2023

I need to include a secret TF_API_TOKEN in the github actions settings in this repository. Not sure how to do that as I am not an admin in this repository.

I can do that for you, if that's fine. (I know it exceeds what I usually should do with my "superadmin" rights, but I believe in this case it is fine)

proxied = false
ttl = 1
type = "CNAME"
value = "npmjs.org"
Copy link
Member

Choose a reason for hiding this comment

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

Aside: that seems like something that should transfer to GitHub/NPM @MylesBorins

proxied = false
ttl = 1
type = "CNAME"
value = "npmjs.org"
Copy link
Member

Choose a reason for hiding this comment

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

Also this one

Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to make packages.nodejs.org point to nphjs.org as I don't see that I just get to the main website when I go to packages.nodejs.org.

But I agree that we should deal with changes in follow on PRs

Copy link
Member

Choose a reason for hiding this comment

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

@mhdawson the 2 npmjs.org ones were fixed in #3395, which is why you are just seeing the normal site now. I don't see the "Resolve" button to hide these, but they'd also go away the next time the sync is run, since the records no longer exist

proxied = true
ttl = 1
type = "CNAME"
value = "logs.libuv.org"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't appear to resolve

Copy link
Member

Choose a reason for hiding this comment

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

logs.libuv.org used to point to a server with slurped IRC logs (definitely for libuv channels but I think some Node.js channels as well). I'm not sure who owned it (or even if libuv has a channel on IRC or anywhere else anymore) -- @bnoordhuis @cjihrig do you remember?

terraform/.gitignore Outdated Show resolved Hide resolved
Co-authored-by: Moshe Atlow <moshe@atlow.co.il>
@UlisesGascon
Copy link
Member Author

@nschonni I am not planning to make DNS record changes in this PR because we are only migrating the current state from the Cloudflare UI to Terraform in this PR, and we don't have writing permissions yet in the GitHub Action.

I think we can move that discussion to a separate PR. In the meantime, I created a subtask for it on #3270 (comment).

Let me know if this approach works for you 👍.

@UlisesGascon UlisesGascon requested a review from MoLow June 21, 2023 09:55
@nschonni
Copy link
Member

Random comment on #3223 reminded me that this can't close #3270 because it doesn't cover things like the firewall rules (yet)

@mhdawson
Copy link
Member

@UlisesGascon my only question is if it would make sense for us to break this into 2 PRs, one to add the records for the iojs.org and s second one for nodejs.org. The idea being that landing the iojs.org one would be lower risk and once we see that all working we land the second. Not sure if it helps but was wonding.

### Local development

#### IMPORTANT ⚠️
If you apply any plan in you local machine, you can trigger changes in the Cloudflare account. So, you must be careful with the changes you are applying in your local environment. In order to review/create changes you are not require to do local development as the github pipeline will generate a plan with the changes.
Copy link
Member

Choose a reason for hiding this comment

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

@UlisesGascon I would have thought you would need Cloudflare access for anything you do locally to have an effect?


#### Requirements
- Terraform version `Terraform v1.4.5` used to build this infrastructure. [How to install Terraform](https://developer.hashicorp.com/terraform/tutorials/aws-get-started/install-cli)
- You will need a `TF_API_TOKEN` in you environment with a valid Terraform cloud API Key. [How to create a Terraform cloud API Key](https://www.terraform.io/docs/cloud/users-teams-organizations/api-tokens.html#creating-an-api-token)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is related to my question above. I assume we don't expect anybody to be doing this. Intead there should be a PR and the action which has the TF_API_TOKEN will apply any changes.

@UlisesGascon
Copy link
Member Author

UlisesGascon commented Jun 29, 2023

@mhdawson @nschonni let me know if this can help solve some of the questions from your previous comments

General overview

Captura de pantalla 2023-06-29 a las 8 58 37

Green path

This is the default way (and recommended) when we want to make any changes in Cloudflare once this PR is merged. Any user can create a PR and add/update/remove resources from Cloudflare. Once the PR is merged, the GitHub actions will notify this change directly to Terraform Cloud. Terraform Cloud will apply the changes in Cloudflare.

Blue path

Any user who has access to Terraform Cloud (can use or generate a token). This token can be used in the same way as the GitHub actions do in the green path. So, even if you don't have access to Cloudflare, when you trigger a new plan directly from Terraform Cloud, it will be applied as Terraform Cloud can authenticate against Cloudflare.

This option is quite interesting as it is the best way to import resources. For example, as @nschonni suggested with the firewall rules.

Red path

As far I know is what we do today, the user has write access to Cloudflare and can make changes via web UI or CLI. This option remains possible even when this PR is merged.

Only a few people in the org have this access level, which will be used, for example, to modify Cloudflare settings for items that are not covered with Terraform.

Specific concerns

@UlisesGascon my only question is if it would make sense for us to break this into 2 PRs, one to add the records for the iojs.org and s second one for nodejs.org. The idea being that landing the iojs.org one would be lower risk and once we see that all working we land the second. Not sure if it helps but was wonding.

We can split this PR into two separate ones, but currently Terraform cannot apply any changes in Cloudflare as we have a read only token.

I think that we can keep this approach (read only tokens) until we feel confident with this jump. I believe that just by keeping these Terraform files in sync with the Cloudflare state, we are already providing value to the org members as now our infrastructure is much more transparent than before and it is easier to propose changes, even if we make manual changes via Cloudflare CLI or web UI. at the end. It will also help us to detect discrepancies between our desired state and the current Cloudflare state

Random comment on #3223 reminded me that this can't close #3270 because it doesn't cover things like the firewall rules (yet)

I think it is a good idea to include the rules in this PR. Should I import them in this PR or do we prefer a separate one?

@nschonni
Copy link
Member

I think it is a good idea to include the rules in this PR. Should I import them in this PR or do we prefer a separate one?

I think either this PR or another is ok, whichever you prefer. I was just flagging that this PR is set to autoclose that other issue, but I think there are one or 2 more things that might still be needed like the firewall stuff.

@UlisesGascon
Copy link
Member Author

I think either this PR or another is ok, whichever you prefer. I was just flagging that this PR is set to autoclose that other issue, but I think there are one or 2 more things that might still be needed like the firewall stuff.

Great point, I just removed the autoclose reference

@mhdawson
Copy link
Member

mhdawson commented Jun 29, 2023

@UlisesGascon thanks for the nice picture. We might want to include that in the doc somewhere. I probably was not quite clear enough in my questions, but your answer confirms it anway. Generally people should not have the cloudflare token and updates will be through the GitHub action. As more of an exceptional case (doing initial reource imports etc.) people can get a token an use it locally.

EDIT: Also seems like we can use a READONLY token for imports that we run locally and that should possibly be the standard approach that we document. In that way people can't accidentally deploy.

@mhdawson
Copy link
Member

I think that we can keep this approach (read only tokens) until we feel confident with this jump

I do think we want to make the jump sooner than later. I'd prefer we make a step of managing some of the DNS entries to start versus waiting longer. I may be making more work, but for example if we added 5 at a time, then if anything when wrong the manual work to fixup would be lower. @richardlau does doing them in smaller batches make any sense to you?

@UlisesGascon
Copy link
Member Author

I will close this PR as we will move this initiative to a different repository. Follow up details in nodejs/admin#848.

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.

None yet

6 participants