Skip to content

helm chart: fix ingress template#2596

Closed
larntz wants to merge 2 commits intohashicorp:mainfrom
larntz:chart-ingress
Closed

helm chart: fix ingress template#2596
larntz wants to merge 2 commits intohashicorp:mainfrom
larntz:chart-ingress

Conversation

@larntz
Copy link
Copy Markdown

@larntz larntz commented Jul 19, 2023

  • blank line between rules: and - host:
  • ingressClass was present when no value was specified

Changes proposed in this PR:

Fix malformed ingress when using the helm chart.

Given this values.yaml snippet:

ui:
  enabled: true
  ingress:
    enabled: true
    hosts:
    - host: consul.localhost

Output before changes:

---
# Source: consul/templates/ui-ingress.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: vault-consul-ui
  namespace: consul
  labels:
    app: consul
    chart: consul-helm
    heritage: Helm
    release: vault
    component: ui
spec:
  ingressClassName: 
  rules:
  
  - host: "consul.localhost"
    http:
      paths:
      - backend:
          service:
            name: vault-consul-ui
            port:
              number: 80
        path: /
        pathType: Prefix

Output after changes:

---
# Source: consul/templates/ui-ingress.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: consul-consul-ui
  namespace: consul
  labels:
    app: consul
    chart: consul-helm
    heritage: Helm
    release: consul
    component: ui
spec:
  rules:
  - host: "consul.localhost"
    http:
      paths:
      - backend:
          service:
            name: consul-consul-ui
            port:
              number: 80
        path: /
        pathType: Prefix

How I've tested this PR:

Used bats as shown at: https://github.com/hashicorp/consul-k8s/blob/main/CONTRIBUTING.md#testing-the-helm-chart

I am seeing a failure for this file, but it would fail before any changes I've made due to the default values file not including a host:

# hosts is a list of host name to create Ingress rules.
#
# ```yaml
# hosts:
# - host: foo.bar
# paths:
# - /example
# - /test
# ```
#
# @type: array<map>
hosts: []

bats consul/test/unit/ui-ingress.bats
ui-ingress.bats
 ✓ ui/Ingress: disabled by default
 ✓ ui/Ingress: enable with ui.ingress.enabled
 ✓ ui/Ingress: disable with ui.ingress.enabled
 ✓ ui/Ingress: disable with ui.ingress.enabled dash string
 ✗ ui/Ingress: no hosts by default
   (in test file consul/test/unit/ui-ingress.bats, line 48)
     `[ "${actual}" = "null" ]' failed
   ---
   # Source: consul/templates/ui-ingress.yaml
   apiVersion: networking.k8s.io/v1
   kind: Ingress
   metadata:
     name: release-name-consul-ui
     namespace: consul
     labels:
       app: consul
       chart: consul-helm
       heritage: Helm
       release: release-name
       component: ui
   spec:
     rules:

 ✓ ui/Ingress: hosts can be set
 ✓ ui/Ingress: exposes single port 80 when global.tls.enabled=false
 ✓ ui/Ingress: exposes single port 443 when global.tls.enabled=true and global.tls.httpsOnly=true
 ✓ ui/Ingress: exposes the port 80 when global.tls.enabled=true and global.tls.httpsOnly=false
 ✓ ui/Ingress: exposes the port 443 when global.tls.enabled=true and global.tls.httpsOnly=false
 ✓ ui/Ingress: no tls by default
 ✓ ui/Ingress: tls can be set
 ✓ ui/Ingress: tls with secret name can be set
 ✓ ui/Ingress: no annotations by default
 ✓ ui/Ingress: annotations can be set
 ✓ ui/Ingress: default PathType Prefix
 ✓ ui/Ingress: set PathType ImplementationSpecific
 ✓ ui/Ingress: no ingressClassName by default
 ✓ ui/Ingress: can set ingressClassName

19 tests, 1 failure

How I expect reviewers to test this PR:

Using the existing tests.

Checklist:

- blank line between rules: and - hosts
- ingressClass was present when no value was specified
@hashicorp-cla
Copy link
Copy Markdown

hashicorp-cla commented Jul 19, 2023

CLA assistant check
All committers have signed the CLA.

@larntz larntz changed the title fix ingress helm chart: fix ingress template Jul 19, 2023
@thisisnotashwin thisisnotashwin added backport/1.0.x backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. labels Jul 19, 2023
@thisisnotashwin
Copy link
Copy Markdown
Contributor

Thank you so much for the fix!!

@thisisnotashwin thisisnotashwin mentioned this pull request Jul 28, 2023
2 tasks
@thisisnotashwin
Copy link
Copy Markdown
Contributor

Closed by #2687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants