Support for netavark backed podman(4.x+) - fixing #2882 #2883

wherka-ama · 2022-08-17T09:03:34Z

Adding support for netavark backed podman(4.x+)

… to the no_proxy list

linux-foundation-easycla · 2022-08-17T09:03:38Z

The committers listed above are authorized under a signed CLA.

✅ login: wherka-ama / name: Waldek Herka (8f7586b)

k8s-ci-robot · 2022-08-17T09:03:42Z

Welcome @wherka-ama!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-08-17T09:03:43Z

Hi @wherka-ama. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

BenTheElder

Thanks for the PR!

BenTheElder · 2022-08-17T09:04:29Z

pkg/cluster/internal/providers/podman/provision.go

 		noProxyJoined := strings.Join(noProxyList, ",")
 		envs[common.NOProxy] = noProxyJoined
 		envs[strings.ToLower(common.NOProxy)] = noProxyJoined
 	}
 	return envs, nil
 }

+type PodmanNetworks []struct {


This type shouldn’t be exported.

Corrected. Thanks!

BenTheElder · 2022-08-17T09:05:14Z

pkg/cluster/internal/providers/podman/provision.go

 		// Note: this is best effort based on the default CoreDNS spec
 		// https://github.com/kubernetes/dns/blob/master/docs/specification.md
 		// Any user created pod/service hostnames, namespaces, custom DNS services
 		// are expected to be no-proxied by the user explicitly.
-		noProxyList = append(noProxyList, ".svc", ".svc.cluster", ".svc.cluster.local")
+		noProxyList = append(noProxyList, ".svc", ".svc.cluster", ".svc.cluster.local", strings.Join([]string{cfg.Name, "control-plane"}, "-"))


Changes to this method seem unrelated to netavark support and should also be kept consistent with the docker implementation

This should be a separate commit or PR, and it should probably not reimplement naming the control plane endpoint with different logic.

@BenTheElder : agreed, this is a separate problem. Should I create a separate issue for that and take this part out from the current PR?

This should be a separate commit or PR, and it should probably not reimplement naming the control plane endpoint with different logic.

Agreed. I've taken it out and will create a separate issue + another PR as an attempt to tackle the no_proxy inconsistencies. Thanks!

aojea · 2022-08-17T09:20:27Z

/hold

holding waiting for podman developers to disclose more about the future plans of the network #2882 (comment)

we already have several places that fork based on the podman version, and we can't keep growing like that

…sigs#2882

aojea · 2022-08-17T09:57:48Z

/ok-to-test

aojea · 2022-08-17T09:59:14Z

pkg/cluster/internal/providers/podman/provision.go

 func getSubnets(networkName string) ([]string, error) {
-	// TODO: unmarshall json and get rid of this complex query
-	format := `{{ range (index (index (index (index . "plugins") 0 ) "ipam" ) "ranges")}}{{ index ( index . 0 ) "subnet" }} {{end}}`


I can't remember now exactly, but I think we only care about the first plugin and subnet

I can't remember now exactly, but I think we only care about the first plugin and subnet

@aojea : would you like me to change that to replicate this behaviour here as well? I'm not sure what was the reasoning behind the first implementation, so would be happy to follow your guidance.

yeah. match the first implementation, it is safer

@aojea : I have a couple of points before I actually push any further commits to this PR

At one point in the issue (current kind does not seem to support netavark backed rootless podman(4.x+) #2882 (comment)) you pointed at the existing checks which I could use as my guideline. I'm fine with the version check(if we are to stick with explicit flows), but the part where there was a check to see if we are in the Rootless mode doesn't seem appropriate to me. The trigger of the error here is the presence of the environment variables linked with http(s) proxy. It makes no difference whether we run it as root or not. I've tested it and we have a consistent behaviour here. It is indeed generic in that regard.

I did a bit of a refactoring of the code(patch attached) to be very explicit with the versions/layouts used so that allows on very easy decommissioning of the legacy flow when the time comes. However, I must stress it became excessively verbose and I count on your judgement here to decide if that's the direction we should take.

In the refactored version I'm referring to the first plugin/subnet - mimicking the old query.

@BenTheElder: I would welcome your opinion on that aspect as well.

patch.txt

aojea · 2022-08-17T14:56:44Z

/hold cancel

BenTheElder

/lgtm
/approve
thanks so much!

BenTheElder · 2022-08-17T17:22:49Z

pkg/cluster/internal/providers/podman/provision.go

 func getSubnets(networkName string) ([]string, error) {
-	// TODO: unmarshall json and get rid of this complex query


New TODO: unit test parsing with some known real data to prevent regressions. (doesn't need to be this PR, we should have done that before)

New TODO: unit test parsing with some known real data to prevent regressions. (doesn't need to be this PR, we should have done that before)

@BenTheElder : funny you mentioned that cause I thought there is something missing in that area indeed and I've already started working on unit tests to cover at least what I've touched ;-)
We can either do it in the next PR, which I'm happy to deliver pretty soon(couple of days max, but hopefully tomorrow) or we can put this one on hold and bundle it with unit tests. Please do let me know what's your preference and I'll go with it.

another PR when you have time would be great!! no rush, I'm trying to land one or two more fixes ahead of k8s 1.25 / kind v0.15 but I think unit tests can wait / are more forward thinking 😅

k8s-ci-robot · 2022-08-17T17:24:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, wherka-ama

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:

~~pkg/cluster/internal/providers/podman/OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Support for netavark backed podman(4.x+) and adding the control-plane…

8f7586b

… to the no_proxy list

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 17, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2022

k8s-ci-robot added the area/provider/podman Issues or PRs related to podman label Aug 17, 2022

k8s-ci-robot requested review from amwat and aojea August 17, 2022 09:03

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 17, 2022

BenTheElder reviewed Aug 17, 2022

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 17, 2022

Post-review corrections: hiding the structure

dce9b27

wherka-ama requested a review from BenTheElder August 17, 2022 09:28

Post-review corrections: removing bits unrelated to issue kubernetes-…

0945078

…sigs#2882

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2022

aojea reviewed Aug 17, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2022

BenTheElder reviewed Aug 17, 2022

View reviewed changes

k8s-ci-robot assigned BenTheElder Aug 17, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2022

k8s-ci-robot merged commit 8eb4a13 into kubernetes-sigs:main Aug 17, 2022

BenTheElder mentioned this pull request Aug 17, 2022

current kind does not seem to support netavark backed rootless podman(4.x+) #2882

Closed

aojea mentioned this pull request Aug 24, 2022

Error in creating cluster, podman is provider #2892

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for netavark backed podman(4.x+) - fixing #2882 #2883

Support for netavark backed podman(4.x+) - fixing #2882 #2883

wherka-ama commented Aug 17, 2022 •

edited

Loading

linux-foundation-easycla bot commented Aug 17, 2022 •

edited

Loading

k8s-ci-robot commented Aug 17, 2022

k8s-ci-robot commented Aug 17, 2022

BenTheElder left a comment

BenTheElder Aug 17, 2022

wherka-ama Aug 17, 2022

BenTheElder Aug 17, 2022

BenTheElder Aug 17, 2022

wherka-ama Aug 17, 2022

wherka-ama Aug 17, 2022

BenTheElder Aug 17, 2022

aojea commented Aug 17, 2022

aojea commented Aug 17, 2022

aojea Aug 17, 2022

wherka-ama Aug 17, 2022

aojea Aug 17, 2022

wherka-ama Aug 17, 2022 •

edited

Loading

aojea commented Aug 17, 2022

BenTheElder left a comment

BenTheElder Aug 17, 2022

wherka-ama Aug 17, 2022

BenTheElder Aug 17, 2022

k8s-ci-robot commented Aug 17, 2022

		func getSubnets(networkName string) ([]string, error) {
		// TODO: unmarshall json and get rid of this complex query

Support for netavark backed podman(4.x+) - fixing #2882 #2883

Support for netavark backed podman(4.x+) - fixing #2882 #2883

Conversation

wherka-ama commented Aug 17, 2022 • edited Loading

linux-foundation-easycla bot commented Aug 17, 2022 • edited Loading

k8s-ci-robot commented Aug 17, 2022

k8s-ci-robot commented Aug 17, 2022

BenTheElder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Aug 17, 2022

aojea commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wherka-ama Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

aojea commented Aug 17, 2022

BenTheElder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 17, 2022

wherka-ama commented Aug 17, 2022 •

edited

Loading

linux-foundation-easycla bot commented Aug 17, 2022 •

edited

Loading

wherka-ama Aug 17, 2022 •

edited

Loading