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

[Reporting] Use the deprecations service to advise reporting_user config changes #100427

Merged
merged 2 commits into from
May 27, 2021

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 20, 2021

Resolves #92485

Follows: #94966

Summary

The reporting_user role is a legacy solution for granting Reporting feature privilege. In 8.0, the role should no longer be automatically created, and users who have that role should not automatically be granted privilege to generate reports in Kibana. Applications will instead use sub-feature control checks to enable Reporting for privileged users.

This PR uses the deprecations service (#94845) provided by core to register this deprecation. All registered deprecations will be displayed in the Upgrade Assistant (to be implemented via #97159).

Screenshots

Call the API with default (deprecated) config and a reporting_user enabled user:
image

Call the API with the recommended config change but still with a reporting_user enabled user:
image

Calling the API with the recommended config change and no reporting_user enabled users will not return any deprecations.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

`starting in 8.0. Please set 'xpack.reporting.roles.enabled' to 'false' and grant reporting privilege to users ` +
`through feature controls in Management > Security > Roles`,
`starting in 8.0. Please set 'xpack.reporting.roles.enabled' to 'false' and grant reporting privileges to users ` +
`using Kibana application privileges **Management > Security > Roles**.`,
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this text to match the new text, which felt more refined.

@tsullivan tsullivan changed the title [Reporting] Use the deprecations service to advise upgradable config changes [Reporting] Use the deprecations service to advise critical config changes May 20, 2021
@tsullivan tsullivan force-pushed the reporting/user-role-deprecation branch from 226ae02 to b0c2504 Compare May 20, 2021 21:45
@@ -69,19 +69,17 @@ export class ReportingCore {
private config?: ReportingConfig; // final config, includes dynamic values based on OS type
private executing: Set<string>;

public getStartContract: () => ReportingStart;
public getContract: () => ReportingSetup;
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed because we can call this from setup as well - ReportingStart and ReportingSetup are the same interface.

@@ -65,6 +64,8 @@ export class ReportingPlugin
logger: this.logger,
});

registerUiSettings(core);
Copy link
Member Author

Choose a reason for hiding this comment

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

put all the register* together for cosmetic purposes

@tsullivan tsullivan force-pushed the reporting/user-role-deprecation branch from c76d3dd to deac77e Compare May 20, 2021 22:53
@tsullivan tsullivan added v7.14.0 v8.0.0 reason:enhancement release_note:skip Skip the PR/issue when compiling release notes labels May 20, 2021
@tsullivan tsullivan marked this pull request as ready for review May 20, 2021 22:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan force-pushed the reporting/user-role-deprecation branch from deac77e to d31e5f2 Compare May 20, 2021 23:17
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Code changes LGTM also tested this locally, great job @tsullivan 🍻

@@ -0,0 +1,52 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I am going to be adding a deprecation action soon for migrating existing reporting indices to the new ILM policy - as part of that I'll probably create a depreactions folder and place this inside a file like reporting_user_role.ts. Nothing to action now, we can take the discussion further on that PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me, as long as we have correct spelling for deprecations 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed 😄

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Code LGTM. Did not test locally.

Great to see a team start to use the deprecations service 🎉 . Feel free to let me or the core team know if you have any feedback on it!

correctiveActions: {
manualSteps: [
...(usingDeprecatedConfig ? [`Set "${upgradableConfig}" in kibana.yml`] : []),
`Create one or more custom roles that provide Kibana application privileges to reporting features in **Management > Security > Roles**.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the current position on translations in deprecation manual steps or was the accidentally left out?

Copy link
Member Author

Choose a reason for hiding this comment

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

While working on this, I referenced this Draft PR to add an FAQ: https://github.com/elastic/kibana/pull/99043/files#r636460961

I asked a similar question, and found that it's currently not recommended to use i18n for deprecations: #99072

Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

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

LGTM. Great job 👍

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 386 342 -44
stackAlerts 101 95 -6
total -342

History

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

@tsullivan tsullivan merged commit 417c06b into elastic:master May 27, 2021
@tsullivan tsullivan deleted the reporting/user-role-deprecation branch May 27, 2021 00:31
tsullivan added a commit to tsullivan/kibana that referenced this pull request May 27, 2021
…anges (elastic#100427)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/reporting/server/core.ts
#	x-pack/plugins/reporting/server/plugin.ts
tsullivan added a commit that referenced this pull request May 27, 2021
…anges (#100427) (#100750)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/reporting/server/core.ts
#	x-pack/plugins/reporting/server/plugin.ts
@@ -45,7 +45,7 @@ describe('deprecations', () => {
const { messages } = applyReportingDeprecations({ roles: { enabled: true } });
expect(messages).toMatchInlineSnapshot(`
Array [
"\\"xpack.reporting.roles\\" is deprecated. Granting reporting privilege through a \\"reporting_user\\" role will not be supported starting in 8.0. Please set 'xpack.reporting.roles.enabled' to 'false' and grant reporting privilege to users through feature controls in Management > Security > Roles",
"\\"xpack.reporting.roles\\" is deprecated. Granting reporting privilege through a \\"reporting_user\\" role will not be supported starting in 8.0. Please set 'xpack.reporting.roles.enabled' to 'false' and grant reporting privileges to users using Kibana application privileges **Management > Security > Roles**.",
Copy link
Member Author

@tsullivan tsullivan Sep 23, 2021

Choose a reason for hiding this comment

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

I think we don't have the authority to remove support for the reporting_user role in 8.0. The ability to switch off the support is still very new in Kibana and we've seen plenty of cases where configuring things properly stumps users. This text should be changed to say that the support will be removed "in a future version"

Edit: filed #113014

@sophiec20 sophiec20 added the enhancement New value added to drive a business result label Mar 27, 2024
@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
@tsullivan tsullivan changed the title [Reporting] Use the deprecations service to advise critical config changes [Reporting] Use the deprecations service to advise reporting_user config changes Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Breaking change] Reporting: remove reporting_user role
7 participants