feat(cmd): import and export namespace flag

[GeorgeMac]

Fixes FLI-258

This PR primarily adds namespace support to both import and export:

flipt import --namespace foo --create-namespace
flipt export --namespace foo

Along the way I moved a lot of stuff around to support it.

Firstly, I set about refactoring the cmd package. Primarily, I moved a lot of global stuff into functions.
This helped me get to a spot where --config is no longer required for import or export (it still is for main and migrate).

I wanted this to be optional for these commands. Such that they can now support import and exporting state into and out of Flipt via the API and using the SDK.

flipt import --import-to-address (http(s)|grpc)://localhost:8080 --import-to-token <client-token>
flipt import --export-from-address (http(s)|grpc)://localhost:8080 --export-from-token <client-token>

This meant updating some of the core import and export code to work over a higher abstraction.
They were (export at least) working directly on the storage APIs.
Now they operate through the grpc APIs and the SDK fits in nicely.

The major motivation here was to adds integrations tests for import and export. Which this PR includes.
I ended up doing a lot of overhaul in the test suite code to fit these in.

However, now that its done, the import and export integration suite runs in the same matrix as the API suite.
This new test suite does the following:

1. flipt import into a target instance with some seed data.
2. Uses the SDK to do some read-only API calls to check that state is reachable.
3. flipt export the data and compares with the original seed data.

As mentioned, this suite is ran in every combination the API tests are ran in.
Meaning we run it protocol x namespace x authenticated times (8).

The integartion suite now runs 16 different configurations.

[codecov]

Codecov Report

Merging #1452 (76e4867) into namespaces (d9500dc) will decrease coverage by 0.37%.
The diff coverage is 75.40%.

@@              Coverage Diff               @@
##           namespaces    #1452      +/-   ##
==============================================
- Coverage       78.19%   77.82%   -0.37%     
==============================================
  Files              44       44              
  Lines            3348     3405      +57     
==============================================
+ Hits             2618     2650      +32     
- Misses            578      602      +24     
- Partials          152      153       +1     

Impacted Files	Coverage Δ
internal/storage/sql/migrator.go	20.00% <0.00%> (-2.79%)	⬇️
internal/ext/importer.go	70.06% <74.54%> (-5.50%)	⬇️
internal/ext/exporter.go	88.72% <100.00%> (+2.11%)	⬆️
internal/server/evaluator.go	94.73% <100.00%> (+0.04%)	⬆️
internal/storage/sql/db.go	91.36% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[markphelps]

still working through but wanted to post some initial thoughts. looking great btw!

[markphelps]

i remember there being some weird reason we had to do this previously, it had something to do with maybe the logger not being initialized properly before trying to use it? I'm guessing the changes to move the logger into the cmds themselves above addresses this?

[GeorgeMac]

I popped cobra to have a look; initializers run just before Run or RunE, but after flag parsing.

There are some bits of validating that go on between invoking the run function and after calling this function.
However, I can't necessarily see that causing problems, since we don't seem to use half the things which get triggered (e.g. PreRun and so on), let alone have a dependency order which is dictated by these:
https://github.com/spf13/cobra/blob/v1.6.1/command.go#L876-L921

Running the same process at the start of Run / RunE should be equivalent in our situation.
Allowing us to not dictate that every function has to parse a config.yaml in a particular location in order to be invoked. That was my motivation at-least, since OnInitialize will run in all circumstances.

I was going to just make the default of --config be empty string and switch on that. But then realized that would actually be a breaking change if folks are relying on just editing the default yaml file at the usual location and ommitting the flag.

[markphelps]

flipt import --namespace foo --create-namespace seems a bit repetitive to me. I also wonder what the user would expect here by default, would they expect that importing to a non-existing namespace creates the namespace without having to specify additional options? similar to how importing flags that don't exist just creates the flags?

Alternatively, I wonder if in the future we might have other types of resources that could not exist but we'd want to create if a given CLI flag was present. In that case, maybe a more generic --force / -f or similar would be appropriate? just wonder how 'special' we want to make namespaces overall

[GeorgeMac]

My motivation here was to match helm (helm/helm#7648). It is repetetive but explicit and does one thing.
Whereas -f to me suggests that perhaps validation is bypassed, not extra resources may be created if not exists.

I can't imagine what other resources we may need to create in this way in the future.
This command inherently creates all the resources in the file you're importing anyway.

I would probably lean on --create-namespace still myself. However, I am open to more ideas.

We could move namespace as a resource inside the manifests themselves and expect the caller to include it.
This is what kubernetes expects. It would make it more explicit and remove the need for a flag.
However, that comes with added complexity in API use and implementation.
K8s actually fails if you try to apply a bunch of resources and the namespace is in the same payload.
You have to do them one at a time to ensure the namespace is created before its contents.

Equally, we could forgo this entirely. Expect it to exists and I can update the tests to work around that.

[markphelps]

yeah that explanation makes sense to me, especially since there is prior art in the k8s/helm space. thanks!

[yquansah]

LGTM, just a couple questions

[yquansah]

Could this be racy at all? I am just wondering in comparison to waiting on the receive from interrupt at the bottom of the function.

Now the interrupt signal can be received whenever this goroutine is initiated. But I guess in the end it's just canceling the context.

[yquansah]

I am wondering if it's worth the additional memory overhead of spinning up another goroutine here as well.

[GeorgeMac]

I wouldn't worry about a single goroutine. There about 4k of memory if I remember correctly (may have changed).
Concerns r.e. memory and goroutines is more when you're creating them in an unbound loop, and worse when there is no way to tear them down. This case is once per execution and exits when the signal gets trapped and closes the interrupt channel.

We could move from signal.Notify (which is what we have always called) to signal.NotifyContext (relatively new in the grand scheme of Go stdlib).
Though, that said, signal.NotifyContext is the same thing. It is just a goroutine that cancels a context when the signal channel sends:
https://cs.opensource.google/go/go/+/refs/tags/go1.20.2:src/os/signal/signal.go;l=277-296

[GeorgeMac]

Last comments on your question r.e. is it racey?

Here the only things we're sharing access to are interrupt and cancel.
The first is a channel that we're receiving on. We're performing a synchronisation operation which is inherently not racey.

The cancel function is also safe to handle concurrently. From the docs:

A CancelFunc may be called by multiple goroutines simultaneously. After the first call, subsequent calls to a CancelFunc do nothing.

[yquansah]

@GeorgeMac Such a great explanantion. I appreciate this George 👍.

[markphelps]

does exiting with error (fatal) / non-user initiated cause the interrupt signal to be sent over the interrupt channel?

previously if run exited we would always cancel the context since cancel() was in a defer call, with moving to this change I'm just wondering if there are any side effects?

[GeorgeMac]

cancel() is still called in a defer. Just in main and not inside the cobra run command:
https://github.com/flipt-io/flipt/pull/1452/files/af7e193555d908c366f091622bf6697259492bab#diff-f0fc8b9cf7ff242b701bc3eece5ca189b334ecf1382930bc34c4ae3b4d312e55R118

Prior to this change we were doing signal handling inside the cobra run commands, as opposed to just once at the top in main for all commands.

When it comes to fatal though, it always results in an immediate call to os.Exit and nothing defered gets run anyway.
With panics you get defer function calls. However, in this situation probably best to just finish asap with non zero exit code.

[yquansah]

LGTM

[markphelps]

Looks GREAT to me!

Just one minor question 🙌🏻