fix: image registry tls failed#3107
Conversation
Signed-off-by: redscholar <blacktiledhouse@gmail.com>
kubesphere-prow
Bot
added
release-note
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: redscholar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubesphere-prow
Bot
added
approved
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reorders the Ansible tasks for containerd, moving the installation and configuration block to the end of the file, after the certificate synchronization tasks. This change introduces two critical issues: first, the certificate synchronization will fail on new installations because the /etc/containerd directory is no longer created beforehand; second, because the configuration generation and service restart are nested within a conditional installation block, registry certificate updates will not be applied to existing nodes. It is recommended to explicitly ensure the directory structure exists before synchronization and to move the configuration and service management tasks outside of the version-check conditional block.
| command: containerd --version | ||
| register: containerd_install_version | ||
|
|
||
| - name: Containerd | Install and configure containerd if not present or version mismatch |
There was a problem hiding this comment.
Removing the installation and configuration block from the beginning of the task list means that the /etc/containerd directory is no longer created before the certificate synchronization task runs. On fresh installations, the subsequent copy tasks (which now run first) will likely fail because the destination directory structure does not yet exist. It is recommended to explicitly ensure the directory exists before attempting to copy the certificates.
| - name: Containerd | Install and configure containerd if not present or version mismatch | ||
| when: or (.containerd_install_version.error | empty | not) (.containerd_install_version.stdout | contains (printf " %s " .cri.containerd_version) | not) |
There was a problem hiding this comment.
The Install and configure block is conditional on the containerd version. Since the template task for config.toml and the service management commands are inside this block, they will be skipped if containerd is already installed. Consequently, if a user adds or updates registry certificates on an existing node, the config.toml will not be regenerated to include the new certificate paths, and the service will not be restarted to apply the changes. To ensure the TLS fix works for both new and existing installations, the configuration generation and service restart tasks should be moved outside of the version-check block.
1 participant
What type of PR is this?
/kind bug
What this PR does / why we need it:
Adjust the certificate transmission order so that containerd can correctly recognize it.
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: