[resource_datadog_domain_allowlist] Domain allowlist provider support #2637
Conversation
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
diab42
changed the title
jack-edmonds-dd
changed the title
There was a problem hiding this comment.
I'm curious whether there's been any product interest in eventually having separate domain allowlists for different email types, or if we plan to have one overarching one? If we might have many different Domain Allowlists, could this provider be easily backwards-compatible? We don't need a full solution now -- just want to get a sense for how much effort it might be to introduce a Allowlist Split in the future.
Perhaps if we still had a "Default" allowlist, and then optional "Override" allowlists for different email types, this would be easy to support.
| @@ -356,6 +357,14 @@ func (i *ApiInstances) GetCSMThreatsApiV2() *datadogV2.CSMThreatsApi { | |||
| return i.csmThreatsApiV2 | |||
| } | |||
|
|
|||
| // GetDoomainAllowlittApiV2 get instance of DowntimesApi | |||
There was a problem hiding this comment.
| // GetDoomainAllowlittApiV2 get instance of DowntimesApi | |
| // GetDomainAllowlistApiV2 get instance of DowntimesApi |
There was a problem hiding this comment.
Corrected and found a few other typos.
| @@ -0,0 +1,4 @@ | |||
| resource "datadog_ip_allowlist" "example" { | |||
| enabled = true | |||
There was a problem hiding this comment.
How would this work if a user tried to set
| enabled = true | |
| enabled = False |
In the API this would be an invalid input -- does Terraform give similar feedback?
There was a problem hiding this comment.
So that should be able to be replicated within the tests. I updated to set that in line 72 in the test file
The error message from the command is
=== RUN TestAccDatadogDomainAllowlist_CreateUpdate
resource_datadog_domain_allowlist_test.go:21: Step 1/2 error: Error running pre-apply refresh: exit status 1
Error: Invalid reference
on terraform_plugin_test.tf line 3, in resource "datadog_domain_allowlist" "foo":
3: enabled = False
A reference to a resource type must be followed by at least one attribute
access, specifying the resource name.
I'm not sure if that's the general command error we return when the input is invalid in the provider.
If you want to confirm the command to run it against backend instead of the saved cassette file is TESTARGS="-run TestAccDatadogDomainAllowlist_CreateUpdate" make cassettes
I have seen some product interest from the beta customers to manage different email types in the domain allowlist from each other. I'm not sure there is any specific customers who have expressed interest in multiple different domain allowlists but it is possible. I'm checking with the PM whether that has ever been asked for by the beta customers. What you're suggesting should be easier to make it backwards compatible and matches the overall requests we have so far. If we had multiple lists to manage, it would be much harder to make this backwards compatible. We could think about making this internally a list to start but that might be overkill if we don't know whether that will ever be wanted. |
diab42
changed the title
diab42
changed the title
| Description: "The domains within the domain allowlist.", | ||
| ElementType: types.StringType, | ||
| Required: true, | ||
| Computed: false, |
There was a problem hiding this comment.
Out-of-curiosity, what does this param computed mean?
There was a problem hiding this comment.
From the hashicorp terraform-provider-framework comment
// Computed indicates whether the provider may return its own value for
// this Attribute or not. Required and Computed cannot both be true. If
// Required and Optional are both false, Computed must be true, and the
// attribute will be considered "read only" for the practitioner, with
// only the provider able to set its value.
| remote_addr: "" | ||
| request_uri: "" | ||
| body: | | ||
| {"data":{"attributes":{"domains":["@test.com","@datadoghq.com"],"enabled":true},"type":"domain_allowlist"}} |
There was a problem hiding this comment.
nit: domains and enabled aren't consistently in the same order in this file, not sure if it's autogenerated?
There was a problem hiding this comment.
Yeah this is autogenerated from the response from the server. Should be fine that they're out of order.
| } | ||
| resp, httpResp, err := r.Api.PatchDomainAllowlist(r.Auth, *domainAllowlistReq) | ||
| if err != nil { | ||
| response.Diagnostics.Append(utils.FrameworkErrorDiag(utils.TranslateClientError(err, httpResp, " error updating domain allowlist"), "")) |
There was a problem hiding this comment.
nit:
| response.Diagnostics.Append(utils.FrameworkErrorDiag(utils.TranslateClientError(err, httpResp, " error updating domain allowlist"), "")) | |
| response.Diagnostics.Append(utils.FrameworkErrorDiag(utils.TranslateClientError(err, httpResp, "error updating domain allowlist"), "")) |
There was a problem hiding this comment.
updated this and the other comment in a new commit
| domainAllowlistReq, _ := buildDomainAllowlistUpdateRequest(state) | ||
| resp, httpResp, err := r.Api.PatchDomainAllowlist(r.Auth, *domainAllowlistReq) | ||
| if err != nil { | ||
| response.Diagnostics.Append(utils.FrameworkErrorDiag(utils.TranslateClientError(err, httpResp, ""), "error updating email domain allowlist")) |
There was a problem hiding this comment.
| response.Diagnostics.Append(utils.FrameworkErrorDiag(utils.TranslateClientError(err, httpResp, ""), "error updating email domain allowlist")) | |
| response.Diagnostics.Append(utils.FrameworkErrorDiag(utils.TranslateClientError(err, httpResp, ""), "error creating email domain allowlist")) |
In this context this might make more sense? But I don't feel strongly about this change, whatever makes more sense for user experience when config.
For the general release of the domain allowlist feature we need to support enabling the domain allowlist via terraform.
When enabled, the domain allowlist blocks emails of certain types from being sent to email domains outside the set list.