-
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
[Search] Search Playground - shared rendering #201302
[Search] Search Playground - shared rendering #201302
Conversation
/ci |
A documentation preview will be available soon. Request a new doc build by commenting
If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here. |
17de46d
to
848d374
Compare
848d374
to
32f1dff
Compare
privileges: { | ||
all: { | ||
app: ['kibana', PLUGIN_ID], | ||
api: [], | ||
catalogue: [PLUGIN_ID], | ||
savedObject: { | ||
all: [], | ||
read: [], | ||
}, | ||
ui: [], | ||
}, | ||
read: { | ||
disabled: true, | ||
savedObject: { | ||
all: [], | ||
read: [], | ||
}, | ||
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.
This is a passing comment, I'll defer to an engineer from @elastic/kibana-security for the full review.
The PR states:
Additionally this PR introduces a KibanaFeature specifically for searchPlayground so that it can be controlled with RBAC independently from the entire search feature set.
But this privilege definition only controls access to the UI application, and does not protect any API endpoints or saved objects. Your feature may not use saved objects, so that might be ok (please confirm) -- but it is rare that a feature exists without both APIs and Saved Objects. Which Kibana endpoints does this feature require? Do any of them:
- Perform actions in ES via the internal
kibana_system
user? - Communicate with an LLM or other third-party service?
- Perform computationally expensive operations on the Kibana server?
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 we are using any saved objects. We do have some APIs. We have follow-up work to expand the feature from All | None
to All | Read | None
but that is outside the scope of this initial effort.
I can look into including the APIs, I was just doing the UI as the first pass as we're new to using features in this way.
cc: @joemcelroy
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 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.
Updated to use the security
config over the access
tag per dev docs.
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 adding route authorization! It looks like it is setup correctly. You may want to consider more granular access control for the APIs, or using a naming convention with more flexibility. That said, if a single privilege to grant access to all APIs works for this plugin then you should be all set!
Once this gets a clean CI run I will test this in a QA serverless project just to ensure everything is working as expected. |
@elasticmachine merge upstream |
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#7551[✅] x-pack/test_serverless/functional/test_suites/search/config.ts: 25/25 tests passed. |
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.
Changes to configs, manifests and usage collection LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
|
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.
Works well!
Tested on serverless and on stateful
Tested this in QA for serverless and ran the FTR tests for MKI and everything seems ✅ |
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.
Kibana Security changes LGTM. Just one thing, are there API integration tests for the search playground endpoints? These should get augmented to validate access/unauthorized based on user privileges.
@jeramysoucy there are currently not any API integration tests for playground 😬 |
🙈 Are there plans to add them in the near future? Could you open an issue for traceability, and include validating authorization in the description? |
@jeramysoucy created an issue for API coverage and includes checking authorization. https://github.com/elastic/search-team/issues/8889 |
Starting backport for target branches: 8.x |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit 434eaa7) # Conflicts: # packages/deeplinks/search/constants.ts # packages/deeplinks/search/deep_links.ts
# Backport This will backport the following commits from `main` to `8.x`: - [[Search] Search Playground - shared rendering (#201302)](#201302) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Rodney Norris","email":"rodney.norris@elastic.co"},"sourceCommit":{"committedDate":"2024-12-05T21:09:51Z","message":"[Search] Search Playground - shared rendering (#201302)","sha":"434eaa78ad7c045f52b2126cdae0f1d8fa7a00f6","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Search","backport:prev-minor","v8.18.0"],"number":201302,"url":"https://github.com/elastic/kibana/pull/201302","mergeCommit":{"message":"[Search] Search Playground - shared rendering (#201302)","sha":"434eaa78ad7c045f52b2126cdae0f1d8fa7a00f6"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201302","number":201302,"mergeCommit":{"message":"[Search] Search Playground - shared rendering (#201302)","sha":"434eaa78ad7c045f52b2126cdae0f1d8fa7a00f6"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
Summary
This PR removes Search Playground rendering from
enterprise_search
in favor ofsearch_playground
rendering the UI for both Serverless and Stack. Included in this are changes needed to support that shift. With this change the search playground UI is now enabled by default, and the search playground plugin explicitly disabled for other Serverless projects.Additionally this PR introduces a
KibanaFeature
specifically forsearchPlayground
so that it can be controlled with RBAC independently from the entire search feature set.Checklist
release_note:*
label is applied per the guidelines