MoJo2600 / pihole-kubernetes

PiHole on kubernetes
498 stars 173 forks source link

Add the ability to specify extra objects (manifests) #210

Closed fernferret closed 1 year ago

fernferret commented 2 years ago

I've seen this trend in other helm charts like ArgoCD and I really like being able to bundle my "extra" bits that shouldn't be part of the chart with the template. This allows tracking them in the same location so they never get forgotten and they're tied to the release (when it gets uninstalled)

In my specific chart I added:

extraObjects:
  - apiVersion: cert-manager.io/v1
    kind: Certificate
    metadata:
      name: pihole-tls-cert
    spec:
      dnsNames:
        - "pihole.foo"
      ipAddresses:
        - "10.10.10.10"
      issuerRef:
        group: cert-manager.io
        kind: ClusterIssuer
        name: my-issuer
      secretName: pihole-tls-cert
  - apiVersion: v1
    kind: ConfigMap
    metadata:
      name: lighttpd-external-conf
    data:
      external.conf: |
        $HTTP["host"] =~ "pihole.foo" {
          # Ensure the Pi-hole Block Page knows that this is not a blocked domain
          setenv.add-environment = ("fqdn" => "true")

          # Enable the SSL engine with a LE cert, only for this specific host
          $SERVER["socket"] == ":443" {
            ssl.engine = "enable"
            ssl.pemfile = "/etc/ssl/lighttpd-private/tls.crt"
            ssl.privkey = "/etc/ssl/lighttpd-private/tls.key"
            ssl.ca-file = "/etc/ssl/lighttpd-private/ca.crt"
            ssl.honor-cipher-order = "enable"
            ssl.cipher-list = "EECDH+AESGCM:EDH+AESGCM:AES256+EECDH:AES256+EDH"
            ssl.use-sslv2 = "disable"
            ssl.use-sslv3 = "disable"
          }
        }
        # Redirect HTTP to HTTPS
        $HTTP["scheme"] == "http" {
          $HTTP["host"] =~ ".*" {
            url.redirect = (".*" => "https://%0$0")
          }
        }

There should be no backward compatibility issues as this is a totally new field.

fernferret commented 2 years ago

Hi @MoJo2600 I bumped the chart version (I always forget this). I'll bump the other one but that might actually require 2 bumps.

I think you use SemVer so I'd call this a minor (feature) bump (vs a bugfix), or should this have bene a bugfix bump?

fernferret commented 2 years ago

Hmm It looks like #196 was a bugfix bump which added new features. I'm still pretty new to semver (and usually use calver for my projects). But I'd done the minor per the semver spec:

MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards compatible manner, and PATCH version when you make backwards compatible bug fixes.

I'm really cool with whatever y'all want, just need to know which version to bump to pass the CI tests (for this and #211). Thanks again for the awesome helm chart 🚀

MoJo2600 commented 2 years ago

Sorry, I'm always confusing the versioning :( You are right, the last Version should have been a feature bump. I have to pay more attention. It is how you describe it.

fernferret commented 2 years ago

No worries at all! I'm just here because I enjoy this chart. Just let me know how you'd like to do versioning (or if you'd like to do it yourself!) and I'll submit changes however you like!

fernferret commented 1 year ago

Hi @MoJo2600 any feedback on this PR?

I'm still using this change actively and it'd be great if this could get merged. If you'd like me to change anything about it, please let me know!

MoJo2600 commented 1 year ago

Hey, sorry for the mucht too late response. I will merge your request now and thank you for your work!