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

[WIP] Defensive code in pull-sandbox-image.sh for custom AMI(s) #1648

Closed
wants to merge 1 commit into from
Closed

[WIP] Defensive code in pull-sandbox-image.sh for custom AMI(s) #1648

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 13 additions & 2 deletions files/pull-sandbox-image.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,25 @@ set -euo pipefail

source <(grep "sandbox_image" /etc/containerd/config.toml | tr -d ' ')

### skip if we don't have a sandbox_image set in config.toml
if [[ -z ${sandbox_image:-} ]]; then
echo >&2 "Skipping ... missing sandbox_image from /etc/containerd/config.toml"
exit 0
fi

### Short-circuit fetching sandbox image if its already present
if [[ "$(sudo ctr --namespace k8s.io image ls | grep $sandbox_image)" != "" ]]; then
if [[ $(sudo ctr --namespace k8s.io image ls | grep "${sandbox_image}") != "" ]]; then
exit 0
fi

# use the region that the sandbox image comes from for the ecr authentication,
# also mitigating the localzone isse: https://github.com/aws/aws-cli/issues/7043
region=$(echo "${sandbox_image}" | cut -f4 -d ".")
### skip if we don't find a region in the sandbox image
if [[ -z ${region:-} ]]; then
Comment on lines 19 to +21
Copy link
Member

Choose a reason for hiding this comment

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

just because we parse a non-empty region doesn't mean its valid right?
registry.k8s.io/pause:3.9 is going to parse 9

Copy link
Member Author

Choose a reason for hiding this comment

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

correct, we need more validation!

echo >&2 "Skipping ... missing region in sandbox_image"
exit 0
fi

MAX_RETRIES=3

Expand All @@ -29,7 +40,7 @@ function retry() {
done
}

ecr_password=$(retry aws ecr get-login-password --region $region)
ecr_password=$(retry aws ecr get-login-password --region "${region}")
Copy link
Member

@ndbaker1 ndbaker1 Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if we add carter's fix https://github.com/awslabs/amazon-eks-ami/pull/1646/files so that we don't break the whole workflow on this failing, the crictl calls should be fine even if the ecr_password is empty but we'd need to remove the explicit exit 1

tested empty

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

Copy link
Member Author

Choose a reason for hiding this comment

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

agree

if [[ -z ${ecr_password} ]]; then
echo >&2 "Unable to retrieve the ECR password."
exit 1
Expand Down
Loading