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.

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 containerd config_path mirrors and remove nerdctl insecure_registry #10196

Merged

Conversation

yckaolalala
Copy link
Contributor

@yckaolalala yckaolalala commented Jun 7, 2023

What type of PR is this?

/kind bug
/kind feature

What this PR does / why we need it:

Currently, containerd_use_config_path disable use mirrors config .

Add containerd_registries_mirrors is similar as crio_registries

crio_registries:
  - prefix: docker.io
    insecure: false
    blocked: false
    unqualified: false
    location: registry-1.docker.io
    mirrors:
      - location: mirror.gcr.io
        insecure: false
        
containerd_registries_mirrors:
  - prefix: docker.io
    mirrors:
      - host: https://mirror.gcr.io
        capabilities: ["pull", "resolve"]
        skip_verify: false
      - host: https://registry-1.docker.io
        capabilities: ["pull", "resolve"]
        skip_verify: false

Currently, it appears that nerdctl is not reading the mirrors config in /etc/containerd/config.toml. nerdctl issue#981. However, nerdctl can read the mirrors config if hosts_dir is set in /etc/nerdctl/nerdctl.toml. Therefore, I have removed the check for containerd_use_config_path when writing to hosts.toml.

Remove the nerdctl_extra_flags and insecure_registry settings. Instead, nerdctl can pull from an insecure registry by configuring the containerd_registries_mirrors .

containerd_registries_mirrors:
  - prefix: 172.19.16.11:5000
    mirrors:
      - host: http://172.19.16.11:5000
        capabilities: ["pull", "resolve", "push"]
        skip_verify: true

Which issue(s) this PR fixes:

Fixes #9743

Special notes for your reviewer:

Should change default containerd_use_config_path to true?
Alternatively, can add a variable such as containerd_config_path_required_version: "1.5".

Does this PR introduce a user-facing change?:

[containerd] containerd config_path enable mirrors config using new variable `containerd_registries_mirrors`  (deprecate and remove `containerd_insecure_registries` for containrd and  `nerdctl_extra_flags` and `insecure_registry` setting for nerdctl

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 7, 2023
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @yckaolalala. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2023
@yankay yankay self-requested a review June 15, 2023 02:20
@yankay
Copy link
Member

yankay commented Jun 15, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 15, 2023
@MrFreezeex
Copy link
Member

MrFreezeex commented Jun 19, 2023

Hi @yckaolalala could you rebase your PR 🙏? I can't get the CI to pass because of some changes that reaches master :(.

@yckaolalala yckaolalala force-pushed the containerd-config-path-mirror branch from 4236156 to 45cf910 Compare June 20, 2023 01:20
@yckaolalala
Copy link
Contributor Author

packet_debian11-calico-upgrade test failed, This job used old config.toml and can not use new containerd_registries.
I add new containerd_registries_mirrors and keep containerd_registries

Copy link
Member

@MrFreezeex MrFreezeex left a comment

Choose a reason for hiding this comment

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

Thanks! Could you just rewrite the release notes accordingly then? Apart from that looks good, many thanks for this nice contribution 🙏
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2023
@yckaolalala yckaolalala force-pushed the containerd-config-path-mirror branch from 4307851 to fd2caa2 Compare June 21, 2023 03:53
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023
@MrFreezeex
Copy link
Member

Thanks for the rebase 🙏
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 21, 2023
@yckaolalala yckaolalala force-pushed the containerd-config-path-mirror branch from fd2caa2 to d004c6f Compare June 26, 2023 01:29
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 26, 2023
@yckaolalala
Copy link
Contributor Author

rebased

@MrFreezeex
Copy link
Member

Thanks 🙏
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2023
@yckaolalala yckaolalala force-pushed the containerd-config-path-mirror branch from d004c6f to 151b513 Compare July 6, 2023 09:09
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2023
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 11, 2023
@MrFreezeex
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2023
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

@yckaolalala Sorry for the delay, thank you for the PR !

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, MrFreezeex, yckaolalala

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023
@floryut floryut added kind/container-managers Containers section in the release note approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 16, 2023
@k8s-ci-robot k8s-ci-robot merged commit 77bda0d into kubernetes-sigs:master Aug 16, 2023
63 checks passed
@yankay yankay mentioned this pull request Aug 24, 2023
@sathieu
Copy link
Contributor

sathieu commented Sep 19, 2023

This is a breaking change. Can it me marked as such in the release notes?

@yankay
Copy link
Member

yankay commented Sep 21, 2023

This is a breaking change. Can it me marked as such in the release notes?

Thanks @sathieu ,
it has been changed , https://github.com/kubernetes-sigs/kubespray/releases/tag/v2.23.0

@sathieu
Copy link
Contributor

sathieu commented Sep 21, 2023

Thanks @yankay. Maybe it should be added to the Deprecation / Removal section too ?

@yankay
Copy link
Member

yankay commented Sep 21, 2023

HI @sathieu

I'm not sure, the Deprecation / Removal is used for the software remove in before release.
Adding ⚠️ to make it more noticed

@sathieu
Copy link
Contributor

sathieu commented Sep 21, 2023

Thanks @yankay This is great!

guytet pushed a commit to guytet/kubespray that referenced this pull request Oct 30, 2023
…ry (kubernetes-sigs#10196)

* Fix containerd_registries in config_path for mirrors and remove nerdctl global insecure_registry setting

* Make containerd hosts.toml mode 0640

* Add containerd_registries_mirrors and keep containerd_registries to pass packet_debian11-calico-upgrade
@VannTen
Copy link
Contributor

VannTen commented Mar 28, 2024

@yankay the releases note is not detailled enough.
In particular, this breaks the usage of container_registries, (not mentioned anywhere). Can you add that to 2.23 release
("Removal of containerd_registries variable, please migrate to containerd_registries_mirrors" + a link to the doc for that).

pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
…ry (kubernetes-sigs#10196)

* Fix containerd_registries in config_path for mirrors and remove nerdctl global insecure_registry setting

* Make containerd hosts.toml mode 0640

* Add containerd_registries_mirrors and keep containerd_registries to pass packet_debian11-calico-upgrade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/container-managers Containers section in the release note lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants