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 nftables binary to the distroless-iptables image #3320

Merged

Conversation

danwinship
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds the nftables binary to the (now-somewhat-badly-named) distroless-iptables image, so that it will support the new kube-proxy nftables backend (KEP-3866).

Which issue(s) this PR fixes:

None
but it unblocks kubernetes/kubernetes#121046

Special notes for your reviewer:

We could create separate images for kube-proxy --proxy-mode iptables (which would contain iptables and the iptables-wrapper script) and kube-proxy --proxy-mode nftables (which would contain nftables and no iptables) but that seems like that would be bad, at least while nftables is in alpha, since it would make it much harder to switch between the two modes.

We could perhaps rename the image to distroless-kube-proxy-base or something, though that would imply it contains ipvs too, which I think it doesn't, although in that case I'm not sure how the e2e ipvs job works (but maybe we could just do the same thing for nftables?). It's not terrible to have it be called distroless-iptables anyway. At least for now.

Does this PR introduce a user-facing change?

The distroless-iptables image (and thus by extension, the official kube-proxy image)
now contains the nftables binaries as well as the iptables binaries.

cc @aojea @rikatz

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 11, 2023
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 11, 2023
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
for other folks to take a look as well

@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 11, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2023
@rikatz
Copy link
Contributor

rikatz commented Oct 11, 2023

/lgtm
Should rename it now to distroless-anytables :P

@aojea
Copy link
Member

aojea commented Oct 11, 2023

Should rename it now to distroless-anytables :P

lol

/lgtm

@aojea
Copy link
Member

aojea commented Oct 11, 2023

/hold cancel

I would not rename it, this is the kube-proxy base image , and it should support all the official modes, being nftables one of them per the reasons explained in the KEP https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/3866-nftables-proxy/README.md

build/common.sh:readonly KUBE_PROXY_BASE_IMAGE="${KUBE_PROXY_BASE_IMAGE:-$KUBE_BASE_IMAGE_REGISTRY/distroless-iptables:$__default_distroless_iptables_version}"

Thanks

@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 Oct 11, 2023
@aojea
Copy link
Member

aojea commented Oct 11, 2023

/hold

I forget we need to bump the version

@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 11, 2023
@aojea
Copy link
Member

aojea commented Oct 11, 2023

see e5e7b63 for reference

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

I bumped it from v0.3.3 to v0.4.0... adding nftables support seemed minor-version-bump-worthy

@aojea
Copy link
Member

aojea commented Oct 11, 2023

/lgtm

@cpanato, @xmudrii or someone from sig-release to unhold confirming they are ok with the minor version bump to 0.4.0 (I'm +1 on that)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2023
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

@danwinship are you going to promote the image as well?

/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 Oct 12, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, saschagrunert, xmudrii

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:
  • OWNERS [saschagrunert,xmudrii]

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 merged commit 11ddef3 into kubernetes:master Oct 12, 2023
11 checks passed
@danwinship danwinship deleted the now-with-100%-more-nftables branch October 12, 2023 15:56
@danwinship
Copy link
Contributor Author

@danwinship are you going to promote the image as well?

"This is a great image! I think everyone should use it!"

Um... I assume that's not what you meant?

@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2023

Well, the day you get tired of software developing you can work with marketing @danwinship

But specifically for image promotion: https://github.com/kubernetes/k8s.io/blob/3d65e59aceec210430ddc6d9f765028640ff3c07/k8s.gcr.io/images/k8s-staging-build-image/images.yaml#L235

@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2023

Checksum for image promotion should be at the end of this job: https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/post-release-push-image-distroless-iptables/1712502638131548160

https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/post-release-push-image-distroless-iptables/1712354775346974720

But @cpanato just did a 0.4.1 to bump also Go version on #3321 so probably we should go straight to 0.4.1.

@saschagrunert
Copy link
Member

@danwinship are you going to promote the image as well?

"This is a great image! I think everyone should use it!"

Um... I assume that's not what you meant?

No, but I appreciate the promotion! 🙏

But @cpanato just did a 0.4.1 to bump also Go version on #3321 so probably we should go straight to 0.4.1.

Good, thanks for checking!

@cpanato
Copy link
Member

cpanato commented Oct 13, 2023

did the promotion for both images just to be there :)

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. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants