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.

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

Fix: When creating a list of files and images with ./generate_list.sh, included ingress-nginx/kube-webhook-certgen image in the list. #11787

Closed
wants to merge 8 commits into from
Closed

Fix: When creating a list of files and images with ./generate_list.sh, included ingress-nginx/kube-webhook-certgen image in the list. #11787

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
31708fe
fix download.yml
DearJey Dec 11, 2024
a46a04a
Merge branch 'kubernetes-sigs:master' into add-kube-webhook-certgen-i…
DearJey Dec 18, 2024
d47e051
fix comments
DearJey Dec 18, 2024
9d3e081
Merge branch 'kubernetes-sigs:master' into add-kube-webhook-certgen-i…
DearJey Dec 23, 2024
58df6a3
fix ingress_nginx_kube_webhook_certgen sha256
DearJey Dec 23, 2024
16d11b5
Merge branch 'kubernetes-sigs:master' into add-kube-webhook-certgen-i…
DearJey Dec 25, 2024
af5dd45
fix groups
DearJey Dec 25, 2024
cbfabd9
fix sha256
DearJey Dec 25, 2024
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
8 changes: 8 additions & 0 deletions roles/kubespray-defaults/defaults/main/download.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,14 @@ downloads:
groups:
- kube_node

ingress_nginx_kube_webhook_certgen:
repo: "{{ ingress_nginx_kube_webhook_certgen_image_repo }}"
tag: "{{ ingress_nginx_kube_webhook_certgen_image_tag }}"
sha256: "{{ ingress_nginx_kube_webhook_certgen_digest_checksum | default(None) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop the d(None). We should have valid checksums for our images.

Copy link
Author

@DearJey DearJey Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I deleted default(None) from this task.

sha256: "{{ ingress_nginx_kube_webhook_certgen_digest_checksum }}"

groups:
- kube_node
Copy link
Contributor

Choose a reason for hiding this comment

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

This would download the webhook image on all nodes, this will slow down kubespray on large clusters.
This should be scoped to smaller subset of nodes 🤔

Copy link
Author

@DearJey DearJey Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated it as described below.

    groups:
      - kube_control_plane
      - infra

Could you confirm if this task is correct?
commit: af5dd45

when: ingress_nginx_webhook_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be enabled: "{{ ingress_nginx_webhook_enabled }}" like the other in this file.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for comments!
I fixed it as descibed below.

enabled: "{{ ingress_nginx_webhook_enabled }}"

Dose this address your cocmments?
commit: d47e051


cert_manager_controller:
enabled: "{{ cert_manager_enabled }}"
container: true
Expand Down