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

Fix k8s watcher issue when node access to list nodes and ns #22714

Merged
merged 3 commits into from
Nov 26, 2020
Merged

Fix k8s watcher issue when node access to list nodes and ns #22714

merged 3 commits into from
Nov 26, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Nov 23, 2020 •
edited
Loading

What does this PR do?

This PR changes Pod's metadata initialisation process so as not to fatally fail when node's or namespace's objects are not accessible via k8s api. Instead we only log an ERROR.

Why is it important?

So as not to fail when users do not have permission to watch nodes and namespaces in a cluster.
Default manifests of Metricbeat provide watch access by default:

beats/deploy/kubernetes/metricbeat-kubernetes.yaml

Line 241 in f5df640

- nodes

Related issues

@ChrsMark ChrsMark added bug Team:Platforms Label for the Integrations - Platforms team v7.11.0 labels Nov 23, 2020
@ChrsMark ChrsMark self-assigned this Nov 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 23, 2020
@ChrsMark ChrsMark added needs_reviewer PR needs to be assigned a reviewer review labels Nov 23, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 23, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #22714 updated]

  • Start Time: 2020-11-25T11:24:45.593+0000

  • Duration: 71 min 48 sec

Test stats 🧪

Test Results
Failed 0
Passed 16703
Skipped 1369
Total 18072

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 16703
Skipped 1369
Total 18072

jsoriano
jsoriano reviewed Nov 24, 2020
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Looks good, but we would need a changelog entry mentioning the fix.

And, do we have any test with the the pod metadata generator initialized without node and namespace generators? If not maybe we should add one.

Thanks!

@ChrsMark
Copy link
Member Author

Thanks @jsoriano ! Do we really need a changelog for this? This one targets to fix an issue introduced at #22189 which will be landed in 7.11.

Regarding tests, Pod meta generator is covered by tests at

beats/libbeat/common/kubernetes/metadata/pod_test.go

Line 36 in 95a88a6

func TestPod_Generate(t *testing.T) {
and there are tests with and without nodeMeta and namespaceMeta. Do you think this should be enough or we should add tests for the wrapper GetPodMetaGen too?

@jsoriano
Copy link
Member

This one targets to fix an issue introduced at #22189 which will be landed in 7.11.

Oh ok, I was thinking that 7.10 was also affected by this, good to go then.

Regarding tests, Pod meta generator is covered by tests at

beats/libbeat/common/kubernetes/metadata/pod_test.go

Line 36 in 95a88a6

func TestPod_Generate(t *testing.T) {

and there are tests with and without nodeMeta and namespaceMeta. Do you think this should be enough or we should add tests for the wrapper GetPodMetaGen too?

Oh ok, I think this is enough 👍

jsoriano
jsoriano approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

@ChrsMark ChrsMark merged commit 1137032 into elastic:master Nov 26, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Nov 26, 2020
@ChrsMark
Fix k8s watcher issue when node access to list nodes and ns (elastic#…
242424a 
…22714)

(cherry picked from commit 1137032)
@ChrsMark ChrsMark mentioned this pull request Nov 26, 2020
Merged
ChrsMark added a commit that referenced this pull request Nov 26, 2020
@ChrsMark
Fix k8s watcher issue when node access to list nodes and ns (#22714) (#…
31f1514 
…22769)

(cherry picked from commit 1137032)
v1v added a commit to v1v/beats that referenced this pull request Dec 2, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/windows-7…
6724419 
…-issues

* upstream/master: (41 commits)
  Fix version parser regex for packaging (elastic#22581)
  Fix local_dynamic documentation and add providers inline doc. (elastic#22657)
  fix: use proper param name for e2e tests (elastic#22836)
  [Heartbeat] Fix exit on disabled monitor (elastic#22829)
  Update Golang to 1.14.12 (elastic#22790)
  docs: fix setup.template.overwrite typos (elastic#22804)
  Add docs section for ECS EC2 monitoring (elastic#22784)
  Fixing logic to keep list of unique cluster UUIDs (elastic#22808)
  Skip somewhat flaky UDP system test on Windows (elastic#22810)
  Fix polling node when it is not ready and monitor by hostname (elastic#22666)
  Skip Filebeat test_shutdown on windows 7 (elastic#22797)
  Make monitoring Namespace thread-safe (elastic#22640)
  Drop pkt_dstaddr and pkt_srcaddr when equals to "-" (elastic#22721)
  Add support for reading from UNIX datagram sockets (elastic#22699)
  Fix export dashboard command from Elastic Cloud (elastic#22746)
  Skip flaky winlogbeat test on Windows-7 (elastic#22754)
  Missing `>` (elastic#22763) (elastic#22766)
  Fix k8s watcher issue when node access to list nodes and ns (elastic#22714)
  [Metricbeat/Kibana/stats] Enforce `exclude_usage=true` (elastic#22732)
  Avoid sending non-numeric floats in cloud foundry integrations (elastic#22634)
  ...
v1v added a commit to v1v/beats that referenced this pull request Dec 2, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/retry-win…
29404d9 
…dows-7

* upstream/master: (41 commits)
  Fix version parser regex for packaging (elastic#22581)
  Fix local_dynamic documentation and add providers inline doc. (elastic#22657)
  fix: use proper param name for e2e tests (elastic#22836)
  [Heartbeat] Fix exit on disabled monitor (elastic#22829)
  Update Golang to 1.14.12 (elastic#22790)
  docs: fix setup.template.overwrite typos (elastic#22804)
  Add docs section for ECS EC2 monitoring (elastic#22784)
  Fixing logic to keep list of unique cluster UUIDs (elastic#22808)
  Skip somewhat flaky UDP system test on Windows (elastic#22810)
  Fix polling node when it is not ready and monitor by hostname (elastic#22666)
  Skip Filebeat test_shutdown on windows 7 (elastic#22797)
  Make monitoring Namespace thread-safe (elastic#22640)
  Drop pkt_dstaddr and pkt_srcaddr when equals to "-" (elastic#22721)
  Add support for reading from UNIX datagram sockets (elastic#22699)
  Fix export dashboard command from Elastic Cloud (elastic#22746)
  Skip flaky winlogbeat test on Windows-7 (elastic#22754)
  Missing `>` (elastic#22763) (elastic#22766)
  Fix k8s watcher issue when node access to list nodes and ns (elastic#22714)
  [Metricbeat/Kibana/stats] Enforce `exclude_usage=true` (elastic#22732)
  Avoid sending non-numeric floats in cloud foundry integrations (elastic#22634)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees

@ChrsMark ChrsMark

Labels
bug needs_reviewer PR needs to be assigned a reviewer review Team:Platforms Label for the Integrations - Platforms team v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle errors about k8s api nodes permissions
3 participants
@ChrsMark @elasticmachine @jsoriano