From 701aade26244684aff2447903953d7cb89fdf473 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Wed, 19 Mar 2025 13:19:34 +0100 Subject: [PATCH] Add --env and --unsetenv to podman update. The --env is used to add new environment variable to container or override the existing one. The --unsetenv is used to remove the environment variable. It is done by sharing "env" and "unsetenv" flags between both "update" and "create" commands and later handling these flags in the "update" command handler. The list of environment variables to add/remove is stored in newly added variables in the ContainerUpdateOptions. The Container.Update API call is refactored to take the ContainerUpdateOptions as an input to limit the number of its arguments. The Env and UnsetEnv lists are later handled using the envLib package and the Container is updated. The remote API is also extended to handle Env and EnvUnset. Fixes: #24875 Signed-off-by: Jan Kaluza --- cmd/podman/common/create.go | 30 ++++----- cmd/podman/containers/update.go | 16 +++++ docs/source/markdown/options/env.update.md | 13 ++++ .../markdown/options/unsetenv.update.md | 10 +++ docs/source/markdown/podman-update.1.md.in | 4 ++ libpod/container_api.go | 14 ++--- libpod/container_internal.go | 56 +++++++++++------ pkg/api/handlers/compat/containers.go | 9 ++- pkg/api/handlers/libpod/containers.go | 11 +++- pkg/api/handlers/types.go | 2 + pkg/bindings/containers/update.go | 2 + pkg/domain/entities/types/containers.go | 4 ++ pkg/domain/infra/abi/containers.go | 4 +- test/e2e/update_test.go | 62 +++++++++++++++++++ 14 files changed, 193 insertions(+), 44 deletions(-) create mode 100644 docs/source/markdown/options/env.update.md create mode 100644 docs/source/markdown/options/unsetenv.update.md diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go index ae18cee3d0..ff0865b821 100644 --- a/cmd/podman/common/create.go +++ b/cmd/podman/common/create.go @@ -114,21 +114,6 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions, ) _ = cmd.RegisterFlagCompletionFunc(envMergeFlagName, completion.AutocompleteNone) - envFlagName := "env" - createFlags.StringArrayP( - envFlagName, "e", Env(), - "Set environment variables in container", - ) - _ = cmd.RegisterFlagCompletionFunc(envFlagName, completion.AutocompleteNone) - - unsetenvFlagName := "unsetenv" - createFlags.StringArrayVar( - &cf.UnsetEnv, - unsetenvFlagName, []string{}, - "Unset environment default variables in container", - ) - _ = cmd.RegisterFlagCompletionFunc(unsetenvFlagName, completion.AutocompleteNone) - createFlags.BoolVar( &cf.UnsetEnvAll, "unsetenv-all", false, @@ -555,6 +540,21 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions, "Disable healthchecks on container", ) + envFlagName := "env" + createFlags.StringArrayP( + envFlagName, "e", Env(), + "Set environment variables in container", + ) + _ = cmd.RegisterFlagCompletionFunc(envFlagName, completion.AutocompleteNone) + + unsetenvFlagName := "unsetenv" + createFlags.StringArrayVar( + &cf.UnsetEnv, + unsetenvFlagName, []string{}, + "Unset environment default variables in container", + ) + _ = cmd.RegisterFlagCompletionFunc(unsetenvFlagName, completion.AutocompleteNone) + healthCmdFlagName := "health-cmd" createFlags.StringVar( &cf.HealthCmd, diff --git a/cmd/podman/containers/update.go b/cmd/podman/containers/update.go index 429bd96821..6f6ed362c4 100644 --- a/cmd/podman/containers/update.go +++ b/cmd/podman/containers/update.go @@ -163,6 +163,22 @@ func update(cmd *cobra.Command, args []string) error { } } + if cmd.Flags().Changed("env") { + env, err := cmd.Flags().GetStringArray("env") + if err != nil { + return err + } + opts.Env = env + } + + if cmd.Flags().Changed("unsetenv") { + env, err := cmd.Flags().GetStringArray("unsetenv") + if err != nil { + return err + } + opts.UnsetEnv = env + } + rep, err := registry.ContainerEngine().ContainerUpdate(context.Background(), opts) if err != nil { return err diff --git a/docs/source/markdown/options/env.update.md b/docs/source/markdown/options/env.update.md new file mode 100644 index 0000000000..dc7af0e491 --- /dev/null +++ b/docs/source/markdown/options/env.update.md @@ -0,0 +1,13 @@ +####> This option file is used in: +####> podman update +####> If file is edited, make sure the changes +####> are applicable to all of those. +#### **--env**, **-e**=*env* + +Add a value (e.g. env=*value*) to the container. Can be used multiple times. +If the value already exists in the container, it is overridden. +To remove an environment variable from the container, use the `--unsetenv` +option. + +Note that the env updates only affect the main container process after +the next start. diff --git a/docs/source/markdown/options/unsetenv.update.md b/docs/source/markdown/options/unsetenv.update.md new file mode 100644 index 0000000000..62ef47f0fb --- /dev/null +++ b/docs/source/markdown/options/unsetenv.update.md @@ -0,0 +1,10 @@ +####> This option file is used in: +####> podman update +####> If file is edited, make sure the changes +####> are applicable to all of those. +#### **--unsetenv**=*env* + +Unset environment variables from the container. + +Note that the env updates only affect the main container process after +the next start. diff --git a/docs/source/markdown/podman-update.1.md.in b/docs/source/markdown/podman-update.1.md.in index de8a7dc252..68b713f760 100644 --- a/docs/source/markdown/podman-update.1.md.in +++ b/docs/source/markdown/podman-update.1.md.in @@ -42,6 +42,8 @@ Updates the configuration of an existing container, allowing changes to resource @@option device-write-iops +@@option env.update + @@option health-cmd @@option health-interval @@ -90,6 +92,8 @@ Changing this setting resets the timer, depending on the state of the container. @@option restart +@@option unsetenv.update + ## EXAMPLEs diff --git a/libpod/container_api.go b/libpod/container_api.go index ddaa6ed950..47e1797a5f 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -14,8 +14,8 @@ import ( "github.com/containers/common/pkg/resize" "github.com/containers/podman/v5/libpod/define" "github.com/containers/podman/v5/libpod/events" + "github.com/containers/podman/v5/pkg/domain/entities" "github.com/containers/storage/pkg/archive" - spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -106,10 +106,10 @@ func (c *Container) Start(ctx context.Context, recursive bool) error { // Update updates the given container. // Either resource limits, restart policies, or HealthCheck configuration can be updated. -// Either resources, restartPolicy or changedHealthCheckConfiguration must not be nil. +// Either resources, restartPolicy or changedHealthCheckConfiguration must not be nil in the updateOptions. // If restartRetries is not nil, restartPolicy must be set and must be "on-failure". // Nil values of changedHealthCheckConfiguration are not updated. -func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string, restartRetries *uint, changedHealthCheckConfiguration *define.UpdateHealthCheckConfig) error { +func (c *Container) Update(updateOptions *entities.ContainerUpdateOptions) error { if !c.batched { c.lock.Lock() defer c.lock.Unlock() @@ -123,7 +123,7 @@ func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string return fmt.Errorf("container %s is being removed, cannot update: %w", c.ID(), define.ErrCtrStateInvalid) } - healthCheckConfig, changedHealthCheck, err := GetNewHealthCheckConfig(&HealthCheckConfig{Schema2HealthConfig: c.HealthCheckConfig()}, *changedHealthCheckConfiguration) + healthCheckConfig, changedHealthCheck, err := GetNewHealthCheckConfig(&HealthCheckConfig{Schema2HealthConfig: c.HealthCheckConfig()}, *updateOptions.ChangedHealthCheckConfiguration) if err != nil { return err } @@ -136,7 +136,7 @@ func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string } } - startupHealthCheckConfig, changedStartupHealthCheck, err := GetNewHealthCheckConfig(&StartupHealthCheckConfig{StartupHealthCheck: c.Config().StartupHealthCheckConfig}, *changedHealthCheckConfiguration) + startupHealthCheckConfig, changedStartupHealthCheck, err := GetNewHealthCheckConfig(&StartupHealthCheckConfig{StartupHealthCheck: c.Config().StartupHealthCheckConfig}, *updateOptions.ChangedHealthCheckConfiguration) if err != nil { return err } @@ -149,7 +149,7 @@ func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string } } - globalHealthCheckOptions, err := changedHealthCheckConfiguration.GetNewGlobalHealthCheck() + globalHealthCheckOptions, err := updateOptions.ChangedHealthCheckConfiguration.GetNewGlobalHealthCheck() if err != nil { return err } @@ -158,7 +158,7 @@ func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string } defer c.newContainerEvent(events.Update) - return c.update(resources, restartPolicy, restartRetries) + return c.update(updateOptions) } // Attach to a container. diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 7ec58be578..51be31196a 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -34,6 +34,8 @@ import ( "github.com/containers/podman/v5/libpod/events" "github.com/containers/podman/v5/libpod/shutdown" "github.com/containers/podman/v5/pkg/ctime" + "github.com/containers/podman/v5/pkg/domain/entities" + envLib "github.com/containers/podman/v5/pkg/env" "github.com/containers/podman/v5/pkg/lookup" "github.com/containers/podman/v5/pkg/rootless" "github.com/containers/podman/v5/pkg/selinux" @@ -2793,11 +2795,11 @@ func (c *Container) extractSecretToCtrStorage(secr *ContainerSecret) error { // Update a container's resources or restart policy after creation. // At least one of resources or restartPolicy must not be nil. -func (c *Container) update(resources *spec.LinuxResources, restartPolicy *string, restartRetries *uint) error { - if resources == nil && restartPolicy == nil { +func (c *Container) update(updateOptions *entities.ContainerUpdateOptions) error { + if updateOptions.Resources == nil && updateOptions.RestartPolicy == nil { return fmt.Errorf("must provide at least one of resources and restartPolicy to update a container: %w", define.ErrInvalidArg) } - if restartRetries != nil && restartPolicy == nil { + if updateOptions.RestartRetries != nil && updateOptions.RestartPolicy == nil { return fmt.Errorf("must provide restart policy if updating restart retries: %w", define.ErrInvalidArg) } @@ -2810,38 +2812,50 @@ func (c *Container) update(resources *spec.LinuxResources, restartPolicy *string oldRestart := c.config.RestartPolicy oldRetries := c.config.RestartRetries - if restartPolicy != nil { - if err := define.ValidateRestartPolicy(*restartPolicy); err != nil { + if updateOptions.RestartPolicy != nil { + if err := define.ValidateRestartPolicy(*updateOptions.RestartPolicy); err != nil { return err } - if restartRetries != nil { - if *restartPolicy != define.RestartPolicyOnFailure { + if updateOptions.RestartRetries != nil { + if *updateOptions.RestartPolicy != define.RestartPolicyOnFailure { return fmt.Errorf("cannot set restart policy retries unless policy is on-failure: %w", define.ErrInvalidArg) } } - c.config.RestartPolicy = *restartPolicy - if restartRetries != nil { - c.config.RestartRetries = *restartRetries + c.config.RestartPolicy = *updateOptions.RestartPolicy + if updateOptions.RestartRetries != nil { + c.config.RestartRetries = *updateOptions.RestartRetries } else { c.config.RestartRetries = 0 } } - if resources != nil { + if updateOptions.Resources != nil { if c.config.Spec.Linux == nil { c.config.Spec.Linux = new(spec.Linux) } - resourcesToUpdate, err := json.Marshal(resources) + resourcesToUpdate, err := json.Marshal(updateOptions.Resources) if err != nil { return err } if err := json.Unmarshal(resourcesToUpdate, c.config.Spec.Linux.Resources); err != nil { return err } - resources = c.config.Spec.Linux.Resources + updateOptions.Resources = c.config.Spec.Linux.Resources + } + + if len(updateOptions.Env) != 0 { + c.config.Spec.Process.Env = envLib.Slice(envLib.Join(envLib.Map(c.config.Spec.Process.Env), envLib.Map(updateOptions.Env))) + } + + if len(updateOptions.UnsetEnv) != 0 { + envMap := envLib.Map(c.config.Spec.Process.Env) + for _, e := range updateOptions.UnsetEnv { + delete(envMap, e) + } + c.config.Spec.Process.Env = envLib.Slice(envMap) } if err := c.runtime.state.SafeRewriteContainerConfig(c, "", "", c.config); err != nil { @@ -2852,22 +2866,28 @@ func (c *Container) update(resources *spec.LinuxResources, restartPolicy *string return err } - if c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStatePaused) && resources != nil { + if c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStatePaused) && + (updateOptions.Resources != nil || updateOptions.Env != nil || updateOptions.UnsetEnv != nil) { // So `podman inspect` on running containers sources its OCI spec from disk. // To keep inspect accurate we need to update the on-disk OCI spec. onDiskSpec, err := c.specFromState() if err != nil { return fmt.Errorf("retrieving on-disk OCI spec to update: %w", err) } - if onDiskSpec.Linux == nil { - onDiskSpec.Linux = new(spec.Linux) + if updateOptions.Resources != nil { + if onDiskSpec.Linux == nil { + onDiskSpec.Linux = new(spec.Linux) + } + onDiskSpec.Linux.Resources = updateOptions.Resources + } + if len(updateOptions.Env) != 0 || len(updateOptions.UnsetEnv) != 0 { + onDiskSpec.Process.Env = c.config.Spec.Process.Env } - onDiskSpec.Linux.Resources = resources if err := c.saveSpec(onDiskSpec); err != nil { logrus.Errorf("Unable to update container %s OCI spec - `podman inspect` may not be accurate until container is restarted: %v", c.ID(), err) } - if err := c.ociRuntime.UpdateContainer(c, resources); err != nil { + if err := c.ociRuntime.UpdateContainer(c, updateOptions.Resources); err != nil { return err } } diff --git a/pkg/api/handlers/compat/containers.go b/pkg/api/handlers/compat/containers.go index 0a26735bd1..e6fae78e84 100644 --- a/pkg/api/handlers/compat/containers.go +++ b/pkg/api/handlers/compat/containers.go @@ -782,7 +782,14 @@ func UpdateContainer(w http.ResponseWriter, r *http.Request) { restartRetries = &localRetries } - if err := ctr.Update(resources, restartPolicy, restartRetries, &define.UpdateHealthCheckConfig{}); err != nil { + updateOptions := &entities.ContainerUpdateOptions{ + Resources: resources, + ChangedHealthCheckConfiguration: &define.UpdateHealthCheckConfig{}, + RestartPolicy: restartPolicy, + RestartRetries: restartRetries, + } + + if err := ctr.Update(updateOptions); err != nil { utils.Error(w, http.StatusInternalServerError, fmt.Errorf("updating container: %w", err)) return } diff --git a/pkg/api/handlers/libpod/containers.go b/pkg/api/handlers/libpod/containers.go index bd6162c2ce..a7ed9283b3 100644 --- a/pkg/api/handlers/libpod/containers.go +++ b/pkg/api/handlers/libpod/containers.go @@ -455,7 +455,16 @@ func UpdateContainer(w http.ResponseWriter, r *http.Request) { return } - err = ctr.Update(resourceLimits, restartPolicy, restartRetries, &options.UpdateHealthCheckConfig) + updateOptions := &entities.ContainerUpdateOptions{ + Resources: resourceLimits, + ChangedHealthCheckConfiguration: &options.UpdateHealthCheckConfig, + RestartPolicy: restartPolicy, + RestartRetries: restartRetries, + Env: options.Env, + UnsetEnv: options.UnsetEnv, + } + + err = ctr.Update(updateOptions) if err != nil { utils.InternalServerError(w, err) return diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go index 278fa2316a..2d85470854 100644 --- a/pkg/api/handlers/types.go +++ b/pkg/api/handlers/types.go @@ -79,6 +79,8 @@ type UpdateEntities struct { specs.LinuxResources define.UpdateHealthCheckConfig define.UpdateContainerDevicesLimits + Env []string + UnsetEnv []string } type Info struct { diff --git a/pkg/bindings/containers/update.go b/pkg/bindings/containers/update.go index 786802434e..1279095b92 100644 --- a/pkg/bindings/containers/update.go +++ b/pkg/bindings/containers/update.go @@ -30,6 +30,8 @@ func Update(ctx context.Context, options *types.ContainerUpdateOptions) (string, LinuxResources: *options.Resources, UpdateHealthCheckConfig: *options.ChangedHealthCheckConfiguration, UpdateContainerDevicesLimits: *options.DevicesLimits, + Env: options.Env, + UnsetEnv: options.UnsetEnv, } requestData, err := jsoniter.MarshalToString(updateEntities) if err != nil { diff --git a/pkg/domain/entities/types/containers.go b/pkg/domain/entities/types/containers.go index 1f16b54d23..a02a700cde 100644 --- a/pkg/domain/entities/types/containers.go +++ b/pkg/domain/entities/types/containers.go @@ -51,12 +51,16 @@ type ContainerUpdateOptions struct { // - DevicesLimits to Limit device // - RestartPolicy to change restart policy // - RestartRetries to change restart retries + // - Env to change the environment variables. + // - UntsetEnv to unset the environment variables. Specgen *specgen.SpecGenerator Resources *specs.LinuxResources DevicesLimits *define.UpdateContainerDevicesLimits ChangedHealthCheckConfiguration *define.UpdateHealthCheckConfig RestartPolicy *string RestartRetries *uint + Env []string + UnsetEnv []string } func (u *ContainerUpdateOptions) ProcessSpecgen() { diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index ff1d44ce35..52ff8797c6 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -1803,12 +1803,12 @@ func (ic *ContainerEngine) ContainerUpdate(ctx context.Context, updateOptions *e } container := containers[0].Container - resourceLimits, err := specgenutil.UpdateMajorAndMinorNumbers(updateOptions.Resources, updateOptions.DevicesLimits) + updateOptions.Resources, err = specgenutil.UpdateMajorAndMinorNumbers(updateOptions.Resources, updateOptions.DevicesLimits) if err != nil { return "", err } - if err = container.Update(resourceLimits, updateOptions.RestartPolicy, updateOptions.RestartRetries, updateOptions.ChangedHealthCheckConfiguration); err != nil { + if err = container.Update(updateOptions); err != nil { return "", err } return containers[0].ID(), nil diff --git a/test/e2e/update_test.go b/test/e2e/update_test.go index 04a8532209..7950b1ffe6 100644 --- a/test/e2e/update_test.go +++ b/test/e2e/update_test.go @@ -8,6 +8,7 @@ import ( "github.com/containers/storage/pkg/fileutils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gexec" ) var _ = Describe("Podman update", func() { @@ -243,4 +244,65 @@ var _ = Describe("Podman update", func() { podmanTest.CheckContainerSingleField(testCtr, restartPolicyName, "always") podmanTest.CheckContainerSingleField(testCtr, restartPolicyRetries, "0") }) + + It("podman update sets/unsets environment variables", func() { + testCtr := "test-ctr-name" + + // Test that the variable is not set. + ctr1 := podmanTest.Podman([]string{"run", "-t", "--name", testCtr, ALPINE, "printenv", "FOO"}) + ctr1.WaitWithDefaultTimeout() + Expect(ctr1).Should(Exit(1)) + + // Test that variable can be set and existing variables are not overridden. + update := podmanTest.Podman([]string{"update", "--env", "FOO=BAR", testCtr}) + update.WaitWithDefaultTimeout() + Expect(update).Should(ExitCleanly()) + + session := podmanTest.Podman([]string{"start", "--attach", testCtr}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + env := session.OutputToString() + Expect(env).To(ContainSubstring("BAR")) + + session = podmanTest.Podman([]string{"inspect", testCtr, "--format", "{{.Config.Env}}"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + env = session.OutputToString() + Expect(env).To(ContainSubstring("FOO=BAR")) + Expect(env).To(ContainSubstring("PATH=")) + + // Test that variable can be updated. + update = podmanTest.Podman([]string{"update", "--env", "FOO=RAB", testCtr}) + update.WaitWithDefaultTimeout() + Expect(update).Should(ExitCleanly()) + + session = podmanTest.Podman([]string{"start", "--attach", testCtr}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + env = session.OutputToString() + Expect(env).To(ContainSubstring("RAB")) + + session = podmanTest.Podman([]string{"inspect", testCtr, "--format", "{{.Config.Env}}"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + env = session.OutputToString() + Expect(env).To(ContainSubstring("FOO=RAB")) + Expect(env).To(ContainSubstring("PATH=")) + + // Test that variable can be unset. + update = podmanTest.Podman([]string{"update", "--unsetenv", "FOO", testCtr}) + update.WaitWithDefaultTimeout() + Expect(update).Should(ExitCleanly()) + + session = podmanTest.Podman([]string{"start", "--attach", testCtr}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(1)) + + session = podmanTest.Podman([]string{"inspect", testCtr, "--format", "{{.Config.Env}}"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + env = session.OutputToString() + Expect(env).ToNot(ContainSubstring("FOO")) + Expect(env).To(ContainSubstring("PATH=")) + }) })