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.

Sign up for GitHub

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

Jump to bottom

add resource 'api_token' and data source 'permission_groups' #862

Merged
merged 20 commits into from
Nov 23, 2020
Merged

add resource 'api_token' and data source 'permission_groups' #862

merged 20 commits into from
Nov 23, 2020

Conversation

UrosSimovic
Copy link
Contributor

No description provided.

@UrosSimovic UrosSimovic mentioned this pull request Nov 5, 2020
Closed
@jacobbednarz jacobbednarz marked this pull request as draft November 8, 2020 21:29
@jacobbednarz
Copy link
Member

marking as draft as this isn't yet ready for review

@reifnir
Copy link

reifnir commented Nov 10, 2020

marking as draft as this isn't yet ready for review

What is required in order for this to be ready for review?

@jacobbednarz
Copy link
Member

jacobbednarz commented Nov 10, 2020 •
edited
Loading

Let's start with a pull request description as to what the change is and any context associated with it. From there we need to:

  • cleanup the comments where functionality isn't used but has been left behind
  • remove the dependency changes to go.mod/go.sum as they come in automatically
  • add documentation for these two new pieces of functionality
  • "WIP" removed from the title

Once we get those things underway, we can start looking at the code itself. Doing these things before someone spends time reviewing the code and approach cuts down on the unnecessary back and forth or scribbling the metaphorical red ink all of the PR.

@jacobbednarz
Copy link
Member

Once you remove your changes to go.{mod,sum} you can merge in the latest master and it will include those changes from cloudflare-go.

@UrosSimovic UrosSimovic changed the title WIP: add reasource 'api_token' and data source 'permission_groups' add reasource 'api_token' and data source 'permission_groups' Nov 19, 2020
@UrosSimovic
Copy link
Contributor Author

UrosSimovic commented Nov 19, 2020 •
edited
Loading

I think this PR can be reviewed (and docs linted) now:

  • cleanup the comments where functionality isn't used but has been left behind
  • remove the dependency changes to go.mod/go.sum as they come in automatically
  • add documentation for these two new pieces of functionality
  • "WIP" removed from the title

@jacobbednarz jacobbednarz marked this pull request as ready for review November 20, 2020 00:30
@UrosSimovic
Copy link
Contributor Author

I hope I've got all proposed changes done.

@jacobbednarz
Copy link
Member

Nearly there! It looks like the test state isn't matching up with the assertions right now.

=== RUN   TestAccAPIToken
    testing.go:705: Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: cloudflare_api_token.uikfrstsfs
          condition.#:                                                                         "1" => "0"
          condition.0.request_ip.#:                                                            "1" => ""
          condition.0.request_ip.0.in.#:                                                       "0" => ""
          condition.0.request_ip.0.not_in.#:                                                   "0" => ""
          id:                                                                                  "b20c3f79a90b9bc33a0cb001ce0606ac" => "b20c3f79a90b9bc33a0cb001ce0606ac"
          name:                                                                                "uikfrstsfs" => "uikfrstsfs"
          policy.#:                                                                            "1" => "1"
          policy.0.effect:                                                                     "deny" => "deny"
          policy.0.permission_groups.#:                                                        "1" => "1"
          policy.0.permission_groups.0:                                                        "82e64a83756745bbbb1c9c2701bf816b" => "82e64a83756745bbbb1c9c2701bf816b"
          policy.0.resources.com.cloudflare.api.account.zone.0da42c8d2132a9ddaf714f9e7c920711: "*" => "*"
          status:                                                                              "active" => "active"
          value:                                                                               "mgdSw-BoxExx7F2Q1me4SAqXDGpfoc07NDDBCV74" => "mgdSw-BoxExx7F2Q1me4SAqXDGpfoc07NDDBCV74"



        STATE:

        cloudflare_api_token.uikfrstsfs:
          ID = b20c3f79a90b9bc33a0cb001ce0606ac
          provider = provider.cloudflare
          condition.# = 1
          condition.0.request_ip.# = 1
          name = uikfrstsfs
          policy.# = 1
          policy.0.effect = deny
          policy.0.permission_groups.# = 1
          policy.0.permission_groups.0 = 82e64a83756745bbbb1c9c2701bf816b
          policy.0.resources.com.cloudflare.api.account.zone.0da42c8d2132a9ddaf714f9e7c920711 = *
          status = active
          value = mgdSw-BoxExx7F2Q1me4SAqXDGpfoc07NDDBCV74
--- FAIL: TestAccAPIToken (9.48s)
=== RUN   TestAccAPITokenAllowDeny
    testing.go:705: Step 0 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        UPDATE: cloudflare_api_token.ywpzjtdhmx
          condition.#:                                                                         "1" => "0"
          condition.0.request_ip.#:                                                            "1" => ""
          condition.0.request_ip.0.in.#:                                                       "0" => ""
          condition.0.request_ip.0.not_in.#:                                                   "0" => ""
          id:                                                                                  "6aa4d5d28e95f8fd56f8bd27ccdb70e1" => "6aa4d5d28e95f8fd56f8bd27ccdb70e1"
          name:                                                                                "ywpzjtdhmx" => "ywpzjtdhmx"
          policy.#:                                                                            "1" => "1"
          policy.0.effect:                                                                     "allow" => "allow"
          policy.0.permission_groups.#:                                                        "1" => "1"
          policy.0.permission_groups.0:                                                        "82e64a83756745bbbb1c9c2701bf816b" => "82e64a83756745bbbb1c9c2701bf816b"
          policy.0.resources.com.cloudflare.api.account.zone.0da42c8d2132a9ddaf714f9e7c920711: "*" => "*"
          status:                                                                              "active" => "active"
          value:                                                                               "bysG-69wNNXXOagUicOqRlXONjKKmNGYjP9dGDvB" => "bysG-69wNNXXOagUicOqRlXONjKKmNGYjP9dGDvB"



        STATE:

        cloudflare_api_token.ywpzjtdhmx:
          ID = 6aa4d5d28e95f8fd56f8bd27ccdb70e1
          provider = provider.cloudflare
          condition.# = 1
          condition.0.request_ip.# = 1
          name = ywpzjtdhmx
          policy.# = 1
          policy.0.effect = allow
          policy.0.permission_groups.# = 1
          policy.0.permission_groups.0 = 82e64a83756745bbbb1c9c2701bf816b
          policy.0.resources.com.cloudflare.api.account.zone.0da42c8d2132a9ddaf714f9e7c920711 = *
          status = active
          value = bysG-69wNNXXOagUicOqRlXONjKKmNGYjP9dGDvB
--- FAIL: TestAccAPITokenAllowDeny (4.17s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	13.690s
FAIL

@UrosSimovic
Copy link
Contributor Author

Had to make some if checks. And to change policy blocks from list to hash set, as CF is returning policies unordered.

@jacobbednarz
Copy link
Member

nice one! looks like we're all green on the integration test suite.

=== RUN   TestAccAPIToken
--- PASS: TestAccAPIToken (12.25s)
=== RUN   TestAccAPITokenAllowDeny
--- PASS: TestAccAPITokenAllowDeny (20.38s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	32.661s

@jacobbednarz
Copy link
Member

Datasource is also green now after fixing the ID

=== RUN   TestAccCloudflareApiTokenPermissionGroups
--- PASS: TestAccCloudflareApiTokenPermissionGroups (2.41s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	2.433s

@jacobbednarz jacobbednarz changed the title add reasource 'api_token' and data source 'permission_groups' add resource 'api_token' and data source 'permission_groups' Nov 23, 2020
@jacobbednarz jacobbednarz merged commit 7f1466b into cloudflare:master Nov 23, 2020
@jacobbednarz
Copy link
Member

Thanks for the persistence on this one @UrosSimovic! Appreciate your contribution here 👍

jacobbednarz added a commit to jacobbednarz/terraform-provider-cloudflare that referenced this pull request Jan 3, 2021
@jacobbednarz
resource/cloudfare_api_token: allow empty conditions
f03b14f 
In the initial PR for adding API token support to Terraform (cloudflare#862), an
assumption was made that empty `condition` blocks would allow all
traffic; this is incorrect and puts the end user in a state where no
traffic is allowed.

To address this, we need to conditionally build the API token based on
what fields are non-zero values. This requires a bit of a rewrite as
this assumption ran deep into the methods and the structure of the code
itself.

While this increases the test coverage, we do now have a restriction in
the API itself whereby if you create an API token with IP restrictions
and then wish to remove them, you cannot -- you must recreate the token.
This issue is outside the scope of this work so I'll leave it as a known
factor for now.

Closes cloudflare#897
@jacobbednarz jacobbednarz mentioned this pull request Jan 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobbednarz jacobbednarz jacobbednarz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@UrosSimovic @jacobbednarz @reifnir