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

Add multi-namespace capabilities to SOR.bulkGet #109197

Closed
pgayvallet opened this issue Aug 19, 2021 · 11 comments · Fixed by #109967
Closed

Add multi-namespace capabilities to SOR.bulkGet #109197

pgayvallet opened this issue Aug 19, 2021 · 11 comments · Fixed by #109967
Labels
Feature:Saved Objects impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort NeededFor:Core Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@pgayvallet
Copy link
Contributor

pgayvallet commented Aug 19, 2021

In #91615, we're planning on adding multi-space support to the SO management page and to the import/export feature. However to do that, there's a couple of the SO repository APIs that needs to be adapted to be multi-space compatible.

  • SOR.bulkGet: Required for the exportById functionality, and for the references gathering in both mode
  • SOR.delete: Required for the delete functionality of the SOM UI (less important than bulkGet, but would greatly ease the UI integration by avoiding to disable deletion based on the object's meta)

These 2 APIs need to be modified to allow to pass the target namespace

The security and spaces wrappers will also need to be modified accordingly.

cc @jportner

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Saved Objects labels Aug 19, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@jportner
Copy link
Contributor

Perhaps it makes sense to implement bulkDelete first (#30503) and then just allow bulkGet/bulkDelete to specify a different target namespace. Thoughts?
I'm not sure what we would name the option for delete to specify a different space to delete from, and we don't have equivalents for get and update.
We do have an equivalent for create, it's the initialNamespaces option, but that allows you to specify multiple spaces so I'm not sure it makes sense to compare the two.

@pgayvallet
Copy link
Contributor Author

To be perfectly honest, I feel like this 'we need to find another option name than namespace because this has been protected for a very long time' doesn't really make any sense now that we're supporting shareable/shared spaces, and I think we'll need to accept it at some point.

Like, only adding multi-NS support to bulk operations because we're allowing to specify namespace in each element of an array of objects, but not to non-bulk operation because atm namespace is considered as protected on 'root' APIs options, doesn't make any sense imho. I mean, we do want multi-NS support for APIs like get and update at some point, don't we?

Imho, If we still need the spaces plugin to internally provide the info of the current namespace, it should now be done with an internal currentNamespace option, which would be the one to be protected and only provided/providable by the space plugin. the current namespace option would then be writable, and would default to be populated by the spaces plugin to the same value as currentNamespace.

That being said, if for now we prefer to only work with bulk operations, I'm fine implementing bulkDelete (I mean, we wanted to do it for quite a while now). But, still, I think we need to have the discussion for non-bulk operations and the approach we want for them regarding multi-NS support, wdyt?

@jportner
Copy link
Contributor

You make some good points about namespace, that (using currentNamespace instead) definitely sounds like a viable non-breaking option! @legrego do you have any reservations or thoughts about that?

@legrego
Copy link
Member

legrego commented Aug 23, 2021

You make some good points about namespace, that (using currentNamespace instead) definitely sounds like a viable non-breaking option! @legrego do you have any reservations or thoughts about that?

No immediate reservations. currentNamespace is a better name given the multi-namespace reality that we find ourselves in.

My only other thought is that the "current namespace" is something that core could start to define in the 8.x era itself if we wanted it to, as Spaces will no longer be an optional feature, and is considered a platform-level construct that features can build on top of. This is forward looking and very hypothetical, so I feel free to disregard all of this and move forward with currentNamespace. We can always refactor this in the future, as we do not offer plugin API stability at this point.

@pgayvallet
Copy link
Contributor Author

My only other thought is that the "current namespace" is something that core could start to define in the 8.x era itself if we wanted it to, as Spaces will no longer be an optional feature, and is considered a platform-level construct that features can build on top of

True, but if we achieve that at some point, the whole spaces wrapper should go away, no?

So, if the security team think this currentNamespace refactoring is acceptable, what's the best way to move forward with this specific issue (that is a blocker for multi-NS import/export)? I suspect that if we do change the current namespace option to currentNamespace, we'd want to do it for all SOR APIs in a single PR, right?

Should I implement bulkDelete, to dodge the issue for now, and open a new issue for this specific currentNamespace thing? wdyt?

cc @lukeelmers

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Aug 24, 2021

Btw, bulkDelete is only required by #91615 as a side-effect, as we're kinda forced to adapt other parts of the SOM UI when adding the multi-NS import/export.

The priority remains to implement the multi-NS support for bulkGet, as this API is directly used by the by-id export. @jportner do you have any visibility on when your team would be able to take this one?

@jportner
Copy link
Contributor

jportner commented Aug 24, 2021

I can pick up bulkGet now.

IMO we should go ahead and add bulkDelete too, and worry about the currentNamespace refactoring later.

@lukeelmers
Copy link
Member

Should I implement bulkDelete, to dodge the issue for now, and open a new issue for this specific currentNamespace thing? wdyt?

IMO we should go ahead and add bulkDelete too, and worry about the currentNamespace refactoring later.

++ Since we have a clear path forward with bulkDelete now, and it is needed in order to progress on #91615, I agree we should just press ahead and revisit currentNamespace as a separate issue.

@pgayvallet pgayvallet changed the title Add multi-namespace capabilities to SOR.delete and SOR.bulkGet Add multi-namespace capabilities to SOR.bulkGet Aug 25, 2021
@pgayvallet
Copy link
Contributor Author

++ Since we have a clear path forward with bulkDelete now,

Ok, I will switch to implementing bulkDelete once #109196 is blocked again (some things were unblocked by #109258 and #99044)

and revisit currentNamespace as a separate issue.

I created #109996 to discuss the specifics. Also renamed the current issue to reflect the new scope.

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Objects impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort NeededFor:Core Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants