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

Add support for ECR as the kubernetes repo #68

Merged
merged 1 commit into from
Dec 11, 2019
Merged

Add support for ECR as the kubernetes repo #68

merged 1 commit into from
Dec 11, 2019

Conversation

aaroniscode
Copy link
Contributor

@aaroniscode aaroniscode commented Oct 6, 2019

Support already exists for a user to choose a private repository to use for the Kubernetes container images. For Cluster API AWS (CAPA), users may choose to leverage Amazon's hosted Docker container registry: Elastic Container Registry (ECR).

One use case for this is to use the VMware signed binaries. A user may may wish to push the VMware container images to ECR and create the CAPA AMI with the images pre-pulled.

kubeadm doesn't currently support authenticating with ECR: kubernetes/kubeadm#1820.

This PR makes the following changes:

  1. Adds a packer variable iam_instance_profile that can assigned to the Packer builder. ECR is a private repository and requires IAM permissions assigned for access.
  2. In the Ansible playbook, moves the providers role up earlier in the process. This is because the awscli is used to log into ECR and pull the images. Initially it was installed AFTER the kubernetes role. I read through the providers role and didn't see any issues with the change in ordering, but it would be good to have a second set of eyes confirm the ordering change is not an issue.
  3. A check is added during the kubernetes role to determine in the kubernetes_container_registry is a Elastic Container Registry. If so, a new set of ansible tasks ecrpull.yml logs into ECR and obtains temporary credentials. Then each image is pulled using the temporary credentials.

/assign @detiber

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 6, 2019
@moshloop
Copy link
Contributor

moshloop commented Oct 9, 2019

/hold

I think this use case is really specific, I think we need a mechanism to enable this, but it shouldn't be part of the core image-builder.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2019
@aaroniscode
Copy link
Contributor Author

@moshloop I don't think the use case is that specific. I'm making a few assumptions. Please let me know where you think my assumption is incorrect.

Assumption 1 -- Amazon Elastic Container Registry has widespread usage with AWS customers.

Assumption 2 -- Many (not an insignificant amount) users of image-builder will want to stamp images and NOT use the upstream container images hosted on Google Container Registry (GCR).

There may be a number of reasons why users would want to use their own or custom container images:

  • Using a patch that is available to some Kubernetes component, but not yet in a formal Kubernetes release
  • Using a vendor's signed images. Either for support or indemnity reasons

Any CAPA user that wants to use custom container images AND wants to use ECR would need the capability introduced in this PR.

I also think there is a valid use case to say this capability belongs in core image-builder:

  1. Core image-builder has support for AWS. ECR is a core part of AWS just like EC2 which is fully supported in image-builder
  2. According to one of the project Goals -- Support end users requirements to customize images for their business needs -- I think this falls into that goal.

Would be interested in your thoughts.

@moshloop
Copy link
Contributor

I agree that the use case is valid, my concern is in the path to achieving the end goal.

If we add an option to disable pre-pulling the images then it opens up other approaches:

  1. Consume a core image in another packer pipeline which includes the ecr pull step
  2. Provide an option to supply an arbitrary ansible-playbook during the image build + patch mechanism for the packer configs.

If this is urgent and 1 is not suitable, then I am not opposed to merging this with a TODO deprecated comment to remove once the extension mechanism is available.

@frapposelli
Copy link
Member

@moshloop this will eventually be fixed once containerd/containerd#6637 is closed.

ECR is a pretty popular container registry with users on AWS so I believe this PR should be treated as urgent if we want to foster utilization of image-builder for CAPA images

Copy link
Member

@frapposelli frapposelli left a comment

Choose a reason for hiding this comment

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

Adding comments.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019
@frapposelli
Copy link
Member

Travis PTAL

/assign @codenrhoden

@codenrhoden
Copy link
Contributor

/lgtm

I'd like to see the suggested comments incorporated so we don't lose the future TODO. I think this would also satisfy @moshloop request and according to #68 (comment), I'd like to see this one go in before the first tag. I know of end-users that will want this capability.

@frapposelli
Copy link
Member

/assign @aaroniscode

PTAL

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2019
@aaroniscode
Copy link
Contributor Author

thanks everyone for your feedback and review. I've incorporated the suggestions. I also rebased on master and tested using k8s 1.16.2 and private images on ECR.

@frapposelli
Copy link
Member

/lgtm
/assign @moshloop

Moshe, does this look good to you?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019
@frapposelli
Copy link
Member

ping @moshloop

@moshloop
Copy link
Contributor

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2019
@moshloop
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaroniscode, moshloop

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 Dec 11, 2019
@k8s-ci-robot k8s-ci-robot merged commit 5d836ce into kubernetes-sigs:master Dec 11, 2019
@figo figo mentioned this pull request Dec 12, 2019
@aaroniscode aaroniscode deleted the ecr_support branch December 20, 2019 01:24
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants