From 05614ee139b9d5ce6d2daed50d9f0b3ed6d4e9a1 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 27 Sep 2021 09:50:07 +0200 Subject: [PATCH 1/3] always add short container id as net alias This matches what docker does. Also make sure the net aliases are also shown when the container is stopped. docker-compose uses this special alias entry to check if it is already correctly connected to the network. [1] Because we do not support static ips on network connect at the moment calling disconnect && connect will loose the static ip. Fixes #11748 [1] https://github.com/docker/compose/blob/0bea52b18dda3de8c28fcfb0c80cc08b8950645e/compose/service.py#L663-L667 Signed-off-by: Paul Holzinger --- libpod/container_internal_linux.go | 2 +- libpod/network/types/network.go | 4 +- libpod/networking_linux.go | 47 +++++++++++++++++++-- test/compose/test-compose | 1 + test/e2e/network_connect_disconnect_test.go | 18 ++++++-- test/system/500-networking.bats | 8 ++++ 6 files changed, 71 insertions(+), 9 deletions(-) diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index dd6f3878a2..867ecc2ada 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -1310,7 +1310,7 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti if err == nil && options.Name == "" && (!options.IgnoreStaticIP || !options.IgnoreStaticMAC) { // The file with the network.status does exist. Let's restore the // container with the same networks settings as during checkpointing. - aliases, err := c.runtime.state.GetAllNetworkAliases(c) + aliases, err := c.GetAllNetworkAliases() if err != nil { return err } diff --git a/libpod/network/types/network.go b/libpod/network/types/network.go index 68a32d4998..2fe4f3da2b 100644 --- a/libpod/network/types/network.go +++ b/libpod/network/types/network.go @@ -151,7 +151,9 @@ type PerNetworkOptions struct { // StaticIPv4 for this container. Optional. StaticIPs []net.IP `json:"static_ips,omitempty"` // Aliases contains a list of names which the dns server should resolve - // to this container. Can only be set when DNSEnabled is true on the Network. + // to this container. Should only be set when DNSEnabled is true on the Network. + // If aliases are set but there is no dns support for this network the + // network interface implementation should ignore this and NOT error. // Optional. Aliases []string `json:"aliases,omitempty"` // StaticMac for this container. Optional. diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index ec9d98b569..e4fcfc271f 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -48,6 +48,41 @@ const ( persistentCNIDir = "/var/lib/cni" ) +// GetAllNetworkAliases returns all configured aliases for this container. +// It also adds the container short ID as alias to match docker. +func (c *Container) GetAllNetworkAliases() (map[string][]string, error) { + allAliases, err := c.runtime.state.GetAllNetworkAliases(c) + if err != nil { + return nil, err + } + + // get the all attached networks, we cannot use GetAllNetworkAliases() + // since it returns nil if there are no aliases + nets, _, err := c.networks() + if err != nil { + return nil, err + } + + // add container short ID as alias to match docker + for _, net := range nets { + allAliases[net] = append(allAliases[net], c.config.ID[:12]) + } + return allAliases, nil +} + +// GetNetworkAliases returns configured aliases for this network. +// It also adds the container short ID as alias to match docker. +func (c *Container) GetNetworkAliases(netName string) ([]string, error) { + aliases, err := c.runtime.state.GetNetworkAliases(c, netName) + if err != nil { + return nil, err + } + + // add container short ID as alias to match docker + aliases = append(aliases, c.config.ID[:12]) + return aliases, nil +} + func (c *Container) getNetworkOptions() (types.NetworkOptions, error) { opts := types.NetworkOptions{ ContainerID: c.config.ID, @@ -61,7 +96,7 @@ func (c *Container) getNetworkOptions() (types.NetworkOptions, error) { if err != nil { return opts, err } - aliases, err := c.runtime.state.GetAllNetworkAliases(c) + aliases, err := c.GetAllNetworkAliases() if err != nil { return opts, err } @@ -872,7 +907,7 @@ func (r *Runtime) reloadContainerNetwork(ctr *Container) (map[string]types.Statu } } - aliases, err := ctr.runtime.state.GetAllNetworkAliases(ctr) + aliases, err := ctr.GetAllNetworkAliases() if err != nil { return nil, err } @@ -975,6 +1010,11 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e for _, net := range networks { cniNet := new(define.InspectAdditionalNetwork) cniNet.NetworkID = net + aliases, err := c.GetNetworkAliases(net) + if err != nil { + return nil, err + } + cniNet.Aliases = aliases settings.Networks[net] = cniNet } } @@ -1009,7 +1049,7 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e return nil, err } - aliases, err := c.runtime.state.GetNetworkAliases(c, name) + aliases, err := c.GetNetworkAliases(name) if err != nil { return nil, err } @@ -1253,6 +1293,7 @@ func (c *Container) NetworkConnect(nameOrID, netName string, aliases []string) e if !exists { return errors.Errorf("no network interface name for container %s on network %s", c.config.ID, netName) } + aliases = append(aliases, c.config.ID[:12]) opts.Networks = map[string]types.PerNetworkOptions{ netName: { Aliases: aliases, diff --git a/test/compose/test-compose b/test/compose/test-compose index 70db6dd553..beaf276fdf 100755 --- a/test/compose/test-compose +++ b/test/compose/test-compose @@ -247,6 +247,7 @@ function podman() { --storage-driver=vfs \ --root $WORKDIR/root \ --runroot $WORKDIR/runroot \ + --cni-config-dir $WORKDIR/cni \ "$@") echo -n "$output" >>$WORKDIR/output.log } diff --git a/test/e2e/network_connect_disconnect_test.go b/test/e2e/network_connect_disconnect_test.go index 217efdeec2..5f7c55d3f3 100644 --- a/test/e2e/network_connect_disconnect_test.go +++ b/test/e2e/network_connect_disconnect_test.go @@ -52,7 +52,6 @@ var _ = Describe("Podman network connect and disconnect", func() { }) It("network disconnect with net mode slirp4netns should result in error", func() { - SkipIfRootless("network connect and disconnect are only rootful") netName := "slirp" + stringid.GenerateNonCryptoID() session := podmanTest.Podman([]string{"network", "create", netName}) session.WaitWithDefaultTimeout() @@ -118,7 +117,6 @@ var _ = Describe("Podman network connect and disconnect", func() { }) It("network connect with net mode slirp4netns should result in error", func() { - SkipIfRootless("network connect and disconnect are only rootful") netName := "slirp" + stringid.GenerateNonCryptoID() session := podmanTest.Podman([]string{"network", "create", netName}) session.WaitWithDefaultTimeout() @@ -146,6 +144,13 @@ var _ = Describe("Podman network connect and disconnect", func() { ctr := podmanTest.Podman([]string{"create", "--name", "test", "--network", netName, ALPINE, "top"}) ctr.WaitWithDefaultTimeout() Expect(ctr).Should(Exit(0)) + cid := ctr.OutputToString() + + // network alias container short id is always added and shown in inspect + inspect := podmanTest.Podman([]string{"container", "inspect", "test", "--format", "{{(index .NetworkSettings.Networks \"" + netName + "\").Aliases}}"}) + inspect.WaitWithDefaultTimeout() + Expect(inspect).Should(Exit(0)) + Expect(inspect.OutputToString()).To(Equal("[" + cid[0:12] + "]")) con := podmanTest.Podman([]string{"network", "connect", netName, "test"}) con.WaitWithDefaultTimeout() @@ -153,7 +158,6 @@ var _ = Describe("Podman network connect and disconnect", func() { }) It("podman network connect", func() { - SkipIfRemote("This requires a pending PR to be merged before it will work") netName := "aliasTest" + stringid.GenerateNonCryptoID() session := podmanTest.Podman([]string{"network", "create", netName}) session.WaitWithDefaultTimeout() @@ -163,6 +167,7 @@ var _ = Describe("Podman network connect and disconnect", func() { ctr := podmanTest.Podman([]string{"run", "-dt", "--name", "test", "--network", netName, ALPINE, "top"}) ctr.WaitWithDefaultTimeout() Expect(ctr).Should(Exit(0)) + cid := ctr.OutputToString() exec := podmanTest.Podman([]string{"exec", "-it", "test", "ip", "addr", "show", "eth0"}) exec.WaitWithDefaultTimeout() @@ -184,6 +189,12 @@ var _ = Describe("Podman network connect and disconnect", func() { Expect(inspect).Should(Exit(0)) Expect(inspect.OutputToString()).To(Equal("2")) + // network alias container short id is always added and shown in inspect + inspect = podmanTest.Podman([]string{"container", "inspect", "test", "--format", "{{(index .NetworkSettings.Networks \"" + newNetName + "\").Aliases}}"}) + inspect.WaitWithDefaultTimeout() + Expect(inspect).Should(Exit(0)) + Expect(inspect.OutputToString()).To(Equal("[" + cid[0:12] + "]")) + exec = podmanTest.Podman([]string{"exec", "-it", "test", "ip", "addr", "show", "eth1"}) exec.WaitWithDefaultTimeout() Expect(exec).Should(Exit(0)) @@ -193,7 +204,6 @@ var _ = Describe("Podman network connect and disconnect", func() { rm.WaitWithDefaultTimeout() Expect(rm).Should(Exit(0)) Expect(rm.ErrorToString()).To(Equal("")) - }) It("podman network connect when not running", func() { diff --git a/test/system/500-networking.bats b/test/system/500-networking.bats index ef00d0366e..548f2d764b 100644 --- a/test/system/500-networking.bats +++ b/test/system/500-networking.bats @@ -415,6 +415,10 @@ load helpers run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").MacAddress}}" mac="$output" + # check network alias for container short id + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname\").Aliases}}" + is "$output" "\[${cid:0:12}\]" "short container id in network aliases" + run_podman network disconnect $netname $cid # check that we cannot curl (timeout after 3 sec) @@ -443,6 +447,10 @@ load helpers # connect a second network run_podman network connect $netname2 $cid + # check network2 alias for container short id + run_podman inspect $cid --format "{{(index .NetworkSettings.Networks \"$netname2\").Aliases}}" + is "$output" "\[${cid:0:12}\]" "short container id in network aliases" + # curl should work run curl --max-time 3 -s $SERVER/index.txt is "$output" "$random_1" "curl 127.0.0.1:/index.txt should work" From d0950f3efe2559049abbf45700309345c12a142f Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 27 Sep 2021 18:11:35 +0200 Subject: [PATCH 2/3] set --cni-config-dir for exit command Signed-off-by: Paul Holzinger --- pkg/specgen/generate/container_create.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index fefa9b4a9d..b6263332e9 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -498,6 +498,7 @@ func CreateExitCommandArgs(storageConfig types.StoreOptions, config *config.Conf "--log-level", logrus.GetLevel().String(), "--cgroup-manager", config.Engine.CgroupManager, "--tmpdir", config.Engine.TmpDir, + "--cni-config-dir", config.Network.NetworkConfigDir, } if config.Engine.OCIRuntime != "" { command = append(command, []string{"--runtime", config.Engine.OCIRuntime}...) From 1c8926285d1ecdfe201fe68896657573dcdc22b7 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 27 Sep 2021 14:21:50 +0200 Subject: [PATCH 3/3] move network alias validation to container create Podman 4.0 currently errors when you use network aliases for a network which has dns disabled. Because the error happens on network setup this can cause regression for old working containers. The network backend should not validate this. Instead podman should check this at container create time and also for network connect. Signed-off-by: Paul Holzinger --- libpod/network/cni/run.go | 3 --- libpod/network/cni/run_test.go | 41 +++++++++++++++++----------------- libpod/networking_linux.go | 8 +++++++ libpod/runtime_ctr.go | 22 ++++++++++++------ 4 files changed, 43 insertions(+), 31 deletions(-) diff --git a/libpod/network/cni/run.go b/libpod/network/cni/run.go index 0f91a407cb..bd873f89b6 100644 --- a/libpod/network/cni/run.go +++ b/libpod/network/cni/run.go @@ -186,9 +186,6 @@ outer: } return errors.Errorf("requested static ip %s not in any subnet on network %s", ip.String(), network.libpodNet.Name) } - if len(netOpts.Aliases) > 0 && !network.libpodNet.DNSEnabled { - return errors.New("cannot set aliases on a network without dns enabled") - } return nil } diff --git a/libpod/network/cni/run_test.go b/libpod/network/cni/run_test.go index 0a2c090e11..965203c2ae 100644 --- a/libpod/network/cni/run_test.go +++ b/libpod/network/cni/run_test.go @@ -966,6 +966,26 @@ var _ = Describe("run CNI", func() { }) }) + It("setup with aliases but dns disabled should work", func() { + runTest(func() { + defNet := types.DefaultNetworkName + intName := "eth0" + setupOpts := types.SetupOptions{ + NetworkOptions: types.NetworkOptions{ + ContainerID: stringid.GenerateNonCryptoID(), + Networks: map[string]types.PerNetworkOptions{ + defNet: { + InterfaceName: intName, + Aliases: []string{"somealias"}, + }, + }, + }, + } + _, err := libpodNet.Setup(netNSContainer.Path(), setupOpts) + Expect(err).ToNot(HaveOccurred()) + }) + }) + }) Context("invalid network setup test", func() { @@ -1052,27 +1072,6 @@ var _ = Describe("run CNI", func() { }) }) - It("setup with aliases but dns disabled", func() { - runTest(func() { - defNet := types.DefaultNetworkName - intName := "eth0" - setupOpts := types.SetupOptions{ - NetworkOptions: types.NetworkOptions{ - ContainerID: stringid.GenerateNonCryptoID(), - Networks: map[string]types.PerNetworkOptions{ - defNet: { - InterfaceName: intName, - Aliases: []string{"somealias"}, - }, - }, - }, - } - _, err := libpodNet.Setup(netNSContainer.Path(), setupOpts) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("cannot set aliases on a network without dns enabled")) - }) - }) - It("setup without networks", func() { runTest(func() { setupOpts := types.SetupOptions{ diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index e4fcfc271f..e792a410cc 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -1262,6 +1262,14 @@ func (c *Container) NetworkConnect(nameOrID, netName string, aliases []string) e // get network status before we connect networkStatus := c.getNetworkStatus() + network, err := c.runtime.network.NetworkInspect(netName) + if err != nil { + return err + } + if !network.DNSEnabled && len(aliases) > 0 { + return errors.Wrapf(define.ErrInvalidArg, "cannot set network aliases for network %q because dns is disabled", netName) + } + if err := c.runtime.state.NetworkConnect(c, netName, aliases); err != nil { return err } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 9a4dbf6261..93bfdd54b4 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -234,13 +234,6 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options .. } func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Container, retErr error) { - // Validate the container - if err := ctr.validate(); err != nil { - return nil, err - } - if ctr.config.IsInfra { - ctr.config.StopTimeout = 10 - } // normalize the networks to names // ocicni only knows about cni names so we have to make // sure we do not use ids internally @@ -265,11 +258,26 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai if err != nil { return nil, err } + network, err := r.network.NetworkInspect(netName) + if err != nil { + return nil, err + } + if !network.DNSEnabled { + return nil, errors.Wrapf(define.ErrInvalidArg, "cannot set network aliases for network %q because dns is disabled", netName) + } netAliases[netName] = aliases } ctr.config.NetworkAliases = netAliases } + // Validate the container + if err := ctr.validate(); err != nil { + return nil, err + } + if ctr.config.IsInfra { + ctr.config.StopTimeout = 10 + } + // Inhibit shutdown until creation succeeds shutdown.Inhibit() defer shutdown.Uninhibit()