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

Refactor keystore interface into 3 different interfaces #16365

Merged
merged 11 commits into from
Apr 2, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Feb 17, 2020

What does this PR do?

This PR refactor keystore interface into 3 different interfaces.

This PR introduces a new Keystore backend which is ReadOnly and retrieves passwords from k8s secrets through k8s API. In order to implement this, the basic Keystore interface is split into 3 different interfaces, Keystore (readonly), WritableKeystore (Keystore + store/write/delete), ListingKeystore (WritableKeystore + list) as proposed on #5832.

The new Keystore that retrieves data from k8s secrets is of Keystore (readonly) type.

#### Autodiscover adoption
In order to support this k8s Keystore in Autodiscover, a keystore object is attached on every event. The keystore object is either created or retrieved if it already exists. We create only one keystore per namespace and we reuse it for events of the same namespace.

Why is it important?

This is important in order to support different keystore backends, like k8s keystore.

This is important in order to support autodiscover hints being able to refer to k8s secrets for password retrieval.

How to test this PR locally

Try all the set of available commands:

  1. ./metricbeat keystore create
  2. ./metricbeat keystore add ES_PWD
  3. ./metricbeat keystore add ES_PWD --force
  4. echo "passpass" | ./metricbeat keystore add ES_PWD --stdin --for
  5. ./metricbeat keystore list
  6. ./metricbeat keystore remove ES_PWD

Create a key and consume it (Elastic Cloud auth or any other ES setting can be used as an example):

  1. ./metricbeat keystore add ES_PWD
  2. ./metricbeat -e -d "*" -E "cloud.id=testkeys:xxxx" -E "cloud.auth=\${ES_PWD}"

or

  1. ./metricbeat keystore add ES_HOST
  2. ./metricbeat -e -d "*" -E output.elasticsearch.hosts=["\${ES_HOST}"]

Related issues

@ChrsMark ChrsMark added enhancement libbeat Team:Integrations Label for the Integrations team autodiscovery Team:Platforms Label for the Integrations - Platforms team labels Feb 17, 2020
@ChrsMark ChrsMark self-assigned this Feb 17, 2020
@exekias
Copy link
Contributor

exekias commented Feb 20, 2020

I would recommend splitting this PR in 2:

  • refactor keystore interface into 3 different interfaces, with all the changes required to code using keystore.
  • add the new k8s keystore

@ChrsMark
Copy link
Member Author

Updated that, to keep only the Keystore refactoring. The implementation of k8s keystore could be retrieved by the previous commit (bd508d6).

I will open it for review to start shaping it more.
One thing that I'm not sure of is how we could better approach the Factory implementation. I thought of it like different factory methods for each different keystore backend. This is how I did it with k8s keystore at bd508d6. But proposals are more than welcome.

Also interface inheritance is something to consider, since for now I followed an approach of having "stronger" interfaces inheriting the ones with the lower privileges.

@ChrsMark ChrsMark changed the title Add k8s secrets Keystore backend Refactor keystore interface into 3 different interfaces Feb 20, 2020
@ChrsMark ChrsMark marked this pull request as ready for review February 20, 2020 10:00
Signed-off-by: chrismark <[email protected]>
@exekias
Copy link
Contributor

exekias commented Feb 24, 2020

One thing that I'm not sure of is how we could better approach the Factory implementation. I thought of it like different factory methods for each different keystore backend. This is how I did it with k8s keystore at bd508d6. But proposals are more than welcome.

I would avoid a common Factory for now, as you don't need it yet? Left a comment about the existing one, but it looks to me that it's file_keystore specifc.

Signed-off-by: chrismark <[email protected]>
@exekias
Copy link
Contributor

exekias commented Feb 27, 2020

This is looking good, but tests seem to be failing

Signed-off-by: chrismark <[email protected]>
@ChrsMark
Copy link
Member Author

Yeap @exekias , tests were failing because the refactoring was not "healthy". I had to ensure that in all places the stores are properly checked if implement the respective interfaces. Feel free to comment on it.

libbeat/keystore/file_keystore.go Outdated Show resolved Hide resolved
libbeat/keystore/file_keystore.go Outdated Show resolved Hide resolved
@ChrsMark
Copy link
Member Author

@urso @exekias any extra comments on this would be much appreciated so as to move this forward 🙂

libbeat/cmd/keystore.go Outdated Show resolved Hide resolved
Signed-off-by: chrismark <[email protected]>
@urso
Copy link

urso commented Apr 1, 2020

LGTM. Let's see what CI says.

@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 1, 2020

jenkins, test this again please

@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 2, 2020

CI looks happy now. I also added manual testing notes that I used in my testing process too.

@ChrsMark ChrsMark added needs_backport PR is waiting to be backported to other branches. v7.8.0 labels Apr 2, 2020
@ChrsMark ChrsMark merged commit d99dde2 into elastic:master Apr 2, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Apr 2, 2020
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Apr 2, 2020
ChrsMark added a commit that referenced this pull request Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autodiscovery enhancement libbeat Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team v7.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants