Skip to content

[SRE-6652] Move ingress to it's own subchart. Refactor strategy POC #270

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.

Sign up for GitHub

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

Open
ifimbres-dave wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[SRE-6652] Move ingress to it's own subchart. Refactor strategy POC #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions charts/common/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,8 @@ version: 0.11.1-beta.5
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
appVersion: 1.17.0

dependencies:
- name: ingress
version: 0.1.0
repository: "file://./charts/ingress"
23 changes: 23 additions & 0 deletions charts/common/charts/ingress/.helmignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Patterns to ignore when building packages.
# This supports shell glob matching, relative path matching, and
# negation (prefixed with !). Only one pattern per line.
.DS_Store
# Common VCS dirs
.git/
.gitignore
.bzr/
.bzrignore
.hg/
.hgignore
.svn/
# Common backup files
*.swp
*.bak
*.tmp
*.orig
*~
# Various IDEs
.project
.idea/
*.tmproj
.vscode/
24 changes: 24 additions & 0 deletions charts/common/charts/ingress/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: v2
name: ingress
description: A Helm chart for Kubernetes

# A chart can be either an 'application' or a 'library' chart.
#
# Application charts are a collection of templates that can be packaged into versioned archives
# to be deployed.
#
# Library charts provide useful utilities or functions for the chart developer. They're included as
# a dependency of application charts to inject those utilities and functions into the rendering
# pipeline. Library charts do not define any templates and therefore cannot be deployed.
type: application

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.0

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "1.16.0"
6 changes: 3 additions & 3 deletions charts/common/templates/ingress.yaml → ...common/charts/ingress/templates/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{{- if .Values.ingress.enabled -}}
{{- if .Values.enabled -}}
{{- $labels := include "common.labels" . -}}
{{- $svcport := .Values.service.port -}}
{{- range .Values.ingress.hosts }}
{{- $svcport := .Values.fromCommon.service.port -}}
{{- range .Values.hosts }}
{{- $hostwithdashes := printf "%s" .host | replace "." "-" -}}
{{- $hostwithdashestls := printf "%s-tls" .host | replace "." "-" }}
---
Expand Down
27 changes: 27 additions & 0 deletions charts/common/charts/ingress/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
enabled: false


fromCommon:
service:
port: 8080

hosts:
- host: chart-example.local
# The name is optional, If not set, a name is generated using host
# name: ""
annotations:
kubernetes.io/ingress.class: nginx-internal
kubernetes.io/tls-acme: "true"
paths:
- backend:
service:
name: chart-service
port:
number:

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example/default values set hosts[].paths[].backend.service.port.number to null (empty). Because the template checks if $service.port (the map), this will be treated as “set” and will render number: with an empty value instead of falling back to the default service port, producing an invalid Ingress if enabled: true. Set a concrete integer here or remove the port block entirely when you want the template to use the default port.

Suggested change
number:
number: 8080

Copilot uses AI. Check for mistakes.
path: /
pathType: ImplementationSpecific

tls:
- secretName: chart-example-tls
hosts:
- chart-example.local
6 changes: 6 additions & 0 deletions charts/common/schemas/ingress.schema.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ properties:
enabled:
type: boolean
default: false
fromCommon:
type: object
properties: {}
global:
type: object
properties: {}
hosts:
type: array
items:
Expand Down
6 changes: 3 additions & 3 deletions charts/common/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
Expand the name of the chart.
*/}}
{{- define "common.name" -}}
{{- default .Release.Name ( required ".Values.name is missing, this can be caused by a mismatch in chart alias reference" .Values.name ) | trunc 63 | trimSuffix "-" }}
{{- default .Release.Name ( required ".Values.name is missing, this can be caused by a mismatch in chart alias reference" .Values.global.name ) | trunc 63 | trimSuffix "-" }}

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required error message still says .Values.name is missing, but the code now requires .Values.global.name. This will be confusing when the error triggers; update the message to reference the correct value path.

Suggested change
{{- default .Release.Name ( required ".Values.name is missing, this can be caused by a mismatch in chart alias reference" .Values.global.name ) | trunc 63 | trimSuffix "-" }}
{{- default .Release.Name ( required ".Values.global.name is missing, this can be caused by a mismatch in chart alias reference" .Values.global.name ) | trunc 63 | trimSuffix "-" }}

Copilot uses AI. Check for mistakes.
{{- end }}

{{/*
Expand Down Expand Up @@ -38,9 +38,9 @@ helm.sh/chart: {{ include "common.chart" . }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- if .Values.additionalLabels }}
{{- if .Values.global.additionalLabels }}
{{- println "" }}
{{- toYaml .Values.additionalLabels }}
{{- toYaml .Values.global.additionalLabels }}
{{- end }}
Comment on lines 40 to 44

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.labels now reads .Values.global.additionalLabels, but other templates still add .Values.additionalLabels specifically to pod/deployment labels. This split means labels may be applied to some resources but not pods. Consider coalescing/merging both (.Values.additionalLabels and .Values.global.additionalLabels) here (and/or updating the other templates) to keep behavior consistent and avoid breaking existing values files.

Copilot uses AI. Check for mistakes.
{{- end }}

Expand Down
8 changes: 8 additions & 0 deletions charts/common/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,14 @@
"type": "boolean",
"default": false
},
"fromCommon": {
"type": "object",
"properties": {}
},
"global": {
"type": "object",
"properties": {}
},
"hosts": {
"type": "array",
"items": {
Expand Down
12 changes: 7 additions & 5 deletions charts/common/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
global:
name: null

# This is used to apply additional labels on deployment, pods and services
# to properly tag the team and environment
additionalLabels: {}

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additionalLabels has been moved under global, but several templates in this chart still reference .Values.additionalLabels (e.g., pod template labels in the deployment templates). As-is, setting global.additionalLabels won’t apply those labels to pods, and existing consumers that set additionalLabels at the root will behave inconsistently. Consider either keeping additionalLabels at the root for backwards compatibility or updating all templates to use a single canonical path (and/or coalescing both paths).

Suggested change
# Additional labels at root level for backwards compatibility with templates
# that still reference `.Values.additionalLabels`.
additionalLabels: {}

Copilot uses AI. Check for mistakes.
# Default values for common.
# This is a YAML-formatted file.
# Declare variables to be passed into your templates.
Expand Down Expand Up @@ -649,11 +656,6 @@ verticalPodAutoscaler:

podSelectorLabelsOverride: {}

# This is used to apply additional labels on deployment, pods and services
# to properly tag the team and environment

additionalLabels: {}

# This section enables by default the topologySpreadConstraits to spread same deployment pods to multiple nodes
# and multiple AZ when possible, rules are soft enforced by default.
# The next are the only values that can be edited, labelSelector are set by the template.
Expand Down