Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor secdist: get rid of DefaultSecdistProvider component #573

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

mnink275
Copy link
Contributor

@mnink275 mnink275 commented May 8, 2024

This PR solves the issue #547
The main changes are made in the directory: core/{include/userver,src}/storages/secdist/

  • The components::DefaultSecdistProvider component has been removed.
  • Added components::SecdistComponentBase - base component for all secret distributors. The default implementation of this class is components::Secdist - ready-to-use component.
  • Implementations of the storages::secdist::SecdistProviderBase class are now used to store the secdist configuration.

Thus it is still possible to implement your own secret distributor by implementing the base classes components::SecdistComponentBase and storages::secdist::SecdistProviderBase.

@mnink275 mnink275 changed the title refactor secdist: get rid of DefaultSecdistProvider component [WIP] refactor secdist: get rid of DefaultSecdistProvider component May 8, 2024
@mnink275 mnink275 changed the title [WIP] refactor secdist: get rid of DefaultSecdistProvider component refactor secdist: get rid of DefaultSecdistProvider component May 8, 2024

const auto provider_name =
config["provider"].As<std::string>("default-secdist-provider");
Copy link
Member

@apolukhin apolukhin Jun 10, 2024

Choose a reason for hiding this comment

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

Wow! You've done much more than we were expecting!

Unfortunately, in our internal codebase we already have a lot of custom providers. It is almost impossible for us, to make a migration in one go - hundreds of services should be adjusted and changed simultaneously.

Could you please split this PR into multiple?

First PR should preserve the provider config parameter and the SecdistProvider hierarchy. The DefaultSecdistProvider component should remain in code but not used in userver static configs. provider option should become empty by default, and in that case the whole functionality is implemented by the Secdist itself. That way we could merge the PR without affecting the services, remove the default-secdist-provider usage in hundreds of services.

After that, the DefaultSecdistProvider could be removed by a separate PR. And after that, we can take a second look on new hierarchy of providers (however, I think that it is fine to leave the provider to customize the Secdist in complicated cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants