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

initial commit of new dmarc2logstash chart #4453

Closed
wants to merge 4 commits into from
Closed

initial commit of new dmarc2logstash chart #4453

wants to merge 4 commits into from

Conversation

jertel
Copy link
Collaborator

@jertel jertel commented Mar 25, 2018

What this PR does / why we need it:
This PR adds a new chart to the stable repository. The chart will deploy an application called dmarc2logstash. This app polls a POP3 account for DMARC aggregated reports, unpacks them, converts them into JSON, and uses filebeat to forward them into Logstash. The purpose of the application is to provide all of the standardized RFC 7489 DMARC summary data into Elasticsearch so that email administrators have an easier job at locating problems with DKIM and SPF email/domain settings. This will reduce the level of effort to troubleshoot email messages not reaching their intended recipients due to various problems, such as messages landing in spam folders, being rejected from MTAs, etc.

A quick review of available, similar solutions resulted in none that were Docker-friendly and Helm-compatible, this PR aims to close that gap.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Not applicable as this is a new chart.

Special notes for your reviewer:
The dmarc2logstash application is open-sourced at https://github.com/jertel/dmarc2logstash, along with the Dockerfile which auto-builds into https://hub.docker.com/r/jertel/dmarc2logstash/.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 25, 2018
@jertel
Copy link
Collaborator Author

jertel commented Mar 25, 2018

/assign @prydonius

Copy link
Member

@unguiculus unguiculus 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 the PR. The is a separate chart for Filebeat (https://github.com/kubernetes/charts/tree/master/stable/filebeat). Could you use that as a dependency instead?

apiVersion: v1
kind: ConfigMap
metadata:
name: filebeat-config
Copy link
Member

Choose a reason for hiding this comment

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

Use fullname template for the name and add standard labels.

kind: Service
apiVersion: v1
metadata:
name: logstash
Copy link
Member

Choose a reason for hiding this comment

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

Use fullname template for name.

metadata:
name: logstash
labels:
app: filebeat
Copy link
Member

Choose a reason for hiding this comment

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

app: {{ template "dmarc2logstash.name" . }}

@unguiculus
Copy link
Member

/assign

@jertel
Copy link
Collaborator Author

jertel commented Mar 27, 2018

Thanks for the quick feedback. I've implemented the chart changes, except for the filebeat requirements.yaml suggestion. I don't think I can use the existing filebeat chart for two reasons:

  1. The filebeat in this dmarc2logstash project is a running as sidecar container, not as a separate pod.
  2. The filebeat chart you referenced is intended to collect arbitrary Kubernetes and Docker engine logs, such as those generated from pods sending console output to STDOUT, for injection into an Elastic stack, whereas dmarc2logstash is using filebeat in a specific way -- it uses filebeat to take a specific JSON file residing within the pod itself.

@timstoop
Copy link
Contributor

Argument makes sense and it generally looks good to me. However, I'm hesitant to allow it to test, as I expect it will fail on the POP3 requirement. We'll need to figure out a way to make a default installation work without crashing, without having a POP3 server available?

@jertel
Copy link
Collaborator Author

jertel commented Mar 28, 2018

Yes, these charts with dependencies upon external running services are problematic when it comes to the automated tests. I've worked around this on this chart as well as another PR by preventing the deployment from being installed if a host config value isn't supplied (see line 1 of https://github.com/jertel/charts/blob/dmarc2logstash-initial/stable/dmarc2logstash/templates/deployment.yaml), which by default it is set to an empty string. When the test runs it will install the config map but not the deployment itself. The NOTES.txt explains why the deployment wasn't installed so for an actual chart user it should be clear that they need to specify some required inputs. It's a workaround but gets us past the issue for now.

@timstoop
Copy link
Contributor

So I understand the reasons for doing so, but I don't feel it's the right solution for a Chart in stable. However, I do not have a solution either. I'm going to leave this to the veterans and see (and learn) from how they'll propose to solve this. But it's a dilemma, for certain.

@unguiculus
Copy link
Member

@timstoop There's no other way than to install things conditionally.

@jertel We tend to go with extraConainers, extraSecrets, extraVars, etc. Would you mind updating the PR to follow this pattern?

@timstoop
Copy link
Contributor

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 29, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jertel, timstoop
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: prydonius

Assign the PR to them by writing /assign @prydonius in a comment when ready.

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 removed the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 6, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 6, 2018
@jertel jertel closed this Apr 6, 2018
@jertel jertel deleted the dmarc2logstash-initial branch April 6, 2018 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants