Skip to content

Latest commit

 

History

History
375 lines (291 loc) · 14.1 KB

review_guidelines.md

File metadata and controls

375 lines (291 loc) · 14.1 KB
Raw

Chart review guidelines

The process to get a pull request merged is fairly simple. First, all required tests need to pass, and you must have signed our CLA. For details on our CI, see the Charts testing GitHub action.

If problems arise with some part of the test, such as timeout issues, contact one of the repository maintainers.

  1. Immutability
  2. Versioning
  3. Compatibility
  4. Chart metadata
  5. Dependencies
  6. Resource Metadata Labels
  7. Labels
  8. Configuration
  9. Kubernetes Native Workfloads
  10. Persistence
  11. AutoScaling / HorizontalPodAutoscaler
  12. Ingress
  13. Formatting
  14. Documentation
  15. Tests

1. Immutability

Chart releases must be immutable. Any change to a chart warrants a chart version bump even if it is only changed to the documentation.

2. Versioning

The chart's version must adhere to SemVer 2.

Stable charts must start at 1.0.0 (for increased maintainability, do not create new pull requests for stable charts simply to meet these criteria; rather, take the opportunity to ensure that this is met when reviewing pull requests).

Breaking (backwards incompatible) changes to a chart must:

  1. Bump the MAJOR version.
  2. Be documented in the README, under a section called Upgrading, with the steps necessary to upgrade to the new (specified) MAJOR version.

3. Compatibility

We officially support compatibility with the current and the previous minor version of Kubernetes. Generated resources should use the latest possible API versions compatible with these versions.

For extended backwards compatibility, conditional logic based on capabilities may be used (see built-in objects).

4. Chart metadata

The accompanying Chart.yaml file should be as complete as possible.

The following fields are mandatory:

  • name (the chart's directory)
  • home
  • version
  • appVersion
  • description
  • maintainers (GitHub usernames)

5. Dependencies

Stable charts should not depend on charts in the incubator.

6. Resource Metadata Labels

Resources and labels should follow some conventions. The standard resource metadata (metadata.labels and spec.template.metadata.labels) should be this:

name: {{ include "myapp.fullname" . }}
labels:
  app.kubernetes.io/name: {{ include "myapp.name" . }}
  app.kubernetes.io/instance: {{ .Release.Name }}
  app.kubernetes.io/managed-by: {{ .Release.Service }}
  helm.sh/chart: {{ include "myapp.chart" . }}

If a chart has multiple components, the app.kubernetes.io/component label should be added (for example, app.kubernetes.io/component: server).

The resource name should get the component as suffix (for example, name: {{ include "myapp.fullname" . }}-server).

Templates must be namespaced. This is helm create's default behavior with Helm 2.7 or higher. For example, app.kubernetes.io/name should use the name template, not fullname, as is still the case with older charts.

7. Labels

7.1. Deployments, StatefulSets, DaemonSets Selectors

spec.selector.matchLabels must be specified. The standard selector should be this:

selector:
  matchLabels:
    app.kubernetes.io/name: {{ include "myapp.name" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}

If a chart has multiple components, a component label should be added to the selector (see above).

spec.selector.matchLabels defined in Deployments/StatefulSets/DaemonSets >=v1/beta2 must not contain the helm.sh/chart label or any label containing a version of the chart, because the selector is immutable.

The chart label string contains the version; if specified, whenever the Chart.yaml version changes, Helm's attempt to change the immutable field would cause the upgrade to fail.

7.1.1. Fixing selectors

For Deployments, StatefulSets, DaemonSets apps/v1beta1 or extensions/v1beta1
  • Set spec.selector.matchLabels if missing
  • Remove the helm.sh/chart label in spec.selector.matchLabels
  • Bump the patch version of the Chart
For Deployments, StatefulSets, DaemonSets >= apps/v1beta2
  • Remove the helm.sh/chart label in spec.selector.matchLabels
  • Bump the major version of the chart -- it's a breaking change

7.2. Service selectors

Label selectors for services must have both app.kubernetes.io/name and app.kubernetes.io/instance labels.

selector:
  app.kubernetes.io/name: {{ include "myapp.name" . }}
  app.kubernetes.io/instance: {{ .Release.Name }}

If a chart has multiple components, an app.kubernetes.io/component label should be added to the selector.

7.3. Persistence Labels

7.3.1. StatefulSet

In case of a Statefulset, spec.volumeClaimTemplates.metadata.labels must have both app.kubernetes.io/name and app.kubernetes.io/instance labels, and must not contain helm.sh/chart or any label containing a version of the chart, because spec.volumeClaimTemplates is immutable.

labels:
  app.kubernetes.io/name: {{ include "myapp.name" . }}
  app.kubernetes.io/instance: {{ .Release.Name }}

If a chart has multiple components, an app.kubernetes.io/component label should be added to the selector.

7.3.2. PersistentVolumeClaim

In case of a PersistentVolumeClaim, matchLabels should not be specified, as it'd prevent automatic PersistentVolume provisioning.

8. Configuration

  • Docker images should be configurable. Image tags should use stable versions.
image:
  repository: myapp
  tag: 1.2.3
  pullPolicy: IfNotPresent
  • The use of the default function should be avoided if possible in favor of defaults in values.yaml.
  • It is usually best to not specify defaults for resources and to just provide sensible values that are commented out as a recommendation, especially when resources are rather high. This makes it easier to test charts on small clusters or Minikube. Setting resources should generally be a conscious choice of the user.

9. Kubernetes native workloads

When reviewing charts that contain workloads such as Deployments, StatefulSets, DaemonSets, and Jobs the below points should be considered. These are to be seen as best practices rather than strict rules.

  • Create workloads that are stateless and long-running (servers) as Deployments. Deployments, in turn, create ReplicaSets.
  • ReplicaSets or ReplicationControllers should be avoided as workload types, unless there is a compelling reason.
  • Create workloads that are stateful, such as databases, key-value stores, message queues, and in-memory caches, as StatefulSets.
  • Deployments and StatefulSets should configure their workloads with a Pod Disruption Budget for high availability.
  • Configure interpod anti-affinity for workloads that replicate data,such as databases, KV stores, etc.
  • Have a default workload update strategy configured that is suitable for your chart.
  • Create Batch workloads using Jobs.
  • Create workloads with the latest supported API version.
  • Do not provide hard resource limits to workloads and leave them configurable unless required.
  • Configure complex pre-app setups using init containers when possible.

10. Persistence

  • Persistence should be enabled by default
  • PVCs should support specifying an existing claim
  • Storage class should be empty by default so that the default storage class is used
  • All options should be shown in README.md

Sample persistence section in values.yaml:

persistence:
  enabled: true
  ## If defined, storageClassName: <storageClass>
  ## If set to "-", storageClassName: "", which disables dynamic provisioning
  ## If undefined (the default) or set to null, no storageClassName spec is
  ##   set, choosing the default provisioner.  (gp2 on AWS, standard on
  ##   GKE, AWS & OpenStack)
  ##
  storageClass: ""
  accessMode: ReadWriteOnce
  size: 10Gi
  # existingClaim: ""

Sample pod spec within a deployment:

volumes:
  - name: data
  {{- if .Values.persistence.enabled }}
    persistentVolumeClaim:
      claimName: {{ .Values.persistence.existingClaim | default (include "myapp.fullname" .) }}
  {{- else }}
    emptyDir: {}
  {{- end -}}

Sample pvc.yaml:

{{- if and .Values.persistence.enabled (not .Values.persistence.existingClaim) }}
kind: PersistentVolumeClaim
apiVersion: v1
metadata:
  name: {{ include "myapp.fullname" . }}
  labels:
    app.kubernetes.io/name: {{ include "myapp.name" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    app.kubernetes.io/managed-by: {{ .Release.Service }}
    helm.sh/chart: {{ include "myapp.chart" . }}
spec:
  accessModes:
    - {{ .Values.persistence.accessMode | quote }}
  resources:
    requests:
      storage: {{ .Values.persistence.size | quote }}
{{- if .Values.persistence.storageClass }}
{{- if (eq "-" .Values.persistence.storageClass) }}
  storageClassName: ""
{{- else }}
  storageClassName: "{{ .Values.persistence.storageClass }}"
{{- end }}
{{- end }}
{{- end }}

11. AutoScaling / HorizontalPodAutoscaler

  • Autoscaling should be disabled by default
  • All options should be shown in the README

Sample autoscaling section in values.yaml:

autoscaling:
  enabled: false
  minReplicas: 1
  maxReplicas: 5
  targetCPUUtilizationPercentage: 50
  targetMemoryUtilizationPercentage: 50

Sample hpa.yaml:

{{- if .Values.autoscaling.enabled }}
apiVersion: autoscaling/v2beta1
kind: HorizontalPodAutoscaler
metadata:
  name: {{ include "myapp.fullname" . }}
  labels:
    app.kubernetes.io/name: {{ include "myapp.name" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    app.kubernetes.io/managed-by: {{ .Release.Service }}
    helm.sh/chart: {{ include "myapp.chart" . }}
    app.kubernetes.io/component: "{{ .Values.name }}"
spec:
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: {{ include "myapp.fullname" . }}
  minReplicas: {{ .Values.autoscaling.minReplicas }}
  maxReplicas: {{ .Values.autoscaling.maxReplicas }}
  metrics:
    - type: Resource
      resource:
        name: cpu
        targetAverageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }}
    - type: Resource
      resource:
        name: memory
        targetAverageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }}
{{- end }}

12. Ingress

Sample ingress section in values.yaml:

ingress:
  enabled: false
  annotations: {}
    # kubernetes.io/ingress.class: nginx
    # kubernetes.io/tls-acme: "true"
  path: /
  hosts:
    - chart-example.test
  tls: []
  #  - secretName: chart-example-tls
  #    hosts:
  #      - chart-example.test

Sample ingress.yaml:

{{- if .Values.ingress.enabled -}}
{{- if .Capabilities.APIVersions.Has "networking.k8s.io/v1beta1" }}
apiVersion: networking.k8s.io/v1beta1
{{ else }}
apiVersion: extensions/v1beta1
{{ end -}}
kind: Ingress
metadata:
  name: {{ include "myapp.fullname" }}
  labels:
    app.kubernetes.io/name: {{ include "myapp.name" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    app.kubernetes.io/managed-by: {{ .Release.Service }}
    helm.sh/chart: {{ include "myapp.chart" . }}
{{- with .Values.ingress.annotations }}
  annotations:
{{ toYaml . | indent 4 }}
{{- end }}
spec:
{{- if .Values.ingress.tls }}
  tls:
  {{- range .Values.ingress.tls }}
    - hosts:
      {{- range .hosts }}
        - {{ . | quote }}
      {{- end }}
      secretName: {{ .secretName }}
  {{- end }}
{{- end }}
  rules:
  {{- range .Values.ingress.hosts }}
    - host: {{ . | quote }}
      http:
        paths:
          - path: {{ .Values.ingress.path }}
            backend:
              serviceName: {{ include "myapp.fullname" }}
              servicePort: http
  {{- end }}
{{- end }}

Sample prepend logic for getting an application URL in NOTES.txt:

{{- if .Values.ingress.enabled }}
{{- range .Values.ingress.hosts }}
  http{{ if $.Values.ingress.tls }}s{{ end }}://{{ . }}{{ $.Values.ingress.path }}
{{- end }}

13. Formatting

  • Use two-space indentation when editing YAML files.
  • Keep list indentation style consistent within files.
  • Add a single space after {{ and before }}.

14. Documentation

README.md and NOTES.txt files are mandatory. README.md should contain a table listing all configuration options. NOTES.txt should provide accurate and useful information on how the chart can be used/accessed.

15. Tests

See the chart testing documentation.