Allows multiple imagePullSecrets in the helm chart. #4656

AlessioCasco · 2023-11-15T14:28:40Z

Hello

Proposed changes

This allows multiple imagePullSecrets in the helm chart as discussed in #4589

Adds 1 new helm value: imagePullSecretsNames that with time will replace imagePullSecretName.
For compatibility reasons, both values work now.

I have not edited any release or changelog file for now, let me know if this is automated or if you want me to add the info.

Tests:

Setting imagePullSecretsNames to something other than an array, raises:

Error: values don't meet the specifications of the schema(s) in the following chart(s):
nginx-ingress:
- controller.serviceAccount.imagePullSecretsNames: Invalid type. Expected: array, given: string

Setting imagePullSecretName to "" and imagePullSecretsNames to [] renders:

# Source: nginx-ingress/templates/controller-serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm

Setting imagePullSecretName to "secret_1" and imagePullSecretsNames to [] renders:

# Source: nginx-ingress/templates/controller-serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: secret_1

Setting imagePullSecretName to "" and imagePullSecretsNames to [secret_2_from_list] renders:

# Source: nginx-ingress/templates/controller-serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: secret_2_from_list

Setting imagePullSecretName to "secret_1" and imagePullSecretsNames to [secret_2_from_list] renders:

# Source: nginx-ingress/templates/controller-serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: secret_1
- name: secret_2_from_list

Setting imagePullSecretName to "secret_1" and imagePullSecretsNames to [secret_2_from_list, secret_3_from_list] renders:

# Source: nginx-ingress/templates/controller-serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: secret_1
- name: secret_2_from_list
- name: secret_3_from_list

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

haywoodsh · 2023-11-27T13:36:55Z

Hi @AlessioCasco I checked out your branch, and it seems to work for me! One thing I noticed, though, is that we currently don't have a mechanism in place to prevent both imagePullSecretName and imagePullSecretsNames from being used simultaneously.

One possible solution could be to introduce some restrictions to the JSON schema, like so:

"oneOf": [
   {
     "required": ["imagePullSecretName"],
     "not": {
       "required": ["imagePullSecretsNames"]
     }
   },
   {
     "required": ["imagePullSecretsNames"],
     "not": {
       "required": ["imagePullSecretName"]
     }
   }
 ]

for more information, see https://pre-commit.ci

AlessioCasco · 2023-11-30T15:47:50Z

Hi! @haywoodsh
I hope I have done what you asked. I must say that I LOOOOOOVE JSON SCHEMA! :-D
I know, it doesn't look pretty at all, but this is the only way I managed to make it work. The error is not perfect but acceptable IMHO.

Alternatives

If the solution doesn't look good to you we can also do:

Set imagePullSecretName to be both string or array, depending on how ppl set it up, and keep one variable only. The naming is not perfect I know.
Move the logic from the JSON schema to the templates.

Tests

I've tested all combinations and all seem to work well now:

Both set

imagePullSecretName: "test"
imagePullSecretsNames: [alessio,casco]

Returns

walk.go:74: found symbolic link in path: /Users/alessio/gitrep/AlessioCasco/kubernetes-ingress/charts/nginx-ingress/crds resolves to /Users/alessio/gitrep/AlessioCasco/kubernetes-ingress/config/crd/bases. Contents of linked file included and used
Error: values don't meet the specifications of the schema(s) in the following chart(s):
nginx-ingress:
- controller.serviceAccount: Must validate one and only one schema (oneOf)
- controller.serviceAccount.imagePullSecretName: String length must be less than or equal to 0

---0---

Both null

imagePullSecretName: ""
imagePullSecretsNames: []

renders

apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
---

---0---

Only one null

imagePullSecretName: ""

renders

apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
---

---0---

The other one null

imagePullSecretsNames: []

renders

apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
---

---0---

No values defined

apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
---

---0---

Only array set

 imagePullSecretsNames: [alessio,casco]

renders

kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: alessio
- name: casco
---

---0---

Only array set + null the other

 imagePullSecretName: ""
 imagePullSecretsNames: [alessio,casco]

renders

kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: alessio
- name: casco
---

---0---

Null the array + set the string

imagePullSecretName: "test"
imagePullSecretsNames: []

renders

apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: test

---0---

Only string set

imagePullSecretName: "test"

renders

apiVersion: v1
kind: ServiceAccount
metadata:
  name: nginx-nginx-ingress
  namespace: default
  labels:
    helm.sh/chart: nginx-ingress-1.0.2
    app.kubernetes.io/name: nginx-ingress
    app.kubernetes.io/instance: nginx
    app.kubernetes.io/version: "3.3.2"
    app.kubernetes.io/managed-by: Helm
