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

Update IdP identifiers to accept a list in access group schema #739

Merged

Conversation

Justin-Holmes
Copy link
Contributor

@Justin-Holmes Justin-Holmes commented Jul 16, 2020

This PR should clean up redundancy in our IdP policy rules and create an API that's consistent with our dashboard UI. Here is an example of the proposed change:

Current

include {
  gsuite {
    email                = "[email protected]"
    identity_provider_id = "xxxx-xxx-xxxx"
  }
  gsuite {
    email                = "[email protected]"
    identity_provider_id = "xxxx-xxx-xxxx"
  }
}

New

include {
  gsuite {
    email                = ["[email protected]", "[email protected]"]
    identity_provider_id = "xxxx-xxx-xxxx"
  }
}

Unfortunately, I wasn't able to run the acceptance tests but I was able to create resources in my local environment without any issues.

@ghost ghost added size/M kind/documentation Categorizes issue or PR as related to documentation. labels Jul 16, 2020
@jacobbednarz
Copy link
Member

The current test suite is all green however we should probably a new test case to excercise multiple values in https://github.com/terraform-providers/terraform-provider-cloudflare/blob/master/cloudflare/resource_cloudflare_access_group_test.go.

@Justin-Holmes
Copy link
Contributor Author

@jacobbednarz Sure thing. Do you know if those tests create/destroy actual resources? If so, I think these tests will require an IdP to be created beforehand (along with their respective IdP groups) and I'm not sure I know the best way to go about that.

@jacobbednarz
Copy link
Member

Yep, the integration tests do create/destroy real resources however I think we can get away with dummy values like we already do at https://github.com/terraform-providers/terraform-provider-cloudflare/blob/55c2d9946b52853ebb4decaa542842ed72544eb9/cloudflare/resource_cloudflare_access_identity_provider_test.go#L174-L185

Within the test, you can try creating the fake IdP and then attaching the groups to it. I know I had issues with it in the past but I think that was for actual reproduction test cases and not integration testing (as we're really only asserting the Terraform state). Let me know if you have issues though and we can either rely on a mocked values or I'll setup a real SSO provider for it.

@Justin-Holmes Justin-Holmes force-pushed the justin/remove-idp-redundancy branch 2 times, most recently from a439159 to 251614f Compare July 20, 2020 20:45
@Justin-Holmes
Copy link
Contributor Author

@jacobbednarz it looks like the acceptance tests are failing because of missing credentials. Let me know if there is anything I can do on my end to help!

@jacobbednarz
Copy link
Member

That's all good and thanks for the offer. Myself and @patryk (mainly @patryk though :p) moved the acceptance tests into GitHub Actions and forks don't get the credentials it needs as secrets aren't shared.

I'll run these locally shortly.

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.

I've pushed up a fix for the resource naming but it still looks like teams/team is missing from the GitHub IdP which we will need. I'm not sure if you're intending on using team or teams but my vote would be for the latter as IIRC, you can have multiple teams.

cloudflare/resource_cloudflare_access_group_test.go Outdated Show resolved Hide resolved
@Justin-Holmes
Copy link
Contributor Author

@jacobbednarz thank you, good catch!

Justin Holmes and others added 4 commits August 25, 2020 08:08
Prior to this change, we were passing through the full resource when it
only required the `rnd` value. In doing so, made the resource:

```
resource "cloudflare_access_group" "cloudflare_access_group.ohvdwhgf"
```

when we only needed

```
resource "cloudflare_access_group" "ohvdwhgf"
```
@Justin-Holmes Justin-Holmes force-pushed the justin/remove-idp-redundancy branch from 3dbf4f1 to f6a5b34 Compare August 25, 2020 13:11
Justin Holmes and others added 4 commits August 25, 2020 08:17
I rebased master and kept the vendor directory without realizing that
it’s no longer needed.
The value ends up being a UUID from another resource which makes
asserting specific values very hard. Instead, just check it's set.
@jacobbednarz
Copy link
Member

we're green! thanks!

@jacobbednarz jacobbednarz merged commit b65a91c into cloudflare:master Sep 1, 2020
@Justin-Holmes Justin-Holmes deleted the justin/remove-idp-redundancy branch September 1, 2020 21:13
@analytically
Copy link

This PR breaks Cloudflare <> Okta for us. 'Error: missing expected [' even though the expected string to string array conversion is there.

image

@jacobbednarz
Copy link
Member

@analytically are you able to provide a new issue with a reproduction case and I can take a look?

@groodt
Copy link

groodt commented Sep 28, 2020

I see the same issue on Okta.

@jacobbednarz
Copy link
Member

@groodt do you mind opening an issue with a reproduction test case and I'll dig into it?

@analytically
Copy link

Sorry don't have time.

boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
…terraformAppLauncherVisible

Add app_launcher_visible to Access Application
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