FLI Issue 3222: Import issue when --address and --drop is used

[devumesh]

Linked Issue: #3222

Approach:

1. Introduced a new API for deleting all the namespaces except default.
2. Endpoint DELETE /api/v1/namespaces
3. When --drop is used with --address, delete all namespaces API is called using SDK client before starting the actual import

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.75%. Comparing base (76ba2d8) to head (d05a775).

Files with missing lines	Patch %	Lines
cmd/flipt/import.go	0.00%	4 Missing ⚠️
internal/server/namespace.go	66.66%	1 Missing and 1 partial ⚠️
internal/storage/fs/store.go	0.00%	2 Missing ⚠️
internal/storage/sql/common/namespace.go	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3530      +/-   ##
==========================================
- Coverage   65.76%   65.75%   -0.02%     
==========================================
  Files         169      169              
  Lines       13651    13676      +25     
==========================================
+ Hits         8977     8992      +15     
- Misses       3989     3997       +8     
- Partials      685      687       +2     

Flag	Coverage Δ
unittests	65.75% <60.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[erka]

@devumesh Thank you for your contribution! Please be patient as it will take some discussion.

@GeorgeMac @markphelps could you please take a look on this? As it's a new API endpoint I would like to hear your thoughts on it. I believe the RBAC, audit and observability could be affected by this. Maybe we could do it differently and allow force deleteNamespace with some kind of option. I also don't know for sure if it should be a part of openapi spec.

wdyt?

[GeorgeMac]

Hey @devumesh thanks for taking this on!

Great points raised @erka too!

I believe the RBAC, audit and observability could be affected by this.

That is right. We will need to implement the Request function on the new DeleteAllNamespacesRequest type:

flipt/rpc/flipt/flipt.proto

Line 502 in d05a775

message DeleteAllNamespacesRequest {}

Example:

flipt/rpc/flipt/request.go

Lines 112 to 114 in d05a775

	func (req *DeleteNamespaceRequest) Request() []Request {
	return []Request{NewRequest(ResourceNamespace, ActionDelete, WithNamespace(req.Key))}
	}

Effectively, we would need to add to this file ☝️ something like:

func (req *DeleteAllNamespacesRequest) Request() []Request {
	return []Request{NewRequest(ResourceNamespace, ActionDelete)}
}

Here I have said the requester needs to be able to delete any namespace in order for this to be authorized (notice there is no resource key).

I believe by implementing this one function that auditing should also be taken care of (correct me if I am wrong cc @markphelps)?
WRT observability I think we're covered. At-least in terms of e.g. API requests being measured by existing middleware.

Maybe we could do it differently and allow force deleteNamespace with some kind of option.

Could you speak to this a bit more @erka ? I am not sure I quite follow yet. As in like, support deleting protected namespaces? Or maybe like... dropping the contents, not deleting? (deleteAllNamespaceContents ?)

I also don't know for sure if it should be a part of openapi spec.

If this API exists, I don't think it is necessarily a bad thing to have it in the OpenAPI spec. I understand the concern though that most folks shouldn't be triggering this and we don't want folks dropping everything by mistake. Maybe there is more we can do from an authz perspective (feels like an authz problem).

[erka]

Could you speak to this a bit more @erka ? I am not sure I quite follow yet. As in like, support deleting protected namespaces? Or maybe like... dropping the contents, not deleting? (deleteAllNamespaceContents ?)

Currently you can't delete the namespace with deleteNamespace while the namespace has some flags. DeleteNamespaceRequest could have force parameter which disables that check. Also protected namespace term is becoming a bit messy right now. From one side Flipt has authz and probably it should care about protecting the namespace. On the other side, there is a namespaces database table with protected field which with new api endpoint lost the functionality. I don't know about the commitments to users what is protected namespace term. Users may mark some namespaces as protected as well directly in database. Someone may delete default and deleteAllNamespaces will create a default namespace which is a bit awkward in some sense. Maybe the deleteAllNamespaces name is not quite correct. There are some edge cases here and there. It would be nice to talk about them so everyone is on the same page.

[markphelps]

Could you speak to this a bit more @erka ? I am not sure I quite follow yet. As in like, support deleting protected namespaces? Or maybe like... dropping the contents, not deleting? (deleteAllNamespaceContents ?)

Currently you can't delete the namespace with deleteNamespace while the namespace has some flags. DeleteNamespaceRequest could have force parameter which disables that check. Also protected namespace term is becoming a bit messy right now. From one side Flipt has authz and probably it should care about protecting the namespace. On the other side, there is a namespaces database table with protected field which with new api endpoint lost the functionality. I don't know about the commitments to users what is protected namespace term. Users may mark some namespaces as protected as well directly in database. Someone may delete default and deleteAllNamespaces will create a default namespace which is a bit awkward in some sense. Maybe the deleteAllNamespaces name is not quite correct. There are some edge cases here and there. It would be nice to talk about them so everyone is on the same page.

@GeorgeMac is correct above about what's required for authz/auditing to work, we should just need to create that requester method to fulfil the interface here:

flipt/rpc/flipt/request.go

Lines 3 to 5 in 81ec576

	type Requester interface {
	Request() []Request
	}

Re: @erka 's comments, I tend to agree that protected may not mean much anymore, it wasn't ever exposed publicly from a UI/API perspective (although it is exposed in the flipt.proto definition for namespace here although we could deprecate this field)

Users may mark some namespaces as protected as well directly in database

IMO if users are messing around in the database then all bets are off, there is only so much we can guard against

Someone may delete default and deleteAllNamespaces will create a default namespace which is a bit awkward in some sense

I agree it is a bit weird that we would create the default namespace again after deletion

Perhaps, instead on Flipt startup, it could check to see if there is a default namespace and if not create one?

That way we could keep this deleteAllNamespaces as is and remove the creation of the default one at the end? Just throwing it out there.

We could then get rid of the notion of a protected namespace, and the only one that is guaranteed to exist at startup is default

If user calls deleteAllNamespace then it will do just that, mostly it should only ever be used for import which as the initial intent of this PR. Just my thoughts

[GeorgeMac]

I am also going to throw a variation of what I mentioned before into the mix:
What if we simply added a deleteNamespaceContents endpoint instead?
This would simply delete the contents of the namespace, not the namespace itself (for the relational backends this should be as simple as a couple delete SQL queries on flags and segments, since they cascade).
It would be useable on protected namespaces (protected would only mean cannot delete but can empty).
Since protected was only there because we depended on at-least default existing as a namespace (no contents is fine).

When --drop is supplied we would list all namespaces and delete them if they're not protected (or rather if their key is not default would be enough).
When it comes to the default namespace we use the new empty endpoint.
Makes import a little more complex than calling a single API call, but no more complex than the actual process it currently is when it adds all the other resources.

WDYT?