imagePullSecrets:
- name: test
---

…o 4589

shaun-nx

@AlessioCasco thank you very much for providing such comprehensive tests!
I've approved & run the pipeline.
There is one reported linter issue with charts/nginx-ingress/README.md that needs to be fixed and then we should be good to merge 🙂

AlessioCasco · 2023-12-01T10:55:13Z

Hi!
Just saw that sorry, let me fix that now.

codecov · 2023-12-01T11:22:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (39f9fa3) 52.09% compared to head (6417a19) 52.09%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4656   +/-   ##
=======================================
  Coverage   52.09%   52.09%           
=======================================
  Files          59       59           
  Lines       17033    17033           
=======================================
  Hits         8873     8873           
  Misses       7862     7862           
  Partials      298      298

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlessioCasco · 2023-12-01T12:28:10Z

Hi!

If you roughly know already when you want to deprecate imagePullSecretName let me know and I will be more than happy to create the issue + PR at that time, otherwise feel free to assign to me the issue you'll create and I'll issue the PR.

shaun-nx · 2023-12-01T14:38:53Z

@AlessioCasco sounds good, and thank you again for contributing! 🎉
We will typically follow the regular depreciation pattern of 3 releases to give folks enough time to move over their configuration 🙂

Fixes #4589 Allows multiple imagePullSecrets in the helm chart.

2b4c0a0

AlessioCasco requested review from a team as code owners November 15, 2023 14:28

github-actions bot added documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart labels Nov 15, 2023

Merge branch 'main' into 4589

c27d1f4

AlessioCasco mentioned this pull request Nov 15, 2023

Allow multiple imagePullSecrets in the helm chart #4589

Closed

Merge branch 'main' into 4589

1298c8a

AlessioCasco added 2 commits November 29, 2023 12:08

Merge branch 'main' into 4589

105da57

Merge branch 'main' into 4589

aac8b24

shaun-nx linked an issue Nov 30, 2023 that may be closed by this pull request

Allow multiple imagePullSecrets in the helm chart #4589

Closed

AlessioCasco and others added 4 commits November 30, 2023 15:10

Merge branch 'main' into 4589

11cb0f7

imagePullSecretName and imagePullSecretsNames mutually exclusive

f4152d1

[pre-commit.ci] auto fixes from pre-commit.com hooks

5ed903f

for more information, see https://pre-commit.ci

Better error now

29b803a

Merge branch '4589' of github.com:AlessioCasco/kubernetes-ingress int…

acf16f9

…o 4589

shaun-nx approved these changes Dec 1, 2023

View reviewed changes

haywoodsh approved these changes Dec 1, 2023

View reviewed changes

Fixed md013

6417a19

shaun-nx merged commit 601f471 into nginxinc:main Dec 1, 2023
62 checks passed

AlessioCasco deleted the 4589 branch December 1, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows multiple imagePullSecrets in the helm chart. #4656

Allows multiple imagePullSecrets in the helm chart. #4656

AlessioCasco commented Nov 15, 2023 •

edited

Loading

haywoodsh commented Nov 27, 2023

AlessioCasco commented Nov 30, 2023 •

edited

Loading

shaun-nx left a comment

AlessioCasco commented Dec 1, 2023

codecov bot commented Dec 1, 2023

AlessioCasco commented Dec 1, 2023

shaun-nx commented Dec 1, 2023 •

edited

Loading

Allows multiple imagePullSecrets in the helm chart. #4656

Allows multiple imagePullSecrets in the helm chart. #4656

Conversation

AlessioCasco commented Nov 15, 2023 • edited Loading

Proposed changes

Tests:

Checklist

haywoodsh commented Nov 27, 2023

AlessioCasco commented Nov 30, 2023 • edited Loading

Alternatives

Tests

Both set

Both null

Only one null

The other one null

No values defined

Only array set

Only array set + null the other

Null the array + set the string

Only string set

shaun-nx left a comment

Choose a reason for hiding this comment

AlessioCasco commented Dec 1, 2023

codecov bot commented Dec 1, 2023

Codecov Report

AlessioCasco commented Dec 1, 2023

shaun-nx commented Dec 1, 2023 • edited Loading

AlessioCasco commented Nov 15, 2023 •

edited

Loading

AlessioCasco commented Nov 30, 2023 •

edited

Loading

shaun-nx commented Dec 1, 2023 •

edited

Loading