Skip to content

Move kibana-keystore from data/ to config/#57856

Merged
jbudz merged 28 commits intoelastic:masterfrom
jbudz:mv/kibana-keystore
Jul 13, 2020
Merged

Move kibana-keystore from data/ to config/#57856
jbudz merged 28 commits intoelastic:masterfrom
jbudz:mv/kibana-keystore

Conversation

@jbudz
Copy link
Contributor

@jbudz jbudz commented Feb 18, 2020

This is a breaking change to move the location of kibana-keystore.
Keystores in other stack products live in the config directory, so this
updates our current path to be consistent.

Closes #25746

This doesn't respect kibana.yml currently, and I would like to push that for the next iteration. The config service currently depends on the server to be running, while the keystore does not start the server. A portable config service is ideal, but it's fairly coupled. The next steps would include

  1. build time kibana.yml with separate paths for deb/rpm and archives
  2. find a way to fetch config values without a running server

This is a breaking change to move the location of kibana-keystore.
Keystores in other stack products live in the config directory, so this
updates our current path to be consistent.

Closes elastic#25746
@jbudz jbudz requested a review from a team as a code owner February 18, 2020 15:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@tylersmalley
Copy link
Member

Mind adding this to the breaking changes markdown as part of this PR?

@joshdover
Copy link
Contributor

joshdover commented Feb 18, 2020

find a way to fetch config values without a running server

This is something @spalger has ran into recently as well. I think it makes sense to extract our config logic into a standalone package. Is there any issue for this?

Is there any reason this can't be done in a BWC way on the 7.x branch as well? It'd be nice to fallback to the old location and log a warning.

@tylersmalley
Copy link
Member

Does this also resolve #31171?

@jbudz
Copy link
Contributor Author

jbudz commented Mar 19, 2020

@elasticmachine merge upstream

@jbudz
Copy link
Contributor Author

jbudz commented Mar 19, 2020

Added breaking changes docs. A new PR targeting 7.x will remove the docs changes and add a logged deprecation. This should be okay for standalone review, but I'll wait to merge until both are approved

@jbudz
Copy link
Contributor Author

jbudz commented Mar 19, 2020

@elasticmachine merge upstream

Comment on lines +61 to +66

/**
* Get the path where the config directories are stored
* @internal
*/
export const getConfigDirectory = () => findFile(CONFIG_DIRECTORIES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Misleading comment. This resolves to the actual config directory.

Outside of the PR, but the getConfigPath description is also wrong (Get the path where the config files are stored), as it resolves the path to the config file itself.

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 for platform changes

@tylersmalley
Copy link
Member

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@tylersmalley
Copy link
Member

@jbudz where are we at with this?

@ghost
Copy link

ghost commented May 5, 2020

Do I understand correctly that key-store file will be created in the same directory as kibana.yml?
I am the owner of https://github.com/elastic/elastic-stack-installers, currently working on Kibana Windows MSI package. I found that currently bundled kibana-keystore.bat doesn't respond to --config and --path.data args. This presents a problem for MSI based install, since on Windows I need to move all user-serviceable data to a different location away from main install path.

ref: elastic/elastic-stack-installers#60

@jbudz
Copy link
Contributor Author

jbudz commented May 5, 2020

Yep you got it. The file location won't be configurable, but it will respect the configuration directories path.

@ghost
Copy link

ghost commented May 5, 2020

The file location won't be configurable

🤔 "Won't" as in "never", or ... ? This will present a problem wrt UX dealing with keystore CLI on Windows.

@tylersmalley
Copy link
Member

@jbudz is this ready for another pass?

@jbudz
Copy link
Contributor Author

jbudz commented Jun 23, 2020

@elasticmachine merge upstream

@jbudz
Copy link
Contributor Author

jbudz commented Jun 23, 2020

Yep it is.

@jbudz
Copy link
Contributor Author

jbudz commented Jun 29, 2020

@elasticmachine merge upstream

Copy link
Member

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

Couple requests, but in testing I am getting an error when running scripts/kibana_keystore

 $ node scripts/kibana_keystore.js --help
/home/tyler/elastic/kibana/src/legacy/server/keystore/keystore.js:78
      throw e;
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be one of type string, Buffer, or URL. Received type undefined
    at Object.openSync (fs.js:435:3)
    at readFileSync (fs.js:343:35)
    at Keystore.load (/home/tyler/elastic/kibana/src/legacy/server/keystore/keystore.js:87:24)
    at new Keystore (/home/tyler/elastic/kibana/src/legacy/server/keystore/keystore.js:34:10)
    at Object.<anonymous> (/home/tyler/elastic/kibana/src/cli_keystore/cli_keystore.js:57:18)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Module._compile (/home/tyler/elastic/kibana/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object.newLoader [as .js] (/home/tyler/elastic/kibana/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:653:32)

.version(pkg.version)
.description('A tool for managing settings stored in the Kibana keystore');

function getKeystore() {
Copy link
Member

Choose a reason for hiding this comment

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

This function is pretty important - can we add test coverage there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skmizuho
Copy link

skmizuho commented Jul 2, 2020

Quick q on this, please. When it comes to upgrading from one's current version to version 8.0, will we need to copy the keystore from data to config directory to get it to work? How will the migration work?
Thank you

@tylersmalley
Copy link
Member

@skmizuho, in this PR we are first checking for the presence of the keystore in the data directory. Be sure that when we make changes, we will include this information in the breaking changes for 8.0 and plan to include a notice for it in the upgrade assistant.

@skmizuho
Copy link

skmizuho commented Jul 2, 2020

Thank you - understood. Big supported of this change by the way! It makes the configuration much simpler.
Cheers

@jbudz
Copy link
Contributor Author

jbudz commented Jul 7, 2020

Hmm - can you give another try? I did have a commit with that error at one point. Also pushed changes up for the other changes

@tylersmalley
Copy link
Member

@elasticmachine merge upstream

@jbudz
Copy link
Contributor Author

jbudz commented Jul 13, 2020

@elasticmachine merge upstream

@jbudz jbudz added the v7.9.0 label Jul 13, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@jbudz jbudz merged commit 94ef03d into elastic:master Jul 13, 2020
jbudz added a commit that referenced this pull request Jul 13, 2020
* Move kibana-keystore from data/ to config/

This is a breaking change to move the location of kibana-keystore.
Keystores in other stack products live in the config directory, so this
updates our current path to be consistent.

Closes #25746

* add breaking changes

* update comment

* wip

* fix docs

* read from both keystore locations, write priority to non-deprecated

* note data directory fallback

* add tests for get_keystore

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@jbudz
Copy link
Contributor Author

jbudz commented Jul 13, 2020

7.9/7.x: c9b6f8a

@gchaps
Copy link
Contributor

gchaps commented Aug 3, 2020

@jbudz This PR has the release_note:breaking label and therefore should be included in the Breaking Changes doc. Please add a description of the change to this PR that I can use in the doc. For an example, take a look at the 7.8 Breaking Changes doc.

jbudz added a commit to jbudz/kibana that referenced this pull request Sep 13, 2021
With elastic#57856 the default keystore
location was moved from the data folder to the config folder to align
with other applications in the stack.  This updates the secure settings
docs to reference the correct location.

Closes elastic#111915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move kibana.keystore to the configuration directory

8 participants