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.

Sign up for GitHub

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

Jump to bottom

Add username/password in Metricbeat autodiscover hints #15349

Merged
merged 6 commits into from
Jan 9, 2020
Merged

Add username/password in Metricbeat autodiscover hints #15349

merged 6 commits into from
Jan 9, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Jan 7, 2020 •
edited
Loading

This PR closes #15115

Manual Testing

  1. Enable autodiscover in metricbeat.yml
metricbeat:
  autodiscover.providers:
    - type: docker
      hints.enabled: true
  1. Start metricbeat with log enabled: ./metricbeat -e -d "hints.builder"
  2. docker run -l co.elastic.metrics/module=nats -l co.elastic.metrics/username=user42 --name nats nats
  3. Check in Metricbeat logs for : 2019-12-16T15:53:18.712+0200 DEBUG [hints.builder] hints/metrics.go:144 generated config: {"enabled":true,"hosts":null,"metricsets":["connections","routes","stats","subscriptions"],"module":"nats","period":"1m","processors":null,"ssl":null,"timeout":"3s","username":"user42"}

@ChrsMark ChrsMark added enhancement in progress Pull request is currently in progress. Team:Integrations Label for the Integrations team autodiscovery labels Jan 7, 2020
@ChrsMark ChrsMark self-assigned this Jan 7, 2020
@ChrsMark ChrsMark changed the title Add creds as hints Add username/password in Metricbeat autodiscover hints Jan 7, 2020
@ChrsMark ChrsMark requested a review from a team January 7, 2020 10:12
mtojek
mtojek reviewed Jan 7, 2020
View reviewed changes
metricbeat/autodiscover/builder/hints/metrics.go Outdated
@@ -198,6 +208,14 @@ func (m *metricHints) getMetricPath(hints common.MapStr) string {
return builder.GetHintString(hints, m.Key, metricspath)
}

func (m *metricHints) getUserName(hints common.MapStr) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick only: isn't it correct in English to use a single word "username" instead two ones "user Name"?

@ChrsMark
minor fix
e9156da 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Add tests and docs
9c5099e 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added review [zube]: In Review and removed in progress Pull request is currently in progress. labels Jan 8, 2020
@zube zube bot removed the [zube]: In Progress label Jan 9, 2020
exekias
exekias reviewed Jan 9, 2020
View reviewed changes
metricbeat/docs/autodiscover-hints.asciidoc Outdated
[float]
===== `co.elastic.metrics/password`

The password to use for authentication
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should put a note here advising not to use plain text passwords here, but references to ENV vars in the Metricbeat container, or passwords stored in keystore. Long term I think we should implement a way to allow referencing passwords from k8s secrets

exekias
exekias suggested changes Jan 9, 2020
View reviewed changes
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! I left a comment about docs, the rest LGTM

@andresrc andresrc requested a review from a team January 9, 2020 11:40
@ChrsMark
Improve docs
7af7e60 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Add changelog entry
65c6dda 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark merged commit 7d08f9b into elastic:master Jan 9, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Jan 9, 2020
@ChrsMark
Add username/password in Metricbeat autodiscover hints (elastic#15349)
61fd2ae 
(cherry picked from commit 7d08f9b)
@ChrsMark ChrsMark mentioned this pull request Jan 9, 2020
Merged
@ChrsMark ChrsMark added the v7.6.0 label Jan 9, 2020
@ChrsMark ChrsMark added the test-plan Add this PR to be manual test plan label Jan 9, 2020
ChrsMark added a commit that referenced this pull request Jan 9, 2020
@ChrsMark
Add username/password in Metricbeat autodiscover hints (#15349) (#15417)
de4ea3f 
(cherry picked from commit 7d08f9b)
@ycombinator ycombinator self-assigned this Jan 15, 2020
@ycombinator
Copy link
Contributor

ycombinator commented Jan 16, 2020 •
edited
Loading

Tested this functionality manually with BC1 and it works as expected.

However, I do have one concern: when the password is provided via a container label it is emitted in the Metricbeat debug log. Should we omit it or mask it? Or is it okay since it's not emitted in the Metricbeat log by default but only when debug level logging is turned on?

@ycombinator
Copy link
Contributor

@ChrsMark pointed me to a related discussion that already happened in this PR: #15349 (comment). So this is a known issue and user should specify the password carefully.

@ycombinator ycombinator added the test-plan-ok This PR passed manual testing label Jan 16, 2020
@ycombinator ycombinator mentioned this pull request Jan 16, 2020
Merged
@ycombinator
Copy link
Contributor

I put up a PR to mask the password in log output: #15616.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtojek mtojek mtojek left review comments

@exekias exekias exekias approved these changes

Assignees

@ycombinator ycombinator

Labels
autodiscovery enhancement review Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-ok This PR passed manual testing v7.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add username/password in Metricbeat autodiscover hints
5 participants
@ChrsMark @ycombinator @exekias @mtojek @andresrc