Handle encryption field for S3 destinations for Logs Archives #2740
Conversation
| @@ -44,6 +44,8 @@ func resourceDatadogLogsArchive() *schema.Resource { | |||
| ValidateDiagFunc: validators.ValidateAWSAccountID, | |||
| }, | |||
| "role_name": {Description: "Your AWS role name", Type: schema.TypeString, Required: true}, | |||
| "encryption_type": {Description: "The type of encryption on your archive.", Type: schema.TypeString, Required: true}, | |||
There was a problem hiding this comment.
If this new field is required wouldn't users need to add them to their terraform configuration? Let's say a user updates the terraform provider version, wouldn't it break their setup until they add it?
If yes then what can/should we do about it? 🤔
There was a problem hiding this comment.
I switched the encryption type to be optional instead of required, it should now be implemented in the code
There was a problem hiding this comment.
@michelledeng30 Nice, although I wasn't necessarily saying to change it to be optional. I was wondering if we had a good reason for it to be either required or optional and if we were aware of the consequences of choosing either option? These weren't rhetorical questions in any way
There was a problem hiding this comment.
I think it makes sense for the field to be optional.
If encryption type is required, then every time users interact with their s3 archives via Terraform, they must deliberately set encryption_type in their Terraform configuration. Most users don't need this added functionality of encrypting their data at an object level, since they use default bucket encryption set in s3. I don't think it's necessary for it to be required, and I could see how making it required would be misleading/confusing to users who aren't familiar with/don't need the ability to set encryption in their PUT requests rather than the bucket.
If the field is optional, we can allow users to only set this field if they need the extra level of encryption, otherwise it's automatically set to NO_OVERRIDE for most users.
Also, comparing the Terraform resource and the logs archives API reference, it looks like the same fields are required or optional. In the public API, we support creating/updating archives without any encryption type, so it makes sense to do the same in Terraform.
if encryption-type is made to be required wouldn't it be set to NO_OVERRIDE by default if users don't explicitly set a value for it in their configuration
I believe if a required parameter is not set in a Terraform configuration, it will fail terraform validate and the user will be forced to define it to pass the check
There was a problem hiding this comment.
Thanks a lot for the explanations, I understand better now and I agree this makes sense like this 🙇
| encryptionKey, ok := d["encryptionKey"] | ||
| var LogsArchiveEncryptionS3 encryption | ||
|
|
||
| if !ok { |
There was a problem hiding this comment.
I assume from reading the code that if the user sets SSE_KMS as the encryption type but doesn't provide an encryption key we do not want to handle this error here and just pass the values along is that correct?
There was a problem hiding this comment.
Yes that's right, technically setting SSE_KMS with no encryption key is also a valid method (just not done very commonly)
There was a problem hiding this comment.
I see, I didn't know that. Thanks for explaining!
docs/resources/logs_archive.md
Outdated
| @@ -87,6 +88,8 @@ Required: | |||
| Optional: | |||
|
|
|||
| - `path` (String) Path where the archive is stored. | |||
| - `encryption_type` (String) The type of server-side encryption to use when uploading data to your s3 bucket. `NO_OVERRIDE`, `SSE_S3`, and `SSE_KMS` are the possible types. `NO_OVERRIDE` is used most commonly to leave data unencrypted or leave encryption to the default encryption set on s3 bucket settings. | |||
There was a problem hiding this comment.
Is this NO_OVERRIDE value set by default for users who don't define the encryption_type explicitly to either of the two other options?
Also is that really true "most commonly to leave data unencrypted" ? I thought AWS encrypted all s3 buckets by default?
There was a problem hiding this comment.
Yes the NO_OVERRIDE value is the default type if users don't choose another type.
It looks like SSE_S3 is the default encryption for S3 buckets, so I will revise this statement to make this clear
There was a problem hiding this comment.
@michelledeng30 Linking this message my other comment i posted today, if NO_OVERRIDE is set by default for the encryption-type field, does it make sense for for the encryption-type field to be optional? I really don't know much about the terraform provider code so I'm only talking hypothetically but if encryption-type is made to be required wouldn't it be set to NO_OVERRIDE by default if users don't explicitly set a value for it in their configuration?
I'm trying to understand which way of doing it makes the most sense but if we really don't know maybe we can ask someone who is more knowledgeable about this.
There was a problem hiding this comment.
Response is in this comment ! - #2740 (comment) @EdwinSri
We want to handle encryption configuration for S3 destinations in logs archives in Terraform. We are supporting the ability for users to control their encryption settings for their S3 archives through Logs Archives public API.
These changes allow TF users to control how their data is encrypted when they upload to their S3 buckets. The possible encryption types are NO_OVERRIDE (either no encryption or using the encryption set on their own S3 bucket), SSE_S3, and SSE_KMS. There is an optional encryption key field to specify an Amazon Resource Name, which identifies an encryption key only for the SSE_KMS type.
The encryption settings, which consist of just the encryption type and key, are not required, so no breaking changes are introduced.
This PR is dependent on this datadog_api_spec PR: https://github.com/DataDog/datadog-api-spec/pull/3311 which is not yet merged and is currently causing the CI checks to fail
Jira