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

Fix getting of gateway IP for podman driver #11300

Merged
merged 12 commits into from
May 15, 2021
Merged

Fix getting of gateway IP for podman driver #11300

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fe392b4
When getting a gateway IP for podman container inspect per-network da…
ilya-zuyev May 5, 2021
61c1b30
Fix linter errors
ilya-zuyev May 5, 2021
adc53aa
Fix error handling
ilya-zuyev May 5, 2021
01f76f7
Add debug logs
ilya-zuyev May 5, 2021
618d09a
Fix network settings parsing
ilya-zuyev May 6, 2021
829cc42
Fix network settings parsing
ilya-zuyev May 6, 2021
6cf6ba2
Fix network settings parsing
ilya-zuyev May 6, 2021
8d2b811
Remove debug logs
ilya-zuyev May 6, 2021
f4e142d
Simplify the code
ilya-zuyev May 6, 2021
6e2eae0
Simplify the code
ilya-zuyev May 6, 2021
a1ae930
Simplify the code
ilya-zuyev May 6, 2021
dcadd32
Add comment, improve template
ilya-zuyev May 14, 2021
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
43 changes: 39 additions & 4 deletions pkg/drivers/kic/oci/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,49 @@ func digDNS(ociBin, containerName, dns string) (net.IP, error) {
return ip, nil
}

// gatewayIP inspects oci container to find a gateway IP string
func gatewayIP(ociBin, containerName string) (string, error) {
rr, err := runCmd(exec.Command(ociBin, "container", "inspect", "--format", "{{.NetworkSettings.Gateway}}", containerName))
if err != nil {
return "", errors.Wrapf(err, "inspect gateway")
}
if gatewayIP := strings.TrimSpace(rr.Stdout.String()); gatewayIP != "" {
return gatewayIP, nil
}

// https://github.com/kubernetes/minikube/issues/11293
// need to check nested network
// check .NetworkSettings.Networks["cluster-name"].Gateway and then
// .NetworkSettings.Networks["bridge"|"podman"].Gateway
for _, network := range []string{containerName, defaultBridgeName(ociBin)} {
medyagh marked this conversation as resolved.
Show resolved Hide resolved
gatewayIP, err := networkGateway(ociBin, containerName, network)
if err != nil {
return "", err
}
if gatewayIP != "" {
return gatewayIP, nil
}
}
klog.Infof("Couldn't find gateway for container %s", containerName)
return "", nil
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be returning error ? instead of nil ?
how about we define an nil error before the for loop

        var gateErr error
	for _, network := range []string{containerName, defaultBridgeName(ociBin)} {....
	       if err != nil {
	      	       gateErr = err 
                }

	
	
	...}

return "", gateErr

Copy link
Contributor Author

@ilya-zuyev ilya-zuyev May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

don't think this is a good idea. it would break a lot. an empty gateway IP is a valid value and the corresponding err is expected to be nil in other places where containerGatewayIP is called

}

func networkGateway(ociBin, container, network string) (string, error) {
format := fmt.Sprintf("{{(index .NetworkSettings.Networks %q).Gateway}}", network)
rr, err := runCmd(exec.Command(ociBin, "container", "inspect", "--format", format, container))
if err != nil {
return "", errors.Wrapf(err, "inspect gateway")
}
return strings.TrimSpace(rr.Stdout.String()), nil
}

// containerGatewayIP gets the default gateway ip for the container
func containerGatewayIP(ociBin string, containerName string) (net.IP, error) {
rr, err := runCmd(exec.Command(ociBin, "container", "inspect", "--format", "{{.NetworkSettings.Gateway}}", containerName))
gatewayIP, err := gatewayIP(ociBin, containerName)
if err != nil {
return nil, errors.Wrapf(err, "inspect gateway")
}
ip := net.ParseIP(strings.TrimSpace(rr.Stdout.String()))
return ip, nil
return net.ParseIP(gatewayIP), nil
}

// ForwardedPort will return port mapping for a container using cli.
Expand Down Expand Up @@ -142,7 +177,7 @@ func podmanContainerIP(ociBin string, name string) (string, string, error) {
return "", "", errors.Wrapf(err, "podman inspect ip %s", name)
}
output := strings.TrimSpace(rr.Stdout.String())
if err == nil && output == "" { // podman returns empty for 127.0.0.1
medyagh marked this conversation as resolved.
Show resolved Hide resolved
if output == "" { // podman returns empty for 127.0.0.1
// check network, if the ip address is missing
ipv4, ipv6, err := dockerContainerIP(ociBin, name)
if err == nil {
Expand Down
20 changes: 13 additions & 7 deletions pkg/drivers/kic/oci/network_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,21 @@ const dockerDefaultBridge = "bridge"
// name of the default bridge network
const podmanDefaultBridge = "podman"

func defaultBridgeName(ociBin string) string {
switch ociBin {
case Docker:
return dockerDefaultBridge
case Podman:
return podmanDefaultBridge
default:
klog.Warningf("Unexpected oci: %v", ociBin)
return dockerDefaultBridge
}
}

// CreateNetwork creates a network returns gateway and error, minikube creates one network per cluster
func CreateNetwork(ociBin string, networkName string) (net.IP, error) {
var defaultBridgeName string
if ociBin == Docker {
defaultBridgeName = dockerDefaultBridge
}
if ociBin == Podman {
defaultBridgeName = podmanDefaultBridge
}
defaultBridgeName := defaultBridgeName(ociBin)
if networkName == defaultBridgeName {
klog.Infof("skipping creating network since default network %s was specified", networkName)
return nil, nil
Expand Down
4 changes: 0 additions & 4 deletions test/integration/functional_test_mount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no
t.Skip("skipping: mount broken on windows: https://github.com/kubernetes/minikube/issues/8303")
}

if GithubActionRunner() && PodmanDriver() {
t.Skip("skipping: https://github.com/kubernetes/minikube/issues/11293")
}

tempDir, err := ioutil.TempDir("", "mounttest")
defer func() { // clean up tempdir
err := os.RemoveAll(tempDir)
Expand Down