-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Move kibana-keystore from data/ to config/ #57856
Conversation
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
Pinging @elastic/kibana-operations (Team:Operations) |
Mind adding this to the breaking changes markdown as part of this PR? |
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. |
Does this also resolve #31171? |
@elasticmachine merge upstream |
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 |
@elasticmachine merge upstream |
src/core/server/path/index.ts
Outdated
|
||
/** | ||
* Get the path where the config directories are stored | ||
* @internal | ||
*/ | ||
export const getConfigDirectory = () => findFile(CONFIG_DIRECTORIES); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
@elasticmachine merge upstream |
merge conflict between base and head |
@jbudz where are we at with this? |
Do I understand correctly that key-store file will be created in the same directory as |
@jbudz is this ready for another pass? |
@elasticmachine merge upstream |
Yep it is. |
@elasticmachine merge upstream |
There was a problem hiding this 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)
src/cli_keystore/cli_keystore.js
Outdated
@@ -42,6 +41,21 @@ program | |||
.version(pkg.version) | |||
.description('A tool for managing settings stored in the Kibana keystore'); | |||
|
|||
function getKeystore() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
@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. |
Thank you - understood. Big supported of this change by the way! It makes the configuration much simpler. |
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 |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* 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 <[email protected]>
7.9/7.x: c9b6f8a |
@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. |
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
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