stakater / Forecastle

Forecastle is a control panel which dynamically discovers and provides a launchpad to access applications deployed on Kubernetes – [✩Star] if you're using it!
https://stakater.com
Apache License 2.0
588 stars 59 forks source link

forecastle should crash/fail when run into panic #345

Closed 1337andre closed 1 year ago

1337andre commented 1 year ago

Hi folks,

thanks for this project!

For reasons we have a strange ingress deployment, which causes unfavourable behaviour of forcastle. In my option, forecastle should crash, when it runs into panic error. Now, it's still running with error logs, but empty page.

maybe it would be possible to skip strange ingress with some detailed error messages, so that all other valid ingress are still available?

basically it looks like this:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    forecastle.stakater.com/expose: "true"
...
  labels:
    app.kubernetes.io/managed-by: Helm
  name: strange-ingress
  namespace: webcms-red-dev
spec:
  ingressClassName: intern
  rules:
  - host: webcms-red-dev.nomatter.com
    http:
      paths:
      - backend:
          service:
            name: management-server
            port:
              name: corba
        path: /
        pathType: ImplementationSpecific
  tls:
  - {}

logs:

forecastle-55c7595d9b-k87kq forecastle time="2023-05-11T12:46:55Z" level=info msg="Found ingress with Name 'strange-ingress' in Namespace 'webcms-red-dev'" _source="filename/filename.go:22"
forecastle-55c7595d9b-k87kq forecastle 2023/05/11 12:46:55 http: panic serving 10.4.177.49:53412: runtime error: index out of range [0] with length 0
forecastle-55c7595d9b-k87kq forecastle goroutine 409 [running]:
forecastle-55c7595d9b-k87kq forecastle net/http.(*conn).serve.func1()
forecastle-55c7595d9b-k87kq forecastle  /usr/local/go/src/net/http/server.go:1850 +0xbf
forecastle-55c7595d9b-k87kq forecastle panic({0x18dbdc0, 0xc000b7e7f8})
forecastle-55c7595d9b-k87kq forecastle  /usr/local/go/src/runtime/panic.go:890 +0x262
forecastle-55c7595d9b-k87kq forecastle github.com/stakater/Forecastle/v1/pkg/kube/wrappers.(*IngressWrapper).tryGetTLSHost(...)
forecastle-55c7595d9b-k87kq forecastle  /go/src/github.com/stakater/Forecastle/pkg/kube/wrappers/ingress.go:103
forecastle-55c7595d9b-k87kq forecastle github.com/stakater/Forecastle/v1/pkg/kube/wrappers.(*IngressWrapper).GetURL(0xc0001fed08)
forecastle-55c7595d9b-k87kq forecastle  /go/src/github.com/stakater/Forecastle/pkg/kube/wrappers/ingress.go:82 +0x279
forecastle-55c7595d9b-k87kq forecastle github.com/stakater/Forecastle/v1/pkg/forecastle/ingressapps.convertIngressesToForecastleApps({0xc0001d7000?, 0x9, 0x1?})
forecastle-55c7595d9b-k87kq forecastle  /go/src/github.com/stakater/Forecastle/pkg/forecastle/ingressapps/apps.go:70 +0x2a5
forecastle-55c7595d9b-k87kq forecastle github.com/stakater/Forecastle/v1/pkg/forecastle/ingressapps.(*List).Populate(0xc0001ff7b8, {0xc00058fac0?, 0xc0009072f0?, 0x1?})
forecastle-55c7595d9b-k87kq forecastle  /go/src/github.com/stakater/Forecastle/pkg/forecastle/ingressapps/apps.go:51 +0x279
forecastle-55c7595d9b-k87kq forecastle github.com/stakater/Forecastle/v1/pkg/handlers.AppsHandler({0x1d8aee0, 0xc000824fc0}, 0xc00017c600?)
forecastle-55c7595d9b-k87kq forecastle  /go/src/github.com/stakater/Forecastle/pkg/handlers/appsHandler.go:54 +0x485
forecastle-55c7595d9b-k87kq forecastle net/http.HandlerFunc.ServeHTTP(0xc00017c500?, {0x1d8aee0?, 0xc000824fc0?}, 0x800?)
forecastle-55c7595d9b-k87kq forecastle  /usr/local/go/src/net/http/server.go:2109 +0x2f
forecastle-55c7595d9b-k87kq forecastle github.com/gorilla/mux.(*Router).ServeHTTP(0xc000260300, {0x1d8aee0, 0xc000824fc0}, 0xc00017c400)
forecastle-55c7595d9b-k87kq forecastle  /go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x1cf
forecastle-55c7595d9b-k87kq forecastle net/http.serverHandler.ServeHTTP({0xc000896ea0?}, {0x1d8aee0, 0xc000824fc0}, 0xc00017c400)
forecastle-55c7595d9b-k87kq forecastle  /usr/local/go/src/net/http/server.go:2947 +0x30c
forecastle-55c7595d9b-k87kq forecastle net/http.(*conn).serve(0xc000826e60, {0x1d8bab8, 0xc0001ee9c0})
forecastle-55c7595d9b-k87kq forecastle  /usr/local/go/src/net/http/server.go:1991 +0x607
forecastle-55c7595d9b-k87kq forecastle created by net/http.(*Server).Serve
forecastle-55c7595d9b-k87kq forecastle  /usr/local/go/src/net/http/server.go:3102 +0x4db

image/version: stakater/forecastle:v1.0.125

hassaanakram commented 1 year ago

Hi @karl-johan-grahn I'd like to work on this enhancement. I have gone through the relevant code and my analysis is as follows.

Cause of this panic In https://github.com/stakater/Forecastle/blob/53888ad5e54279585c9b6cde43651fe806dfcae0/pkg/kube/wrappers/ingress.go#L103 the Hosts slice is empty and therefore attempting to access Hosts[0] causes the runtime panic.

What can be done to make this access safe Add a new method getTLSHosts on the IngressWrapper:

func (iw *IngressWrapper) getTLSHosts() []string {
    if iw.supportsTLS() {
        return iw.ingress.Spec.TLS[0].Hosts
    }
    return []string{}
}

Similarly change the method tryGetTLSHost to:

func (iw *IngressWrapper) tryGetTLSHost() (string, bool) {
    if iw.supportsTLS() && len(iw.getTLSHosts()) > 0 {
        return "https://" + iw.ingress.Spec.TLS[0].Hosts[0], true
    }

    return "", false
}

Test cases In https://github.com/stakater/Forecastle/blob/master/pkg/testutil/kube.go add the following function to simulate a case where TLS hosts is empty:

func CreateIngressWithHostAndEmptyTLSHost(name string, host string) *v1.Ingress {
    ingress := CreateIngressWithHost(name, host)
    ingress.Spec.TLS = []v1.IngressTLS{
        {
            Hosts: []string{},
        },
    }

    return ingress
}

Add the following test case in https://github.com/stakater/Forecastle/blob/53888ad5e54279585c9b6cde43651fe806dfcae0/pkg/kube/wrappers/ingress_test.go#L251

{
            name: "IngressWithTLSAndNoHosts",
            fields: fields{
                ingress: testutil.CreateIngressWithHostAndEmptyTLSHost("someIngress", "google.com"),
            },
            want:  "",
            want1: false,
        },

Let me know what do you think about this.