Add SaaS Application support #1762

ouranos · 2022-07-08T08:56:52Z

Added in cloudflare-go: cloudflare/cloudflare-go#900

I've tested in our environment which had a custom fork of the SDK previously and the plan is not showing any difference when using the new SDK. Opening a draft PR to get some feedback and I'll do more testing in the meantime.

I need a bit of help handling the domain attribute. From my research (before the doc was available), it seems that it is not required for SaaS applications and will be computed for them (eg: https://<organisation>.cloudflareaccess.com/cdn-cgi/access/sso/saml/<uuid>.
The updated documentation seems to confirm it: https://api.cloudflare.com/#access-applications-properties

github-actions · 2022-07-08T08:58:04Z

changelog detected ✅

ouranos · 2022-07-11T01:11:23Z

@jacobbednarz here's a first draft. I'll do a bit more testing on our workspace.

not sure how I'm supposed to handle the domain attribute. It seems to be working fine though.
it's not handling the custom_attributes yet as it seems to have been added after I did my original implementation.

jacobbednarz · 2022-07-20T22:00:06Z

@Justin-Holmes can i get your eyes on this too please?

jacobbednarz · 2022-07-20T22:01:51Z

internal/provider/resource_cloudflare_access_application.go

+		saasApp := convertSaasSchemaToStruct(d)
+		newAccessApplication.SaasApplication = saasApp


given convertSaasSchemaToStruct doesn't return an error, why not just assign this directly?

Suggested change

saasApp := convertSaasSchemaToStruct(d)

newAccessApplication.SaasApplication = saasApp

newAccessApplication.SaasApplication = convertSaasSchemaToStruct(d)

internal/provider/resource_cloudflare_access_application_test.go

internal/provider/schema_cloudflare_access_application.go

Justin-Holmes · 2022-07-21T16:00:19Z

Looks great! And you're correct @ouranos, the domain is not required for saas applications and is generated for you on creation.

Also, the custom_attributes field is not used often so I'm cool omitting that for now.

ouranos · 2022-07-21T18:38:20Z

Thanks,

the domain is not required for saas applications and is generated for you on creation.

So this is the correct way then?

"domain": {
    Type:     schema.TypeString,
-     Required: true,
+     Optional: true,
+     Computed: true, 
    ...
}

Let me know if you are happy with it or if it needs more change and I'll rebase/clean up/squash.

jacobbednarz · 2022-07-22T02:58:08Z

So this is the correct way then?

Computed: true is fine. Optional implies the end user is also able to set it.

internal/provider/schema_cloudflare_access_application.go

jacobbednarz · 2022-07-22T03:05:42Z

internal/provider/schema_cloudflare_access_application.go

+			Type:        schema.TypeList,
+			Optional:    true,
+			Description: "SaaS configuration for the Access Application. See below for reference structure.",


Suggested change

Type: schema.TypeList,

Optional: true,

Description: "SaaS configuration for the Access Application. See below for reference structure.",

Type: schema.TypeList,

Optional: true,

MaxItems: 1,

Description: "SaaS configuration for the Access Application.",

jacobbednarz · 2022-07-22T03:16:32Z

don't forget to run make docs to get the documentation updated here. you'll need to fix the tests as well now that the field is computed.

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareAccessApplication_" -count 1 -parallel 1 -timeout 120m -parallel 1
?       github.com/cloudflare/terraform-provider-cloudflare     [no test files]
=== RUN   TestAccCloudflareAccessApplication_BasicZone
    resource_cloudflare_access_application_test.go:25: Step 1/1 error: Error running pre-apply refresh: exit status 1

        Error: Value for unconfigurable attribute

          with cloudflare_access_application.npamchgzmo,
          on terraform_plugin_test.tf line 5, in resource "cloudflare_access_application" "npamchgzmo":
           5:   domain                    = "npamchgzmo.terraform.cfapi.net"

        Can't configure a value for "domain": its value will be decided automatically
        based on the result of applying this configuration.
--- FAIL: TestAccCloudflareAccessApplication_BasicZone (2.27s)
=== RUN   TestAccCloudflareAccessApplication_BasicAccount
    resource_cloudflare_access_application_test.go:54: Step 1/1 error: Error running pre-apply refresh: exit status 1

        Error: Value for unconfigurable attribute

          with cloudflare_access_application.ebaoediygg,
          on terraform_plugin_test.tf line 5, in resource "cloudflare_access_application" "ebaoediygg":
           5:   domain                    = "ebaoediygg.terraform.cfapi.net"

        Can't configure a value for "domain": its value will be decided automatically
        based on the result of applying this configuration.
--- FAIL: TestAccCloudflareAccessApplication_BasicAccount (2.00s)
=== RUN   TestAccCloudflareAccessApplication_WithCORS
    resource_cloudflare_access_application_test.go:83: Step 1/1 error: Error running pre-apply refresh: exit status 1

        Error: Value for unconfigurable attribute

          with cloudflare_access_application.rlcwyzkcoe,
          on terraform_plugin_test.tf line 5, in resource "cloudflare_access_application" "rlcwyzkcoe":
           5:   domain           = "rlcwyzkcoe.terraform.cfapi.net"

        Can't configure a value for "domain": its value will be decided automatically
        based on the result of applying this configuration.
--- FAIL: TestAccCloudflareAccessApplication_WithCORS (1.85s)

internal/provider/resource_cloudflare_access_application_test.go

ouranos · 2022-07-22T07:08:38Z

So this is the correct way then?

Computed: true is fine. Optional implies the end user is also able to set it.

It's computed for SaaS apps (and returns a Cloudflare generated domain) but it's required for the other types (eg: self-hosted)

ouranos · 2022-07-22T09:07:00Z

don't forget to run make docs to get the documentation updated here. you'll need to fix the tests as well now that the field is computed.

Squashed all previous commits together
Addressed the new review comments
Generated the docs

So I think the only remaining points are:

domain: it's required for self_hosted but computed for saas
acceptance tests: I only ran make test so far and I'm now trying to set up an environment to run make testacc. We're only using Zero-Trust with Saas apps so we don't have any domains/zones set up. Looks like I can only add root domains on the free plan (we do have a paid plan for ZeroTrust). I'm going to try to set up one of our unused test domains.

internal/provider/schema_cloudflare_access_application.go

internal/provider/resource_cloudflare_access_application_test.go

ouranos · 2022-07-25T08:44:38Z

if you fix these, the acceptance tests all pass

Great thanks 🙏

I've:

applied all suggested changes
cleaned up the comments regarding domain
rebased from latest master & squashed

Let me know if I need to do anything else

jacobbednarz · 2022-07-25T09:02:38Z

did you re-run make docs? that should be the only thing.

ouranos · 2022-07-25T09:11:58Z

did you re-run make docs? that should be the only thing.

Nope 🤦‍♂️
Done now. It fixes doc for #1792 too

jacobbednarz · 2022-07-25T09:25:43Z

cool! I'll run the acceptance tests again in the morning and we should be good, thanks!

p.s for future PRs if you check the box to allow maintainers to make changes, we can limit the back and forth as I can provide a patch within the branch for you.

github-actions · 2022-07-26T23:45:39Z

This functionality has been released in v3.20.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

ouranos force-pushed the allow-saas-for-access-application branch from e9e7a47 to c00507c Compare July 8, 2022 08:57

ouranos force-pushed the allow-saas-for-access-application branch 2 times, most recently from 76b30a7 to 282085e Compare July 11, 2022 00:17

ouranos force-pushed the allow-saas-for-access-application branch from 282085e to 6129ebf Compare July 18, 2022 07:31

ouranos marked this pull request as ready for review July 18, 2022 08:08

ouranos requested a review from jacobbednarz as a code owner July 18, 2022 08:08

jacobbednarz reviewed Jul 20, 2022

View reviewed changes

internal/provider/resource_cloudflare_access_application_test.go Show resolved Hide resolved

jacobbednarz reviewed Jul 20, 2022

View reviewed changes

internal/provider/schema_cloudflare_access_application.go Show resolved Hide resolved

jacobbednarz reviewed Jul 22, 2022

View reviewed changes

internal/provider/schema_cloudflare_access_application.go Outdated Show resolved Hide resolved

jacobbednarz reviewed Jul 22, 2022

View reviewed changes

internal/provider/resource_cloudflare_access_application_test.go Show resolved Hide resolved

ouranos force-pushed the allow-saas-for-access-application branch from 2ec2c02 to a609a34 Compare July 22, 2022 07:24

jacobbednarz reviewed Jul 25, 2022

View reviewed changes

internal/provider/schema_cloudflare_access_application.go Outdated Show resolved Hide resolved

jacobbednarz reviewed Jul 25, 2022

View reviewed changes

internal/provider/schema_cloudflare_access_application.go Outdated Show resolved Hide resolved

jacobbednarz reviewed Jul 25, 2022

View reviewed changes

internal/provider/resource_cloudflare_access_application_test.go Outdated Show resolved Hide resolved

jacobbednarz reviewed Jul 25, 2022

View reviewed changes

internal/provider/resource_cloudflare_access_application_test.go Outdated Show resolved Hide resolved

ouranos force-pushed the allow-saas-for-access-application branch 2 times, most recently from 898f48f to 4b3f018 Compare July 25, 2022 08:43

ouranos added 3 commits July 25, 2022 11:11

Add sass app

fcc08a9

Add changelog entry

5c66957

📝 Fix doc for cloudflare#1792

db5d1d9

ouranos force-pushed the allow-saas-for-access-application branch from 4b3f018 to db5d1d9 Compare July 25, 2022 09:11

jacobbednarz merged commit 5831ca8 into cloudflare:master Jul 26, 2022

github-actions bot added this to the v3.20.0 milestone Jul 26, 2022

github-actions bot pushed a commit that referenced this pull request Jul 26, 2022

Update CHANGELOG.md for #1762

00ee985

ouranos deleted the allow-saas-for-access-application branch March 14, 2023 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SaaS Application support #1762

Add SaaS Application support #1762

ouranos commented Jul 8, 2022 •

edited by jacobbednarz

Loading

github-actions bot commented Jul 8, 2022 •

edited

Loading

ouranos commented Jul 11, 2022

jacobbednarz commented Jul 20, 2022

jacobbednarz Jul 20, 2022

ouranos Jul 21, 2022

Justin-Holmes commented Jul 21, 2022

ouranos commented Jul 21, 2022

jacobbednarz commented Jul 22, 2022

jacobbednarz Jul 22, 2022

jacobbednarz commented Jul 22, 2022 •

edited

Loading

ouranos commented Jul 22, 2022

ouranos commented Jul 22, 2022

ouranos commented Jul 25, 2022

jacobbednarz commented Jul 25, 2022

ouranos commented Jul 25, 2022

jacobbednarz commented Jul 25, 2022

github-actions bot commented Jul 26, 2022

		saasApp := convertSaasSchemaToStruct(d)
		newAccessApplication.SaasApplication = saasApp

	saasApp := convertSaasSchemaToStruct(d)
	newAccessApplication.SaasApplication = saasApp
	newAccessApplication.SaasApplication = convertSaasSchemaToStruct(d)

Add SaaS Application support #1762

Add SaaS Application support #1762

Conversation

ouranos commented Jul 8, 2022 • edited by jacobbednarz Loading

github-actions bot commented Jul 8, 2022 • edited Loading

ouranos commented Jul 11, 2022

jacobbednarz commented Jul 20, 2022

jacobbednarz Jul 20, 2022

Choose a reason for hiding this comment

ouranos Jul 21, 2022

Choose a reason for hiding this comment

Justin-Holmes commented Jul 21, 2022

ouranos commented Jul 21, 2022

jacobbednarz commented Jul 22, 2022

jacobbednarz Jul 22, 2022

Choose a reason for hiding this comment

jacobbednarz commented Jul 22, 2022 • edited Loading

ouranos commented Jul 22, 2022

ouranos commented Jul 22, 2022

ouranos commented Jul 25, 2022

jacobbednarz commented Jul 25, 2022

ouranos commented Jul 25, 2022

jacobbednarz commented Jul 25, 2022

github-actions bot commented Jul 26, 2022

ouranos commented Jul 8, 2022 •

edited by jacobbednarz

Loading

github-actions bot commented Jul 8, 2022 •

edited

Loading

jacobbednarz commented Jul 22, 2022 •

edited

Loading