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

Migrate defaultIndex attribute for config saved object #133339

Merged

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Jun 1, 2022

Resolves #133241

Overview

This PR adds a new mechanism to apply custom transform functions to the config saved object for UI settings. These transform functions are complementary to saved object migrations.

The config saved objects are versioned and isolated to each space.
When a client attempts to fetch or create a config saved object, Kibana first checks if there's already an old one that needs to be upgraded. This PR takes advantage of the existing functionality to simply call any necessary transform functions during that process. This way, we can ensure that transforms will be applied to each config after upgrading, but we don't need to add a complicated "on start" loop to search and update saved objects.

Testing

To manually test this end-to-end to verify the migration is applied:

  1. Load the test data and scripts that are attached to Add enhancements for legacy URL aliases #125960
    • This will create a bunch of test data, but we are primarily interested in the index pattern ff959d40-b880-11e8-a6d9-e546fe2bba5f in the Default space, along with a legacy URL alias redirect-with-toast that points to the aforementioned ID
  2. Log in with superduperuser/changeme which was created by the script and has access to modify system indices
  3. Using Dev Tools, create an old config object that will be upgraded, which uses the legacy URL alias ID:
    POST .kibana/_doc/config:8.2.0
    {
      "config": {
        "buildNum": 123,
        "defaultIndex": "redirect-with-toast"
      },
      "type": "config",
      "references": [],
      "migrationVersion": {
        "config": "8.1.0"
      },
      "coreMigrationVersion": "8.2.0",
      "updated_at": "2022-06-07T13:21:34.759Z"
    }
    
  4. Using Dev Tools, delete the existing config object (for 8.4.0):
    DELETE .kibana/_doc/config:8.4.0
    
  5. Refresh the page to fetch the config object again, triggering the upgrade
  6. Using Dev Tools, search for the config objects and verify that it was upgraded correctly:
    GET .kibana/_search
    {
      "query": {
        "term": {
          "type": {
            "value": "config"
          }
        }
      }
    }
    
    The result should look like this:
    "hits": [
      {
        "_index": ".kibana_8.4.0_001",
        "_id": "config:8.2.0",
        "_score": 3.6888795,
        "_source": {
          "config": {
            "buildNum": 123,
            "defaultIndex": "redirect-with-toast"
          },
          "type": "config",
          "references": [],
          "migrationVersion": {
            "config": "8.1.0"
          },
          "coreMigrationVersion": "8.4.0",
          "updated_at": "2022-06-07T13:21:34.759Z"
        }
      },
      {
        "_index": ".kibana_8.4.0_001",
        "_id": "config:8.4.0",
        "_score": 3.6888795,
        "_source": {
          "config": {
            "buildNum": 9007199254740991,
            "isDefaultIndexMigrated": true,
            "defaultIndex": "ff959d40-b880-11e8-a6d9-e546fe2bba5f"
          },
          "type": "config",
          "references": [],
          "migrationVersion": {
            "config": "8.1.0"
          },
          "coreMigrationVersion": "8.4.0",
          "updated_at": "2022-06-07T13:32:22.612Z"
        }
      }
    ]

Release note: The advanced setting for defaultIndex could be broken (not pointing to the correct index pattern) if it was set in a custom space before upgrading to the 8.0 release. The setting is now migrated correctly the first time that space is accessed by any user that has All access to the Advanced Settings feature.

@jportner jportner added release_note:fix auto-backport Deprecated - use backport:version if exact versions are needed v8.4.0 v8.3.1 labels Jun 1, 2022
@jportner jportner changed the title Upgrade defaultIndex attribute for config saved object Migrate defaultIndex attribute for config saved object Jun 1, 2022
) {
defaultIndex = upgradeableConfig.attributes.defaultIndex; // Keep the existing defaultIndex attribute if the data view is not found
try {
// The defaultIndex for this config object was created prior to 8.3, and it might refer to a data view ID that is no longer valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

I love all the code comments but it begs the question of "Why do we need to explain what we're doing?". The whole config/settings situation needs an overhaul to avoid similar situations. For now, though, could we also add this information to the dev docs, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea.
Looking at the docs I found a page for Advanced Settings, which mentions each setting and what it is for.
https://www.elastic.co/guide/en/kibana/8.3/advanced-options.html#defaultindex

Do you think I should add a "NOTE" admonition there? Or did you have something else in mind?

Copy link
Contributor

@TinaHeiligers TinaHeiligers Jun 2, 2022

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 need to concern end-users with the internals of what we're doing here, it's more for internal developer guidance.
I was thinking about adding a note to the tutorial on how to register a uiSetting migration. The file itself lives here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jportner jportner marked this pull request as ready for review June 3, 2022 17:24
@jportner jportner requested a review from a team as a code owner June 3, 2022 17:24
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Haven't gotten a chance to test locally yet, but the approach here makes sense overall

@jportner jportner added v8.3.0 and removed v8.3.1 labels Jun 6, 2022
src/core/server/ui_settings/saved_objects/transforms.ts Outdated Show resolved Hide resolved
src/core/server/ui_settings/saved_objects/transforms.ts Outdated Show resolved Hide resolved
src/core/server/ui_settings/saved_objects/transforms.ts Outdated Show resolved Hide resolved
* The `config` object type contains many attributes that are defined by consumers.
*/
export interface ConfigAttributes {
buildNum: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: should we add isDefaultIndexMigrated here too?

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 so, this was to address Luke's feedback in #133339 (comment), we have a separate UpgradeableConfigAttributes type that extends ConfigAttributes and specifies other fields such as isDefaultIndexMigrated.

@jportner jportner requested a review from pgayvallet June 7, 2022 14:06
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this

@jportner jportner enabled auto-merge (squash) June 7, 2022 14:15
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all of the edits!

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.

LGTM too!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@jportner jportner merged commit d732ebe into elastic:main Jun 7, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.3 Backport failed because of merge conflicts
8.2 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 133339

Questions ?

Please refer to the Backport tool documentation

jportner added a commit to jportner/kibana that referenced this pull request Jun 7, 2022
(cherry picked from commit d732ebe)

# Conflicts:
#	src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
@jportner jportner deleted the issue-133241-add-defaultIndex-migration branch June 7, 2022 16:30
jportner added a commit to jportner/kibana that referenced this pull request Jun 7, 2022
(cherry picked from commit d732ebe)

# Conflicts:
#	src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
jportner added a commit that referenced this pull request Jun 7, 2022
…3805)

(cherry picked from commit d732ebe)

# Conflicts:
#	src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
jportner added a commit that referenced this pull request Jun 7, 2022
…3807)

(cherry picked from commit d732ebe)

# Conflicts:
#	src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
@KOTungseth KOTungseth added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Jun 8, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@jportner jportner added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Jun 8, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

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 release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.2.3 v8.3.0 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The defaultIndex advanced setting breaks after 8.0 upgrade
8 participants