From ab536ae3c8e3e0c58854c830e47629b2925a8513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= <66256922+daniel-weisse@users.noreply.github.com> Date: Mon, 8 Aug 2022 11:04:17 +0200 Subject: [PATCH] AB#2278 Remove hardcoded values from config (#346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update file handler to avoid incorrect usage of file.Option * Remove hardcoded values Signed-off-by: Daniel Weiße --- cli/internal/cmd/configgenerate.go | 7 ++- cli/internal/cmd/readconfig.go | 7 ++- cli/internal/cmd/readconfig_test.go | 50 ++++++++++++++----- internal/config/config.go | 25 ++++------ internal/config/config_test.go | 8 +-- internal/file/file.go | 41 +++++++++------ .../kubernetesca/kubernetesca_test.go | 8 +-- 7 files changed, 95 insertions(+), 51 deletions(-) diff --git a/cli/internal/cmd/configgenerate.go b/cli/internal/cmd/configgenerate.go index cb9430074..12de467ef 100644 --- a/cli/internal/cmd/configgenerate.go +++ b/cli/internal/cmd/configgenerate.go @@ -58,7 +58,12 @@ func configGenerate(cmd *cobra.Command, fileHandler file.Handler, provider cloud return err } - return fileHandler.WriteYAML(flags.file, conf, 0o644) + if err := fileHandler.WriteYAML(flags.file, conf, file.OptMkdirAll); err != nil { + return err + } + cmd.Println("Config file written to", flags.file) + cmd.Println("Please fill in your CSP specific configuration before proceeding.") + return nil } func parseGenerateFlags(cmd *cobra.Command) (generateFlags, error) { diff --git a/cli/internal/cmd/readconfig.go b/cli/internal/cmd/readconfig.go index a1f7677f1..c748d0ad1 100644 --- a/cli/internal/cmd/readconfig.go +++ b/cli/internal/cmd/readconfig.go @@ -11,6 +11,10 @@ import ( ) func readConfig(out io.Writer, fileHandler file.Handler, name string, provider cloudprovider.Provider) (*config.Config, error) { + if name == "" { + return config.Default(), nil + } + cnf, err := config.FromFile(fileHandler, name) if err != nil { return nil, err @@ -32,7 +36,8 @@ func validateConfig(out io.Writer, cnf *config.Config, provider cloudprovider.Pr for _, m := range msgs { fmt.Fprintln(out, "\t"+m) } - return errors.New("invalid configuration. Fix the invalid entries or generate a new configuration using `constellation config generate`") + fmt.Fprintln(out, "Fix the invalid entries or generate a new configuration using `constellation config generate`") + return errors.New("invalid configuration") } if provider != cloudprovider.Unknown && !cnf.HasProvider(provider) { diff --git a/cli/internal/cmd/readconfig_test.go b/cli/internal/cmd/readconfig_test.go index 89c07837d..e63028a98 100644 --- a/cli/internal/cmd/readconfig_test.go +++ b/cli/internal/cmd/readconfig_test.go @@ -17,8 +17,44 @@ func TestValidateConfig(t *testing.T) { wantOutput bool wantErr bool }{ - "default config is valid": { - cnf: config.Default(), + "default config is not valid": { + cnf: config.Default(), + wantOutput: true, + wantErr: true, + }, + "default Azure config is not valid": { + cnf: func() *config.Config { + cnf := config.Default() + az := cnf.Provider.Azure + cnf.Provider = config.ProviderConfig{} + cnf.Provider.Azure = az + return cnf + }(), + provider: cloudprovider.Azure, + wantOutput: true, + wantErr: true, + }, + "default GCP config is not valid": { + cnf: func() *config.Config { + cnf := config.Default() + gcp := cnf.Provider.GCP + cnf.Provider = config.ProviderConfig{} + cnf.Provider.GCP = gcp + return cnf + }(), + provider: cloudprovider.GCP, + wantOutput: true, + wantErr: true, + }, + "default QEMU config is valid": { + cnf: func() *config.Config { + cnf := config.Default() + qemu := cnf.Provider.QEMU + cnf.Provider = config.ProviderConfig{} + cnf.Provider.QEMU = qemu + return cnf + }(), + provider: cloudprovider.QEMU, }, "config with an error": { cnf: func() *config.Config { @@ -36,16 +72,6 @@ func TestValidateConfig(t *testing.T) { return cnf }(), }, - "config with only required provider": { - cnf: func() *config.Config { - cnf := config.Default() - az := cnf.Provider.Azure - cnf.Provider = config.ProviderConfig{} - cnf.Provider.Azure = az - return cnf - }(), - provider: cloudprovider.Azure, - }, "config without required provider": { cnf: func() *config.Config { cnf := config.Default() diff --git a/internal/config/config.go b/internal/config/config.go index df7ae7304..569c07662 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -212,18 +212,18 @@ func Default() *Config { Provider: ProviderConfig{ // TODO remove our subscriptions from the default config Azure: &AzureConfig{ - SubscriptionID: "0d202bbb-4fa7-4af8-8125-58c269a05435", - TenantID: "adb650a8-5da3-4b15-b4b0-3daf65ff7626", - Location: "North Europe", + SubscriptionID: "", + TenantID: "", + Location: "", + UserAssignedIdentity: "", Image: "/subscriptions/0d202bbb-4fa7-4af8-8125-58c269a05435/resourceGroups/CONSTELLATION-IMAGES/providers/Microsoft.Compute/galleries/Constellation/images/constellation-coreos/versions/0.0.1659453699", StateDiskType: "StandardSSD_LRS", // TODO: Replace with Premium_LRS when we replace the default VM size (Standard_D2a_v4) since the size does not support Premium_LRS Measurements: azurePCRs, - UserAssignedIdentity: "/subscriptions/0d202bbb-4fa7-4af8-8125-58c269a05435/resourceGroups/constellation-images/providers/Microsoft.ManagedIdentity/userAssignedIdentities/constellation-dev-identity", }, GCP: &GCPConfig{ - Project: "constellation-331613", - Region: "europe-west3", - Zone: "europe-west3-b", + Project: "", + Region: "", + Zone: "", Image: "projects/constellation-images/global/images/constellation-coreos-1659453699", ServiceAccountRoles: []string{ "roles/compute.instanceAdmin.v1", @@ -336,18 +336,13 @@ func (c *Config) RemoveProviderExcept(provider cloudprovider.Provider) { // FromFile returns config file with `name` read from `fileHandler` by parsing // it as YAML. -// If name is empty, the default configuration is returned. func FromFile(fileHandler file.Handler, name string) (*Config, error) { - if name == "" { - return Default(), nil - } - - var emptyConf Config - if err := fileHandler.ReadYAMLStrict(name, &emptyConf); err != nil { + var conf Config + if err := fileHandler.ReadYAMLStrict(name, &conf); err != nil { if errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("unable to find %s - use `constellation config generate` to generate it first", name) } return nil, fmt.Errorf("could not load config from file %s: %w", name, err) } - return &emptyConf, nil + return &conf, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5ade0c4e4..a55b8923e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -43,7 +43,7 @@ func TestFromFile(t *testing.T) { "default config when path empty": { config: nil, configName: "", - wantResult: Default(), + wantErr: true, }, "err when path not exist": { config: nil, @@ -154,7 +154,7 @@ func TestValidate(t *testing.T) { }{ "default config is valid": { cnf: Default(), - wantMsgCount: 0, + wantMsgCount: 7, // expect 7 error messages by default because user-specific values are not set }, "config with 1 error": { cnf: func() *Config { @@ -162,7 +162,7 @@ func TestValidate(t *testing.T) { cnf.Version = "v0" return cnf }(), - wantMsgCount: 1, + wantMsgCount: 8, }, "config with 2 errors": { cnf: func() *Config { @@ -171,7 +171,7 @@ func TestValidate(t *testing.T) { cnf.StateDiskSizeGB = -1 return cnf }(), - wantMsgCount: 2, + wantMsgCount: 9, }, } diff --git a/internal/file/file.go b/internal/file/file.go index 4d6a05fc2..4da4e0548 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -19,20 +19,33 @@ import ( ) // Option is a bitmask of options for file operations. -type Option uint +type Option struct { + uint +} -// Has determines if a set of options contains the given options. -func (o Option) Has(op Option) bool { - return o&op == op +// hasOption determines if a set of options contains the given option. +func hasOption(options []Option, op Option) bool { + for _, ops := range options { + if ops == op { + return true + } + } + return false } const ( // OptNone is a no-op. - OptNone Option = 1 << iota / 2 + optNone uint = iota // OptOverwrite overwrites an existing file. - OptOverwrite + optOverwrite // OptMkdirAll creates the path to the file. - OptMkdirAll + optMkdirAll +) + +var ( + OptNone = Option{optNone} + OptOverwrite = Option{optOverwrite} + OptMkdirAll = Option{optMkdirAll} ) // Handler handles file interaction. @@ -57,14 +70,14 @@ func (h *Handler) Read(name string) ([]byte, error) { } // Write writes the data bytes into the file with the given name. -func (h *Handler) Write(name string, data []byte, options Option) error { - if options.Has(OptMkdirAll) { +func (h *Handler) Write(name string, data []byte, options ...Option) error { + if hasOption(options, OptMkdirAll) { if err := h.fs.MkdirAll(path.Dir(name), os.ModePerm); err != nil { return err } } flags := os.O_WRONLY | os.O_CREATE | os.O_EXCL - if options.Has(OptOverwrite) { + if hasOption(options, OptOverwrite) { flags = os.O_WRONLY | os.O_CREATE | os.O_TRUNC } file, err := h.fs.OpenFile(name, flags, 0o600) @@ -89,12 +102,12 @@ func (h *Handler) ReadJSON(name string, content any) error { } // WriteJSON marshals the content interface to JSON and writes it to the path with the given name. -func (h *Handler) WriteJSON(name string, content any, options Option) error { +func (h *Handler) WriteJSON(name string, content any, options ...Option) error { jsonData, err := json.MarshalIndent(content, "", "\t") if err != nil { return err } - return h.Write(name, jsonData, options) + return h.Write(name, jsonData, options...) } // ReadYAML reads a YAML file from name and unmarshals it into the content interface. @@ -120,7 +133,7 @@ func (h *Handler) readYAML(name string, content any, strict bool) error { } // WriteYAML marshals the content interface to YAML and writes it to the path with the given name. -func (h *Handler) WriteYAML(name string, content any, options Option) (err error) { +func (h *Handler) WriteYAML(name string, content any, options ...Option) (err error) { defer func() { if r := recover(); r != nil { err = errors.New("recovered from panic") @@ -130,7 +143,7 @@ func (h *Handler) WriteYAML(name string, content any, options Option) (err error if err != nil { return err } - return h.Write(name, data, options) + return h.Write(name, data, options...) } // Remove deletes the file with the given name. diff --git a/joinservice/internal/kubernetesca/kubernetesca_test.go b/joinservice/internal/kubernetesca/kubernetesca_test.go index cbfbd9b37..b24a836c8 100644 --- a/joinservice/internal/kubernetesca/kubernetesca_test.go +++ b/joinservice/internal/kubernetesca/kubernetesca_test.go @@ -169,18 +169,18 @@ Q29uc3RlbGxhdGlvbg== assert := assert.New(t) require := require.New(t) - file := file.NewHandler(afero.NewMemMapFs()) + fileHandler := file.NewHandler(afero.NewMemMapFs()) if len(tc.caCert) > 0 { - require.NoError(file.Write(caCertFilename, tc.caCert, 0o644)) + require.NoError(fileHandler.Write(caCertFilename, tc.caCert, file.OptNone)) } if len(tc.caKey) > 0 { - require.NoError(file.Write(caKeyFilename, tc.caKey, 0o644)) + require.NoError(fileHandler.Write(caKeyFilename, tc.caKey, file.OptNone)) } ca := New( logger.NewTest(t), - file, + fileHandler, ) signingRequest, err := tc.createSigningRequest()