-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Sharing saved objects, phase 2.5 #89344
Sharing saved objects, phase 2.5 #89344
Conversation
Note for reviewers: the UI for the Share to Space flyout can only be accessed if an object's namespaceType is Lines 29 to 34 in 7bb8d3a
|
e9a48bd
to
7b2415e
Compare
x-pack/plugins/encrypted_saved_objects/server/create_migration.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/encrypted_saved_objects/server/create_migration.ts
Outdated
Show resolved
Hide resolved
c3506dd
to
bfd5d02
Compare
ba29e31
to
5cc7b56
Compare
This will be used to convert saved objects in the 8.0 release. It will allow us to regenerate object IDs, create aliases, and force objects to use unique IDs across namespaces. However, objects of this type are "share-capable" but not shareable across multiple namespaces.
ESO uses object "descriptors" as part of additionally authenticated data (AAD) when encrypting and decrypting objects. Historically the descriptors for single-namespace objects have included the object namespace, but in a world where saved objects can be shared across spaces, that no longer makes sense. This commit allows consumers to define an ESO migration that would allow for flexible decryption of a saved object using a legacy descriptor that includes a namespace, then encrypts the object with a new descriptor that omits the object's namespace.
The saved object migration context now describes what migration version is currently being run, and the object type's registered `convertToMultiNamespaceTypeVersion` field (if it exists). This allows the ESO migration function to more intelligently make decisions about how to handle object descriptors for additionally authenticated data (AAD).
The existing component is now called ShareToSpaceFlyoutInternal, which implies that it should not be used by external plugins.
The ShareToSpaceFlyout depended on NotificationsSetup, when it already had the ability to access the notifications service via the KibanaReactContextProvider that it uses.
Includes changes to labels and i18n. Also adds configurable options for whether or not to display the "create new copy" callout and/or the "create new space" link text, and adds new test cases accordingly.
If the user cannot change the object's spaces, a warning callout is displayed in addition to the tooltip. Also added unit tests to exercise this functionality and the ShareModeControl in general.
This will allow the flyout to behave in a space-agnostic manner (instead of the default, which is space-aware). In other words, it will no longer treat the active space differently -- allowing the user to freely deselect the active space if they desire. This will be useful for ML, and for the saved objects management page in the future when we eventually show objects from all spaces.
This React context fetches Spaces data one time, allowing any children to consume it without re-fetching. The first such children to use the SpacesContext are the ShareToSpaceFlyout and the ShareToSpaceAction.
Previously it rendered spaces as badges with their full names. Now it renders them as SpaceAvatar components. It also allows consumers to change the limit on the number of spaces that are displayed, and to enable space-agnostic behavior (e.g., render the active space).
When a feature ID is specified on a SpaceContext, other Space UI components will behave accordingly when the feature is disabled in a given space. In SpacesList, the affected spaces will be moved to the end of the list. In ShareToSpaceFlyout, the affected spaces will only be shown if the object already exists in those spaces, and will be differentiated with a tooltip explaining why.
5cc7b56
to
68018a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on a technical level regarding core changes, just a few NITs
Now, sorry if this was already answered, but what's the goal of this intermediary stage 😅 ?
src/plugins/saved_objects_management/public/management_section/mount_section.tsx
Outdated
Show resolved
Hide resolved
...lugins/saved_objects_management/public/management_section/objects_table/components/table.tsx
Show resolved
Hide resolved
src/plugins/saved_objects_management/public/management_section/saved_objects_table_page.tsx
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects_management/public/management_section/saved_objects_table_page.tsx
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects_management/public/management_section/saved_objects_table_page.tsx
Show resolved
Hide resolved
Great! I addressed your feedback in 9d1e02e 👍
Sorry about that, I updated the PR description and added an "Overview" at the top to better describe why we want to make this change. |
.../plugins/spaces/public/share_saved_objects_to_space/components/selectable_spaces_control.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an impressive amount of work, LGTM! I tested locally as much use cases as I could think of and haven't noticed anything obviously wrong.
The feature/API surface is very large though, I think once UI and API stabilizes, and we release sharing capabilities our team will need to gradually cover this functionality with UI tests (thanks for the unit/api-integration tests you've already added!)
.../plugins/spaces/public/share_saved_objects_to_space/components/selectable_spaces_control.tsx
Show resolved
Hide resolved
...paces/public/share_saved_objects_to_space/components/share_to_space_flyout_internal.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - reviewed almost everything, and did cursory testing. I defer to Aleh as the primary reviewer here, and ML's review of the integration.
Nice work, Joe! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI copy LGTM
width: '90px', | ||
}); | ||
} | ||
// Note: this code path is commented because it is currently unreachable, it will need to be refactored to use the SpacesApi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this still needs changing. It's not currently possible to assign DFA jobs to spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I don't know why I thought the DataFrameAnalyticsList was only used in one place, it's actually used in two places. That's what screwed me up.
Fixed in 265b85c and tested to make sure I could create + share a DFA job.
It's worth noting that I couldn't keep the SpacesContext at the JobsListPage level where it used to be, because that component re-renders a few times and somehow that winds up causing an infinite re-rendering loop when the SpacesContext is introduced there. So, instead, I added the SpacesContext wrapper within each tab section. Works like a charm, and the Spaces get reloaded (along with all the job data) when you switch tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an odd issue now on the DFA jobs list in the management page where the search bar loses focus after each keypress. This is the cause of the failing functional test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer, that helped me figure out what was going on. The SpacesContext wrapper (along with all other components on this page) gets re-rendered multiple times with each key press. I discovered this by adding a console log inside of it 😄 As a side effect of that, the SpacesContext wrapper was creating a brand new context object each time it was rendered, which caused the wonky behavior of losing focus.
This was not a problem on the Saved Objects Management page, which is a bit simpler and only renders the SpacesContext wrapper once.
In d28bbda I changed the SpacesContext wrapper a bit to be more resilient, and because of that I was able to move it back to the top of the JobsListPage where it used to be. So I think it's all working perfectly now, you can take another look!
But FYI I think the JobsListPage has some problems that cause unnecessary re-rendering of components, might want to make a mental note to look into that in the future.
The wrapper would recreate the underlying context object each time it is re-rendered. That was not a problem for the Saved Objects Management page, which only rendered it once -- but it turned out to be a problem for the Machine Learning Jobs management page which re-renders all of its children multiple times.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/tls·ts.apis SecuritySolution Endpoints Tls Test with Packetbeat Tls Test "before all" hook for "Ensure data is returned for FlowTarget.Source"Standard Out
Stack Trace
Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/tls·ts.apis SecuritySolution Endpoints Tls Test with Packetbeat Tls Test "after all" hook for "Ensure data is returned for FlowTarget.Destination"Standard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM!
Backport result
|
🙁 you did your best, KibanaMachine |
Resolves: #85791
Overview
"Share-capable" objects
For the 8.0 release, we want to regenerate single-namespace object IDs without necessarily making them shareable. I call this making the objects "share-capable".
The easiest / best way is to convert the objects using the code we already built, as that will remove the namespace prefix from the serialized object and prevent any additional objects from being created with the same ID in a different space.
However, we also want to provide a way for consumers (Spaces UI) to differentiate which objects should be shareable and which are not. And, IMO, we want to guarantee to plugin owners that Kibana's APIs won't allow their objects to be shared before they are ready to support it.
My approach to this is to actually have a fourth namespaceType,
'multiple-isolated'
, that is an intermediate between'single'
and'multiple'
. This is treated like a multi-namespace type by Core (serialization, queries, repository methods) but behaves like a single-namespace type (cannot be shared or unshared, either from the UI or by using an API).When a plugin owner is ready to make their object type shareable -- either in the 8.0 release, or in a subsequent minor -- they can "flip a switch" by changing its namespaceType to
'multiple'
, at which point the object will be fully shareable.Reusable UI components
The Spaces plugin needs to provide some reusable UI components to plugin owners to use when their objects are shareable. For example, when dashboards are shareable, you may want to see the list of other spaces the current dashboard is shared to, and you might want to be able to click a button to change that. With these reusable UI components, users can accomplish all that without leaving their dashboard.
Primary Changes
Core
'multiple-isolated'
. This will be used to convert object types in the 8.0 release to become "share-capable", regenerating their IDs and creating legacy URL aliases without making the object types actually shareableEncryptedSavedObjects
Spaces
SpacesContext
component, which creates a React Context that allows components in the Spaces plugin to avoid fetching Space-related data multiple timesShareToSpaceFlyout
component to make it reusable. It now depends on the SpacesContext. It now has several more options to customize its appearance and behavior.SpaceList
component, based on the existing ShareToSpaceSavedObjectsManagementColumn. It now depends on the SpacesContext, and renders the list of spaces as avatars instead of full badges. It also includes a few options to customize its appearance and behavior.LegacyUrlConflict
component. It renders a callout that informs the user there are two objects with the same URL.redirectLegacyUrl
function. It redirects a user to a new URL, and displays a toast that informs the user they used a legacy URL.Machine Learning
Screenshots
Click to expand
Environment:
Anywhere that you see 'object', it is the default
objectNoun
value that can be replaced with anything else by the consumer ('dashboard', 'visualization', etc.)SpaceList
componentScreenshot 1: Saved Objects Management page
Screenshot 2: Machine Learning Jobs management page
The job is shared to Alpha, Bravo, Charlie, and Delta. The avatar for Charlie is at the end (because ML is disabled in Charlie), and Bravo is not shown (because the user does not have any privileges in the Bravo space).
ShareToSpaceFlyout
componentScreenshot 1: Saved Objects Management page, object that is shared to all spaces
Screenshot 2: Saved Objects Management page, object that is shared to explicit spaces
Screenshot 3: Machine Learning Jobs management page, job that is shared to explicit spaces
LegacyUrlConflict
componentredirectLegacyUrl
toastConversion Examples
These examples demonstrate usage of the new
'multiple-isolated'
namespaceType.Example 1: Convert an existing regular object type
A consumer has an existing single-namespace saved object type in 7.12 that needs to be converted to become "share-capable" in 8.0, and fully shareable in 8.1.
Click to see code
Example of a single-namespace type in 7.12:
Example after converting to a multi-namespace (isolated) type in 8.0:
Example after converting to a multi-namespace (shareable) type in 8.1:
Example 2: Convert an existing encrypted object type
A consumer has an existing single-namespace encrypted saved object type in 7.12 that needs to be converted to become "share-capable" in 8.0, and fully shareable in 8.1. To accomplish this, the consumer needs to define an ESO migration in 8.0.0 as well.
Click to see code
Example of a single-namespace encrypted type in 7.12:
Example after converting to a multi-namespace (isolated) type in 8.0:
Example after converting to a multi-namespace (shareable) type in 8.1: