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

[RAC] Disable RAC multi-tenancy #108506

Merged
merged 5 commits into from
Aug 16, 2021

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Aug 13, 2021

Summary

Implements #108393.

Customisation of the index used by the rule registry is no longer allowed. User's with kibana.index set will, by default, have no rule data written. They can however opt into an unsafe experience using xpack.ruleRegistry.unsafe.legacyMultiTenancy.enabled.

(Please see the ticket for more details).

Testing

Make sure you have a user with the ability to create indices for the later steps, when you set a custom kibana.index

The expectation is still that xpack.ruleRegistry.write.enabled has been set to true

Flows

Flow: User without a custom kibana.index
Expectation: Data is written and viewable as normal

Flow: User has a custom kibana.index set
Expectation: Data isn't written

Flow: User has a custom kibana.index set and xpack.ruleRegistry.unsafe.legacyMultiTenancy.enabled set to true
Expectation: Data is written and viewable

UI disabling

Right now the alerts UI and table isn't disabled (this won't break anything as there will be no data to query). We don't have access to kibana.index easily on the client side as we do on the server side. So we need to share information from the server side of the rule registry plugin, with the client side of the observability plugin. As it's server -> client we can't do a simple contract access. We could setup an API route. (Maybe I've missed an option 🤔). Jason and I discussed this, and it might be okay that the table displays, but just doesn't contain any data.

@Kerry350 Kerry350 self-assigned this Aug 13, 2021
@Kerry350 Kerry350 marked this pull request as ready for review August 13, 2021 12:43
@Kerry350 Kerry350 added release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0 labels Aug 13, 2021
@jasonrhodes
Copy link
Member

Jason and I discussed this, and it might be okay that the table displays, but just doesn't contain any data.

Yeah, I think this is absolutely acceptable and let's prioritize other work over figuring that out. Thanks!

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for walking me through the functionality!

@Kerry350 Kerry350 requested a review from a team as a code owner August 13, 2021 15:27
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -13,8 +13,14 @@ export const config = {
write: schema.object({
enabled: schema.boolean({ defaultValue: false }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we change xpack.ruleRegistry.write.enabled to true by default now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll happen as part of #105237 and the "final wrap up" 👍

Comment on lines +94 to +101
if (!hasEnabledWrite) return false;

// Not using legacy multi-tenancy
if (!hasSetCustomKibanaIndex) {
return hasEnabledWrite;
} else {
return hasSetUnsafeAccess;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe this could be easier to grasp (totally subjective)

if (hasEnabledWrite) {
  return hasSetCustomKibanaIndex ? hasSetUnsafeAccess : true;
}

return false;

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
ruleRegistry 114 115 +1

API count missing comments

id before after diff
ruleRegistry 94 95 +1

History

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

cc @Kerry350

Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

LGTM

@Kerry350 Kerry350 added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 16, 2021
@Kerry350 Kerry350 merged commit 85e0766 into elastic:master Aug 16, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 16, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 16, 2021
* Disable RAC multi-tenancy

Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Observability RAC Feature:RAC label obsolete release_note:skip Skip the PR/issue when compiling release notes Theme: rac label obsolete v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants