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

retrieveUsedIngress: panic: runtime error: invalid memory address or nil pointer dereference #109

Closed
northys opened this issue Oct 19, 2023 · 9 comments · Fixed by #110
Closed
Labels
bug Something isn't working

Comments

@northys
Copy link
Contributor

northys commented Oct 19, 2023

Describe the bug
I've deployed kor using helm provided in README and it worked in our test cluster (EKS 1.27), however it crashlooped on this error in our "legacy" EKS cluster (which runs 1.27 but is here with us for quite a while) on this error:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x15d54f3]

goroutine 23 [running]:
github.com/yonahd/kor/pkg/kor.retrieveUsedIngress({0x1c158d0, 0xc0002c5380}, {0xc000046e50, 0xa})
        /build/pkg/kor/ingresses.go:48 +0x4b3
github.com/yonahd/kor/pkg/kor.processNamespaceIngresses({0x1c158d0, 0xc0002c5380}, {0xc000046e50, 0xa})
        /build/pkg/kor/ingresses.go:78 +0x4b
github.com/yonahd/kor/pkg/kor.getUnusedIngresses({0x1c158d0?, 0xc0002c5380?}, {0xc000046e50, 0xa})
        /build/pkg/kor/all.go:105 +0x5d
github.com/yonahd/kor/pkg/kor.GetUnusedAllStructured({{0x0?, 0x0?}, {0x0?, 0x0?}}, {0x1c158d0, 0xc0002c5380}, {0x19a42d6, 0x4})
        /build/pkg/kor/all.go:204 +0xa1f
github.com/yonahd/kor/pkg/kor.exportMetrics({{0x0?, 0xc000131560?}, {0x0?, 0xc00005a7d0?}}, {0x1c158d0, 0xc0002c5380}, {0x19a42d6, 0x4})
        /build/pkg/kor/exporter.go:52 +0x145
created by github.com/yonahd/kor/pkg/kor.Exporter
        /build/pkg/kor/exporter.go:34 +0x190

To Reproduce

We use https://github.com/nginxinc/kubernetes-ingress in production and we're trying ALB controller as ingress on test cluster. I've listed on ingresses in all namespaces and found it that some have class <none>. This may be helpful.

Installed today (2023-10-19) using:

helm upgrade -i kor \
    --namespace kor \
    --create-namespace \
    ./charts/kor

Expected behavior

No panic.

Screenshots

OS version, architecture and kor version
EKS 1.27 AWS managed AMI (Amazon Linux), kernel 5.10, amd64
Kor version: latest images as of 2023-10-19

Additional context

@yonahd yonahd added the bug Something isn't working label Oct 19, 2023
yonahd added a commit that referenced this issue Oct 19, 2023
@yonahd
Copy link
Owner

yonahd commented Oct 19, 2023

We are missing a validation that the field
rule.HTTP.Paths exists.

I'll add this patch in a bit.

@yonahd
Copy link
Owner

yonahd commented Oct 19, 2023

If you can describe the ingress and paste it here that would validate my assumption

@northys
Copy link
Contributor Author

northys commented Oct 19, 2023

I have checked multiple ingresses by the oldest and most suspicious and all 5 I checked had the paths :( But we have like 100 ingresses and I'm not sure how to find the right one.

This is the oldest one:

k get ingresses.networking.k8s.io -n default cafe-ingress -o yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  creationTimestamp: "2019-01-14T01:29:18Z"
  generation: 1
  name: cafe-ingress
  namespace: default
  resourceVersion: "5481104"
  uid: cffab627-179b-11e9-aafa-02e4cecc4162
spec:
  rules:
  - host: cafe.k8s.obfuscated.com
    http:
      paths:
      - backend:
          service:
            name: tea-svc
            port:
              number: 80
        path: /tea
        pathType: ImplementationSpecific
      - backend:
          service:
            name: coffee-svc
            port:
              number: 80
        path: /coffee
        pathType: ImplementationSpecific
  tls:
  - hosts:
    - cafe.k8s.obfuscated.com
    secretName: cafe-secret
status:
  loadBalancer:
    ingress:
    - {}

@yonahd
Copy link
Owner

yonahd commented Oct 19, 2023

@northys Can you please run
kubectl get ingress --all-namespaces -o json | jq -r '.items[] | select(.spec.rules[].http.paths | length == 0) | .metadata.name + " " + .metadata.namespace'

@northys
Copy link
Contributor Author

northys commented Oct 19, 2023

wow, i need to skill jq myself. good job!

yes there are multiple of these... because nginx inc ingress supports something i call "chunks merging" which results in single ingress at the end. so yeah, there are many of these (the "master chunk" doesn't have paths and other "child chunks" just add their paths to it)

(they call it mergeable type)

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    meta.helm.sh/release-name: stage
    meta.helm.sh/release-namespace: obfuscated
    nginx.org/client-max-body-size: 150M
    nginx.org/mergeable-ingress-type: master
  creationTimestamp: "2023-10-13T12:14:11Z"
  generation: 1
  labels:
    app.kubernetes.io/managed-by: Helm
  name: stage-complex-master
  namespace: obfuscated
  resourceVersion: "970309192"
  uid: 4a1bce12-26fb-40b9-a20c-73bc2c184f3e
spec:
  ingressClassName: nginx-stage
  rules:
  - host: '*.stage.obfuscatedapp.com'
  tls:
  - hosts:
    - '*.stage.obfuscatedapp.com'
status:
  loadBalancer: {}

yonahd added a commit that referenced this issue Oct 19, 2023
@yonahd
Copy link
Owner

yonahd commented Oct 19, 2023

wow, i need to skill jq myself. good job!

yes there are multiple of these... because nginx inc ingress supports something i call "chunks merging" which results in single ingress at the end. so yeah, there are many of these (the "master chunk" doesn't have paths and other "child chunks" just add their paths to it)

(they call it mergeable type)

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    meta.helm.sh/release-name: stage
    meta.helm.sh/release-namespace: obfuscated
    nginx.org/client-max-body-size: 150M
    nginx.org/mergeable-ingress-type: master
  creationTimestamp: "2023-10-13T12:14:11Z"
  generation: 1
  labels:
    app.kubernetes.io/managed-by: Helm
  name: stage-complex-master
  namespace: obfuscated
  resourceVersion: "970309192"
  uid: 4a1bce12-26fb-40b9-a20c-73bc2c184f3e
spec:
  ingressClassName: nginx-stage
  rules:
  - host: '*.stage.obfuscatedapp.com'
  tls:
  - hosts:
    - '*.stage.obfuscatedapp.com'
status:
  loadBalancer: {}

Perfect!
This change should fix it.
I'll create a release in the next few hours.

@yonahd
Copy link
Owner

yonahd commented Oct 19, 2023

@northys Please update if there is a further issue

@northys
Copy link
Contributor Author

northys commented Oct 24, 2023

deployed to both clusters and works like a charm! thanks!

@yonahd
Copy link
Owner

yonahd commented Oct 24, 2023

deployed to both clusters and works like a charm! thanks!

Amazing!
Thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants