Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Security Solution] Discover Timeline Integration URL Sync + Fixes #163305
[Security Solution] Discover Timeline Integration URL Sync + Fixes #163305
Changes from 23 commits
e88eb3c
5e2322a
e6f85fe
7c8ab33
9a49dd0
7934b9b
eb4bd14
4b5ca55
c89c7c9
bf1e79b
d96de65
76ab819
9a35ea0
19798f2
94c2418
7049c10
842d5ea
95d4ee2
75d8e04
4a0ba9f
721d26c
db07cc0
fa18ca7
7f0e552
9142901
130f5a1
2fce8f0
7a8000b
ff085d5
8b66bf1
08b01bb
69ebb48
8300e46
dd23497
f59b1ec
828f5ab
456be46
fc171ed
76fd62f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 makes sense, embedded | standalone. What do you think about a 2nd prop, an appId to redirect if mode === 'embedded'
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 I understand this suggestion. Are you referring to redirecting requests to Discover to another app based on an
appId
prop?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, for determining path, if discover is being used at a different path in both security and observability serverless modes.
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.
guess that might not work with locators and the like, as those run during discover setup. Probably makes more sense to do something at a config level then? maybe a discover.location setting that we set only in serverless.obit.yml/serverless.security.yml?
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.
Does it make sense to add parameter in
discover.locator
called, appId where locator will develop the URL based on the appID given?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.
Observability currently doesn't use Discover at a different path or embedded in another app. They're using the
profile
functionality in the customization framework to customize Discover as a top level app, so their use case around URLs and paths is a bit different (and already supported).Yeah I agree, I don't think this would be possible through the container component.
I'd like for the Security redirecting logic not to live in the Discover codebase or be owned/maintained by the Data Discovery team, so I'd prefer not to take this approach. I'd be open to a YAML config that disables access to Discover as a top level app or similar, but owning and maintaining the redirect logic should be the responsibility of the Security Solution.
I'd also prefer to avoid this since it would introduce knowledge of the Security redirect logic directly into the Discover codebase.
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.
In light of today's Discover Working Stream, scratch this -- looks like Observability will be embedding the Discover container now (I just learned of this now). With that said, they seem to be planning to include the main Discover app in their project, so their use case around URLs and redirecting is still different since existing links will presumably continue to take users to the main Discover app.
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.
@davismcphee thanks for the info. What do you think if client can pass callback to locator to provide a client specific location. This will prevent any other plugin's logic in Discover?
cc: @kqualters-elastic
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.