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

Prevent Spaces from being disabled #115283

Merged

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Oct 15, 2021

Resolves #82467.

* Change config
* Remove related deprecations
* Remove space-disabled elements from role management UI
* Remove security_only functional tests
@jportner jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Oct 15, 2021
Copy link
Contributor Author

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Author's notes for reviewers.

After this merges, we'll probably need to rebalance the CI jobs since we're getting rid of so many integration tests 🎉

Comment on lines +518 to +521
FooAllAtEverythingSpace,
FooAllAtNothingSpace,
FooReadAtEverythingSpace,
FooReadAtNothingSpace,
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 added these scenarios because tests with the FooAll and FooRead users were present in the security_only test suite, but not in this security_and_spaces test suite.

Copy link
Member

Choose a reason for hiding this comment

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

🏅 Thanks for observing this and not just blindly deleting the other test suites. This was done intentionally back in the day in order to reduce unnecessary duplication between the test suites.

Comment on lines -424 to +503
export const UserAtSpaceScenarios: [
NoKibanaPrivilegesAtEverythingSpace,
NoKibanaPrivilegesAtNothingSpace,
SuperuserAtEverythingSpace,
SuperuserAtNothingSpace,
LegacyAllAtEverythingSpace,
LegacyAllAtNothingSpace,
DualPrivilegesAllAtEverythingSpace,
DualPrivilegesAllAtNothingSpace,
DualPrivilegesReadAtEverythingSpace,
DualPrivilegesReadAtNothingSpace,
GlobalAllAtEverythingSpace,
GlobalAllAtNothingSpace,
GlobalReadAtEverythingSpace,
GlobalReadAtNothingSpace,
EverythingSpaceAllAtEverythingSpace,
EverythingSpaceAllAtNothingSpace,
EverythingSpaceReadAtEverythingSpace,
EverythingSpaceReadAtNothingSpace,
NothingSpaceAllAtEverythingSpace,
NothingSpaceAllAtNothingSpace,
NothingSpaceReadAtEverythingSpace,
NothingSpaceReadAtNothingSpace
] = [
export const UserAtSpaceScenarios = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to specify the type like this when we use as const below 😉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
security 459 456 -3

Async chunks

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

id before after diff
security 514.6KB 507.8KB -6.8KB

Page load bundle

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

id before after diff
security 51.3KB 51.1KB -202.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner jportner marked this pull request as ready for review October 16, 2021 00:17
@jportner jportner requested review from a team as code owners October 16, 2021 00:17
@jportner jportner requested a review from legrego October 16, 2021 00:18
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

stack management changes lgtm 🚀

@jportner
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Looks great! Still testing locally, but I don't expect any complications there

@@ -7,11 +7,6 @@

By default, spaces is enabled in {kib}. To secure spaces, <<security-settings-kb,enable security>>.

`xpack.spaces.enabled`::
Copy link
Member

Choose a reason for hiding this comment

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

I can't leave a comment on an unedited file, but we should update our docs to reflect the fact that we can't disable Spaces anymore. My preference is to keep the section, but change the wording to indicate that this is no longer possible as of 8.0. That said, I'll be ok if you'd rather remove this altogether.

We should also remove the language about all objects existing in the Default space after upgrade. That was only really relevant for folks upgrading from pre-6.5 to >= 6.5, and I sincerely hope that everyone has done that by now. It's just confusing/misleading for everyone else at this point.

[float]
[[spaces-delete-started]]
=== Disable and version updates
Spaces are automatically enabled in {kib}. If you don't want use this feature,
you can disable it. For more information, refer to <<spaces-settings-kb,Spaces settings in {kib}>>.
When you upgrade {kib}, the default space contains all of your existing saved objects.

Copy link
Contributor Author

@jportner jportner Oct 18, 2021

Choose a reason for hiding this comment

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

Great catch, I'll change this section to:

Starting in {kib} 8.0, the Spaces feature cannot be disabled.

* 2.0.
*/

export { SimplePrivilegeSection } from './simple_privilege_section';
Copy link
Member

Choose a reason for hiding this comment

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

You have no idea how happy I am to see this SimplePrivilegeSection removed ❤️

Comment on lines +518 to +521
FooAllAtEverythingSpace,
FooAllAtNothingSpace,
FooReadAtEverythingSpace,
FooReadAtNothingSpace,
Copy link
Member

Choose a reason for hiding this comment

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

🏅 Thanks for observing this and not just blindly deleting the other test suites. This was done intentionally back in the day in order to reduce unnecessary duplication between the test suites.

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

kibana-docker LGTM

docs/spaces/index.asciidoc Outdated Show resolved Hide resolved
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM!

I think as a followup item (post FF) we should make the spaces plugin required within the security plugin, and remove the small amount of dead code we have on the server to account for Spaces being disabled.

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Cases changes LGTM

@jportner jportner enabled auto-merge (squash) October 18, 2021 13:37
@jportner jportner merged commit 0f1c7cc into elastic:master Oct 18, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
security 459 456 -3

Async chunks

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

id before after diff
security 514.6KB 507.8KB -6.8KB

Page load bundle

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

id before after diff
security 51.3KB 51.1KB -202.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner jportner deleted the issue-82467-prevent-disabling-spaces branch October 18, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting buildkite-ci release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Breaking change] Prevent disabling the spaces plugin
6 participants