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.

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

feat: add exec-plugin argument and environment support #5316

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions kyaml/fn/runtime/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ type Filter struct {
// Args are the arguments to the executable
Args []string `yaml:"args,omitempty"`

// Env is exposed to the environment
Env []string `yaml:"env,omitempty"`

// WorkingDir is the working directory that the executable
// should run in
WorkingDir string
Expand All @@ -35,6 +38,7 @@ func (c *Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {

func (c *Filter) Run(reader io.Reader, writer io.Writer) error {
cmd := exec.Command(c.Path, c.Args...) //nolint:gosec
cmd.Env = append(os.Environ(), c.Env...)
cmd.Stdin = reader
cmd.Stdout = writer
cmd.Stderr = os.Stderr
Expand Down
7 changes: 4 additions & 3 deletions kyaml/fn/runtime/exec/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
func TestFunctionFilter_Filter(t *testing.T) {
wd, err := os.Getwd()
require.NoError(t, err)
var tests = []struct {
tests := []struct {
name string
input []string
functionConfig string
Expand Down Expand Up @@ -57,8 +57,9 @@ metadata:
},
expectedError: "",
instance: exec.Filter{
Path: "sed",
Args: []string{"s/Deployment/StatefulSet/g"},
Path: "sh",
Env: []string{"TARGET=StatefulSet"},
Args: []string{"-c", `sed "s/Deployment/$TARGET/g"`},
WorkingDir: wd,
},
},
Expand Down
6 changes: 6 additions & 0 deletions kyaml/fn/runtime/runtimeutil/functiontypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ type FunctionSpec struct {

type ExecSpec struct {
Path string `json:"path,omitempty" yaml:"path,omitempty"`

// Args is a slice of args that will be passed as arguments to script
Args []string `json:"args,omitempty" yaml:"args,omitempty"`

// Env is a slice of env string that will be exposed to container
Env []string `json:"envs,omitempty" yaml:"envs,omitempty"`
}

// ContainerSpec defines a spec for running a function as a container
Expand Down
28 changes: 26 additions & 2 deletions kyaml/runfn/runfn.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ func (r RunFns) getFunctionsFromFunctions() ([]kio.Filter, error) {
return r.getFunctionFilters(true, r.Functions...)
}

// mergeContainerEnv will merge the envs specified by command line (imperative) and config
// file (declarative). If they have same key, the imperative value will be respected.
// mergeContainerEnv is container-specific and will merge the envs specified by command line (imperative)
// and config file (declarative). If they have same key, the imperative value will be respected.
func (r RunFns) mergeContainerEnv(envs []string) []string {
imperative := runtimeutil.NewContainerEnvFromStringSlice(r.Env)
declarative := runtimeutil.NewContainerEnvFromStringSlice(envs)
Expand All @@ -303,6 +303,28 @@ func (r RunFns) mergeContainerEnv(envs []string) []string {
return declarative.Raw()
}

// mergeExecEnv will merge the envs specified by command line (imperative) and config
// file (declarative). If they have same key, the imperative value will be respected.
Copy link
Member

Choose a reason for hiding this comment

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

@xvzf if the same value is specified through command line and env var, would it make sense to error out instead?

I'm fine with any option that makes sense from the user perspective - as this is a one off case with exec functions. We would just need to explicitly specify docs on the stance we take.

cc: @koba1t @natasha41575

Copy link
Author

Choose a reason for hiding this comment

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

Hey

It's the same behavior as for the container env merging (aka CLI flag takes precedence). Afaik it's the default behavior in most applications. Open to change/document it though!

func (r RunFns) mergeExecEnv(envs []string) []string {
envMap := map[string]string{}

for _, env := range append(envs, r.Env...) {
res := strings.Split(env, "=")
//nolint:gomnd
if len(res) == 2 {
envMap[res[0]] = res[1]
}
}

mergedEnv := []string{}
for key, value := range envMap {
mergedEnv = append(mergedEnv, fmt.Sprintf("%s=%s", key, value))
}
// Sort the envs to make the output deterministic
sort.Strings(mergedEnv)
return mergedEnv
}

func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) (
[]kio.Filter, error) {
var fltrs []kio.Filter
Expand Down Expand Up @@ -535,6 +557,8 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode, currentUser
if r.EnableExec && spec.Exec.Path != "" {
ef := &exec.Filter{
Path: spec.Exec.Path,
Args: spec.Exec.Args,
Env: r.mergeExecEnv(spec.Exec.Env),
WorkingDir: r.WorkingDir,
}

Expand Down
52 changes: 52 additions & 0 deletions kyaml/runfn/runfn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1371,3 +1371,55 @@ func TestRunFns_mergeContainerEnv(t *testing.T) {
})
}
}

func TestRunFns_mergeExecEnv(t *testing.T) {
testcases := []struct {
name string
instance RunFns
inputEnvs []string
expect []string
}{
{
name: "all empty",
instance: RunFns{},
expect: []string{},
},
{
name: "empty command line envs",
instance: RunFns{},
inputEnvs: []string{"foo=bar"},
expect: []string{"foo=bar"},
},
{
name: "empty declarative envs",
instance: RunFns{
Env: []string{"foo=bar"},
},
expect: []string{"foo=bar"},
},
{
name: "same key",
instance: RunFns{
Env: []string{"foo=bar"},
},
inputEnvs: []string{"foo=bar1"},
expect: []string{"foo=bar"},
},
{
name: "same exported key",
instance: RunFns{
Env: []string{"foo=bar"},
},
inputEnvs: []string{"foo1=bar1"},
expect: []string{"foo1=bar1", "foo=bar"},
},
}

for i := range testcases {
tc := testcases[i]
t.Run(tc.name, func(t *testing.T) {
envs := tc.instance.mergeExecEnv(tc.inputEnvs)
assert.Equal(t, tc.expect, envs)
})
}
}
Loading