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

Add Ingress TLS Hosts slice length check to avoid runtime panic #358

Merged
merged 3 commits into from
Jun 15, 2023

Conversation

hassaanakram
Copy link
Contributor

Description

In pkg/kube/wrappers/ingress.go the tryGetTLSHost method accesses ingress.Spec.TLS[0].Hosts[0] without ensuring that the Hosts slice is non-empty. This causes a runtime panic if an Ingress object with empty TLS is encountered as given below:

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:
  - {}

This PR introduces a new method on the IngressWrapper to fetch TLS Hosts

func (iw *IngressWrapper) getTLSHosts() []string

The output of this method is used as a check in the tryGetTLSHosts method to ensure that out of index access panic does not occur.

Closes #345

Test cases

A new test case has been introduced to simulate an Ingress with empty TLS.

In accordance with the issue stakater#345 a check has been added to ensure that the Hosts slice in Ingress TLS is not accessed while it is empty.
@github-actions
Copy link

github-actions bot commented Jun 10, 2023

@LilShah Image is available for testing. docker pull stakater/forecastle:SNAPSHOT-PR-358-96cf42be

@hassaanakram hassaanakram requested a review from LilShah June 14, 2023 11:17
@LilShah LilShah merged commit d9d4ef5 into stakater:master Jun 15, 2023
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.

forecastle should crash/fail when run into panic
2 participants