-
Notifications
You must be signed in to change notification settings - Fork 423
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
Embed go-runner into the image #426
Conversation
Signed-off-by: Jyoti Mahapatra <[email protected]>
Hi @jyotimahapatra. 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 Once the patch is verified, the new status will be reflected by the 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. |
Signed-off-by: Jyoti Mahapatra <[email protected]>
This reverts commit 175ae6a.
|
||
FROM golang:1.16 AS builder | ||
WORKDIR /go/src/github.com/kubernetes-sigs/aws-iam-authenticator | ||
COPY . . | ||
RUN make bin | ||
RUN chown 65532 _output/bin/aws-iam-authenticator | ||
|
||
FROM public.ecr.aws/eks-distro/kubernetes/go-runner:v0.9.0-eks-1-21-4 as go-runner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just use go-runner as the base and have 1 less step? Its already a distroless image (see the entire Dockerfile here). This is what we use for other CNCF-owned projects as well (cloud-provider-aws).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im trying to be consistent about which registry we use. Since we're using the public.ecr.aws/eks-distro-build-tooling
already , i preferred the go-runner from the same source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I didn't even see that we were pulling from the eks-distro version of go-runner (which I can't find the dockerfile for, so I'm not sure how its built). My suggestion was use go runner as the base instead of the minimal base to remove a step, but this works too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated latest tag. The eks-distro go-runner is just a build of upstream go-runner ref.
In case of cve findings, it will be much easier to update the eks-distro minimal base image, and hence keeping the dependency on everything eks-distro than using upstream go-runner. Of course we can revisit that later.
Dockerfile
Outdated
@@ -11,14 +11,17 @@ | |||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
ARG image=public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nonroot:2021-08-26-1630012071 | |||
ARG image=public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nonroot:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the tag but don't use latest, which results in less deterministic builds.
Looks good to me, just update the minimal base tag to something other than latest. |
Signed-off-by: Jyoti Mahapatra <[email protected]>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jyotimahapatra, nckturner 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 |
go-runner is essential in the minimal distroless image to redirect stderr logs. For example panic logs not captured by klog land up in stderr. Without go-runner, we'd lose these logs.
Signed-off-by: Jyoti Mahapatra [email protected]