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

Adds ciphers to zone settings override #899

Closed
wants to merge 1 commit into from

Conversation

riuvshyn
Copy link
Contributor

@riuvshyn riuvshyn commented Dec 28, 2020

Adds zone settings override for ciphers: https://api.cloudflare.com/#zone-settings-change-ciphers-setting

Also I've noticed that client/v4/zones/.../settings returns some more options that are not available in terraform:

{
  "result": [

...

    {
      "id": "ciphers",
      "value": [],
      "modified_on": null,
      "editable": true
    },

...

    {
      "id": "edge_cache_ttl",
      "value": 7200,
      "modified_on": null,
      "editable": true
    },

...

    {
      "id": "orange_to_orange",
      "value": "off",
      "modified_on": null,
      "editable": true
    },

...

    {
      "id": "visitor_ip",
      "value": "on",
      "modified_on": null,
      "editable": true
    },

...

  ],
  "success": true,
  "errors": [],
  "messages": []
}

Should I also add visitor_ip, orange_to_orange, edge_cache_ttl settings? Also I do not see anything for these settings in the docs...

@riuvshyn riuvshyn changed the title Adds zone settings override for ciphers Adds missing attirbutes to zone settings override and LB Monitor resources Dec 30, 2020
@jacobbednarz
Copy link
Member

Can you please split this into two separate PRs? The changes are not related and should be reviewed separately.

@jacobbednarz jacobbednarz added the workflow/pending-op-response Indicates an issue or PR requires a response from the original poster. label Jan 3, 2021
@riuvshyn riuvshyn changed the title Adds missing attirbutes to zone settings override and LB Monitor resources Adds ciphers to zone settings override Jan 4, 2021
@riuvshyn
Copy link
Contributor Author

riuvshyn commented Jan 4, 2021

@jacobbednarz sure, thanks for looking into this
probe_zone change is here: #903

Also can you please comment on my question in description about visitor_ip, orange_to_orange, edge_cache_ttl attributes that returned by API? I am struggling to find any documentation related to these

@jacobbednarz
Copy link
Member

Should I also add visitor_ip, orange_to_orange, edge_cache_ttl settings?

I'll confirm about these first two however edge_cache_ttl was removed in #653. I'm assuming they are enterprise only features too which will make the pool of who will be using them smaller again.

@@ -62,6 +62,8 @@ func TestAccCloudflareZoneSettingsOverride_Full(t *testing.T) {
name, "settings.0.zero_rtt", "off"),
resource.TestCheckResourceAttr(
name, "settings.0.universal_ssl", "off"),
resource.TestCheckResourceAttr(
Copy link
Member

Choose a reason for hiding this comment

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

This test is failing due to the setting not being persisted to state.

=== RUN   TestAccCloudflareZoneSettingsOverride_Full
    testing.go:705: Step 1 error: Check failed: Check 8/8 error: cloudflare_zone_settings_override.test: Attribute 'settings.0.ciphers' not found
--- FAIL: TestAccCloudflareZoneSettingsOverride_Full (21.01s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	21.386s
FAIL

Check out the README for running these integration tests if you haven't already.

Instead of looking for specific values (which I don't think TypeList handles too well), just check the number of elements. This line would become resource.TestCheckResourceAttr(name, "settings.0.ciphers.#", "2") however if you swap it to that, you'll see it's empty without any elements too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm now it fails with:

Error: error from makeRequest: HTTP status 400: content "{\"success\":false,\"errors\":[{\"code\":1023,\"message\":\"Advanced Certificate Manager is required to set custom cipher suites\"}],\"messages\":[],\"result\":null}"

seems it requires some extra configuration tobe enabled...

Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

we'll need to address the failing test on this one before merging

@elovelan
Copy link

@riuvshyn, are you still looking into this? would you like me to take it over since I have a need for this?

@jacobbednarz
Copy link
Member

@elovelan if you want this functionality, i'd recommend grabbing a new PR and submitting as this one is now stagnent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow/pending-op-response Indicates an issue or PR requires a response from the original poster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants