Skip to content

Commit

Permalink
Simplify config handling
Browse files Browse the repository at this point in the history 
In the quest for removing code and eventually the special handling
of the e2e/systemdlogs plugins, I had to start untangling some of the
different configuration management.

It was always more complex than desired but we never had enough
of a reason to spend the time and untangle it. We have that
motivation now since in order to remove those plugins from the core
I have to wire up numerous types and flags differently which impacts
all of the other config 'resolution' code.

In this PR, I took the 4 fields from the genFlags type which were
duplicated in the config (it had a config itself as well as these
four fields for convenience setting from the cmdline). Now it properly
merges the different flags/configs appropriately regardless where they
are set.

Signed-off-by: John Schnake <jschnake@vmware.com>
  • Loading branch information
@johnSchnake
johnSchnake committed Jul 1, 2021
1 parent 709de5b commit 7af6504
Show file tree
Hide file tree
Showing 22 changed files with 108 additions and 744 deletions.
8 changes: 3 additions & 5 deletions cmd/sonobuoy/app/args.go
View file
Expand Up @@ -30,7 +30,6 @@ import (
"github.com/spf13/pflag"
ops "github.com/vmware-tanzu/sonobuoy/pkg/client"
"github.com/vmware-tanzu/sonobuoy/pkg/config"
v1 "k8s.io/api/core/v1"
)

const (
Expand Down Expand Up @@ -318,10 +317,9 @@ func AddWaitOutputFlag(mode *WaitOutputMode, flags *pflag.FlagSet, defaultMode W
}

// AddImagePullPolicyFlag adds a boolean flag for deleting everything (including E2E tests).
func AddImagePullPolicyFlag(policy *ImagePullPolicy, flags *pflag.FlagSet) {
*policy = ImagePullPolicy(v1.PullAlways) //default
flags.Var(
policy, imagePullPolicyFlag,
func AddImagePullPolicyFlag(policy *string, flags *pflag.FlagSet) {
flags.StringVar(
policy, imagePullPolicyFlag, config.DefaultSonobuoyPullPolicy,
fmt.Sprintf("The ImagePullPolicy Sonobuoy should use for the aggregators and workers. Valid options are %s.", strings.Join(ValidPullPolicies(), ", ")),
)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/sonobuoy/app/e2e.go
View file
Expand Up @@ -117,7 +117,7 @@ func e2es(e2eflags *e2eFlags) func(cmd *cobra.Command, args []string) {

if !e2eflags.skipPreflight {
pcfg := &client.PreflightConfig{
Namespace: e2eflags.runFlags.namespace,
Namespace: e2eflags.runFlags.sonobuoyConfig.Namespace,
DNSNamespace: e2eflags.runFlags.dnsNamespace,
DNSPodLabels: e2eflags.runFlags.dnsPodLabels,
}
Expand Down
118 changes: 8 additions & 110 deletions cmd/sonobuoy/app/gen.go
View file
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/vmware-tanzu/sonobuoy/pkg/image"
imagepkg "github.com/vmware-tanzu/sonobuoy/pkg/image"

"github.com/imdario/mergo"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
Expand All @@ -39,17 +38,13 @@ type genFlags struct {
mode client.Mode
rbacMode RBACMode
kubecfg Kubeconfig
namespace string
dnsNamespace string
dnsPodLabels []string
sonobuoyImage string
kubeConformanceImage string
systemdLogsImage string
sshKeyPath string
sshUser string
k8sVersion imagepkg.ConformanceImageVersion
imagePullPolicy ImagePullPolicy
timeoutSeconds int
showDefaultPodSpec bool

// plugins will keep a list of the plugins we want. Custom type for
Expand All @@ -69,24 +64,24 @@ type genFlags struct {
// configs we need to tell whether or not values are default values or the user
// provided them on the command line/config file.
e2eflags *pflag.FlagSet
genflags *pflag.FlagSet
}

func GenFlagSet(cfg *genFlags, rbac RBACMode) *pflag.FlagSet {
genset := pflag.NewFlagSet("generate", pflag.ExitOnError)
AddModeFlag(&cfg.mode, genset)
cfg.sonobuoyConfig.Config = *config.New()
AddSonobuoyConfigFlag(&cfg.sonobuoyConfig, genset)
AddModeFlag(&cfg.mode, genset)
AddKubeconfigFlag(&cfg.kubecfg, genset)
cfg.e2eflags = AddE2EConfigFlags(genset)
AddRBACModeFlags(&cfg.rbacMode, genset, rbac)
AddImagePullPolicyFlag(&cfg.imagePullPolicy, genset)
AddTimeoutFlag(&cfg.timeoutSeconds, genset)
AddImagePullPolicyFlag(&cfg.sonobuoyConfig.ImagePullPolicy, genset)
AddTimeoutFlag(&cfg.sonobuoyConfig.Aggregation.TimeoutSeconds, genset)
AddShowDefaultPodSpecFlag(&cfg.showDefaultPodSpec, genset)

AddNamespaceFlag(&cfg.namespace, genset)
AddNamespaceFlag(&cfg.sonobuoyConfig.Namespace, genset)
AddDNSNamespaceFlag(&cfg.dnsNamespace, genset)
AddDNSPodLabelsFlag(&cfg.dnsPodLabels, genset)
AddSonobuoyImage(&cfg.sonobuoyImage, genset)
AddSonobuoyImage(&cfg.sonobuoyConfig.WorkerImage, genset)
AddKubeConformanceImage(&cfg.kubeConformanceImage, genset)
AddSystemdLogsImage(&cfg.systemdLogsImage, genset)
AddSSHKeyPathFlag(&cfg.sshKeyPath, genset)
Expand All @@ -99,7 +94,6 @@ func GenFlagSet(cfg *genFlags, rbac RBACMode) *pflag.FlagSet {

AddKubeConformanceImageVersion(&cfg.k8sVersion, genset)
AddKubernetesVersionFlag(&cfg.k8sVersion, genset)
cfg.genflags = genset
return genset
}

Expand Down Expand Up @@ -160,11 +154,11 @@ func (g *genFlags) Config() (*client.GenConfig, error) {

return &client.GenConfig{
E2EConfig: e2ecfg,
Config: g.resolveConfig(),
Config: &g.sonobuoyConfig.Config,
EnableRBAC: rbacEnabled,
KubeConformanceImage: e2eImage,
SystemdLogsImage: g.systemdLogsImage,
ImagePullPolicy: g.imagePullPolicy.String(),
ImagePullPolicy: g.sonobuoyConfig.ImagePullPolicy,
SSHKeyPath: g.sshKeyPath,
SSHUser: g.sshUser,
DynamicPlugins: g.plugins.DynamicPlugins,
Expand Down Expand Up @@ -229,99 +223,3 @@ func getClient(kubeconfig *Kubeconfig) (*kubernetes.Clientset, error) {

return client, kubeError
}

// getConfig generates a config which has the the following rules:
// - command line options override config values
// - plugins specified manually via flags specifically override plugins implied by mode flag
// - config values override default values
// NOTE: Since it mutates plugin values, it should be called before using them.
func (g *genFlags) resolveConfig() *config.Config {
if g == nil {
return config.New()
}

conf := config.New()
suppliedConfig := g.sonobuoyConfig.Get()
if suppliedConfig != nil {
mergeConfigs(suppliedConfig, conf)
conf = suppliedConfig
}

// Resolve plugins.
// - If using the plugin flags, no actions needed.
// - Otherwise use the supplied config and mode to figure out the plugins to run.
// This only works for e2e/systemd-logs which are internal plugins so then "Set" them
// as if they were provided on the cmdline.
// Gate the logic with a nil check because tests may not specify flags and intend the legacy logic.
if g.genflags == nil || !g.genflags.Changed("plugin") {
// Use legacy logic; conf.SelectedPlugins or mode if not set
if conf.PluginSelections == nil {
modeConfig := g.mode.Get()
if modeConfig != nil {
conf.PluginSelections = modeConfig.Selectors
}
}

// Set these values as if the user had requested the defaults.
if g.genflags != nil {
for _, v := range conf.PluginSelections {
g.genflags.Lookup("plugin").Value.Set(v.Name)
}
}
}

// Have to embed the flagset itself so we can inspect if these fields
// have been set explicitly or not on the command line. Otherwise
// we fail to properly prioritize command line/config/default values.
if g.genflags == nil {
return conf
}

if g.genflags.Changed(namespaceFlag) {
conf.Namespace = g.namespace
}

if g.genflags.Changed(sonobuoyImageFlag) {
conf.WorkerImage = g.sonobuoyImage
}

if g.genflags.Changed(imagePullPolicyFlag) {
conf.ImagePullPolicy = g.imagePullPolicy.String()
}

if g.genflags.Changed(timeoutFlag) {
conf.Aggregation.TimeoutSeconds = g.timeoutSeconds
}

return conf
}

func mergeConfigs(dst, src *config.Config) {
// Workaround for the fact that an explicitly stated empty slice is still
// considered a zero value by mergo. This means that the value given
// by the user is not respected. Even a custom transformation can't
// get around this. See https://github.com/imdario/mergo/issues/118
emptyResources := false
if len(dst.Resources) == 0 && dst.Resources != nil {
emptyResources = true
}

// Workaround to differentiate a false value and a nil value
// Only override dst.Limits.PodLogs.SonobuoyNamespace when it's nil
// See https://github.com/imdario/mergo/issues/89
var sonobuoyNamespace *bool
if dst.Limits.PodLogs.SonobuoyNamespace != nil {
sonobuoyNamespace = new(bool)
*sonobuoyNamespace = *dst.Limits.PodLogs.SonobuoyNamespace
}

// Provide defaults but don't overwrite any customized configuration.
mergo.Merge(dst, src)

if emptyResources {
dst.Resources = []string{}
}
if sonobuoyNamespace != nil {
dst.Limits.PodLogs.SonobuoyNamespace = sonobuoyNamespace
}
}
7 changes: 4 additions & 3 deletions cmd/sonobuoy/app/gen_config.go
View file
Expand Up @@ -14,12 +14,14 @@ import (
// the default sonobuoy config in a json format.
func NewCmdGenConfig() *cobra.Command {
var f genFlags

var GenCommand = &cobra.Command{
Use: "config",
Short: "Generates a sonobuoy config for input to sonobuoy gen or run.",
Run: genConfigCobra(&f),
Args: cobra.ExactArgs(0),
}
GenCommand.Flags().AddFlagSet(GenFlagSet(&f, EnabledRBACMode))

return GenCommand
}
Expand All @@ -39,12 +41,11 @@ func genConfigCobra(f *genFlags) func(cmd *cobra.Command, args []string) {

// genConfig is the actual functional logic of the command.
func genConfig(f *genFlags) ([]byte, error) {
// Using genflags.getConfig() instead of config.New() because
// Using genflags instead of config.New() because
// it will include any defaults we have on the command line such
// as default plugin selection. We didn't want to wire this into
// the `config` package, but it will be a default value the CLI
// users expect.
c := f.resolveConfig()
b, err := json.Marshal(c)
b, err := json.Marshal(f.sonobuoyConfig.Get())
return b, errors.Wrap(err, "unable to marshal configuration")
}

0 comments on commit 7af6504

Please sign in to comment.