harden pull-sandbox-image script

[ndbaker1]

Issue #, if available:

Description of changes:

• remove the pipefail so that authentication errors don't fast fail the workflow since it is ok if the ecr call fails.
• add additional bash shell hardening.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done

an empty credential pull for public repositories:

$ crictl pull --creds "AWS:" registry.k8s.io/pause:3.9
Image is up to date for sha256:e6f1816883972d4be47bd48879a08919b96afcd344132622e4d444987919323c

in the case where a region is malformed or incorrect, this will still succeed for a public image:

$ grep sandbox_image /etc/containerd/config.toml
sandbox_image = "registry.k8s.io/pause:3.9"

$ /etc/eks/containerd/pull-sandbox-image.sh
[root@ip-192-168-5-111 bin]# /etc/eks/containerd/pull-sandbox-image.sh

Provided region_name '9' doesn't match a supported format.
Attempt 1 of 3

Provided region_name '9' doesn't match a supported format.
Attempt 2 of 3

Provided region_name '9' doesn't match a supported format.
Attempt 3 of 3

Provided region_name '9' doesn't match a supported format.
Unable to retrieve the ECR password. Image pull may not be properly authenticated.

Image is up to date for sha256:e6f1816883972d4be47bd48879a08919b96afcd344132622e4d444987919323c

# verify "caching" works
$ /etc/eks/containerd/pull-sandbox-image.sh
Skipping ... sandbox_image 'registry.k8s.io/pause:3.9' is already present

existing ecr uris:

$ grep sandbox_image /etc/containerd/config.toml
sandbox_image = "602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause:3.5"

$ /etc/eks/containerd/pull-sandbox-image.sh
Image is up to date for sha256:6996f8da07bd405c6f82a549ef041deda57d1d658ec20a78584f9f436c9a3bb7

$ /etc/eks/containerd/pull-sandbox-image.sh
Skipping ... sandbox_image '602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause:3.5' is already present

See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.

[ndbaker1]

built on top of #1648 and #1646
@dims @cartermckinnon can you guys check me that this covers all the bases?

[dims]

one problem here is ... if the sandbox_image is specified from another region ( different from where the node is running in :( )

[dims]

could we check if the echo/cut version (from the sandbox_image) is present in aws account list-regions? and skip if not?

[ndbaker1]

oh but this would require new permissions on the role 😖

[ndbaker1]

are there docs stating required naming conventions of ecr registries? I'm not 100% sure what i can/can't check for

[ndbaker1]

if the sandbox_image is specified from another region

but this is only for user provided images right (based on the assumptions for the eks generated uri), which I'm not sure had a solution even prior to this 🤔

[dims]

then we can let it slide and leave it to the imds variation. (cross fingers!)

[cartermckinnon]

I'm hesitant to touch this in this PR. Cutting the region out of the image reference has its caveats, for sure; but that logic has been in prod for over a year and it's proven solid. I think we should consider this change in a separate PR, since this one is mostly for mitigation

[ndbaker1]

that logic has been in prod for over a year and it's proven solid.

This logic has existed yes (https://github.com/awslabs/amazon-eks-ami/blame/master/files/pull-image.sh#L4), but its only been used in context where the image source was already known and decided by us right?

The only prior usage i know of is in

amazon-eks-ami/scripts/install-worker.sh

Line 484 in baef6f0

if /etc/eks/containerd/pull-image.sh "${img}"; then

[cartermckinnon]

pull-sandbox-image.sh used to call pull-image.sh instead of using crictl: 7fa037a#diff-57a6aadbbb1d3df65f4675ae80c562f7e406bcb11e41f6afb974043a2ede0aa0L11

[ndbaker1]

🤦 that escaped my mind (and my github searches)
yea i think pulling it out makes sense then since it won't break the expectation beyond whats already changed in last commit

[cartermckinnon]

/ci launch

[github-actions]

@cartermckinnon roger that! I've dispatched a workflow. 👍

[github-actions]

@cartermckinnon the workflow that you requested has completed. 🎉

Kubernetes version	Build	Launch	Test
1.23	success ✅	success ✅	skipped ⏭️
1.24	success ✅	success ✅	skipped ⏭️
1.25	success ✅	success ✅	skipped ⏭️
1.26	success ✅	success ✅	skipped ⏭️
1.27	success ✅	success ✅	skipped ⏭️
1.28	success ✅	success ✅	skipped ⏭️
1.29	success ✅	success ✅	skipped ⏭️