sealed-secrets icon indicating copy to clipboard operation
sealed-secrets copied to clipboard

Move kubeseal logic into pkg

Open adamkirk opened this issue 2 years ago • 3 comments

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

  • fixes #348

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.

adamkirk avatar Jul 16 '22 14:07 adamkirk

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

agarcia-oss avatar Jul 21 '22 07:07 agarcia-oss

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

alvneiayu avatar Jul 21 '22 14:07 alvneiayu

Integration tests throw me a similar error to what I was getting when working on https://github.com/bitnami-labs/sealed-secrets/pull/901 IMHO, if we merge that one first, and probably also https://github.com/bitnami-labs/sealed-secrets/pull/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.

josvazg avatar Aug 04 '22 09:08 josvazg

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 avatar Aug 25 '22 14:08 alemorcuq