Only send tenant setting flags if values changed

[emilhdiaz]

Proposed Changes

This modification makes it such that tenant setting flags will only be included in the PATCH /api/v2/tenants/settings call if they have changed. This proposed change addresses use cases for Auth0 private cloud and enterprise customers whose tenants do not allow updates of certain tenant settings flags. Even the mere presence of these flags in the API call, irrespective of the values changing or not, will cause the API to return an error response. With these proposed changes, the tenant setting flags will simply be omitted from the API call if unchanged, thus bypassing the issue altogether.

NOTE: For other Auth0 public cloud customers, the previous behavior will continue to function as it previously did. In other words, this change is backwards compatible.

Fixes #160

Acceptance Test Output

The following acceptance test was conducted against an Auth0 public cloud enterprise tenant. These specific code changes have been verified against an Auth0 private cloud enterprise tenant.

$ make testacc TESTS=TestAccXXX
==> Checking that code complies with gofmt requirements...
?   	github.com/alexkappa/terraform-provider-auth0	[no test files]
=== RUN   TestAccClientGrant
--- PASS: TestAccClientGrant (24.28s)
=== RUN   TestAccClient
--- PASS: TestAccClient (1.36s)
=== RUN   TestAccClientZeroValueCheck
--- PASS: TestAccClientZeroValueCheck (9.35s)
=== RUN   TestAccClientRotateSecret
--- PASS: TestAccClientRotateSecret (8.39s)
=== RUN   TestAccClientInitiateLoginUri
--- PASS: TestAccClientInitiateLoginUri (0.03s)
=== RUN   TestAccClientJwtScopes
--- PASS: TestAccClientJwtScopes (2.16s)
=== RUN   TestAccClientMobile
--- PASS: TestAccClientMobile (8.43s)
=== RUN   TestAccClientMobileValidationError
--- PASS: TestAccClientMobileValidationError (0.02s)
PASS
coverage: 24.5% of statements
ok  	github.com/alexkappa/terraform-provider-auth0/auth0	54.363s	coverage: 24.5% of statements
?   	github.com/alexkappa/terraform-provider-auth0/auth0/internal/debug	[no test files]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/alexkappa/terraform-provider-auth0/auth0/internal/random	0.249s	coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/alexkappa/terraform-provider-auth0/auth0/internal/validation	0.107s	coverage: 0.0% of statements [no tests to run]
?   	github.com/alexkappa/terraform-provider-auth0/version	[no test files]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

[yvovandoorn]

This is a really good change @emilhdiaz. There are often Auth0 customers who could have unique tenant flags enabled, whether it's evaluating new features, specific protection capabilities are enabled, etc.

@alexkappa could you review this one as it is a behavioral change (even if it is backwards compatible) that changes the way the provider applies changes. This change opens up the fact that the state over a tenant is co-managed server/Auth0 side.

[alexkappa]

