-
Notifications
You must be signed in to change notification settings - Fork 742
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
VPC-CNI minimal image builds #2146
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdn5126
force-pushed
the
minimal_build
branch
from
November 21, 2022 20:31
a3ccb92
to
57aa440
Compare
Nice! Do you know the full image size differences? |
Comparing upstream (v1.12.0) to my personal ECR: |
M00nF1sh
reviewed
Nov 28, 2022
M00nF1sh
reviewed
Nov 28, 2022
M00nF1sh
reviewed
Nov 28, 2022
M00nF1sh
reviewed
Nov 28, 2022
kishorj
reviewed
Nov 29, 2022
jdn5126
force-pushed
the
minimal_build
branch
from
November 29, 2022 23:30
10adf3c
to
efe208b
Compare
jayanthvn
reviewed
Dec 2, 2022
jayanthvn
reviewed
Dec 2, 2022
jayanthvn
reviewed
Dec 2, 2022
M00nF1sh
reviewed
Dec 6, 2022
achevuru
reviewed
Dec 6, 2022
achevuru
reviewed
Dec 6, 2022
achevuru
reviewed
Dec 6, 2022
achevuru
reviewed
Dec 6, 2022
achevuru
reviewed
Dec 6, 2022
Since init container is required to always run, let binary installation for external plugins happen in init container. This simplifies the main container entrypoint and the dockerfile for each image.
jdn5126
force-pushed
the
minimal_build
branch
from
December 6, 2022 22:25
ae3d538
to
cb59c07
Compare
achevuru
approved these changes
Dec 7, 2022
awesome work @jdn5126 🎉 |
jdn5126
added a commit
that referenced
this pull request
Dec 12, 2022
* create publisher with logger (#2119) * Add missing rules when NodePort support is disabled (#2026) * Add missing rules when NodePort support is disabled * the rules that need to be installed for NodePort support and SNAT support are very similar. The same traffic mark is needed for both. As a result, rules that are currently installed only when NodePort support is enabled should also be installed when external SNAT is disabled, which is the case by default. * remove "-m state --state NEW" from a rule in the nat table. This is always true for packets that traverse the nat table. * fix typo in one rule's name (extra whitespace). Fixes #2025 Co-authored-by: Quan Tian <[email protected]> Signed-off-by: Antonin Bas <[email protected]> * Fix typos and unit tests Signed-off-by: Antonin Bas <[email protected]> * Minor improvement to code comment Signed-off-by: Antonin Bas <[email protected]> * Address review comments * Delete legacy nat rule * Fix an unrelated log message Signed-off-by: Antonin Bas <[email protected]> Signed-off-by: Antonin Bas <[email protected]> Co-authored-by: Jayanth Varavani <[email protected]> Co-authored-by: Sushmitha Ravikumar <[email protected]> * downgrade test go.mod to align with root go.mod (#2128) * skip addon installation when addon info is not available (#2131) * Merging test/Makefile and test/go.mod to the root Makefil and go.mod, adjust the .github/workflows and integration test instructions (#2129) * update troubleshooting docs for CNI image (#2132) fix location where make command is run * fix env name in test script (#2136) * optionally allow CLUSTER_ENDPOINT to be used rather than the cluster-ip (#2138) * optionally allow CLUSTER_ENDPOINT to be used rather than the kubernetes cluster ip * remove check for kube-proxy * add version to readme * Add resources config option to cni metrics helper (#2141) * Add resources config option to cni metrics helper * Remove default-empty resources block; replace with conditional * Add metrics for ec2 api calls made by CNI and expose via prometheus (#2142) Co-authored-by: Jay Deokar <[email protected]> * increase workflow role duration to 4 hours (#2148) * Update golang 1.19.2 EKS-D (#2147) * Update golang * Move to EKS distro builds * [HELM]: Move CRD resources to a separate folder as per helm standard (#2144) Co-authored-by: Jay Deokar <[email protected]> * VPC-CNI minimal image builds (#2146) * VPC-CNI minimal image builds * update dependencies for ginkgo when running integration tests * address review comments and break up init main function * review comments for sysctl * Simplify binary installation, fix review comments Since init container is required to always run, let binary installation for external plugins happen in init container. This simplifies the main container entrypoint and the dockerfile for each image. * when IPAMD connection fails, try to teardown pod network using prevResult (#2145) * add env var to enable nftables (#2155) * fix failing weekly cron tests (#2154) * Deprecate AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER and remove no-op setter (#2153) * Deprecate AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER * update release version comments Signed-off-by: Antonin Bas <[email protected]> Co-authored-by: Jeffrey Nelson <[email protected]> Co-authored-by: Antonin Bas <[email protected]> Co-authored-by: Jayanth Varavani <[email protected]> Co-authored-by: Sushmitha Ravikumar <[email protected]> Co-authored-by: Jerry He <[email protected]> Co-authored-by: Brandon Wagner <[email protected]> Co-authored-by: Jonathan Ogilvie <[email protected]> Co-authored-by: Jay Deokar <[email protected]>
haouc
pushed a commit
to haouc/amazon-vpc-cni-k8s
that referenced
this pull request
Dec 13, 2022
* VPC-CNI minimal image builds * update dependencies for ginkgo when running integration tests * address review comments and break up init main function * review comments for sysctl * Simplify binary installation, fix review comments Since init container is required to always run, let binary installation for external plugins happen in init container. This simplifies the main container entrypoint and the dockerfile for each image.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Enhancement
Which issue does this PR fix:
N/A
What does this PR do / Why do we need it:
This pull request migrates the VPC-CNI and VPC-CNI init images to use EKS minimal build image. This PR applied the diff from #1726 and updated it to handle recent changes. It was also tested to be compatible with #2137 , but those changes were left commented out as #2137 is not yet complete.
The VPC CNI image is based on
public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-iptables:latest.2
, which installsiptables-legacy
andiptables-nft
, but setsiptables-legacy
as the default usingupdate-alternatives
.The VPC CNI init image is based on
public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-glibc:latest.2
. The reason I could not use the base image is that the binaries are built inpie
mode for security reasons, and building withpie
requires the loader to be loaded despite the binary being static.I considered splitting this into two PRs: one to add the Golang entrypoints and one to do the move, but concluded that it is easiest to do as one PR, and if there are any issues, it is simple to just revert the move.
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
N/A
Testing done on this change:
I manually verified that all integration tests pass against this change. Output for init container:
Output for VPC-CNI:
Automation added to e2e:
N/A
Will this PR introduce any new dependencies?:
No, it will actually reduce dependencies.
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
This will not break upgrades or downgrades. Upgrade and downgrade has been tested with a running cluster.
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
The image base is different.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.