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

[devumesh]

Linked Issue: https://github.com/flipt-io/flipt/issues/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[bot]]

Codecov Report

Attention: Patch coverage is 21.21212% with 26 lines in your changes missing coverage. Please review.

Project coverage is 64.08%. Comparing base (3223ce4) to head (aeca754). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/flipt/import.go	0.00%	23 Missing :warning:
internal/server/namespace.go	70.00%	2 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3530      +/-   ##
==========================================
- Coverage   64.17%   64.08%   -0.09%     
==========================================
  Files         169      169              
  Lines       16917    16942      +25     
==========================================
+ Hits        10856    10858       +2     
- Misses       5378     5401      +23     
  Partials      683      683              

Flag	Coverage Δ
unittests	64.08% <21.21%> (-0.09%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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: https://github.com/flipt-io/flipt/blob/d05a775bdce27d17784361072a742e51c372f7bb/rpc/flipt/flipt.proto#L502

Example: https://github.com/flipt-io/flipt/blob/d05a775bdce27d17784361072a742e51c372f7bb/rpc/flipt/request.go#L112-L114

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: https://github.com/flipt-io/flipt/blob/81ec576151b75065259f628b8b9427159949adac/rpc/flipt/request.go#L3-L5

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?

[erka]

That way we could keep this deleteAllNamespaces as is and remove the creation of the default one at the end?

@markphelps I think you are right.

Probably default namespace is only matter for quick adoption/usage in general. There is no problem to recreate default using UI if it's missing. The protected field could be removed in another PR if needed.

@GeorgeMac if only import command would use that API endpoint, it would be the great idea. But as it's a public API, it will create extra complexity with authz configuration for end users and for maintainers.

It would be nice to have some resolution here so this PR could move forward.

[markphelps]

That way we could keep this deleteAllNamespaces as is and remove the creation of the default one at the end?

@markphelps I think you are right.

Probably default namespace is only matter for quick adoption/usage in general. There is no problem to recreate default using UI if it's missing. The protected field could be removed in another PR if needed.

@GeorgeMac if only import command would use that API endpoint, it would be the great idea. But as it's a public API, it will create extra complexity with authz configuration for end users and for maintainers.

It would be nice to have some resolution here so this PR could move forward.

Agree, would like to get this in the next release (hopefully tomorrow).

There is no problem to recreate default using UI if it's missing. The protected field could be removed in another PR if needed.

Good point. We could check for the default namespace when loading the ui and prompt the user to create it if it doesn't exist.

If instead they simply are using deleteAllNamespaces API for import, it should be created for them anyways in the import

[GeorgeMac]

That way we could keep this deleteAllNamespaces as is and remove the creation of the default one at the end?

@markphelps I think you are right.

Probably default namespace is only matter for quick adoption/usage in general. There is no problem to recreate default using UI if it's missing. The protected field could be removed in another PR if needed.

@GeorgeMac if only import command would use that API endpoint, it would be the great idea. But as it's a public API, it will create extra complexity with authz configuration for end users and for maintainers.

It would be nice to have some resolution here so this PR could move forward.

@erka I see the concern. There is the question of "is delete namespace contents a verb/action in authz language of its own?". There is the question will folks want to support both the following via a policy:

• Allow deleting any flag or segment on an individual basis, but not allow a single bulk delete operation of all resources in a namespace
• Allow delete of all resources in a namespace in bulk, while still preventing deletion of the namespace itself

To have both, we need to distinguish that in the policy with potentially a new action or resource. Which we may not want in our auth language. Unless we choose to ignore this case, then we can just say something like requires delete namespace. If you can delete the namespace, you can just delete its contents. There is no way to express can delete all contents, but not the namespace records themselves (all or nothing).

The force delete option to drop namespace contents is a good idea, but I don't think it helps us with the protected case. If we support force delete on protected, then how does the default namespace get created again? We need it to exist, and we currently don't have a way to create protected namespaces through the API. Do we additionally add support for making protected namespaces?

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.

@markphelps Flipt startup wouldn't be an appropriate time, since you can import into it while Flipt is online (in fact it needs to be online for import to go through the API). We're not going to restart Flipt after the import is done, we don't want an API that causes a restart and that would just be silly anyway.

Really we need the default namespace to exist and be protected at the end of this operation. Unless we go about changing Flipt to not require it.

[GeorgeMac]

Hey @devumesh we had a talk in the community hours today about how to tackle this change. Thanks for all your patience!

We decided that it might actually be best to change the way this works slightly. Instead of a new endpoint, we would like to tackle this by adding a new force argument onto the existing DeleteNamespace call.

The force flag would have two behaviours:

1. Skip the guard against deleting namespaces with contents. i.e. delete it anyway and cascade to delete its contents.
2. Skip protected and delete the default namespace.

The --drop would walk over the namespaces calling delete on each one.

We appreciate all the effort you've put into making this approach work, sorry for the delay and lack of clarity on how we wanted this solved. Would you still be interested in updating this PR to do what we described here?

[devumesh]

Hi @GeorgeMac, yeah I'm interested

[devumesh]

Hi @GeorgeMac , I have a question when --drop is included in import we would be going through the list of namespaces and delete all the namespaces with force set to true. Now if I try to create a default namespace back using CreateNamespace I was not able to make that protected. How should I handle this scenario?

[erka]

@devumesh There’s no need to worry about the protected field for now. Flipt has authorization in place to manage the protection effectively.

[markphelps]

@devumesh There’s no need to worry about the protected field for now. Flipt has authorization in place to manage the protection effectively.

@erka I think what @devumesh is saying is that after dropping the default namespace if the --drop flag is used, that once its created again (on import) that it will no longer be protected. which means if the user tries to delete it from the UI they will not get the error that they would see originally:

https://github.com/flipt-io/flipt/blob/cb9cfaad138b130971628d8167a2884d0ad3ee57/internal/server/namespace.go#L76

[erka]

@erka I think what @devumesh is saying is that after dropping the default namespace if the --drop flag is used, that once its created again (on import) that it will no longer be protected. which means if the user tries to delete it from the UI they will not get the error that they would see originally:

@markphelps yep, I've got that message. We could add the notice about it in the docs but I think it makes very little impact overall.

[markphelps]

@all-contributors please add @devumesh for code

[allcontributors[bot]]

@markphelps

@devumesh already contributed before to code