-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ENDPOINT][INGEST]Task/endpoint ingest update #67234
[ENDPOINT][INGEST]Task/endpoint ingest update #67234
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
Pinging @elastic/endpoint-management (Team:Endpoint Management) |
id="xpack.ingestManager.editDatasource.stepConfigure.endpointConfiguration" | ||
defaultMessage="See security app policy tab for additional configuration options: " | ||
/> | ||
<EuiLink href={linkToSiemApp}>Click me to configure</EuiLink> |
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.
Missing i18n here.
Also, UX recommendation re: links. Phrase it like:
See <a href="">security app policy tab</a> for additional configuration options.
You can google ux "here" link
to see a lot of articles which all agree (but with various reasonings.)
The a11y reason is that an assistive device that lists links would show 'Click me to configure' as a place you can link to. In this case 'security app policy tab' is a better description.
The usability reason is basically this: If a user scans for buttons/links in order to take their next action, they'll see a 'Click me...' link, and then have to scan the previous paragraph to figure out what it does. The idea is that by placing a description of where the link goes in the link itself, the user can scan the page for links and see what they do.
id="xpack.ingestManager.editDatasource.stepConfigure.endpointConfiguration" | ||
defaultMessage="See security app policy tab for additional configuration options: " | ||
/> | ||
<EuiLink href={linkToSiemApp}>Click me to configure</EuiLink> |
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.
Since we'll be switching apps here, I think we have do a little bit more to avoid the full app refresh.
@paul-tavares does something similar here https://github.com/elastic/kibana/blob/master/x-pack/plugins/siem/public/endpoint_policy/view/policy_list.tsx#L137
@oatkiller I am waiting on @caitlinbetz and @bfishel to confirm the new wording. Everything is just a placeholder right now. but i shall i18n! |
@elasticmachine merge upstream |
(jenkins upgrade required killing all jobs) |
Since other plugins might also need this ability, it'd be nice if we could do it in a way local to the plugin vs inside Ingest. I don't want to stop progress because we don't have an answer today, but I think we should confirm other plugins need this and come up with reasonable way for them to do it. Essentially, I would like an O(1) approach in Ingest vs O(n) |
@jfsiii We thought about this too and agree that it'd be preferred if the code lived inside the actual plugin, but went ahead with the workaround for now. I made a discussion ticket to track any conversation about this: https://github.com/elastic/endpoint-app-team/issues/443 |
@@ -229,15 +230,15 @@ const IngestManagerApp = ({ | |||
const isDarkMode = useObservable<boolean>(coreStart.uiSettings.get$('theme:darkMode')); | |||
return ( | |||
<coreStart.i18n.Context> | |||
<CoreContext.Provider value={coreStart}> | |||
<KibanaContextProvider services={{ ...coreStart }}> |
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.
Ingest Team: FYI: I actually learned about this functionality from Kibana from one of you - maybe @jfsiii 😄
); | ||
|
||
export const CustomConfigureDatasource = (props: CustomConfigureDatasourceProps) => { | ||
const ConfigureDatasourceContent = |
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.
@jen-huang The package definition has a flag called solution
whose comment indicates that it should be the trigger for displaying a custom configuration UI - see: https://github.com/elastic/package-storage/blob/master/packages/endpoint/0.1.0/manifest.yml#L23
I don't see that property defined in Datasources type, so I assume that is future functionality?
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.
Yes, we need to implement that on the ingest management side. Could you make an issue for it? I wasn't aware that flag was already implemented on registry side :)
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 @jen-huang . Created #67939 to track
return ( | ||
<> | ||
{editMode === true ? ( | ||
<EuiLink |
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.
Can we move the components that are returned here to our source and export it from there?
It would be best if we keep all domain specific code in one place. Maybe you just need to add a Prop to the endpoint's <ConfigureEndpointDatasource>
component called editMode
and have it drive the returned UI.
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.
++ to above, CustomConfigureDatasourceProps
has property called from
which you can test from === 'edit'
to detect create vs edit mode
Edit: Oh, I think this is just a left over file from the initial POC and not referenced anywhere. There is an equivalent file under /siem in this PR
@@ -11,3 +11,5 @@ export { IngestManagerStart } from './plugin'; | |||
export const plugin = (initializerContext: PluginInitializerContext) => { | |||
return new IngestManagerPlugin(initializerContext); | |||
}; | |||
|
|||
export { CustomConfigureDatasourceContent } from './applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource'; |
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.
Why is this exported out of Ingest?
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.
CustomConfigureDatasourceContent
is a type which Endpoint (and other consumers) should implement to ensure that the custom content will be rendered correctly by Ingest Manager. I believe @parkiino pulled this from my POC, I elaborate on this here: https://github.com/elastic/endpoint-app-team/issues/443#issuecomment-635021439
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.
ahh. Got it. Thank @jen-huang
I had been importing types from ...ingest_manager/public/common
so I was perhaps expecting it to come from there.
x-pack/plugins/siem/public/index.ts
Outdated
@@ -11,3 +11,5 @@ import { PluginSetup, PluginStart } from './types'; | |||
export const plugin = (context: PluginInitializerContext): Plugin => new Plugin(context); | |||
|
|||
export { Plugin, PluginSetup, PluginStart }; | |||
|
|||
export { ConfigureEndpointDatasource } from './management/pages/policy/view/ingest_manager_integration/configure_datasource'; |
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.
We should add JSDocs to this, since its a plugin level export.
import { EuiEmptyPrompt, EuiText } from '@elastic/eui'; | ||
import { useKibana } from '../../../../../../../../../src/plugins/kibana_react/public'; | ||
import { LinkToApp } from '../../../../../common/components/endpoint/link_to_app'; | ||
import { CustomConfigureDatasourceContent } from '../../../../../../../ingest_manager/public'; |
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.
Hmmm... 🤔 this was weird to me too. I will read up on what @oatkiller found with above link to understand better.
export const ConfigureEndpointDatasource = memo<CustomConfigureDatasourceContent>( | ||
({ from }: { from: string }) => { | ||
const { services } = useKibana(); | ||
const pathname = useLocation().pathname.split('/'); |
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.
I don't suggest us using custom logic to parse out the URL. I suggest that this component be given the datasourceId
as an input prop, and then drive the UI below based on that. It may also address my earlier comment about moving code from Ingest here.
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.
👍
my comments were minor - I'm ok with this merging.
@@ -43,6 +43,9 @@ export interface DatasourceInput extends Omit<NewDatasourceInput, 'streams'> { | |||
streams: DatasourceInputStream[]; | |||
} | |||
|
|||
/** | |||
* Type of `datasource` prop in CustomConfigureDatasourceContent |
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.
Not sure about this comment.
NewDatasource
is the interface needed to supplement the create API for datasource and I think is reused on the server as well - so its not only a client side UI type.
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.
ok i might just take that comment out
packageInfo: PackageInfo; | ||
datasource: NewDatasource; | ||
datasource: NewDatasource | (NewDatasource & { id: string }); |
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.
maybe? 😬
If also export the CustomConfigureDatasourceProps
, then you can:
datasource: NewDatasource | (NewDatasource & { id: string }); | |
datasource: CustomConfigureDatasourceProps['datasource']; |
@@ -69,7 +69,8 @@ export const EditDatasourcePage: React.FunctionComponent = () => { | |||
const [loadingError, setLoadingError] = useState<Error>(); | |||
const [agentConfig, setAgentConfig] = useState<AgentConfig>(); | |||
const [packageInfo, setPackageInfo] = useState<PackageInfo>(); | |||
const [datasource, setDatasource] = useState<NewDatasource>({ | |||
const [datasource, setDatasource] = useState<NewDatasource & { id: string }>({ |
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.
Same here for the type, if you like 😃
@@ -11,3 +11,5 @@ export { IngestManagerStart } from './plugin'; | |||
export const plugin = (initializerContext: PluginInitializerContext) => { | |||
return new IngestManagerPlugin(initializerContext); | |||
}; | |||
|
|||
export { CustomConfigureDatasourceContent } from './applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource'; |
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.
ahh. Got it. Thank @jen-huang
I had been importing types from ...ingest_manager/public/common
so I was perhaps expecting it to come from there.
x-pack/plugins/siem/public/index.ts
Outdated
@@ -11,3 +11,9 @@ import { PluginSetup, PluginStart } from './types'; | |||
export const plugin = (context: PluginInitializerContext): Plugin => new Plugin(context); | |||
|
|||
export { Plugin, PluginSetup, PluginStart }; | |||
|
|||
/** |
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.
did you forget to move these comments to the actual component?
if (from === 'edit') { | ||
policyUrl = getManagementUrl({ | ||
name: 'policyDetails', | ||
policyId: (datasource as DatasourceWithId).id, |
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.
is this cast safe? or should you be checking if datasource has an id? (just asking, not familiar w/ the code.)
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.
@oatkiller The cast is safe and necessary since tsserver doesn't realize that datasource does have an id if it's coming from the edit
flow. the edit flow always sends in the id of the datasource
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.
Sorry this didn't submit earlier. I'm back at my computer now and will look into it more deeply
import React from 'react'; | ||
import { FormattedMessage } from '@kbn/i18n/react'; | ||
import { EuiEmptyPrompt, EuiText } from '@elastic/eui'; | ||
import { ConfigureEndpointDatasource } from '../../../../../../../../siem/public'; |
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.
Doesn't this create a circular dependency? Ingest now depends on SIEM which already depends on Ingest.
I'm away from my computer for a bit so I can't dig in, but can Ingest definite/export this instead?
Did a quick test, but have to step away for evening routine. How about something like this? The Ingest Manager will export a Like I said, I didn't get to run this through in the UI but the TS types work in the editor and I think the flow works. If so, Ingest no longer knows anything about siem and the cross dependency would be resolved. 🤞 Take a look and let me know diff --git a/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource.tsx b/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource.tsx
index 0c350b407e8..dc0d7693eea 100644
--- a/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource.tsx
+++ b/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource.tsx
@@ -6,7 +6,6 @@
import React from 'react';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiEmptyPrompt, EuiText } from '@elastic/eui';
-import { ConfigureEndpointDatasource } from '../../../../../../../../siem/public';
import { NewDatasource } from '../../../../types';
import { CreateDatasourceFrom } from '../types';
@@ -22,9 +21,17 @@ export interface CustomConfigureDatasourceProps {
*/
export type CustomConfigureDatasourceContent = React.FC<CustomConfigureDatasourceProps>;
-const ConfigureDatasourceMapping: { [key: string]: CustomConfigureDatasourceContent } = {
- endpoint: ConfigureEndpointDatasource,
-};
+type AllowedDatasourceKey = 'endpoint';
+const ConfigureDatasourceMapping: {
+ [key: string]: CustomConfigureDatasourceContent; // I don't know why AllowedDatasourceKey isn't a valid type
+} = {};
+
+export function registerDatasource(
+ key: AllowedDatasourceKey,
+ value: CustomConfigureDatasourceContent
+) {
+ ConfigureDatasourceMapping[key] = value;
+}
const EmptyConfigureDatasource: CustomConfigureDatasourceContent = () => (
<EuiEmptyPrompt
diff --git a/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/index.ts b/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/index.ts
index 42848cc0f5e..b2e004dc41c 100644
--- a/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/index.ts
+++ b/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/create_datasource_page/components/index.ts
@@ -6,4 +6,4 @@
export { CreateDatasourcePageLayout } from './layout';
export { DatasourceInputPanel } from './datasource_input_panel';
export { DatasourceInputVarField } from './datasource_input_var_field';
-export { CustomConfigureDatasource } from './custom_configure_datasource';
+export { CustomConfigureDatasource, registerDatasource } from './custom_configure_datasource';
diff --git a/x-pack/plugins/ingest_manager/public/index.ts b/x-pack/plugins/ingest_manager/public/index.ts
index 10a7eff30fa..0fbcbb43967 100644
--- a/x-pack/plugins/ingest_manager/public/index.ts
+++ b/x-pack/plugins/ingest_manager/public/index.ts
@@ -12,6 +12,9 @@ export const plugin = (initializerContext: PluginInitializerContext) => {
return new IngestManagerPlugin(initializerContext);
};
-export { CustomConfigureDatasourceContent } from './applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource';
+export {
+ CustomConfigureDatasourceContent,
+ registerDatasource,
+} from './applications/ingest_manager/sections/agent_config/create_datasource_page/components/custom_configure_datasource';
export { NewDatasource } from './applications/ingest_manager/types'; |
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.
I don't think the current import/export from Ingest and SIEM create a circular dependency as the import of the Endpoint component from Ingest is just a static file import. I like @jfsiii's approach to of exposing registration from Ingest's /public
(and I agree that exposing from lifecycle method would be even better).
Giving this a 👍 as the code between plugins is separated and we can iterate on the custom component registration logic from here.
I also like the approach detailed by @jfsiii in exposing these methods via the Plugin's lifecycle - I think it fits even more into the overall architecture. As @jen-huang mentioned, I'm also ok with getting this in and opening up an issue to iterate over its implementation. |
@@ -131,6 +132,7 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S | |||
|
|||
public start(core: CoreStart, plugins: StartPlugins) { | |||
KibanaServices.init({ ...core, ...plugins, kibanaVersion: this.kibanaVersion }); | |||
plugins.ingestManager.registerDatasource('endpoint', ConfigureEndpointDatasource); |
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.
this is cool
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Custom endpoint Datasource configuration in ingest Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Jen Huang <[email protected]>
Custom endpoint Datasource configuration in ingest Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Jen Huang <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Jen Huang <[email protected]>
…ms-column * 'master' of github.com:elastic/kibana: (63 commits) remove scripts. prettire update has been done (elastic#68130) Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198) [apm] docs: add deployment annotation example (elastic#67408) [ML] Extend population preview chart to show actual and typical value (elastic#67569) Refactor index management client integration tests for scalability (elastic#67917) Add generator function that creates multiple alerts (elastic#67713) chore(NA): remove config arg from os packages (elastic#67871) [Reporting] Move code out of Legacy (elastic#67904) [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125) [Security] [Cases] Manage timeline UI API (elastic#67719) [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234) Fix code coverage for jest, upload merged reports (elastic#68149) Update documentation/examples of deprecated namespaceAgnostic field (elastic#68039) [DOCS] Updates Canvas docs with new menus (elastic#66061) chore(NA): avoids imports of server or public code into common (elastic#67231) [SIEM] Fix GetOneTimeline graphql type (elastic#68137) skip flaky suite (elastic#67838) [Uptime] Add loading message for monitor list no items (elastic#67378) [Ingest Manager] Update indexing strategy docs to use dataset.* (elastic#68068) [Ingest Manager] Fix datasource validation for streams without vars (elastic#67950) ... # Conflicts: # x-pack/plugins/index_management/__jest__/client_integration/helpers/index.ts # x-pack/plugins/index_management/__jest__/client_integration/home.test.ts # x-pack/plugins/index_management/__jest__/client_integration/home/index_templates_tab.helpers.ts
* master: (26 commits) [Console]remove completion for type for filter queries and aggs (elastic#68103) [ML] Transforms: Filter aggregation support (elastic#67591) [ES UI Shared] Monaco XJSON (elastic#67485) [Index Management] Add data streams functionality to indices tab (elastic#67940) [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577) [SECURITY] Rename siem plugin to security_solution (elastic#67902) [Uptime] Fix Telemetry Api flaky test (elastic#67358) [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847) remove scripts. prettire update has been done (elastic#68130) Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198) [apm] docs: add deployment annotation example (elastic#67408) [ML] Extend population preview chart to show actual and typical value (elastic#67569) Refactor index management client integration tests for scalability (elastic#67917) Add generator function that creates multiple alerts (elastic#67713) chore(NA): remove config arg from os packages (elastic#67871) [Reporting] Move code out of Legacy (elastic#67904) [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125) [Security] [Cases] Manage timeline UI API (elastic#67719) [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234) Fix code coverage for jest, upload merged reports (elastic#68149) ...
* master: (22 commits) Partial revert of "Sync Kerberos + Anonymous access tests with the latest `security/_authenticate` API (user roles now include roles of anonymous user)." (elastic#68624) adapt some snapshot test (elastic#68489) [APM] Service maps - Fix missing ML status for services with jobs but no anomalies (elastic#68486) [skip test] apis Kerberos security Kerberos authentication finishing SPNEGO should properly set cookie and authenticate user [SIEM][Exceptions] - ExceptionsViewer UI component part 2 (elastic#68294) Surface data streams in Index Management. (elastic#67806) Fix edit datasource not working following changes in elastic#67234 (elastic#68583) [Logs + Metrics UI] Clean up async plugin initialization (elastic#67654) APM Storybook fixes (elastic#68671) Upgrade EUI to v24.1.0 (elastic#68141) [ML] DF Analytics: Creation wizard part 2 (elastic#68462) [Uptime] Fix race on overview page query (elastic#67843) Prefer using npm_execpath when spawning Yarn (elastic#68673) [Security] [Cases] Attach timeline to existing case (elastic#68580) Use Search API in Vega (elastic#68257) [Component templates] Table view (elastic#68031) [Uptime] Added relative date info in cert status column (elastic#67612) [Endpoint] Re-enable Functional test case for Endpoint related pages (elastic#68445) run page_load_metrics tests in visual regresssion jobs (elastic#68570) Enable exhaustive-deps; correct any lint warnings (elastic#68453) ...
Summary
Issues:
https://github.com/elastic/endpoint-app-team/issues/400
https://github.com/elastic/endpoint-app-team/issues/382
Checklist
Delete any items that are not applicable to this PR.
For maintainers