duckdb / duckdb_aws

MIT License
34 stars 12 forks source link

Add support for secrets #23

Closed samansmink closed 6 months ago

samansmink commented 6 months ago

This PR adds support for a new concept in duckdb: secrets (https://github.com/duckdb/duckdb/pull/10042)

To summarize, secrets are, in the most generic sense, scoped sets of configuration that provide the necessary information to do something. In most cases this "something" will be an authorized request to some API.

For the AWS extension this means the what was previously configured through duckdb settings, will now be done through secrets. Check out the aforementioned PR in duckdb on what benefits this brings. This means that the load_aws_credentials() function call is now implemented as a Secret Provider called credential_chain.

credential_chain secret provider overview

The credential_chain provider uses the AWS SDK to create an S3 secret. This is done in exactly the same way as the load_aws_credentials function now does, except instead of setting the duckdb_settings, it creates a secret.

The easiest way to use it is:

CREATE SECRET (
    TYPE S3,
    PROVIDER credential_chain
)

This will create an S3 secret that basically does everything automatically, just like load_aws_credentials.

Note that all of the fields that you can normally set for s3 secrets are also implemented, including the ones set automatically. For example to automatically load the config, override the loaded region, and disable ssl:

CREATE SECRET (
    TYPE S3,
    PROVIDER credential_chain,
    REGION 'eu-west-1',
    USE_SSL false
)

Small feature addition

While implementing, I added a small feature similar to what already existed in the azure extension: the ability to specify the credential chain used. This means that you can change (the order in) which different places are searched for credentials by the aws sdk. For example, to search for credentials firstly in the config, and then the env:

CREATE SECRET (
    TYPE S3,
    PROVIDER credential_chain,
    CHAIN 'config;env'
)

This feature fixes https://github.com/duckdb/duckdb_aws/issues/16

Also this should fix issue https://github.com/duckdb/duckdb_aws/issues/14 and https://github.com/duckdb/duckdb_aws/issues/10 by providing a way to pass the profile to the sso chain:

CREATE SECRET (
    TYPE S3,
    PROVIDER credential_chain,
    CHAIN 'sso',
    PROFILE 'my-profile'
)

Finally, the credential chain provider can take various arguments to configure the several other aws sdk providers, however, these are currently not yet tested properly. (see test/sql/aws_secret_chains.test for more info)

carlopi commented 6 months ago

Idea: would it make sense have load_aws_credentials also invoke CREATE SECRET ( TYPE S3, PROVIDER credential_chain ) behind the scenes?

So the idea would be that users that want default behaviours can keep using load_aws_credentials and that will keep working also post deprecations of the s3_* settings.

And users that will want more control can move to use explicit CREATE SECRET (or potentially yet to be introduced overloads for load_aws_credentials).

Obv, this can also added later after landing this PR.

samansmink commented 6 months ago

would it make sense have load_aws_credentials also invoke CREATE SECRET ( TYPE S3, PROVIDER credential_chain ) behind the scenes

Yea good point, I've considered this, but i think there's not really a point. If we're already deprecating the s3_* variables, why keep this legacy function around while it doesn't really add any functionality. I would say we deprecate it along with the s3_* settings to keep things uniform.

Mytherin commented 6 months ago

Thanks! LGTM.

A related question - previously this extension would be auto-loaded when CALL load_aws_credentials() was called. Is the extension now autoloaded when a credential_chain secret is created for S3? If not, perhaps it makes sense to add that behavior as well?

samansmink commented 6 months ago

@Mytherin Yea it is! We can autoload on secret load and creation https://github.com/duckdb/duckdb/blob/1e216074028c0aaef838ae2075e2e30705aa48e7/src/main/secret/secret_manager.cpp#L196

Mytherin commented 6 months ago

Cool, very nice!