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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Aug 17, 2022
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f7586b
Support for netavark backed podman(4.x+) and adding the control-plane…
wherka-ama Aug 17, 2022
dce9b27
Post-review corrections: hiding the structure
wherka-ama Aug 17, 2022
0945078
Post-review corrections: removing bits unrelated to issue #2882
wherka-ama Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 45 additions & 5 deletions pkg/cluster/internal/providers/podman/provision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package podman

import (
"context"
"encoding/json"
"fmt"
"net"
"path/filepath"
Expand Down Expand Up @@ -264,15 +265,54 @@ func getProxyEnv(cfg *config.Cluster, networkName string) (map[string]string, er
return envs, nil
}

type podmanNetworks []struct {
// v4+
Subnets []struct {
Subnet string `json:"subnet"`
Gateway string `json:"gateway"`
} `json:"subnets"`
// v3 and anything still using CNI/IPAM
Plugins []struct {
Ipam struct {
Ranges [][]struct {
Gateway string `json:"gateway"`
Subnet string `json:"subnet"`
} `json:"ranges"`
} `json:"ipam,omitempty"`
} `json:"plugins"`
}

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

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 😅

format := `{{ range (index (index (index (index . "plugins") 0 ) "ipam" ) "ranges")}}{{ index ( index . 0 ) "subnet" }} {{end}}`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. match the first implementation, it is safer

Copy link
Contributor Author

@wherka-ama wherka-ama Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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.
  3. 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

cmd := exec.Command("podman", "network", "inspect", "-f", format, networkName)
lines, err := exec.OutputLines(cmd)
cmd := exec.Command("podman", "network", "inspect", networkName)
out, err := exec.Output(cmd)

if err != nil {
return nil, errors.Wrap(err, "failed to get subnets")
}
return strings.Split(strings.TrimSpace(lines[0]), " "), nil

networks := podmanNetworks{}
jsonErr := json.Unmarshal([]byte(out), &networks)
if jsonErr != nil {
return nil, errors.Wrap(jsonErr, "failed to get subnets")
}
subnets := []string{}
for _, network := range networks {
if len(network.Subnets) > 0 {
for _, subnet := range network.Subnets {
subnets = append(subnets, subnet.Subnet)
}
}
if len(network.Plugins) > 0 {
for _, plugin := range network.Plugins {
for _, r := range plugin.Ipam.Ranges {
for _, rr := range r {
subnets = append(subnets, rr.Subnet)
}
}
}
}
}
return subnets, nil
}

// generateMountBindings converts the mount list to a list of args for podman
Expand Down