Adding Helm Chart for Simpler Deploy

[ejether]

No description provided.

[robscott]

Thanks for taking the initiative to add a Helm chart here! This looks like it'll be a really great way to deploy Fairwinds. I've left a few comments and questions below. At a higher level, I noticed that some of the yaml template files start with --- while others don't. I think it would be good for that to be consistent, whether it's with or without that prefix. My personal preference is without.

[rbren]

If you run this config with the webhook server enabled, it'll choke due to the fact that certs aren't available.

Rob, any idea what the right way to set up certificates would be?

[ejether]

@robscott this chart also lacks readiness and liveness checks which means Fairwinds detects itself as borked. I'll add them in and test them but can you suggest what the right checks would be?

[robscott]

@ejether Sadly neither have readiness or liveness endpoints that would work well yet. Changing the config to make that a warning would help here - on that note, rebasing on master once the config PR is merged in would make "warning" an option.

@bobby-brennan Not sure why certs are an issue, they should be generated automatically with controller-runtime as it creates the validating webhook config. Unfortunately because it's creating these resources out of band, they need to be cleaned up outside of a helm delete, so maybe the issue is some older config stuck around from a previous deploy?

[ejether]

I added some liveness and readiness probes that are working. Better than nothing I think.

[ejether]

Also, I'm seeing this error: {"level":"error","ts":1553619494.175966,"logger":"fairwinds.entrypoint","msg":"unable to run manager","error":"namespaces \"fairwinds\" not found","stacktrace":"github.com/reactiveops/fairwinds/vendor/github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/src/github.com/reactiveops/fairwinds/vendor/github.com/go-logr/zapr/zapr.go:128\nmain.startWebhookServer\n\t/go/src/github.com/reactiveops/fairwinds/main.go:145\nmain.main\n\t/go/src/github.com/reactiveops/fairwinds/main.go:62\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:201"}
with the webhook. It looks like the namespace might be hard coded somewhere. I don't think it is coming from the chart itself.

[robscott]

Hey @ejether, that is a downside of the current implementation - a number of resources, including the namespace are hardcoded inside Fairwinds because it provisions its own cert, secret, and webhook config. It would be relatively straightforward to make that driven by an env var, with a default of fairwinds. The relevant Fairwinds code for creating all those resources is fairly straightforward. That is all driven by the variable set closer to the top of that file. And for good measure, here's an example of getting config from an env var in go if you're interested in making that part of this PR.

[ejether]

I'm not going to pretend that I know what I'm doing with go, but I'm happy to take a look at this. Top of mind, is there any reason the webhook can't detect what namespace it is deployed in? https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/namespace.go#L34

Clean up is a good question that I'm interested in too but that may be for another issue.

[ejether]

@robscott is anything holding up this PR?

[robscott]

Hey EJ, thanks for the work on this! Sorry for the delay in adding some feedback here. I think this looks really close, just a few comments.

[ejether]

Requested changes made. Ready to rock now?

[robscott]

I think this looks great now, thanks for getting it in! I did just notice that there are two commits with the "adding helm chart to readme" message, but the actual content of the PR looks great to me.

[ejether]

Fixed history