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

Support use of API tokens in addition to API Keys #450

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Support use of API tokens in addition to API Keys #450

merged 1 commit into from
Aug 22, 2019

Conversation

hunts
Copy link
Contributor

@hunts hunts commented Aug 20, 2019

This is the alternative way to add support of using API Tokens. We can use this one if the release date of the breaking changes is too far away.

This pull request introduced a new field called "api_token" as alternative to the "email"+"token" (api key) auth scheme.

Changes

  • Added optional api_token as an alternative to email+token (api key)
  • Marked email and token (api key) as optional

Changes
- Added optional api_token as an alternative to email+token (key)
- Marked email and token (key) as optional

Related-to #419
@ghost ghost added size/M kind/documentation Categorizes issue or PR as related to documentation. labels Aug 20, 2019
@jacobbednarz
Copy link
Member

I like the intention here however I don't think we should proceed with this. We already have a field that is "token" and I think this is just going to create confusion and problems due to being too closely named.

Can you explain a little more why this needs to be an expedited process to roll out the ability to use account tokens? Right now, majority of users are still using this so I think we need a pretty good reason to introduce potential confusion here.

@garrettgalow
Copy link
Contributor

garrettgalow commented Aug 20, 2019

Hunts and I were talking and it seems really poor experience to suddenly rename the value for the api key they are using. It is a breaking change that doesn't need to be one. I recognize that this is me walking back from previous statements.

While its shitty that TF calls the api key a token and our new functionality is called API tokens - we need to make this easier to adopt. I'm not in love with the name api_token, but I don't really have a better suggestion. One differentiator is that with the api token the email is no longer required. I don't know if we can use that to help differentiate.

@jacobbednarz
Copy link
Member

Hunts and I were talking and it seems really poor experience to suddenly rename the value for the api key they are using.

I agree breaking changes are a 💩 experience which is also part of the driver why we're intending to do a few of them at once to limit the disruption on end users. I do however still feel it's a necessary evil to prevent confusing conversations (email/token + api tokens) and carrying around a bunch of extra, fragile code (zone => zone_id) because we don't want end users to be doing any work. Making these types of changes not only gives us an opportunity to take stock of the decisions made in the past and re-evaluate whether they still make sense today but also provide improvements to supporting the project itself to ensure we're not making maintenance harder than it needs to be.

While its shitty that TF calls the api key a token and our new functionality is called API tokens - we need to make this easier to adopt.

FWIW, the change we're talking about here (and we're currently running internally on our fork) is this:

@@ -1,5 +1,5 @@
 provider "cloudflare" {
   email = "${var.cloudflare_email}"
-  token = "${var.cloudflare_token}"
+  key   = "${var.cloudflare_key}"
 }

We automated the swapping of this value with sed across the 100-ish files we have and it took a matter of minutes.

For moving to API Tokens, we dropped the email value and then swapped the key value back to token and it was a drop in replacement which felt like a pretty reasonable change.

One differentiator is that with the api token the email is no longer required. I don't know if we can use that to help differentiate.

I've got no issue with the solution technically, either is possible. As you mention, it is easy enough to differentiate using the presence of the email value.

In saying that, if others don't think the points above are worth waiting for (2 of the 3 proposed breaking changes are already complete awaiting a merge) I'm not going to loose any sleep over shipping api_token and renaming it later when we deprecate the email/key authentication 🙂

@jacobbednarz
Copy link
Member

Sorry, for what it's worth, all three breaking changes are ready to go. I think the zone name => zone id will need a gradual rollout but other than that, we're ready to 🚢 if speed to market is a concern here.

@patryk
Copy link
Contributor

patryk commented Aug 22, 2019

Thanks for doing it in a backwards-compatible way. We could do a proper rename in breaking-change release (2.x).

@patryk patryk merged commit 724d943 into cloudflare:master Aug 22, 2019
boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants