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

[Search] Search Playground - shared rendering #201302

Conversation

TattdCodeMonkey
Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey commented Nov 21, 2024

Summary

This PR removes Search Playground rendering from enterprise_search in favor of search_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 for searchPlayground so that it can be controlled with RBAC independently from the entire search feature set.

Checklist

@TattdCodeMonkey TattdCodeMonkey added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Team:Search backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.18.0 labels Nov 21, 2024
@TattdCodeMonkey
Copy link
Contributor Author

/ci

Copy link
Contributor

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

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.

@TattdCodeMonkey TattdCodeMonkey force-pushed the search/search_playground-stack-rendering branch from 17de46d to 848d374 Compare November 30, 2024 17:09
@TattdCodeMonkey TattdCodeMonkey force-pushed the search/search_playground-stack-rendering branch from 848d374 to 32f1dff Compare December 2, 2024 20:32
@TattdCodeMonkey TattdCodeMonkey marked this pull request as ready for review December 2, 2024 21:58
@TattdCodeMonkey TattdCodeMonkey requested review from a team as code owners December 2, 2024 21:58
Comment on lines 62 to 81
privileges: {
all: {
app: ['kibana', PLUGIN_ID],
api: [],
catalogue: [PLUGIN_ID],
savedObject: {
all: [],
read: [],
},
ui: [],
},
read: {
disabled: true,
savedObject: {
all: [],
read: [],
},
ui: [],
},
},
Copy link
Member

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:

  1. Perform actions in ES via the internal kibana_system user?
  2. Communicate with an LLM or other third-party service?
  3. Perform computationally expensive operations on the Kibana server?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legrego I added API to the feature in 7e109f0, I think I got that right and will test it further after I finish adding min license to the feature as well which I just realized should also be included.

Copy link
Contributor Author

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.

Copy link
Contributor

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!

@TattdCodeMonkey
Copy link
Contributor Author

Once this gets a clean CI run I will test this in a QA serverless project just to ensure everything is working as expected.

@TattdCodeMonkey
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

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.

see run history

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a 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

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 3, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 3c78793
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-201302-3c787935cb72

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 2299 2296 -3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/deeplinks-search 21 22 +1
searchNavigation 21 22 +1
searchPlayground 16 13 -3
total -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 2.6MB 2.6MB -1.8KB
searchPlayground 169.9KB 164.1KB -5.8KB
total -7.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
searchNavigation 0 1 +1
searchPlayground 1 0 -1
total -0

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
enterpriseSearch 54.6KB 53.9KB -636.0B
searchNavigation 4.1KB 5.0KB +954.0B
searchPlayground 7.2KB 7.0KB -226.0B
total +92.0B
Unknown metric groups

API count

id before after diff
@kbn/deeplinks-search 21 22 +1
searchNavigation 21 22 +1
searchPlayground 22 13 -9
total -7

async chunk count

id before after diff
searchPlayground 6 2 -4

ESLint disabled line counts

id before after diff
searchPlayground 4 5 +1

Total ESLint disabled count

id before after diff
searchPlayground 4 5 +1

History

Copy link
Member

@joemcelroy joemcelroy left a 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

@TattdCodeMonkey
Copy link
Contributor Author

Tested this in QA for serverless and ran the FTR tests for MKI and everything seems ✅

Copy link
Contributor

@jeramysoucy jeramysoucy left a 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.

@TattdCodeMonkey
Copy link
Contributor Author

@jeramysoucy there are currently not any API integration tests for playground 😬

@jeramysoucy
Copy link
Contributor

@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?

@joemcelroy
Copy link
Member

@jeramysoucy created an issue for API coverage and includes checking authorization. https://github.com/elastic/search-team/issues/8889

@TattdCodeMonkey TattdCodeMonkey merged commit 434eaa7 into elastic:main Dec 5, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12188102874

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 201302

Questions ?

Please refer to the Backport tool documentation

@TattdCodeMonkey
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

TattdCodeMonkey added a commit to TattdCodeMonkey/kibana that referenced this pull request Dec 6, 2024
(cherry picked from commit 434eaa7)

# Conflicts:
#	packages/deeplinks/search/constants.ts
#	packages/deeplinks/search/deep_links.ts
TattdCodeMonkey added a commit that referenced this pull request Dec 6, 2024
# 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":"[email protected]"},"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-->
markov00 pushed a commit to markov00/kibana that referenced this pull request Dec 7, 2024
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Dec 9, 2024
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Dec 9, 2024
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 9, 2024
Samiul-TheSoccerFan pushed a commit to Samiul-TheSoccerFan/kibana that referenced this pull request Dec 10, 2024
mykolaharmash pushed a commit to mykolaharmash/kibana that referenced this pull request Dec 11, 2024
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Search v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants