Skip to content

Use SecretsResolver from Airlift#22633

Merged
Praveen2112 merged 1 commit intotrinodb:masterfrom
Praveen2112:praveen/configuration_provider_2
Aug 8, 2024
Merged

Use SecretsResolver from Airlift#22633
Praveen2112 merged 1 commit intotrinodb:masterfrom
Praveen2112:praveen/configuration_provider_2

Conversation

@Praveen2112
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 commented Jul 9, 2024 •
edited
Loading

Description

This PR utilizes the framework for injecting custom secrets provider from Airlift across various modules like connectors, group providers, resource managers etc -

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Introduce pluggable secrets provider. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jul 9, 2024
@Praveen2112 Praveen2112 mentioned this pull request Jul 9, 2024
Merged
@Praveen2112 Praveen2112 marked this pull request as ready for review July 11, 2024 15:27
@Praveen2112 Praveen2112 marked this pull request as draft July 11, 2024 15:28
@Praveen2112 Praveen2112 force-pushed the praveen/configuration_provider_2 branch 5 times, most recently from bf916fa to 825b41c Compare August 1, 2024 22:47
@Praveen2112 Praveen2112 marked this pull request as ready for review August 1, 2024 22:48
@Praveen2112 Praveen2112 force-pushed the praveen/configuration_provider_2 branch from 825b41c to 168119d Compare August 2, 2024 12:41
@Praveen2112 Praveen2112 requested a review from electrum August 2, 2024 12:41
@Praveen2112 Praveen2112 changed the title Use ConfigurationResolver from Airlift Use SecretsResolver from Airlift Aug 2, 2024
@Praveen2112 Praveen2112 force-pushed the praveen/configuration_provider_2 branch from 168119d to e4b153a Compare August 2, 2024 12:51
kokosing
kokosing approved these changes Aug 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is very straightforward. It is nice you have added product tests, but maybe more integration tests would be nice to make sure that other things work too. Consider system access control, group provider...

...sources/docker/presto-product-tests/conf/environment/multinode-secrets-provider/secrets.toml Outdated
Copy link
Copy Markdown
Member

@kokosing kokosing Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add some REAMDE file next to credential.jckes so one would know how to recreate it, just in case

@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Aug 5, 2024

It is nice you have added product tests, but maybe more integration tests would be nice to make sure that other things work too. Consider system access control, group provider.

We are using them as a part of ENV secrets provider so which allows us to use it across a bunch of components.

@Praveen2112 Praveen2112 requested review from dain and kokosing August 5, 2024 14:27
@Praveen2112 Praveen2112 force-pushed the praveen/configuration_provider_2 branch 5 times, most recently from 6db0ac0 to fa44c20 Compare August 7, 2024 11:42
@Praveen2112 Praveen2112 force-pushed the praveen/configuration_provider_2 branch from fa44c20 to 0eadaf3 Compare August 7, 2024 12:10
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Aug 7, 2024

@kokosing Based on your review comments - Have migrated the keystore file creation to a script which would be generated in the runtime and have also tested it for config.properties, password-authenticator and connector.properties - These are the modules which are tested in our ProductTest and for other modules we might need to increase the test coverage in an incremental way. Please share your feedback for the same.

@kokosing
Copy link
Copy Markdown
Member

kokosing commented Aug 7, 2024

The CI is red

@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Aug 7, 2024

@electrum I am going to merge this soon, please let me know if you have any comments. I will be happy to address any more comments you might have as follow pull request just in case. Thank

@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Aug 7, 2024

@Praveen2112 I'd like to review it too. Give me couple of hours please

@Praveen2112 Praveen2112 requested a review from wendigo August 7, 2024 13:46
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Aug 7, 2024

@wendigo Thanks a lot

@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Aug 7, 2024

@kokosing CI is green now.

@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Aug 7, 2024

@Praveen2112 go for it!

@Praveen2112 Praveen2112 merged commit f38f725 into trinodb:master Aug 8, 2024
@github-actions github-actions bot added this to the 454 milestone Aug 8, 2024
@Praveen2112 Praveen2112 mentioned this pull request Aug 8, 2024
Merged
@colebow colebow mentioned this pull request Aug 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wendigo wendigo wendigo approved these changes

@kokosing kokosing kokosing approved these changes

@electrum electrum Awaiting requested review from electrum

@dain dain Awaiting requested review from dain

Assignees

No one assigned

Labels

cla-signed

Milestone

454

Development

Successfully merging this pull request may close these issues.

3 participants

@Praveen2112 @kokosing @wendigo