Skip to content

Pass process environment variables by reference#4702

Merged
rhatdan merged 2 commits intocontainers:mainfrom
cbandy:env-passthrough
Apr 8, 2023
Merged

Pass process environment variables by reference#4702
rhatdan merged 2 commits intocontainers:mainfrom
cbandy:env-passthrough

Conversation

@cbandy
Copy link
Copy Markdown
Contributor

@cbandy cbandy commented Apr 2, 2023

What type of PR is this?

/kind api-change

Which issue(s) this PR fixes:

Special notes for your reviewer:

This PR contains only some of the changes discussed in #4688.

Does this PR introduce a user-facing change?

The first commit allows buildah run --env to lookup environment variables from the process environment using a rudimentary "glob" syntax.

The second commit does the same for buildah build --env, but alters the behavior when only a key is passed. Prior to this change, passing --env A when no A is defined in the environment sets A= (blank) in the container. After this change, A remains unset in the container.

cbandy added 2 commits April 2, 2023 18:19
See: containers#4688
Signed-off-by: Chris Bandy <bandy.chris@gmail.com>
See: containers#4688
Signed-off-by: Chris Bandy <bandy.chris@gmail.com>
@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 2, 2023
@cbandy
Copy link
Copy Markdown
Contributor Author

cbandy commented Apr 3, 2023

I should have opened this as a draft. This is only part of what has been discussed in #4688 because I quickly found differences in how the flags work across different commands.

  1. It seems reasonable to me that when there is no A in the host environment, buildah run --env A does nothing to the environment inside the container. This thinking/approach differs from buildah build, however:

  2. buildah build --env already handles key-only arguments. When the referenced key does not exist in the environment, it is assigned the empty string within the container.

    value := os.Getenv(key)
    envLine += fmt.Sprintf(" %q=%q", key, value)

  3. buildah config --env already handles key-only arguments. When the referenced key does not exist in the environment or is blank, an error is returned.

    value := os.Getenv(env[0])
    if value == "" {
    return fmt.Errorf("setting env %q: no value given", env[0])
    }

  4. buildah config --env already has a special syntax: the - key unsets all variables and the - suffix unsets one variable. Should I proceed with handling two suffixes such that *- means to unset all variables with a certain prefix? 🤔 It almost sounds better to add a buildah config --unsetenv flag such that * unsets all variables, but IIUC ordering sets with unsets would become impossible.

@rhatdan I stopped implementing after (2) for feedback. Which of the three behaviors in (1), (2), and (3) should I use while implementing (1) ? Should I alter the other (either 2 or 3) to match?

@TomSweeneyRedHat
Copy link
Copy Markdown
Member

/approve

@openshift-ci openshift-ci bot added the approved label Apr 5, 2023
CNIConfigDir: iopts.CNIConfigDir,
AddCapabilities: iopts.capAdd,
DropCapabilities: iopts.capDrop,
Env: iopts.env,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm surprised this isn't being used elsewhere and that it's not complaining loudly...

CNIConfigDir: iopts.CNIConfigDir,
AddCapabilities: iopts.capAdd,
DropCapabilities: iopts.capDrop,
Env: iopts.env,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The line below can be just used here, no reason to break it out.
Env: buildahcli.LookupEnvVarReferences(iopts.env, os.Environ())

//
// - When a string in specs is exactly "*", all keys and values in environ
// are returned.
func LookupEnvVarReferences(specs, environ []string) []string {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you planning on making podman use this function as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have no plans for that. It would be nice for Buildah and Podman to do these things consistently, but the tools satisfy different use cases.

I encountered difficulty making env flags consistent within Buildah.

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Apr 8, 2023

Since I want to get this into release, I am merging. But we could do some minor cleanups and add tests.
/approve
/lgtm
Thanks @cbandy

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cbandy, rhatdan, TomSweeneyRedHat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [TomSweeneyRedHat,rhatdan]

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

@rhatdan rhatdan merged commit 538c665 into containers:main Apr 8, 2023
@cbandy
Copy link
Copy Markdown
Contributor Author

cbandy commented Apr 9, 2023

Sorry for the delay. I was expecting a response to this question:

@rhatdan I stopped implementing after (2) for feedback. Which of the three behaviors in (1), (2), and (3) should I use while implementing (1) ? Should I alter the other (either 2 or 3) to match?

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Apr 10, 2023

@flouthoc Could you make these common between the two packages.

@flouthoc
Copy link
Copy Markdown
Collaborator

flouthoc commented May 2, 2023

Already modified podman to use this from buildah's package here: containers/podman#18130

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm locked - please file new issue/PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants