Skip to content

Commit

Permalink
Exit the agent of reflection on flags doesn't work the way we expect
Browse files Browse the repository at this point in the history
This method is used to ensure Environment Variables that configure the
agent process aren't automatically passed down to the child process that
runs each job. We want to be very explicit about which variables end up
in the job process.

It's implemented using reflection on the flag structs from the ufave/cli
package we use for CLI structure. It checks the EnvVar field on the flag
struct, which works just fine with ufave/cli/v1. However, that field
doesn't exist any more in ufave/cli/v2 and this method has started
silently failing since we upgraded to v2 in #1233.

I haven't added tests because no files in this package have tests and
I'm not 100% sure where to start adding them. However, I'm treating this
like an assertion. If the reflection doesn't work the way we expect, we
hard exit with an error, like other should-never-happen errors in
agent_start.go.

I'll fix the method to work with ufave/cli/v2 in the following commit.
  • Loading branch information
yob committed Jul 20, 2020
1 parent 1a2de94 commit 088c5c0
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
6 changes: 5 additions & 1 deletion clicommand/agent_start.go
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,11 @@ var AgentStartCommand = cli.Command{
defer done()

// Remove any config env from the environment to prevent them propagating to bootstrap
UnsetConfigFromEnvironment(c)
err = UnsetConfigFromEnvironment(c)
if err != nil {
fmt.Printf("%s", err)
os.Exit(1)
}

// Check if git-mirrors are enabled
if experiments.IsEnabled(`git-mirrors`) {
Expand Down
7 changes: 6 additions & 1 deletion clicommand/global.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package clicommand

import (
"errors"
"fmt"
"os"
"reflect"
Expand Down Expand Up @@ -150,19 +151,23 @@ func HandleGlobalFlags(l logger.Logger, cfg interface{}) func() {
return HandleProfileFlag(l, cfg)
}

func UnsetConfigFromEnvironment(c *cli.Context) {
func UnsetConfigFromEnvironment(c *cli.Context) error {
flags := append(c.App.Flags, c.Command.Flags...)
for _, fl := range flags {
// use golang reflection to find EnvVar values on flags
r := reflect.ValueOf(fl)
f := reflect.Indirect(r).FieldByName(`EnvVar`)
if !f.IsValid() {
return errors.New("EnvVar field not found on flag")
}
// split comma delimited env
if envVars := f.String(); envVars != `` {
for _, env := range strings.Split(envVars, ",") {
os.Unsetenv(env)
}
}
}
return nil
}

func loadAPIClientConfig(cfg interface{}, tokenField string) api.Config {
Expand Down

0 comments on commit 088c5c0

Please sign in to comment.