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

make nginx-ingress-controller work with k8s #4855

Merged
merged 13 commits into from
Dec 20, 2022

Conversation

gsantoro
Copy link
Contributor

@gsantoro gsantoro commented Dec 16, 2022

What does this PR do?

Adapt the nginx-ingress-controller integration to work with logs from /var/log/containers/*.log since /var/log/nginx/access.log and /var/log/nginx/error.log are respectively symlinks to /dev/stdout and /dev/stderr.

List of changes:

  • input changed from logfile to filestream to consume from k8s logs (eg. in CRI format and symlinks) instead of static files created by nginx
  • access datastream consumes from container stdout stream while error datastream consumes from container stderr stream
  • default path has now being changed to /var/log/containers/*${kubernetes.container.id}.log from /var/log/nginx/ingress.log*
  • added condition to filter all k8s container logs to only the nginx-ingress-controller pod
  • added prospector.scanner.symlinks to consume from symbolic links
  • added parsers.container to consume container logs with either CRI or docker format. By default it uses auto format so that the input can detect automatically which format to use
  • removed system tests since they were not compatible with running the integration on k8s. Adapt the system tests to run on k8s will be a future task since it requires changes in elastic-package. A future issue will be created a linked to this PR for reference.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

@gsantoro gsantoro added the bug Something isn't working label Dec 16, 2022
@gsantoro gsantoro requested a review from a team as a code owner December 16, 2022 11:55
@gsantoro gsantoro self-assigned this Dec 16, 2022
@elasticmachine
Copy link

elasticmachine commented Dec 16, 2022

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2022-12-20T14:08:59.252+0000

  • Duration: 13 min 22 sec

Test stats 🧪

Test Results
Failed 0
Passed 28
Skipped 0
Total 28

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Dec 16, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (2/2) 💚
Files 100.0% (2/2) 💚
Classes 100.0% (2/2) 💚
Methods 92.0% (23/25) 👎 -8.0
Lines 94.428% (322/341) 👎 -3.354
Conditionals 100.0% (0/0) 💚

multi: false
required: true
show_user: true
default: ${kubernetes.labels.app.kubernetes.io/name} == 'ingress-nginx'
title: Nginx Ingress Controller access logs
description: Collect Nginx Ingress Controller access logs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is correct. the name at https://kubernetes.github.io/ingress-nginx/ is Nginx Ingress Controller

@elastic elastic deleted a comment from gizas Dec 16, 2022
@elastic elastic deleted a comment from gizas Dec 16, 2022
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

lgtm. Left just some minors.

@ChrsMark
Copy link
Member

So only #4855 (comment) seems to be a blocker by now. @gsantoro let me know when the PR is ready for a final look.

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM! I think we can merge this.

Please file a follow up issue for the testing part so as to keep track and not forget.

@gsantoro gsantoro merged commit 07c21d5 into elastic:main Dec 20, 2022
@gsantoro gsantoro deleted the feature/nginx_ingress_controller branch December 20, 2022 14:31
@elasticmachine
Copy link

Package nginx_ingress_controller - 1.6.0 containing this change is available at https://epr.elastic.co/search?package=nginx_ingress_controller

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adapt nginx_ingress_controller integration to ingest logs from K8s
4 participants