Skip to content

Add username/password in Metricbeat autodiscover hints#15349

Merged
ChrsMark merged 6 commits intoelastic:masterfrom
ChrsMark:add_creds_as_hints
Jan 9, 2020
Merged

Add username/password in Metricbeat autodiscover hints#15349
ChrsMark merged 6 commits intoelastic:masterfrom
ChrsMark:add_creds_as_hints

Conversation

@ChrsMark
Copy link
Copy Markdown
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
return builder.GetHintString(hints, m.Key, metricspath)
}

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

@mtojek mtojek Jan 7, 2020

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
Copy Markdown
Contributor

@exekias exekias Jan 9, 2020

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
Copy Markdown
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 ChrsMark force-pushed the add_creds_as_hints branch from 097c49d to 7af7e60 Compare January 9, 2020 13:40
@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
Copy Markdown
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
Copy Markdown
Contributor

ycombinator commented Jan 16, 2020

@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
Copy Markdown
Contributor

ycombinator commented Jan 16, 2020

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

2 more reviewers

@mtojek mtojek mtojek left review comments

@exekias exekias exekias approved these changes

Reviewers whose approvals may not affect merge requirements

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