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

Rewrite controller #60

Merged
merged 24 commits into from
Apr 25, 2021
Merged

Rewrite controller #60

merged 24 commits into from
Apr 25, 2021

Conversation

Embraser01
Copy link
Member

@Embraser01 Embraser01 commented Nov 21, 2020

Basically, this PR keeps the base of the controller but move things around to make it easier to add new features and understand the codebase. It fixes some issues I find, add support for multiple instances and add features (like OnDemandTLS).

I apologise for the size of the PR. I wanted to implement important features like multi-instances support, more ConfigMap option without making the code hard to read but it was hard to do so without moving refactoring some parts of the controller. I thought about spliting it in multiple PRs but it would have been counter productive IMO.

I'll try to list all things that I have done:

Move internal/ code into pkg/ folder

This change was made because I'm not sure why we would want to prevent access to the Controller or other things.
But I'm still new into Golang so I may have missed something. I can move it back if necessary

Update to latest Caddy

I upgraded caddy to 2.3.0-rc.1 (#54) but there was a bug in certmagic for distributed challenges so I upgraded it to master.

Add metrics

Prometheus metrics are now integrated in caddy so use it instead of a custom prometheus server.
I also added the necessary annotations in the Helm charts to be scraped automaticaly.

Note that I used the port 9765 for metrics

If metrics are disabled (by config map), this port will only be used as an health check for the pod.

Health check

The Helm chart now has a Readiness Probe configured to the metrics endpoint.

Pod disruption budget

The helm chart now has a pod disruption budget to be sure at least one Pod is running in the cluster.

Default replicas

The Helm chart has a default to 2 replicas.

Proxy protocol

I added support for Proxy Protocol using a custom Caddy Module and github.com/pires/go-proxyproto

I tried to use https://github.com/mastercactapus/caddy2-proxyprotocol/ but because it doesn't support TLV, it couldn't work with AWS Network Load Balancer

Logger

I change the logger to use zap to be the same as Caddy

Update K8S Client to v0.19.4

Close #49.

Secret Storage Lock

Certmagic storage adapter has the Lock and Unlock functions implemented using K8S leases objects.

JSON Caddy config

I removed the possibility of loading a custom caddy config. This is because there wasn't an easy way to make it work with Ingresses and other options. To use a raw json caddy config, users should use the official Caddy docker image.

On Demand TLS

I added the On Demand TLS feature through Config Map options.

Ingress PathType support

We now support the Ingress PathType PathTypePrefix.

TLS Ingress config

TLS ingress should work as before (still needs testing though)

Resource store

Resource store doesn't load existing resources at startup because the Informers already synchronise resources when they start.

Ingress statuses are now correct

Current implementation was setting the wrong status to handled ingresses. Instead of setting the pod IP adress we now look for the service handling the pod and set the Hostname/IP of the LoadBalancer as spec want.

Ingress class support

I also added support for kubernetes.io/ingress.class annotation.

@mholt
Copy link
Member

mholt commented Nov 24, 2020

Looks like a lot of effort, thanks for your work!

I removed the possibility of loading a custom caddy config. This is because there wasn't an easy way to make it work with Ingresses and other options. To use a raw json caddy config, users should use the official Caddy docker image.

What do you mean by this? Don't people always need to use a custom config?

There's a lot here, so if there's anything Caddy-specific you would like me to address/review, feel free to point me at it and I'll take a look.

internal/caddy/convert.go Outdated Show resolved Hide resolved
@Embraser01 Embraser01 marked this pull request as ready for review January 12, 2021 14:43
@Embraser01
Copy link
Member Author

I think the PR is ready to be merged if everything LGTM @mavimo

@mholt
Copy link
Member

mholt commented Apr 26, 2021

Congrats on getting this merged!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

caddy ingress controller clusterrole is missing configmap resource access
4 participants