Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/filebeat] Allow volumes, env vars, and http metrics to be configurable #4372

Merged
merged 8 commits into from
Mar 29, 2018

Conversation

rifelpet
Copy link
Contributor

What this PR does / why we need it:
This PR allows additional volumes to be mounted along with additional prospectors configured.

This way we can watch logs from the host's /var/log that aren't from container logs themselves.

Additional environment variables can be specified as a key-value map in order to support interpolation in the prospector configurations.

Example values.yaml for capturing kube-proxy logs on a Kops cluster:

volumes:
  - hostPath:
      path: /var/log
    name: varlog
volumeMounts:
  - name: varlog
    mountPath: /host/var/log
    readOnly: true

extraEnv:
  CLUSTER_NAME: set-me-at-runtime

config:
  kube-proxy.yml: |-
    - type: log
      paths:
        - /host/var/log/kube-proxy.log
      tags: ["proxy"]
      processors:
        - add_kubernetes_metadata:
            in_cluster: true
            namespace: kube-system
        - add_cloud_metadata:
      fields:
        kubernetes.cluster: ${CLUSTER_NAME}
      fields_under_root: true

I also cleaned up some trailing whitespace and added checksums of the Secret and ConfigMap to the Daemonset annotations so pods will be rolled whenever either the Secret or ConfigMap change.

Which issue this PR fixes: N/A

Special notes for your reviewer:

Default behavior should be unchanged, but pods will be rolled upon upgrading due to the annotation change.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 20, 2018
@ghost ghost mentioned this pull request Mar 22, 2018
@rifelpet
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 22, 2018
@rifelpet
Copy link
Contributor Author

@sstarcher I've rebased and added some additional functionality - NODE_NAME env var for adding as a field to host logs not originating from docker containers (syslog in my case), as well as exposing the http metrics endpoint for monitoring. I also added a home to Chart.yaml to pass the stricter linting test that was added today.

@rifelpet rifelpet changed the title [stable/filebeat] Allow prospectors, volumes, and env vars to be configurable [stable/filebeat] Allow volumes, env vars, and http metrics to be configurable Mar 27, 2018
@@ -40,6 +40,23 @@ config:
rotate_every_kb: 10000
number_of_files: 5

http.enabled: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is http.enabled a filebeat configuration? If not I would separate it out into it's own thing. Also the dot in the string may cause issues with people trying to set it on the command line as I believe it will be treated as a map.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just looked at the docs, I see this is a filebeat configuration. LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, having a period in the key makes it a bit tricky to use with --set and the usage of the key in the template is not the most intuitive. We could try to add a separate, more helm-friendly key value pair that enables the containerPort but I'm not sure how we could conditionally merge a yaml block into the filebeat.yml Secret as its setup now. Any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be fine with keeping it as it is, but I would add a comment around it saying that it can't be set on the command line.

@unguiculus
Copy link
Member

In an effort to standardize things across charts, we tend to go with extraContainers, extraVars, extraSecrets, extraVolumes , etc. Would you mind following this pattern?

@unguiculus
Copy link
Member

/assign

@rifelpet
Copy link
Contributor Author

@unguiculus done 👍

@unguiculus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet, unguiculus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2018
@k8s-ci-robot k8s-ci-robot merged commit ecde925 into helm:master Mar 29, 2018
rolanddb pushed a commit to Eneco/charts that referenced this pull request Apr 9, 2018
…figurable (helm#4372)

* [stable/filebeat] Remove trailing whitespace

* [stable/filebeat] Add checksum annotations to Daemonset

* [stable/filebeat] Add NODE_NAME environment variable

* [stable/filebeat] Allow env vars to be configurable

* [stable/filebeat] Support exposing http metrics

* [stable/filebeat] Support additional volumes

* [stable/filebeat] Bump chart version

* [stable/filebeat] Add a Chart home to pass linting test
ichtar pushed a commit to Bestmile/charts that referenced this pull request May 15, 2018
…figurable (helm#4372)

* [stable/filebeat] Remove trailing whitespace

* [stable/filebeat] Add checksum annotations to Daemonset

* [stable/filebeat] Add NODE_NAME environment variable

* [stable/filebeat] Allow env vars to be configurable

* [stable/filebeat] Support exposing http metrics

* [stable/filebeat] Support additional volumes

* [stable/filebeat] Bump chart version

* [stable/filebeat] Add a Chart home to pass linting test
voron pushed a commit to dysnix/helm-charts that referenced this pull request Sep 5, 2018
…figurable (helm#4372)

* [stable/filebeat] Remove trailing whitespace

* [stable/filebeat] Add checksum annotations to Daemonset

* [stable/filebeat] Add NODE_NAME environment variable

* [stable/filebeat] Allow env vars to be configurable

* [stable/filebeat] Support exposing http metrics

* [stable/filebeat] Support additional volumes

* [stable/filebeat] Bump chart version

* [stable/filebeat] Add a Chart home to pass linting test

Signed-off-by: voron <av@arilot.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants