Added S3 SSEKMSKeyId config option

[jackwellsxyz]

• [X ] ❗ I have followed the Contributing to DVC checklist.

• [(will do) ] 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

• [X ] ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

I added an sse_kms_key_id parameter to the S3 config section.

Ultimately, it may make sense to iterate through the S3 ALLOWED_UPLOAD_ARGS and add any that exist to extra_args, but you'd need to enable ALLOW_EXTRA in the S3 voluptuous schema. With the way the schema is set up, you'd end up allowing extra arguments in the entire configuration schema, which I'm not sure is what we want.

Here's an example of the S3 init sequence if you did ALLOW_EXTRA in the S3 schema - and you could git rid of the sse, acl, and _append_aws_grants_to_extra_args() code.

        self.extra_args = {}
        for key in self.ALLOWED_UPLOAD_ARGS:
            value = config.get(key)
            if value:
                self.extra_args[key] = value

[efiop]

Thanks @jackwellsxyz ! Could you also submit a doc PR for your change, please? https://dvc.org/doc/command-reference/remote/modify

Also, could you confirm that this works for you?

Obviously we can't really create a good test for this, but it would be great to at least add a unit test for this to tests/unit/remote/test_s3.py just to make sure that this config option is being read.

[jackwellsxyz]

Hi @efiop, thanks for taking a look! Where do you think would be the best place to document this new configuration option? I'm not seeing much of the existing S3 options in the documentation, including the sse parameter, for example. I thought perhaps to put it under dvc.org/content/docs/command-reference/config.md, but that doc seems more higher level, and there isn't an S3-specific page either. What do you think? Here's the command you can type to add the SSE-KMS key: dvc remote modify s3remote sse_kms_key_id <your key alias or id>.

Also, I've verified that yes dvc does actually work with the KMS key id and uploads to S3 using the key. I'm loving it!

[efiop]

@jackwellsxyz sse is here https://github.com/iterative/dvc.org/blob/master/content/docs/command-reference/remote/modify.md , it is just under the foldable "details" button. You could easily find it if you click raw or edit there.

[efiop]

@jackwellsxyz Btw, your idea about ALLOWED_UPLOAD_ARGS is great! I'm a bit worried about introducing config options that we don't have any user to verify that they work though, so I would not do it for now. Plus, we need to document them anyway, which is easier when we manually add them 🙂 And also we are able to give them names that are following the style of our other config options.

[jackwellsxyz]

Thanks @efiop I've just submitted a PR in dvc.org to update the documentation in remote modify. Also, makes sense to me on the ALLOWED_UPLOAD_ARGS - I can appreciate wanting to lock down the schema. Thanks for considering!

[efiop]

Thanks @jackwellsxyz ! 🎉