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

Turnstile with "no" domains generates invalid config #655

Closed
2 tasks done
punkeel opened this issue Dec 20, 2023 · 4 comments
Closed
2 tasks done

Turnstile with "no" domains generates invalid config #655

punkeel opened this issue Dec 20, 2023 · 4 comments

Comments

@punkeel
Copy link
Member

punkeel commented Dec 20, 2023

Confirmation

  • My issue isn't already found on the issue tracker.
  • I have replicated my issue using the latest version of the library and it is still present.

cf-terraforming version

0.16.1

Expected outcome

cf-terraforming generates valid blocks for all possible configurations.

Actual outcome

cf-terraforming generates invalid code blocks :(


│ Error: Missing required argument

│   on turnstile_widgets.tf line 20, in resource "cloudflare_turnstile_widget" "terraform_managed_resource_...":
│   20: resource "cloudflare_turnstile_widget" "terraform_managed_resource_..." {

│ The argument "domains" is required, but no definition was found.

The generated block should include domain = [], likely because it is the default "Go" value for an array.

Steps to reproduce

  1. Get the entitlement for not specifying any domains
  2. Create a Turnstile widget with an empty list of domains
  3. Try to import it

Note: this requires a paid entitlement.

References

No response

@jacobbednarz
Copy link
Member

if you didn't need to provide domains, wouldn't you expect not to have them present anywhere? i suspect this is a change that should be made in the provider instead to make domains optional instead (which in turn, will be correctly handled here since the schema will dictate it can be empty and not generate an attribute for it.

@punkeel
Copy link
Member Author

punkeel commented Dec 21, 2023

if you didn't need to provide domains

I'd say the opposite: I need to provide the empty list.

99.9% of users must provide a list of domains as they won't have the entitlement. The 0.1% of users who can not provide it must do it on purpose, not by mistake.

I don't think the provider is wrong here - but happy to be convinced otherwise

Note for clarity & future reference: I'm on the Turnstile team

@jacobbednarz
Copy link
Member

jacobbednarz commented Dec 21, 2023

needing to explicitly provide an empty list feels a little wrong (similar to initialising empty blocks like foo {}) but i don't feel strongly about it. my expectation for this situation would be that the field is optional with documentation as to when to use it and the API service has the entitlement check to fall back on as a guard rail for ensuring only those who should use this functionality can do so.

i'm happy to review a PR that updates the mappings to always provide the value (even an empty list) which can later be populated with additional domains if they are present. there are quite a few examples of prior art in the generation if you're keen to take a pass at this.

punkeel added a commit to punkeel/cf-terraforming that referenced this issue Dec 21, 2023
When Turnstile is configured with an empty list of domains, no validation
is performed by Cloudflare - the widget can be used anywhere. The website
owner is responsible for validating the challenge was solved on the "right"
website.
This feature is gated behind an entitlement, and `domains = []` is used to
enable this. This is *not* a default value for domains, it must be picked
explicitly.

Fixes cloudflare#655.
punkeel added a commit to punkeel/cf-terraforming that referenced this issue Dec 21, 2023
When Turnstile is configured with an empty list of domains, no validation
is performed by Cloudflare - the widget can be used anywhere. The website
owner is responsible for validating the challenge was solved on the "right"
website.
This feature is gated behind an entitlement, and `domains = []` is used to
enable this. This is *not* a default value for domains, it must be picked
explicitly.

Fixes cloudflare#655.
punkeel added a commit to punkeel/cf-terraforming that referenced this issue Dec 21, 2023
When Turnstile is configured with an empty list of domains, no validation
is performed by Cloudflare - the widget can be used anywhere. The website
owner is responsible for validating the challenge was solved on the "right"
website.
This feature is gated behind an entitlement, and `domains = []` is used to
enable this. This is *not* a default value for domains, it must be picked
explicitly.

Fixes cloudflare#655.
@punkeel
Copy link
Member Author

punkeel commented Dec 21, 2023

I strongly believe the list of domains should always be present in the Terraform configuration, it is a fundamental part of a Turnstile widget, and a default value doen't exist for it.
The empty list has a special meaning (not a default value!)–if we could represent this using enums, we would likely use AnyHostname | NonEmptyListOfDomains(string[]).

During their lifetimes, widgets will more likely switch NonEmptyListOfDomains -> AnyHostname than back.

At creation time, domains is required by the API (it can be empty iff you have the entitlement).
When updating a widget, we distinguish between the absence of the domains field (don't update it), and the empty list (update the list to be empty).

(Now that I think of it, I'm not sure cloudflare-go allows this... oops)

I don't know how Terraform would behave if you didn't specify a list of domains - would it try to empty the list of domains, or leave it unchanged. Making it a required guarantees tf will do the right thing: what the code block says! (this concern may very well be made up!)


I opened #659 for this. I tried to keep it short & simple

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

No branches or pull requests

2 participants