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

Initialize Paths before loading the keystore #10706

Merged
merged 4 commits into from
Feb 18, 2019

Conversation

ph
Copy link
Contributor

@ph ph commented Feb 12, 2019

The paths were incorrectly initialized meaning that instead of creating
the keystore in the data directory it was created next to the binary.

The problem was the call to paths.InitPaths() was done after loading
the keystore, this was causing a chicken and egg situation and
paths.Resolve(path.Data, "hello") was returning "hello" instead of
data/hello.

To solve that situation we do a partial extract of the configuration,
just enough to initialize the paths and we move on to the keystore and
the complete unpack.

@ph ph requested a review from a team as a code owner February 12, 2019 20:58
@ph
Copy link
Contributor Author

ph commented Feb 12, 2019

@urso @kvch I would like your input here, due to a bug the location of the keystore was not clear and could have been created in the config dir instead of the data dir. So by making the above change, it ensures that the Keystore file is always created in the data dir. But after slack discussion maybe the real location of the keystore should be in the configuration directory next to the yaml. WDYT?

@graphaelli
Copy link
Member

Copying over from our chat, the issue arose when the default keystore create fails by default under docker:

docker run -it --rm docker.elastic.co/beats/filebeat:7.0.0-SNAPSHOT filebeat -c /usr/share/filebeat/filebeat.yml keystore create
Error creating the keystore: cannot open file to save the keystore to 'filebeat.keystore', error: open filebeat.keystore.tmp: permission denied

vs:

docker run -it --rm docker.elastic.co/beats/filebeat:7.0.0-SNAPSHOT filebeat -c /usr/share/filebeat/filebeat.yml -E keystore.path=/usr/share/filebeat/data/filebeat.keystore keystore create
Created filebeat keystore

The docs are currently clear that the keystore goes in the config dir but 7.0 is an opportunity to change this. https://www.elastic.co/guide/en/apm/server/current/keystore.html#creating-keystore.

libbeat/cmd/instance/beat.go Outdated Show resolved Hide resolved
@kvch
Copy link
Contributor

kvch commented Feb 13, 2019

I think keystore should go under the data dir. It is a file manipulated by a Beat, so it is more similar to a registry file. Under the config folder, there are files which can be edited by users using a text editor of their choice. However, keystore is not intended to be modified via such tools, only using the Beat.

@ph
Copy link
Contributor Author

ph commented Feb 13, 2019

@kvch It is indeed a file manipulated by the beat CLI, but at the same time creating a file on a single host and deploy it to multiple machines is a possibility.

@kvch
Copy link
Contributor

kvch commented Feb 13, 2019

@ph True. Then I agree with you, it should go under the dir config.

The paths were incorrectly initialized meaning that instead of creating
the keystore in the data directory it was created next to the binary.

The problem was the call to `paths.InitPaths()` was done after loading
the keystore, this was causing a chicken and egg situation and
`paths.Resolve(path.Data, "hello")` was returning "hello" instead of
`data/hello`.

To solve that situation we do a partial extract of the configuration,
just enough to initialize the paths and we move on to the keystore and
the complete unpack.
@ph
Copy link
Contributor Author

ph commented Feb 18, 2019

Ok, I will do the path change in another PR, this still need to go in and it will make sure the data path is correctly respected.

@ph ph force-pushed the fix/correctly-initialize-paths-before-keystore branch from cf2933a to fb1949b Compare February 18, 2019 15:37
@ph ph requested a review from urso February 18, 2019 15:37
@ph
Copy link
Contributor Author

ph commented Feb 18, 2019

rebased on top of master.

@ph ph added the needs_backport PR is waiting to be backported to other branches. label Feb 18, 2019
@ph ph merged commit 3dbc233 into elastic:master Feb 18, 2019
graphaelli pushed a commit to graphaelli/beats that referenced this pull request Mar 19, 2019
* Initialize Paths before loading the keystore

The paths were incorrectly initialized meaning that instead of creating
the keystore in the data directory it was created next to the binary.

The problem was the call to `paths.InitPaths()` was done after loading
the keystore, this was causing a chicken and egg situation and
`paths.Resolve(path.Data, "hello")` was returning "hello" instead of
`data/hello`.

To solve that situation we do a partial extract of the configuration,
just enough to initialize the paths and we move on to the keystore and
the complete unpack.

(cherry picked from commit 3dbc233)
@graphaelli graphaelli added v7.0.0 and removed needs_backport PR is waiting to be backported to other branches. labels Mar 19, 2019
graphaelli added a commit that referenced this pull request Mar 19, 2019
…re (#11325)

* Initialize Paths before loading the keystore (#10706)

* Initialize Paths before loading the keystore

The paths were incorrectly initialized meaning that instead of creating
the keystore in the data directory it was created next to the binary.

The problem was the call to `paths.InitPaths()` was done after loading
the keystore, this was causing a chicken and egg situation and
`paths.Resolve(path.Data, "hello")` was returning "hello" instead of
`data/hello`.

To solve that situation we do a partial extract of the configuration,
just enough to initialize the paths and we move on to the keystore and
the complete unpack.

(cherry picked from commit 3dbc233)

* Update CHANGELOG.next.asciidoc
ph added a commit to ph/beats that referenced this pull request Mar 27, 2019
* Initialize Paths before loading the keystore

The paths were incorrectly initialized meaning that instead of creating
the keystore in the data directory it was created next to the binary.

The problem was the call to `paths.InitPaths()` was done after loading
the keystore, this was causing a chicken and egg situation and
`paths.Resolve(path.Data, "hello")` was returning "hello" instead of
`data/hello`.

To solve that situation we do a partial extract of the configuration,
just enough to initialize the paths and we move on to the keystore and
the complete unpack.

(cherry picked from commit 3dbc233)
ph added a commit that referenced this pull request Mar 29, 2019
…re (#11497)

Cherry-pick of PR #10706  to 6.7 branch. Original message: 

The paths were incorrectly initialized meaning that instead of creating
the keystore in the data directory it was created next to the binary.

The problem was the call to `paths.InitPaths()` was done after loading
the keystore, this was causing a chicken and egg situation and
`paths.Resolve(path.Data, "hello")` was returning "hello" instead of
`data/hello`.

To solve that situation we do a partial extract of the configuration,
just enough to initialize the paths and we move on to the keystore and
the complete unpack.
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.

4 participants