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

Move kubeseal logic into pkg #887

Closed
wants to merge 2 commits into from
Closed

Conversation

adamkirk
Copy link

Description of the change

It's really just a farely widely scoped refactor, moving all of the code cmd/kubeseal/main.go into the pkg/kubeseal. I've tried to keep the code changes mapped closely to the original. So the seal function from cmd/kubeseal/main.go is now called via kubeseal.Seal.

Also I've tried to separate the cli skeleton from the code where possible. It was previouslt pretty intertwined, but now the functions in the kubeseal are mostly pure, and don't rely on global variables. Any CLI arguments needed by the functions are passed in from the cmd/kubeseal/main.go file.

To avoid having large amounts of arguments to certain functions (e.g. Seal) I've introduced some 'Instruction' structs. These act largely as DTO's to contain all of the values needed by the functions. I think this makes the functions easier to consume, as the instructions are quite explicit.

Benefits

It will allow custom tooling to be built around kubeseal without having to rely on external system calls and dependencies.

The main driver for myself wanting this, is to make the process of managing secrets a bit simpler for dev teams (or maybe just less scary, for those unfamiliar with k8s). My aim is to build a small wrapper around kubeseal, which will pull secrets from a centralised store (AWS secrets manager, onepassword, bitwarden, vault etc), and will then create a set of sealed secrets files based on the secrets in the store, which can then be committed to the repositories.

I think this actually makes certain components easier to test in isolation which could be an improvement.

Possible drawbacks

In theory I don't really see any drawbacks for this as there aren't any behavioural changes. The library pkg is used from the main command, as well as being exposed, so there shouldn't be any duplication.

The main drawback I think is that this is a pretty large amount of change, and could potentially introduce bugs.

Applicable issues

Additional information

Pretty new to this library and tool, so any suggestions or pointers on the architecture here are more than welcome!

All the tests are passing, and i've compiled the tool locally and tested some scenarios against the latest helm chart running in a kind cluster.

@agarcia-oss agarcia-oss added enhancement backlog Issues/PRs that will be included in the project roadmap labels Jul 21, 2022
@agarcia-oss
Copy link
Member

Thanks for the PR, @adamkirk ! we'll review the code changes today!

@alvneiayu
Copy link
Collaborator

hi @adamkirk

First of all, thanks a lot for sending us the PR and for your hard work. Right now the integration tests are failing and this is a PR doing a lot of changes. We want to go really carefully with the PR. Could you take a look and solve the test integration errors, please?

Thanks a lot again.

Álvaro

@josvazg
Copy link
Collaborator

josvazg commented Aug 4, 2022

Integration tests throw me a similar error to what I was getting when working on #901
IMHO, if we merge that one first, and probably also #440 we should be on a better position to start go this route.

I agree with @alvneiayu that we probably want to make this happen in smaller steps.

@alemorcuq
Copy link
Collaborator

Thanks for this PR. Since the PR has been stale for a while, we have been discussing this internally and we will work on it as it is very core to the functionality.

@alemorcuq alemorcuq closed this Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues/PRs that will be included in the project roadmap enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubeseal: client library
5 participants