@emilhdiaz @yvovandoorn this issue has been troubling us for a really long time. Originally the provider would only send fields if they have changed, which was causing several issues (#185, #197). The fix came with PR #194. Issue #185 is particularly interesting as it has all the backstory, as well as issues that will resurface if this PR gets merged.

Perhaps if we are more selective in using the IsNewResource() and HasChange() conditions we can maintain the fixes that #194 brought, and solving the issue you are facing as well.

Which flags in particular, when set, give you errors?

[yvovandoorn]

Which flags in particular, when set, give you errors?

I can't speak to @emilhdiaz specific issues but as someone at Auth0, the tenant flags portion of a tenant is one of the areas of configuration that could differ configuration-wise versus the state held by Terraform.

• New features often get released behind feature flags that are stored as values in the tenant flags (for example new dashboard experience)
• Deprecated features often get "turned off" behind flags in the tenant flags (for example impersonation)
• Support has the ability to enable/disable certain functionality hidden behind flags based on the usage patterns in a tenant.

The flags aren't specific as they are ever-changing as the product evolves. Some are well defined. Some are flags only to be used in certain situations.

[emilhdiaz]

Thanks for the response @alexkappa! I'll look through the history of those issues shortly. These are the are specific config options I noticed that gave us trouble in our private cloud instance:

• disable_clickjack_protection_headers
• trust_azure_adfs_email_verified_connection_property
• enable_sso

[emilhdiaz]

@alexkappa a clarification on this comment you made earlier: #185 (comment)

Is this indicating that because the Auth0 API call is a PATCH only those fields provided will be updated (which I believe is the correct behavior in REST)? Do we still have to worry about Auth0 reverting other fields, that were not provided, to some default?

Maybe a hybrid of the two approaches could work here. Send ALL fields that have been explicitly defined by the developer on the resource, irrespective of whether they changed, ignore the rest. That would make the API call match the resource in a very declarative way, while still allowing the developer to omit fields that are not supported in their environment. Thoughts?

[alexkappa]

Hi @emilhdiaz, what you are suggesting makes sense, but the mechanism that determines whether a value has explicitly been defined is from the terraform SDKs ResourceData.GetOk / ResourceData.GetOkExists methods. The latter is a special case for booleans. By the SDK documentation, this should already be the expected behavior but I'm afraid it doesn't work that way in practice for reasons I can't quite understand.

In #241 I've made another attempt in this direction, but as you can see I kept boolean fields as it was breaking in unexpected ways.

If you are feeling adventurous, try to see if Bool can be switched to using GetOk instead of GetOkExists.

If that doesn't get us where we want to be, we could perhaps consider sending fields that are safe to send, and all others conditionally (i.e. IsNewResource, HasChange)?

EDIT: I did a quick test and GetOk when used for booleans won't work when changing to a zero value (i.e. false) 😞

[emilhdiaz]

@alexkappa I've been spending some time tinkering with a couple options as well, and I think I'm starting to gain a better appreciation for the complexity of the problem 😄 !

It appears, that irrespective of what flags are present when state is read from the API and flattened into the ResourceData object via the flattenTenantFlags() method, terraform will use the schema defined in resource_auth0_tenant.go to construct the full ResourceData. So flags that are not returned by the API will simply be defaulted to false when terraform constructs the local state. After those values are in the local state they are passed through to the create and update methods without any distinction from the other flags that were indeed returned by the API.

Like you described, it also appears there is no way to know whether a value has explicitly been defined by the user.... at least, not that I know of.

So then that leaves us with your last suggestion of always sending all safe fields and make the problematic ones conditional.

Another option, though it's a much bigger change, is to expose a new provider configuration option that allows a user to define "blacklisted" fields from the schema. We would then have to make schema definitions dynamic OR check the blacklist before constructing Auth0 update calls. Thoughts?

EDIT: I made the entire flags map conditional on HasChange and IsNewResource, while also keeping the individual bool flags conditional as well. This prevents a regression of the issue from #185. The BIG assumption here is that the Auth0's tenant API PATCH operation is only updating the flags explicitly being passed. From my testing, it appears that is indeed the case. Pushed up a new commit with the additional change.

[emilhdiaz]

Hey @alexkappa, any luck on your end? What do you think of the recent changes I made here?

[alexkappa]

I'm not sure its a good idea to have flags sent conditionally. HasChange could be true if we remove its last element.

For example, this might pass the condition and send an empty object, causing the issue reported in #185.

-{"foo":true}
+{}

What if certain keys, we can always send while most others we send on condition (HasChange, IsNewResource)? Problem is, I don't know which are safe to always be sent across public, private cloud or enterprise.

[willvedd]

@emilhdiaz Thanks for submitting this, this seems like a great first step in addressing this pain point. However, for the reasons that @yvovandoorn and @alexkappa listed above, I'm realizing that the solution is non-trivial. Our team is still familiarizing ourselves with the project and will need a bit more time to understand the issue and strategize an appropriate solution. One initial thought I'm having is if it is more appropriate to address this from the API-side, either by adhering to more RESTful conventions or something else, rather than this project assuming the responsibility entirely. But again, our team just needs to reevaluate.

This project is currently in the process of being migrated to the Auth0 organization and we're unable to migrate PRs over to the new repo. Combined with this tricky issue, we'll need to close this in favor of #513 where the conversation will continue.

[emilhdiaz]

@willvedd Understood! Thanks for the update and the consideration.