From 2b90ff511da9b3bcf8e0741bfbcdbbb63056f01a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 17 Sep 2024 12:25:55 +0200 Subject: [PATCH 01/16] undent: move the testutil.Undent func to its own file for clarity --- pkg/testutil/{testutil.go => envtest.go} | 96 +++++--------- pkg/testutil/undent.go | 161 +++++++++++++++++++++++ pkg/testutil/undent_test.go | 45 +++++++ 3 files changed, 241 insertions(+), 61 deletions(-) rename pkg/testutil/{testutil.go => envtest.go} (84%) create mode 100644 pkg/testutil/undent.go create mode 100644 pkg/testutil/undent_test.go diff --git a/pkg/testutil/testutil.go b/pkg/testutil/envtest.go similarity index 84% rename from pkg/testutil/testutil.go rename to pkg/testutil/envtest.go index 4f99417a..79a80f4a 100644 --- a/pkg/testutil/testutil.go +++ b/pkg/testutil/envtest.go @@ -1,6 +1,8 @@ package testutil import ( + "context" + "crypto/tls" "crypto/x509" "io" "net/http" @@ -10,6 +12,7 @@ import ( "sync" "testing" + "github.com/jetstack/preflight/pkg/client" "github.com/jetstack/venafi-connection-lib/api/v1alpha1" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -99,71 +102,42 @@ func WithKubeconfig(t testing.TB, restCfg *rest.Config) string { return kubeconfig.Name() } -// Undent removes leading indentation/white-space from given string and returns -// it as a string. Useful for inlining YAML manifests in Go code. Inline YAML -// manifests in the Go test files makes it easier to read the test case as -// opposed to reading verbose-y Go structs. -// -// This was copied from https://github.com/jimeh/Undent/blob/main/Undent.go, all -// credit goes to the author, Jim Myhrberg. -func Undent(s string) string { - const ( - tab = 9 - lf = 10 - spc = 32 - ) - - if len(s) == 0 { - return "" - } - - // find smallest indent relative to each line-feed - min := 99999999999 - count := 0 - - lfs := make([]int, 0, strings.Count(s, "\n")) - if s[0] != lf { - lfs = append(lfs, -1) - } +// Tests calling to VenConnClient.PostDataReadingsWithOptions must call this +// function to start the VenafiConnection watcher. If you don't call this, the +// test will stall. +func VenConnStartWatching(t *testing.T, cl client.Client) { + t.Helper() - indent := 0 - for i := 0; i < len(s); i++ { - if s[i] == lf { - lfs = append(lfs, i) - indent = 0 - } else if indent < min { - switch s[i] { - case spc, tab: - indent++ - default: - if indent > 0 { - count++ - } - if indent < min { - min = indent - } - } - } - } + require.IsType(t, &client.VenConnClient{}, cl) + + // This `cancel` is important because the below func `Start(ctx)` needs to + // be stopped before the apiserver is stopped. Otherwise, the test fail with + // the message "timeout waiting for process kube-apiserver to stop". See: + // https://github.com/jetstack/venafi-connection-lib/pull/158#issuecomment-1949002322 + // https://github.com/kubernetes-sigs/controller-runtime/issues/1571#issuecomment-945535598 + ctx, cancel := context.WithCancel(context.Background()) + go func() { + err := cl.(*client.VenConnClient).Start(ctx) + require.NoError(t, err) + }() + t.Cleanup(cancel) +} - // extract each line without indentation - out := make([]byte, 0, len(s)-(min*count)) +func VenConnTrustCA(t *testing.T, cl client.Client, cert *x509.Certificate) { + t.Helper() + require.IsType(t, &client.VenConnClient{}, cl) + vcCl := cl.(*client.VenConnClient) - for i := 0; i < len(lfs); i++ { - offset := lfs[i] + 1 - end := len(s) - if i+1 < len(lfs) { - end = lfs[i+1] + 1 - } + pool := x509.NewCertPool() + pool.AddCert(cert) - if offset+min <= end { - out = append(out, s[offset+min:end]...) - } else if offset < end { - out = append(out, s[offset:end]...) - } + if vcCl.Client.Transport == nil { + vcCl.Client.Transport = http.DefaultTransport } - - return string(out) + if vcCl.Client.Transport.(*http.Transport).TLSClientConfig == nil { + vcCl.Client.Transport.(*http.Transport).TLSClientConfig = &tls.Config{} + } + vcCl.Client.Transport.(*http.Transport).TLSClientConfig.RootCAs = pool } // Parses the YAML manifest. Useful for inlining YAML manifests in Go test @@ -215,7 +189,7 @@ func FakeVenafiCloud(t *testing.T) (_ *httptest.Server, _ *x509.Certificate, set if r.URL.Path == "/v1/tlspk/upload/clusterdata/no" { if r.URL.Query().Get("name") != "test cluster name" { w.WriteHeader(http.StatusBadRequest) - _, _ = w.Write([]byte(`{"error":"unexpected name query param in the test server: ` + r.URL.Query().Get("name") + `"}`)) + _, _ = w.Write([]byte(`{"error":"unexpected name query param in the test server: ` + r.URL.Query().Get("name") + `, expected: 'test cluster name'"}`)) return } _, _ = w.Write([]byte(`{"status":"ok","organization":"756db001-280e-11ee-84fb-991f3177e2d0"}`)) diff --git a/pkg/testutil/undent.go b/pkg/testutil/undent.go new file mode 100644 index 00000000..1d5e3df1 --- /dev/null +++ b/pkg/testutil/undent.go @@ -0,0 +1,161 @@ +package testutil + +import ( + "fmt" +) + +// Undent removes leading indentation/white-space from given string and returns +// it as a string. Useful for inlining YAML manifests in Go code. Inline YAML +// manifests in the Go test files makes it easier to read the test case as +// opposed to reading verbose-y Go structs. +// +// This was copied from https://github.com/jimeh/Undent/blob/main/Undent.go, all +// credit goes to the author, Jim Myhrberg. +// +// For code readability purposes, it is possible to start the literal string +// with "\n", in which case, the first line is ignored. For example, in the +// following example, name and labels have the same indentation level but aren't +// aligned due to the leading '`': +// +// Undent( +// ` name: foo +// labels: +// foo: bar`) +// +// Instead, you can write a well-aligned text like this: +// +// Undent(` +// name: foo +// labels: +// foo: bar`) +// +// For code readability purposes, it is also possible to not have the correct +// number of indentations in the last line. For example: +// +// Undent(` +// foo +// bar +// `) +// +// For code readability purposes, you can also omit the indentations for empty +// lines. For example: +// +// Undent(` +// foo <---- 4 spaces +// <---- no indentation here +// bar <---- 4 spaces +// `) +func Undent(s string) string { + if len(s) == 0 { + return "" + } + + // indentsPerLine is the minimal indent level that we have found up to now. + // For example, "\t\t" corresponds to an indentation of 2, and " " an + // indentation of 3. + indentsPerLine := 99999999999 + indentedLinesCnt := 0 + + // lineOffsets tells you where the beginning of each line is in terms of + // offset. Example: + // "\tfoo\n\tbar\n" -> [0, 5] + // 0 5 + var lineOffsets []int + + // For code readability purposes, users can leave the first line empty. + if s[0] != '\n' { + lineOffsets = append(lineOffsets, 0) + } + + curLineIndent := 0 // Number of tabs or spaces in the current line. + for pos := 0; pos < len(s); pos++ { + if s[pos] == '\n' { + if pos+1 < len(s) { + lineOffsets = append(lineOffsets, pos+1) + } + curLineIndent = 0 + continue + } + + // Skip to the next line if we are already beyond the minimal indent + // level that we have found so far. The rest of this line will be kept + // as-is. + if curLineIndent >= indentsPerLine { + continue + } + + // The minimal indent level that we have found so far in previous lines + // might not be the smallest indent level. Once we hit the first + // non-indent char, let's check whether it is the new minimal indent + // level. + if s[pos] != ' ' && s[pos] != '\t' { + if curLineIndent != 0 { + indentedLinesCnt++ + } + indentsPerLine = curLineIndent + continue + } + + curLineIndent++ + } + + // Extract each line without indentation. + out := make([]byte, 0, len(s)-(indentsPerLine*indentedLinesCnt)) + + for line := 0; line < len(lineOffsets); line++ { + first := lineOffsets[line] + + // Index of the last character of the line. It is often the '\n' + // character, except for the last line. + var last int + if line == len(lineOffsets)-1 { + last = len(s) - 1 + } else { + last = lineOffsets[line+1] - 1 + } + + var lineStr string + switch { + // Case 0: if the first line is empty, let's skip it. + case line == 0 && first == last: + lineStr = "" + + // Case 1: we want the user to be able to omit some tabs or spaces in + // the last line for readability purposes. + case line == len(lineOffsets)-1 && s[last] != '\n' && isIndent(s[first:last]): + lineStr = "" + + // Case 2: we want the user to be able to omit the indentations for + // empty lines for readability purposes. + case first == last: + lineStr = "\n" + + // Case 3: error when a line doesn't contain the correct indentation + // level. + case first+indentsPerLine > last: + panic(fmt.Sprintf("line %d has an incorrect indent level: %q", line, s[first:last])) + + // Case 4: at this point, the indent level is correct, so let's remove + // the indentation and keep the rest. + case first+indentsPerLine <= last: + lineStr = s[first+indentsPerLine : last+1] + + default: + panic(fmt.Sprintf("unexpected case: first: %d, last: %d, indentsPerLine: %d, line: %q", first, last, indentsPerLine, s[first:last])) + } + out = append(out, lineStr...) + } + + return string(out) +} + +// isIndent returns true if the given string is only made of spaces or a +// tabs. +func isIndent(s string) bool { + for _, r := range s { + if r != ' ' && r != '\t' { + return false + } + } + return true +} diff --git a/pkg/testutil/undent_test.go b/pkg/testutil/undent_test.go new file mode 100644 index 00000000..b0a70003 --- /dev/null +++ b/pkg/testutil/undent_test.go @@ -0,0 +1,45 @@ +package testutil + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// This is a test for the testing func "Undent". I wasn't confident with +// Undent's behavior, so I wrote this test to verify it. +func Test_Undent(t *testing.T) { + t.Run("empty string", runTest_Undent(``, ``)) + + t.Run("if last line has the same indent as other lines and, it is ignored", runTest_Undent(` + foo + bar + `, "foo\nbar\n")) + + t.Run("you can un-indent the last line to make the Go code more readable", runTest_Undent(` + foo + bar + `, "foo\nbar\n")) + + t.Run("last line may not be an empty line", runTest_Undent(` + foo + bar`, "foo\nbar")) + + t.Run("1 empty line is preserved", runTest_Undent("\t\tfoo\n\t\t\n\t\tbar\n", "foo\n\nbar\n")) + + t.Run("2 empty lines are preserved", runTest_Undent("\t\tfoo\n\t\t\n\t\t\n\t\tbar\n", "foo\n\n\nbar\n")) + + t.Run("you can also omit the tabs or spaces for empty lines", runTest_Undent(` + foo + + bar + `, "foo\n\nbar\n")) +} + +func runTest_Undent(given, expected string) func(t *testing.T) { + return func(t *testing.T) { + t.Helper() + got := Undent(given) + assert.Equal(t, expected, got) + } +} From bb569cb873d53756577733a85dc4247aaa345475 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 17 Sep 2024 15:08:16 +0200 Subject: [PATCH 02/16] rm unauthenticated, rm impossible flag combinations, better err msgs - The unauthenticated mode no longer exists. The unauthenticated mode caused a lot of confusion as it was randomly triggered when missing some flags. Now, if you don't provide the required flags, the command will fail with a helpful error showing you what you missed. - I have removed an impossible combination (that did not work): you can no longer use `--private-key-path` along with `--credentials-path`. Previously, `--private-key-path` would be ignored if `--credentials-path` was provided. Now, the two options are mutually exclusive and a helpful message is shown when trying to use both. - The field `uploader_id` in the configuration file is deprecated. Setting this field will no longer do anything. A warning is now shown when using this field. The reason this field was deprecated is that it was never used by the Venafi Cloud API. Behind the scenes, the uploader_id is arbitrarily set to `no` so that the API doesn't complain. - The --private-key-path flag now defaults to the empty string. Previously, it defaulted to `/etc/venafi/agent/key/privatekey.pem`. This location wasn't used by any of our deployment options, and was confusing to users trying to use `--client-id`. A helpful message is now shown when trying to run `--client-id` without `--private-key-path`. - The `--period` flag was required even though the `period` field in the config was set. You can now set the period using the `period` field in the config file without having to also pass `--period` or `-p`. Note that the `--period` flag takes precedence over the config file. --- cmd/agent.go | 10 +- cmd/echo.go | 3 +- pkg/agent/config.go | 598 ++++++++++---- pkg/agent/config_test.go | 1139 +++++++++++++++----------- pkg/agent/run.go | 51 +- pkg/client/client.go | 2 +- pkg/client/client_oauth.go | 34 +- pkg/client/client_unauthenticated.go | 85 -- pkg/client/client_venafi_cloud.go | 22 +- pkg/client/client_venconn.go | 8 +- pkg/client/client_venconn_test.go | 195 ++--- pkg/datagatherer/k8s/cache.go | 5 +- pkg/datagatherer/k8s/cache_test.go | 3 +- pkg/datagatherer/k8s/discovery.go | 3 +- pkg/datagatherer/k8s/dynamic.go | 2 +- pkg/datagatherer/k8s/dynamic_test.go | 8 +- pkg/echo/echo.go | 3 +- pkg/permissions/generate.go | 5 +- pkg/permissions/generate_test.go | 5 +- pkg/testutil/undent_test.go | 2 +- 20 files changed, 1280 insertions(+), 903 deletions(-) delete mode 100644 pkg/client/client_unauthenticated.go diff --git a/cmd/agent.go b/cmd/agent.go index 98fa430f..c0d142cb 100644 --- a/cmd/agent.go +++ b/cmd/agent.go @@ -4,10 +4,11 @@ import ( "fmt" "os" + "github.com/spf13/cobra" + "github.com/jetstack/preflight/pkg/agent" "github.com/jetstack/preflight/pkg/logs" "github.com/jetstack/preflight/pkg/permissions" - "github.com/spf13/cobra" ) var agentCmd = &cobra.Command{ @@ -39,11 +40,16 @@ var agentRBACCmd = &cobra.Command{ if err != nil { logs.Log.Fatalf("Failed to read config file: %s", err) } - cfg, err := agent.ParseConfig(b, false) + cfg, err := agent.ParseConfig(b) if err != nil { logs.Log.Fatalf("Failed to parse config file: %s", err) } + err = agent.ValidateDataGatherers(cfg.DataGatherers) + if err != nil { + logs.Log.Fatalf("Failed to validate data gatherers: %s", err) + } + out := permissions.GenerateFullManifest(cfg.DataGatherers) fmt.Print(out) }, diff --git a/cmd/echo.go b/cmd/echo.go index f6fb7038..34b302e4 100644 --- a/cmd/echo.go +++ b/cmd/echo.go @@ -1,8 +1,9 @@ package cmd import ( - "github.com/jetstack/preflight/pkg/echo" "github.com/spf13/cobra" + + "github.com/jetstack/preflight/pkg/echo" ) var echoCmd = &cobra.Command{ diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 05125cdc..47a2a085 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -9,6 +9,11 @@ import ( "time" "github.com/hashicorp/go-multierror" + "github.com/pkg/errors" + "github.com/spf13/cobra" + "gopkg.in/yaml.v3" + "k8s.io/client-go/rest" + "github.com/jetstack/preflight/api" "github.com/jetstack/preflight/pkg/client" "github.com/jetstack/preflight/pkg/datagatherer" @@ -16,10 +21,6 @@ import ( "github.com/jetstack/preflight/pkg/datagatherer/local" "github.com/jetstack/preflight/pkg/kubeconfig" "github.com/jetstack/preflight/pkg/version" - "github.com/pkg/errors" - "github.com/spf13/cobra" - "gopkg.in/yaml.v3" - "k8s.io/client-go/rest" ) const ( @@ -62,8 +63,9 @@ type DataGatherer struct { } type VenafiCloudConfig struct { - // UploaderID is the upload ID that will be used when - // creating a cluster connection + // UploaderID is the upload ID that will be used when creating a cluster + // connection. This field is ignored by the backend and is often arbitrarily + // set to "no". UploaderID string `yaml:"uploader_id,omitempty"` // UploadPath is the endpoint path for the upload API. UploadPath string `yaml:"upload_path,omitempty"` @@ -74,7 +76,8 @@ type AgentCmdFlags struct { // YAML file. ConfigFilePath string - // Period (--period, -p) is the time waited between scans. + // Period (--period, -p) is the time waited between scans. It takes + // precedence over the config field `period`. Period time.Duration // OneShot (--one-shot) flag causes agent to run once. @@ -84,11 +87,12 @@ type AgentCmdFlags struct { // config and credential type. VenafiCloudMode bool - // ClientID (--client-id) is the clientID in case of Venafi Cloud mode. + // ClientID (--client-id) is the clientID in case of Venafi Cloud Key Pair + // Service Account mode. ClientID string // PrivateKeyPath (--private-key-path) is the path for the service account - // private key in case of Venafi Cloud mode. + // private key in case of Venafi Cloud Key Pair Service Account mode. PrivateKeyPath string // CredentialsPath (--credentials-file, -k) is the path to the credentials ) @@ -110,8 +114,8 @@ type AgentCmdFlags struct { // StrictMode (--strict) causes the agent to fail at the first attempt. StrictMode bool - // APIToken (--api-token) is an authentication token used for the backend - // API as an alternative to OAuth flows. + // APIToken (--api-token) is meant for the old Jetstack Secure API and is an + // alternative to OAuth. APIToken string // VenConnName (--venafi-connection) is the name of the VenafiConnection @@ -160,28 +164,30 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { "credentials-file", "k", "", - "Location of the credentials file. For OAuth2 based authentication.", + fmt.Sprintf("Location of the credentials file. For the %s and %s modes.", JetstackSecureOAuth, VenafiCloudKeypair), ) c.PersistentFlags().BoolVarP( &cfg.VenafiCloudMode, "venafi-cloud", "", false, - "Runs agent with parsing config (and credentials file if provided) in Venafi Cloud format if true.", + fmt.Sprintf("Turn on the %s mode. The flag --credentials-file must also be passed.", JetstackSecureOAuth), ) c.PersistentFlags().StringVarP( &cfg.ClientID, "client-id", "", "", - "Venafi Cloud Service Account client ID. If you use this flag you don't need to use --venafi-cloud as it will assume you are authenticating against Venafi Cloud. Using this removes the need to use a credentials file with Venafi Cloud mode.", + fmt.Sprintf("Turns on the %s mode. If you use this flag you don't need to use --venafi-cloud "+ + "as it will assume you are authenticating with Venafi Cloud. Using this removes the need to use a "+ + "credentials file.", VenafiCloudKeypair), ) c.PersistentFlags().StringVarP( &cfg.PrivateKeyPath, "private-key-path", "", - "/etc/venafi/agent/key/privatekey.pem", - "Venafi Cloud Service Account private key path.", + "", + fmt.Sprintf("To be used in conjunction with --client-id. The path to the private key file for the service account."), ) c.PersistentFlags().BoolVarP( &cfg.OneShot, @@ -222,25 +228,29 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { &cfg.APIToken, "api-token", os.Getenv("API_TOKEN"), - "Token used for authentication when API tokens are in use on the backend", + fmt.Sprintf("Turns on the %s mode. Defaults to the value of the env var API_TOKEN.", JetstackSecureAPIToken), ) c.PersistentFlags().StringVar( &cfg.VenConnName, "venafi-connection", "", - "Name of the VenafiConnection to be used. Using this flag will enable the VenafiConnection mode.", + fmt.Sprintf("Turns on the %s mode. This flag configures the name of the "+ + "VenafiConnection to be used.", VenafiCloudVenafiConnection), ) c.PersistentFlags().StringVar( &cfg.VenConnNS, "venafi-connection-namespace", "", - "Namespace of the VenafiConnection to be used. It is only useful when the VenafiConnection isn't in the same namespace as the agent. The field `allowReferencesFrom` must be present on the cross-namespace VenafiConnection for the agent to use it.", + "Namespace of the VenafiConnection to be used. It is only useful when the "+ + "VenafiConnection isn't in the same namespace as the agent. The field `allowReferencesFrom` "+ + "must be present on the cross-namespace VenafiConnection for the agent to use it.", ) c.PersistentFlags().StringVar( &cfg.InstallNS, "install-namespace", "", - "Namespace in which the agent is running. Only needed when running the agent outside of Kubernetes. Used for testing purposes.", + fmt.Sprintf("Namespace in which the agent is running. Only needed with the %s mode"+ + "when running the agent outside of Kubernetes. Used for testing purposes.", VenafiCloudVenafiConnection), ) c.PersistentFlags().BoolVarP( &cfg.Profiling, @@ -258,151 +268,430 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { ) } -// getConfiguration combines the input configuration with the flags passed to -// the agent and returns the final configuration as well as the Venafi client to -// be used to upload data. -func getConfiguration(log *log.Logger, cfg Config, flags AgentCmdFlags) (Config, client.Client, error) { - // If the ClientID of the service account is specified, then assume we are in Venafi Cloud mode. - if flags.ClientID != "" || flags.VenConnName != "" { - flags.VenafiCloudMode = true - } +type AuthMode string + +const ( + JetstackSecureOAuth AuthMode = "Jetstack Secure OAuth" + JetstackSecureAPIToken AuthMode = "Jetstack Secure API Token" + VenafiCloudKeypair AuthMode = "Venafi Cloud Key Pair Service Account" + VenafiCloudVenafiConnection AuthMode = "Venafi Cloud VenafiConnection" +) + +// The command-line flags and the config file are combined into this struct by +// ValidateAndCombineConfig. +type CombinedConfig struct { + AuthMode AuthMode + + // Used by all modes. + ClusterID string + DataGatherers []DataGatherer + Period time.Duration + BackoffMaxTime time.Duration + StrictMode bool + OneShot bool + + // Used by JetstackSecureOAuth, JetstackSecureAPIToken, and + // VenafiCloudKeypair. Ignored in VenafiCloudVenafiConnection mode. + Server string + + // JetstackSecureOAuth and JetstackSecureAPIToken modes only. + OrganizationID string + EndpointPath string // Deprecated. + + // VenafiCloudKeypair mode only. + UploadPath string + ClusterDescription string + + // VenafiCloudVenafiConnection mode only. + VenConnName string + VenConnNS string + InstallNS string + + // Only used for testing purposes. + OutputPath string + InputPath string +} - // In VenafiConnection mode, we don't need the server field. For the other - // modes, we do need to validate the server field. - var baseURL string - if flags.VenConnName != "" { - if cfg.Server != "" { - log.Printf("ignoring the server field specified in the config file. In Venafi Connection mode, this field is not needed. Use the VenafiConnection's spec.vcp.url field instead.") +// ValidateAndCombineConfig combines and validates the input configuration with +// the flags passed to the agent and returns the final configuration as well as +// the Venafi client to be used to upload data. Does not do any network call. +// The logger can be changed for testing purposes. You do not need to call +// ValidateDataGatherers as ValidateAndCombineConfig already does that. +// +// The error returned may be a multierror.Error. Use multierror.Prefix(err, +// "context:") rather than fmt.Errorf("context: %w", err) when wrapping the +// error. +func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags) (CombinedConfig, client.Client, error) { + res := CombinedConfig{} + var errs error + + { + var mode AuthMode + switch { + case flags.VenafiCloudMode && flags.CredentialsPath != "": + mode = VenafiCloudKeypair + log.Printf("Using the %s auth mode since --venafi-cloud and --credentials-path were specified.", mode) + case flags.ClientID != "" && flags.PrivateKeyPath != "": + mode = VenafiCloudKeypair + log.Printf("Using the %s auth mode since --client-id and --private-key-path were specified.", mode) + case flags.ClientID != "": + return CombinedConfig{}, nil, fmt.Errorf("if --client-id is specified, --private-key-path must also be specified") + case flags.PrivateKeyPath != "": + return CombinedConfig{}, nil, fmt.Errorf("--private-key-path is specified, --client-id must also be specified") + case flags.VenConnName != "": + mode = VenafiCloudVenafiConnection + log.Printf("Using the %s auth mode since --venafi-connection was specified.", mode) + case flags.APIToken != "": + mode = JetstackSecureAPIToken + log.Printf("Using the %s auth mode since --api-token was specified.", mode) + case !flags.VenafiCloudMode && flags.CredentialsPath != "": + mode = JetstackSecureOAuth + log.Printf("Using the %s auth mode since --credentials-file was specified without --venafi-cloud.", mode) + default: + return CombinedConfig{}, nil, fmt.Errorf("no auth mode specified. You can use one of four auth modes:\n" + + " - Use --venafi-cloud with --credentials-file or --client-id with --private-key-path to use the " + string(VenafiCloudKeypair) + " mode.\n" + + " - Use --venafi-connection for the " + string(VenafiCloudVenafiConnection) + " mode.\n" + + " - Use --credentials-file alone if you want to use the " + string(JetstackSecureOAuth) + " mode.\n" + + " - Use --api-token if you want to use the " + string(JetstackSecureAPIToken) + " mode.\n") } - } else { - baseURL = cfg.Server - if baseURL == "" { + res.AuthMode = mode + } + + // Validation and defaulting of `server` and the deprecated `endpoint.path`. + { + hasEndpointField := cfg.Endpoint.Host != "" && cfg.Endpoint.Path != "" + hasServerField := cfg.Server != "" + var server string + var endpointPath string // Deprecated. Only used when the `endpoint` field is set. + switch { + case hasServerField && !hasEndpointField: + server = cfg.Server + case hasServerField && hasEndpointField: + // The `server` field takes precedence over the deprecated + // `endpoint` field. + log.Printf("The `server` and `endpoint` fields are both set in the config; using the `server` field.") + server = cfg.Server + case !hasServerField && hasEndpointField: log.Printf("Using deprecated Endpoint configuration. User Server instead.") - baseURL = fmt.Sprintf("%s://%s", cfg.Endpoint.Protocol, cfg.Endpoint.Host) - _, err := url.Parse(baseURL) - if err != nil { - return Config{}, nil, fmt.Errorf("failed to parse server URL: %w", err) + if cfg.Endpoint.Protocol == "" && cfg.Server == "" { + cfg.Endpoint.Protocol = "http" } + server = fmt.Sprintf("%s://%s", cfg.Endpoint.Protocol, cfg.Endpoint.Host) + endpointPath = cfg.Endpoint.Path + case !hasServerField && !hasEndpointField: + server = "https://preflight.jetstack.io" + if res.AuthMode == VenafiCloudKeypair { + // The VenafiCloudVenafiConnection mode doesn't need a server. + server = client.VenafiCloudProdURL + } + } + url, urlErr := url.Parse(server) + if urlErr != nil || url.Hostname() == "" { + errs = multierror.Append(errs, fmt.Errorf("server %q is not a valid URL", server)) } + if res.AuthMode == VenafiCloudVenafiConnection && server != "" { + log.Printf("ignoring the server field specified in the config file. In %s mode, this field is not needed.", VenafiCloudVenafiConnection) + server = "" + } + res.Server = server + res.EndpointPath = endpointPath } - if flags.Period == 0 && cfg.Period == 0 && !flags.OneShot { - return Config{}, nil, fmt.Errorf("period must be set as a flag or in config") - } + // Validation of `venafi-cloud.upload_path`. + { + var uploadPath string + switch { + case res.AuthMode == VenafiCloudKeypair: + if cfg.VenafiCloud == nil || cfg.VenafiCloud.UploadPath == "" { + errs = multierror.Append(errs, fmt.Errorf("the venafi-cloud.upload_path field is required when using the %s mode", res.AuthMode)) + break // Skip to the end of the switch statement. + } + _, urlErr := url.Parse(cfg.VenafiCloud.UploadPath) + if urlErr != nil { + errs = multierror.Append(errs, fmt.Errorf("upload_path is not a valid URL")) + break // Skip to the end of the switch statement. + } - var credentials client.Credentials - var err error - if flags.ClientID != "" { - credentials = &client.VenafiSvcAccountCredentials{ - ClientID: flags.ClientID, - PrivateKeyFile: flags.PrivateKeyPath, + uploadPath = cfg.VenafiCloud.UploadPath + case res.AuthMode == VenafiCloudVenafiConnection: + // The venafi-cloud.upload_path was initially meant to let users + // configure HTTP proxies, but it has never been used since HTTP + // proxies don't rewrite paths. Thus, we've disabled the ability to + // change this value with the new --venafi-connection flag, and this + // field is simply ignored. + if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploadPath != "" { + log.Printf(`ignoring the venafi-cloud.upload_path field in the config file. In %s mode, this field is not needed.`, res.AuthMode) + } + uploadPath = "" } - } else if flags.CredentialsPath != "" { - file, err := os.Open(flags.CredentialsPath) - if err != nil { - return Config{}, nil, fmt.Errorf("failed to load credentials from file %s: %w", flags.CredentialsPath, err) + res.UploadPath = uploadPath + } + + // Validation of `uploader_id`. + // + // We found that `venafi-cloud.uploader_id` doesn't do anything in the + // backend. Since the backend requires it for historical reasons (but cannot + // be empty), we just ignore whatever the user has set in the config file, + // and set it to an arbitrary value in the client since it doesn't matter. + { + if res.AuthMode == VenafiCloudVenafiConnection || res.AuthMode == VenafiCloudKeypair { + if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploaderID != "" { + log.Printf(`ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in %s mode.`, res.AuthMode) + } } - defer file.Close() + } - b, err := io.ReadAll(file) - if err != nil { - return Config{}, nil, fmt.Errorf("failed to read credentials file: %w", err) + // Validation of `cluster_id` and `organization_id`. + { + var clusterID string + var organizationID string // Only used by the old jetstack-secure mode. + switch { + case res.AuthMode == VenafiCloudKeypair: + if cfg.ClusterID == "" { + errs = multierror.Append(errs, fmt.Errorf("cluster_id is required in %s mode", res.AuthMode)) + } + clusterID = cfg.ClusterID + case res.AuthMode == VenafiCloudVenafiConnection: + if cfg.ClusterID == "" { + errs = multierror.Append(errs, fmt.Errorf("cluster_id is required in %s mode", res.AuthMode)) + } + clusterID = cfg.ClusterID + case res.AuthMode == JetstackSecureOAuth || res.AuthMode == JetstackSecureAPIToken: + if cfg.OrganizationID == "" { + errs = multierror.Append(errs, fmt.Errorf("organization_id is required")) + } + if cfg.ClusterID == "" { + errs = multierror.Append(errs, fmt.Errorf("cluster_id is required")) + } + organizationID = cfg.OrganizationID + clusterID = cfg.ClusterID } - if flags.VenafiCloudMode { - credentials, err = client.ParseVenafiCredentials(b) - } else { - credentials, err = client.ParseOAuthCredentials(b) + res.OrganizationID = organizationID + res.ClusterID = clusterID + res.ClusterDescription = cfg.ClusterDescription + + // Validation of `data-gatherers`. + dgErr := ValidateDataGatherers(cfg.DataGatherers) + if dgErr != nil { + errs = multierror.Append(errs, dgErr) } - if err != nil { - return Config{}, nil, fmt.Errorf("failed to parse credentials file: %w", err) + res.DataGatherers = cfg.DataGatherers + } + + // Validation of --period, -p, and the `period` field, as well as + // --backoff-max-time, --one-shot, and --strict. The flag --period/-p takes + // precedence over the config `period`. + { + var period time.Duration + switch { + case flags.OneShot: + // OneShot mode doesn't need a period, skipping validation. + case flags.Period == 0 && cfg.Period == 0: + errs = multierror.Append(errs, fmt.Errorf("period must be set using --period or -p, or using the 'period' field in the config file")) + case flags.Period == 0 && cfg.Period > 0: + log.Printf("Using period from config %s", cfg.Period) + period = cfg.Period + case flags.Period > 0 && cfg.Period == 0: + period = flags.Period + case flags.Period > 0 && cfg.Period > 0: + // The flag takes precedence. + log.Printf("Both the 'period' field and --period are set. Using the value provided with --period.") + period = flags.Period } + res.Period = period + res.OneShot = flags.OneShot + res.BackoffMaxTime = flags.BackoffMaxTime + res.StrictMode = flags.StrictMode } - venConnMode := flags.VenConnName != "" + // Validation of --venafi-connection, --venafi-connection-namespace, and + // --install-namespace. + if res.AuthMode == VenafiCloudVenafiConnection { + var installNS string = flags.InstallNS + if flags.InstallNS == "" { + var err error + installNS, err = getInClusterNamespace() + if err != nil { + errs = multierror.Append(errs, fmt.Errorf("could not guess which namespace the agent is running in: %w", err)) + } + } + res.InstallNS = installNS + res.VenConnName = flags.VenConnName - if venConnMode && flags.InstallNS == "" { - flags.InstallNS, err = getInClusterNamespace() - if err != nil { - return Config{}, nil, fmt.Errorf("could not guess which namespace the agent is running in: %w", err) + var venConnNS string = flags.VenConnNS + if flags.VenConnNS == "" { + venConnNS = installNS + } + res.VenConnNS = venConnNS + } + + // Validation of --output-path, --input-path, `output-path`, and + // `input-path`. The flags --output-path and --input-path take precedence. + { + res.InputPath = cfg.InputPath + res.OutputPath = cfg.OutputPath + if flags.OutputPath != "" { + res.OutputPath = flags.OutputPath + } + if flags.InputPath != "" { + res.InputPath = flags.InputPath } } - if venConnMode && flags.VenConnNS == "" { - flags.VenConnNS = flags.InstallNS + + if errs != nil { + return CombinedConfig{}, nil, errs } - agentMetadata := &api.AgentMetadata{ - Version: version.PreflightVersion, - ClusterID: cfg.ClusterID, + preflightClient, err := validateCredsAndCreateClient(log, flags.CredentialsPath, flags.ClientID, flags.PrivateKeyPath, flags.APIToken, res) + if err != nil { + return CombinedConfig{}, nil, multierror.Prefix(err, "validating creds:") } + return res, preflightClient, nil +} + +// Validation of --credentials-file/-k, --client-id, and --private-key-path, +// --api-token, and creation of the client. +// +// The error returned may be a multierror.Error. Use multierror.Prefix(err, +// "context:") rather than fmt.Errorf("context: %w", err) when wrapping the +// error. +func validateCredsAndCreateClient(log *log.Logger, flagCredentialsPath, flagClientID, flagPrivateKeyPath, flagAPIToken string, cfg CombinedConfig) (client.Client, error) { + var errs error + var preflightClient client.Client + metadata := &api.AgentMetadata{Version: version.PreflightVersion, ClusterID: cfg.ClusterID} switch { - case credentials != nil: - preflightClient, err = createCredentialClient(log, credentials, cfg, agentMetadata, baseURL) - case flags.VenConnName != "": - // Why wasn't this added to the createCredentialClient instead? Because - // the --venafi-connection mode of authentication doesn't need any - // secrets (or any other information for that matter) to be loaded from - // disk (using --credentials-path). Everything is passed as flags. - log.Println("Venafi Connection mode was specified, using Venafi Connection authentication.") - - // The venafi-cloud.upload_path was initially meant to let users - // configure HTTP proxies, but it has never been used since HTTP proxies - // don't rewrite paths. Thus, we've disabled the ability to change this - // value with the new --venafi-connection flag, and this field is simply - // ignored. - if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploadPath != "" { - log.Printf(`ignoring venafi-cloud.upload_path. In Venafi Connection mode, this field is not needed.`) + case cfg.AuthMode == JetstackSecureOAuth: + // Note that there are no command line flags to configure the + // JetstackSecureOAuth mode. + credsBytes, err := readCredentialsFile(flagCredentialsPath) + if err != nil { + errs = multierror.Append(errs, multierror.Prefix(err, "credentials file:")) + break // Don't continue with parsing if could not load the file. + } + + creds, err := client.ParseOAuthCredentials(credsBytes) + if err != nil { + errs = multierror.Append(errs, multierror.Prefix(err, "credentials file:")) + break // Don't continue with the client if credentials file invalid. + } + + preflightClient, err = createCredentialClient(log, creds, cfg, metadata) + if err != nil { + errs = multierror.Append(errs, err) + } + case cfg.AuthMode == VenafiCloudKeypair: + var creds client.Credentials + + if flagClientID != "" && flagCredentialsPath != "" { + errs = multierror.Append(errs, fmt.Errorf("--client-id and --credentials-file cannot be used simultanously")) + break + } + if flagPrivateKeyPath != "" && flagCredentialsPath != "" { + errs = multierror.Append(errs, fmt.Errorf("--private-key-path and --credentials-file cannot be used simultanously")) + break + } + if flagClientID == "" && flagPrivateKeyPath == "" && flagCredentialsPath == "" { + errs = multierror.Append(errs, fmt.Errorf("either --client-id and --private-key-path or --credentials-file must be provided")) + break } - // Regarding venafi-cloud.uploader_id, we found that it doesn't do - // anything in the backend. Since the backend requires it for historical - // reasons (but cannot be empty), we just ignore whatever the user has - // set in the config file, and set it to an arbitrary value in the - // client since it doesn't matter. - if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploaderID != "" { - log.Printf(`ignoring venafi-cloud.uploader_id. In Venafi Connection mode, this field is not needed.`) + if flagClientID != "" && flagPrivateKeyPath != "" { + // If --client-id and --private-key-path are passed, then + // --credentials-file is ignored. + creds = &client.VenafiSvcAccountCredentials{ + ClientID: flagClientID, + PrivateKeyFile: flagPrivateKeyPath, + } + } else if flagCredentialsPath != "" { + credsBytes, err := readCredentialsFile(flagCredentialsPath) + if err != nil { + errs = multierror.Append(errs, multierror.Prefix(err, "credentials file:")) + break // Don't continue if couldn't read the creds file. + } + creds, err = client.ParseVenafiCredentials(credsBytes) + if err != nil { + errs = multierror.Append(errs, multierror.Prefix(err, "credentials file:")) + break // Don't continue with the client since creds is invalid. + } + } else { + return nil, fmt.Errorf("programmer mistake: --client-id and --private-key-path or --credentials-file must have been provided") } + var err error + preflightClient, err = createCredentialClient(log, creds, cfg, metadata) + if err != nil { + errs = multierror.Append(errs, err) + } + case cfg.AuthMode == VenafiCloudVenafiConnection: var restCfg *rest.Config - restCfg, err = kubeconfig.LoadRESTConfig("") + restCfg, err := kubeconfig.LoadRESTConfig("") if err != nil { - return Config{}, nil, fmt.Errorf("failed to load kubeconfig: %w", err) + errs = multierror.Append(errs, fmt.Errorf("loading kubeconfig: %w", err)) + break // Don't continue with the client if kubeconfig wasn't loaded. } - preflightClient, err = client.NewVenConnClient(restCfg, agentMetadata, flags.InstallNS, flags.VenConnName, flags.VenConnNS, nil) - case flags.APIToken != "": - log.Println("An API token was specified, using API token authentication.") - preflightClient, err = client.NewAPITokenClient(agentMetadata, flags.APIToken, baseURL) + preflightClient, err = client.NewVenConnClient(restCfg, metadata, cfg.InstallNS, cfg.VenConnName, cfg.VenConnNS, nil) + if err != nil { + errs = multierror.Append(errs, err) + } + case cfg.AuthMode == JetstackSecureAPIToken: + var err error + preflightClient, err = client.NewAPITokenClient(metadata, flagAPIToken, cfg.Server) + if err != nil { + errs = multierror.Append(errs, err) + } default: - log.Println("No credentials were specified, using with no authentication.") - preflightClient, err = client.NewUnauthenticatedClient(agentMetadata, baseURL) + panic(fmt.Errorf("programmer mistake: auth mode not implemented: %s", cfg.AuthMode)) } - if err != nil { - return Config{}, nil, fmt.Errorf("failed to create client: %w", err) + if errs != nil { + return nil, fmt.Errorf("failed loading config using the %s mode: %w", cfg.AuthMode, errs) } - return cfg, preflightClient, nil + return preflightClient, nil } -func createCredentialClient(log *log.Logger, credentials client.Credentials, config Config, agentMetadata *api.AgentMetadata, baseURL string) (client.Client, error) { +// Same as ValidateAndCombineConfig but just for validating the data gatherers. +// This is separate because the `rbac` command only needs to validate the data +// gatherers, nothing else. +// +// The error returned may be a multierror.Error. Use multierror.Prefix(err, +// "context:") rather than fmt.Errorf("context: %w", err) when wrapping the +// error. +func ValidateDataGatherers(dataGatherers []DataGatherer) error { + var err error + for i, v := range dataGatherers { + if v.Kind == "" { + err = multierror.Append(err, fmt.Errorf("datagatherer %d/%d is missing a kind", i+1, len(dataGatherers))) + } + if v.Name == "" { + err = multierror.Append(err, fmt.Errorf("datagatherer %d/%d is missing a name", i+1, len(dataGatherers))) + } + } + + return err +} + +// The error returned may be a multierror.Error. Instead of adding context to +// the error with fmt.Errorf("%w", err), use multierror.Prefix(err, "context"). +func createCredentialClient(log *log.Logger, credentials client.Credentials, cfg CombinedConfig, agentMetadata *api.AgentMetadata) (client.Client, error) { switch creds := credentials.(type) { case *client.VenafiSvcAccountCredentials: - log.Println("Venafi Cloud mode was specified, using Venafi Service Account authentication.") // check if config has Venafi Cloud data, use config data if it's present - uploaderID := creds.ClientID + uploaderID := "" // The uploader ID isn't actually used in the backend. uploadPath := "" - if config.VenafiCloud != nil { - log.Println("Loading uploader_id and upload_path from \"venafi-cloud\" configuration.") - uploaderID = config.VenafiCloud.UploaderID - uploadPath = config.VenafiCloud.UploadPath + if cfg.AuthMode == VenafiCloudKeypair { + // We don't do this for the VenafiCloudVenafiConnection mode because + // the upload_path field is ignored in that mode. + log.Println("Loading upload_path from \"venafi-cloud\" configuration.") + uploadPath = cfg.UploadPath } - return client.NewVenafiCloudClient(agentMetadata, creds, baseURL, uploaderID, uploadPath) + return client.NewVenafiCloudClient(agentMetadata, creds, cfg.Server, uploaderID, uploadPath) case *client.OAuthCredentials: - log.Println("A credentials file was specified, using oauth authentication.") - return client.NewOAuthClient(agentMetadata, creds, baseURL) + return client.NewOAuthClient(agentMetadata, creds, cfg.Server) default: return nil, errors.New("credentials file is in unknown format") } @@ -498,47 +787,10 @@ func (c *Config) Dump() (string, error) { return string(d), nil } -func (c *Config) validate(isVenafiCloudMode bool) error { - var result *multierror.Error - - // configured for Venafi Cloud - if c.VenafiCloud != nil { - if c.VenafiCloud.UploadPath == "" { - result = multierror.Append(result, fmt.Errorf("upload_path is required in Venafi Cloud mode")) - } - - if _, err := url.Parse(c.VenafiCloud.UploadPath); err != nil { - result = multierror.Append(result, fmt.Errorf("upload_path is not a valid URL")) - } - } else if !isVenafiCloudMode { - if c.OrganizationID == "" { - result = multierror.Append(result, fmt.Errorf("organization_id is required")) - } - if c.ClusterID == "" { - result = multierror.Append(result, fmt.Errorf("cluster_id is required")) - } - } - - if c.Server != "" { - if url, err := url.Parse(c.Server); err != nil || url.Hostname() == "" { - result = multierror.Append(result, fmt.Errorf("server is not a valid URL")) - } - } - - for i, v := range c.DataGatherers { - if v.Kind == "" { - result = multierror.Append(result, fmt.Errorf("datagatherer %d/%d is missing a kind", i+1, len(c.DataGatherers))) - } - if v.Name == "" { - result = multierror.Append(result, fmt.Errorf("datagatherer %d/%d is missing a name", i+1, len(c.DataGatherers))) - } - } - - return result.ErrorOrNil() -} - -// ParseConfig reads config into a struct used to configure running agents -func ParseConfig(data []byte, isVenafiCloudMode bool) (Config, error) { +// ParseConfig only parses. It does not validate anything except for the data +// gatherer types. To validate the config, use ValidateDataGatherers or +// getConfiguration. +func ParseConfig(data []byte) (Config, error) { var config Config err := yaml.Unmarshal(data, &config) @@ -546,21 +798,27 @@ func ParseConfig(data []byte, isVenafiCloudMode bool) (Config, error) { return config, err } - if config.Server == "" && config.Endpoint.Host == "" && config.Endpoint.Path == "" { - config.Server = "https://preflight.jetstack.io" - if config.VenafiCloud != nil || isVenafiCloudMode { - config.Server = client.VenafiCloudProdURL - } - } + return config, nil +} - if config.Endpoint.Protocol == "" && config.Server == "" { - config.Endpoint.Protocol = "http" +type credType string + +const ( + CredOldJetstackSecureOAuth credType = "CredOldJetstackSecureOAuth" + CredVenafiCloudKeypair credType = "CredVenafiCloudKeypair" +) + +func readCredentialsFile(path string) ([]byte, error) { + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("failed to load credentials from file %s: %w", path, err) } + defer file.Close() - err = config.validate(isVenafiCloudMode) + b, err := io.ReadAll(file) if err != nil { - return config, err + return nil, fmt.Errorf("failed to read credentials file: %w", err) } - return config, nil + return b, nil } diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index f274f6a4..80278b98 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -3,218 +3,581 @@ package agent import ( "bytes" "context" - "crypto/x509" "fmt" "io" "log" "net/http" "os" - "strings" "testing" "time" - "github.com/jetstack/preflight/pkg/client" - "github.com/jetstack/preflight/pkg/testutil" - "github.com/kylelemons/godebug/diff" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" + + "github.com/jetstack/preflight/pkg/client" + "github.com/jetstack/preflight/pkg/testutil" ) -func TestGetConfiguration(t *testing.T) { - t.Run("minimal successful configuration", func(t *testing.T) { - got, cl, err := getConfiguration(discardLogs(t), - Config{Server: "http://api.venafi.eu", Period: 1 * time.Hour}, - AgentCmdFlags{}, +func Test_ValidateAndCombineConfig(t *testing.T) { + // For common things like validating `server` and `data-gatherers`, we don't + // need to test every auth mode. We just test them using the Jetstack Secure + // OAuth mode. + fakeCredsPath := withFile(t, `{"user_id":"foo","user_secret":"bar","client_id": "baz","client_secret": "foobar","auth_server_domain":"bazbar"}`) + + t.Run("period must be given with either --period/-p or period field in config", func(t *testing.T) { + _, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--credentials-file", fakeCredsPath)) + assert.EqualError(t, err, "1 error occurred:\n\t* period must be set using --period or -p, or using the 'period' field in the config file\n\n") + + }) + + t.Run("period can be provided using --period or -p", func(t *testing.T) { + given := withConfig(testutil.Undent(` + server: https://api.venafi.eu + organization_id: foo + cluster_id: bar + `)) + + got, _, err := ValidateAndCombineConfig(discardLogs(), given, withCmdLineFlags("--period", "5m", "--credentials-file", fakeCredsPath)) + + require.NoError(t, err) + assert.Equal(t, 5*time.Minute, got.Period) + + got, _, err = ValidateAndCombineConfig(discardLogs(), given, withCmdLineFlags("-p", "3m", "--credentials-file", fakeCredsPath)) + require.NoError(t, err) + assert.Equal(t, 3*time.Minute, got.Period) + }) + + t.Run("period can be provided using the period field in config file", func(t *testing.T) { + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 7m + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--credentials-file", fakeCredsPath)) + require.NoError(t, err) + assert.Equal(t, 7*time.Minute, got.Period) + }) + + t.Run("--period flag takes precedence over period field in config, shows warning", func(t *testing.T) { + log, gotLogs := recordLogs() + got, _, err := ValidateAndCombineConfig(log, + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1111m + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--period", "99m", "--credentials-file", fakeCredsPath)) + require.NoError(t, err) + assert.Equal(t, testutil.Undent(` + Using the Jetstack Secure OAuth auth mode since --credentials-file was specified without --venafi-cloud. + Both the 'period' field and --period are set. Using the value provided with --period. + + `), gotLogs.String()) + assert.Equal(t, 99*time.Minute, got.Period) + }) + + t.Run("jetstack-secure-oauth-auth: server field is not required", func(t *testing.T) { + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + period: 1h + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--credentials-file", fakeCredsPath)) + require.NoError(t, err) + assert.Equal(t, "https://preflight.jetstack.io", got.Server) + }) + + t.Run("venafi-cloud-keypair-auth: server field is not required", func(t *testing.T) { + credsPath := withFile(t, `{"client_id": "foo","private_key_file": "`+withFile(t, fakePrivKeyPEM)+`"}`) + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + period: 1h + cluster_id: bar + venafi-cloud: + upload_path: /foo/bar + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath)) + require.NoError(t, err) + assert.Equal(t, "https://api.venafi.cloud", got.Server) + }) + + t.Run("server URL must be valid", func(t *testing.T) { + _, _, gotErr := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: "something not a URL" + period: 1h + organization_id: "my_org" + cluster_id: "my_cluster" + data-gatherers: + - kind: dummy + name: dummy + `)), + withCmdLineFlags("--credentials-file", fakeCredsPath)) + assert.EqualError(t, gotErr, testutil.Undent(` + 1 error occurred: + * server "something not a URL" is not a valid URL + + `)) + }) + + t.Run("--strict is passed down", func(t *testing.T) { + got, _, gotErr := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + period: 1h + organization_id: "my_org" + cluster_id: "my_cluster" + `)), + withCmdLineFlags("--strict", "--credentials-file", fakeCredsPath)) + require.NoError(t, gotErr) + assert.Equal(t, true, got.StrictMode) + }) + + t.Run("error when no auth method specified", func(t *testing.T) { + _, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + organization_id: foo + cluster_id: bar + `)), + withoutCmdLineFlags(), ) - assert.NoError(t, err) - assert.Equal(t, Config{ - Server: "http://api.venafi.eu", - Period: 1 * time.Hour, - }, got) - assert.IsType(t, &client.UnauthenticatedClient{}, cl) - }) - - t.Run("period must be given", func(t *testing.T) { - _, _, err := getConfiguration(discardLogs(t), - Config{Server: "http://api.venafi.eu"}, - AgentCmdFlags{}) - assert.EqualError(t, err, "period must be set as a flag or in config") - }) - - t.Run("server must be given", func(t *testing.T) { - got, _, err := getConfiguration(discardLogs(t), - Config{Period: 1 * time.Hour}, - AgentCmdFlags{}) - assert.EqualError(t, err, `failed to parse server URL: parse "://": missing protocol scheme`) - assert.Equal(t, Config{}, got) - }) - - t.Run("auth defaults to 'unauthenticated'", func(t *testing.T) { - got, cl, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{}) - assert.NoError(t, err) - assert.Equal(t, fillRequired(Config{}), got) - assert.IsType(t, &client.UnauthenticatedClient{}, cl) - }) - - t.Run("old jetstack-secure auth", func(t *testing.T) { - t.Run("--credential-path alone means jetstack-secure auth", func(t *testing.T) { - // `client_id`, `client_secret`, and `auth_server_domain` are - // usually injected at build time, but we can't do that in tests, so - // we need to provide them in the credentials file. - path := withFile(t, `{"user_id":"fpp2624799349@affectionate-hertz6.platform.jetstack.io","user_secret":"foo","client_id": "k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo","client_secret": "f39w_3KT9Vp0VhzcPzvh-uVbudzqCFmHER3Huj0dvHgJwVrjxsoOQPIw_1SDiCfa","auth_server_domain":"auth.jetstack.io"}`) - got, cl, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{CredentialsPath: path}) - assert.NoError(t, err) - assert.Equal(t, fillRequired(Config{}), got) - assert.IsType(t, &client.OAuthClient{}, cl) - }) - t.Run("--credential-path but file is missing", func(t *testing.T) { - got, _, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{CredentialsPath: "credentials.json"}) - assert.EqualError(t, err, "failed to load credentials from file credentials.json: open credentials.json: no such file or directory") - assert.Equal(t, Config{}, got) - }) + assert.EqualError(t, err, testutil.Undent(` + no auth mode specified. You can use one of four auth modes: + - Use --venafi-cloud with --credentials-file or --client-id with --private-key-path to use the Venafi Cloud Key Pair Service Account mode. + - Use --venafi-connection for the Venafi Cloud VenafiConnection mode. + - Use --credentials-file alone if you want to use the Jetstack Secure OAuth mode. + - Use --api-token if you want to use the Jetstack Secure API Token mode. + `)) + assert.Nil(t, cl) }) - t.Run("vcp auth: private key jwt service account", func(t *testing.T) { - // When --client-id is used, --venafi-cloud is implied. - t.Run("--private-key-path is required when --client-id is used", func(t *testing.T) { - got, _, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{ - ClientID: "test-client-id", - PrivateKeyPath: "", - }) - assert.EqualError(t, err, "failed to create client: cannot create VenafiCloudClient: 1 error occurred:\n\t* private_key_file cannot be empty\n\n") - assert.Equal(t, Config{}, got) - }) - t.Run("valid --client-id and --private-key-path", func(t *testing.T) { - path := withFile(t, "-----BEGIN PRIVATE KEY-----\nMHcCAQEEIFptpPXOvEWDrYkiMhyEH1+FB1GwtwX2tyXH4KtBO6g7oAoGCCqGSM49\nAwEHoUQDQgAE/BsIwagYc4YUjSSFyqcStj2qliAkdVGlMoJbMuXupzQ9Qs4TX5Pl\ndFjz6J/j6Gu4fLPqXmM61Hj6kiuRHx5eHQ==\n-----END PRIVATE KEY-----\n") - got, cl, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{ - ClientID: "5bc7d07c-45da-11ef-a878-523f1e1d7de1", - PrivateKeyPath: path, - }) - assert.NoError(t, err) - assert.Equal(t, fillRequired(Config{}), got) - assert.IsType(t, &client.VenafiCloudClient{}, cl) - }) + t.Run("jetstack-secure-oauth-auth: sample config", func(t *testing.T) { + // `client_id`, `client_secret`, and `auth_server_domain` are usually + // injected at build time, but we can't do that in tests, so we need to + // provide them in the credentials file. + credsPath := withFile(t, `{"user_id":"fpp2624799349@affectionate-hertz6.platform.jetstack.io","user_secret":"foo","client_id": "k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo","client_secret": "f39w_3KT9Vp0VhzcPzvh-uVbudzqCFmHER3Huj0dvHgJwVrjxsoOQPIw_1SDiCfa","auth_server_domain":"auth.jetstack.io"}`) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + period: 5m + endpoint: + host: example.com + path: api/v1/data + schedule: "* * * * *" + organization_id: "example" + cluster_id: "example-cluster" + data-gatherers: + - name: d1 + kind: dummy + config: + always-fail: false + `)), + withCmdLineFlags("--credentials-file", credsPath), + ) + expect := CombinedConfig{ + AuthMode: "Jetstack Secure OAuth", + ClusterID: "example-cluster", + DataGatherers: []DataGatherer{{Kind: "dummy", + Name: "d1", + Config: &dummyConfig{}, + }}, + Period: 5 * time.Minute, + Server: "http://example.com", + OrganizationID: "example", + EndpointPath: "api/v1/data", + } + require.NoError(t, err) + assert.Equal(t, expect, got) + assert.IsType(t, &client.OAuthClient{}, cl) + }) - // --credentials-path + --venafi-cloud can be used instead of - // --client-id and --private-key-path. Unfortunately, --credentials-path - // can't contain the private key material, just a path to it, so you - // still need to have the private key file somewhere one the filesystem. - t.Run("valid --venafi-cloud + --credential-path + private key stored to disk", func(t *testing.T) { - privKeyPath := withFile(t, "-----BEGIN PRIVATE KEY-----\nMHcCAQEEIFptpPXOvEWDrYkiMhyEH1+FB1GwtwX2tyXH4KtBO6g7oAoGCCqGSM49\nAwEHoUQDQgAE/BsIwagYc4YUjSSFyqcStj2qliAkdVGlMoJbMuXupzQ9Qs4TX5Pl\ndFjz6J/j6Gu4fLPqXmM61Hj6kiuRHx5eHQ==\n-----END PRIVATE KEY-----\n") - credsPath := withFile(t, fmt.Sprintf(`{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1","private_key_file": "%s"}`, privKeyPath)) - got, _, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{ - CredentialsPath: credsPath, - VenafiCloudMode: true, - }) - assert.NoError(t, err) - assert.Equal(t, fillRequired(Config{}), got) - }) + t.Run("venafi-cloud-keypair-auth: extended config using --venafi-cloud and --credentials-file", func(t *testing.T) { + privKeyPath := withFile(t, fakePrivKeyPEM) + credsPath := withFile(t, `{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1","private_key_file": "`+privKeyPath+`"}`) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: "http://localhost:8080" + cluster_id: "the cluster name" + period: 1h + data-gatherers: + - name: d1 + kind: dummy + config: + always-fail: false + input-path: "/home" + output-path: "/nothome" + venafi-cloud: + uploader_id: test-agent + upload_path: "/testing/path" + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath, "--backoff-max-time", "5m"), + ) + expect := CombinedConfig{ + Server: "http://localhost:8080", + Period: time.Hour, + DataGatherers: []DataGatherer{ + {Name: "d1", Kind: "dummy", Config: &dummyConfig{AlwaysFail: false}}, + }, + InputPath: "/home", + OutputPath: "/nothome", + UploadPath: "/testing/path", + AuthMode: VenafiCloudKeypair, + ClusterID: "the cluster name", + BackoffMaxTime: 5 * time.Minute, + } + require.NoError(t, err) + assert.Equal(t, expect, got) + assert.IsType(t, &client.VenafiCloudClient{}, cl) + }) - t.Run("--private-key-file can be passed with --credential-path", func(t *testing.T) { - privKeyPath := withFile(t, "-----BEGIN PRIVATE KEY-----\nMHcCAQEEIFptpPXOvEWDrYkiMhyEH1+FB1GwtwX2tyXH4KtBO6g7oAoGCCqGSM49\nAwEHoUQDQgAE/BsIwagYc4YUjSSFyqcStj2qliAkdVGlMoJbMuXupzQ9Qs4TX5Pl\ndFjz6J/j6Gu4fLPqXmM61Hj6kiuRHx5eHQ==\n-----END PRIVATE KEY-----\n") - credsPath := withFile(t, `{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1"}`) - got, _, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{ - CredentialsPath: credsPath, - PrivateKeyPath: privKeyPath, - VenafiCloudMode: true, - }) - assert.EqualError(t, err, "failed to parse credentials file: 1 error occurred:\n\t* private_key_file cannot be empty\n\n") - assert.Equal(t, Config{}, got) - }) + t.Run("venafi-cloud-keypair-auth: using --client-id and --private-key-path", func(t *testing.T) { + privKeyPath := withFile(t, fakePrivKeyPEM) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: "http://localhost:8080" + period: 1h + cluster_id: "the cluster name" + venafi-cloud: + upload_path: "/foo/bar" + `)), + withCmdLineFlags("--client-id", "5bc7d07c-45da-11ef-a878-523f1e1d7de1", "--private-key-path", privKeyPath), + ) + require.NoError(t, err) + assert.Equal(t, VenafiCloudKeypair, got.AuthMode) + assert.IsType(t, &client.VenafiCloudClient{}, cl) + }) - t.Run("config.venafi-cloud", func(t *testing.T) { - privKeyPath := withFile(t, "-----BEGIN PRIVATE KEY-----\nMHcCAQEEIFptpPXOvEWDrYkiMhyEH1+FB1GwtwX2tyXH4KtBO6g7oAoGCCqGSM49\nAwEHoUQDQgAE/BsIwagYc4YUjSSFyqcStj2qliAkdVGlMoJbMuXupzQ9Qs4TX5Pl\ndFjz6J/j6Gu4fLPqXmM61Hj6kiuRHx5eHQ==\n-----END PRIVATE KEY-----\n") - credsPath := withFile(t, `{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1"}`) - got, _, err := getConfiguration(discardLogs(t), - fillRequired(Config{ - VenafiCloud: &VenafiCloudConfig{ - UploaderID: "test-agent", - UploadPath: "/testing/path", - }, - }), - AgentCmdFlags{ - CredentialsPath: credsPath, - PrivateKeyPath: privKeyPath, - VenafiCloudMode: true, - }) - assert.EqualError(t, err, "failed to parse credentials file: 1 error occurred:\n\t* private_key_file cannot be empty\n\n") - assert.Equal(t, Config{}, got) - }) + t.Run("jetstack-secure-oauth-auth: fail if organization_id or cluster_id is missing and --venafi-cloud not enabled", func(t *testing.T) { + credsPath := withFile(t, `{"user_id":"fpp2624799349@affectionate-hertz6.platform.jetstack.io","user_secret":"foo","client_id": "k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo","client_secret": "f39w_3KT9Vp0VhzcPzvh-uVbudzqCFmHER3Huj0dvHgJwVrjxsoOQPIw_1SDiCfa","auth_server_domain":"auth.jetstack.io"}`) + _, _, err := ValidateAndCombineConfig(discardLogs(), withConfig(""), withCmdLineFlags("--credentials-file", credsPath)) + assert.EqualError(t, err, testutil.Undent(` + 3 errors occurred: + * organization_id is required + * cluster_id is required + * period must be set using --period or -p, or using the 'period' field in the config file + + `)) }) - t.Run("vcp auth: workload identity federation", func(t *testing.T) { - os.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) + t.Run("venafi-cloud-keypair-auth: authenticated if --client-id set", func(t *testing.T) { + path := withFile(t, fakePrivKeyPEM) + _, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + cluster_id: foo + venafi-cloud: + upload_path: /foo/bar + `)), + withCmdLineFlags("--venafi-cloud", "--period", "1m", "--client-id", "test-client-id", "--private-key-path", path)) + require.NoError(t, err) + assert.IsType(t, &client.VenafiCloudClient{}, cl) + }) - t.Run("valid --venafi-connection", func(t *testing.T) { - got, cl, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{VenConnName: "venafi-components", InstallNS: "venafi"}) - assert.NoError(t, err) - assert.Equal(t, fillRequired(Config{}), got) - assert.IsType(t, &client.VenConnClient{}, cl) - }) + t.Run("venafi-cloud-keypair-auth: valid 1: --client-id and --private-key-path", func(t *testing.T) { + path := withFile(t, fakePrivKeyPEM) + _, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + cluster_id: foo + venafi-cloud: + upload_path: /foo/bar + `)), + withCmdLineFlags("--venafi-cloud", "--period", "1m", "--private-key-path", path, "--client-id", "test-client-id")) + require.NoError(t, err) + assert.IsType(t, &client.VenafiCloudClient{}, cl) + }) - t.Run("namespace can't be read from disk", func(t *testing.T) { - got, _, err := getConfiguration(discardLogs(t), - fillRequired(Config{}), - AgentCmdFlags{VenConnName: "venafi-components"}) - assert.EqualError(t, err, "could not guess which namespace the agent is running in: not running in cluster, please use --install-namespace to specify the namespace in which the agent is running") - assert.Equal(t, Config{}, got) - }) + t.Run("venafi-cloud-keypair-auth: valid 2: --venafi-cloud and --credentials-file", func(t *testing.T) { + credsPath := withFile(t, fmt.Sprintf(`{"client_id": "foo","private_key_file": "%s"}`, withFile(t, fakePrivKeyPEM))) + _, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + cluster_id: foo + venafi-cloud: + upload_path: /foo/bar + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath, "--period", "1m")) + require.NoError(t, err) + assert.IsType(t, &client.VenafiCloudClient{}, cl) + }) - t.Run("warning about server, venafi-cloud.uploader_id, and venafi-cloud.upload_path being skipped", func(t *testing.T) { - log, out := withLogs(t) - cfg := fillRequired(Config{VenafiCloud: &VenafiCloudConfig{ - UploaderID: "test-agent", - UploadPath: "/testing/path", - }}) - got, _, err := getConfiguration(log, - cfg, - AgentCmdFlags{VenConnName: "venafi-components", InstallNS: "venafi"}) - assert.NoError(t, err) - assert.Equal(t, cfg, got) - assert.Contains(t, out.String(), "ignoring venafi-cloud.uploader_id") - assert.Contains(t, out.String(), "ignoring venafi-cloud.upload_path") - assert.Contains(t, out.String(), "ignoring the server field") - }) + t.Run("venafi-cloud-keypair-auth: when --venafi-cloud is used, upload_path is required", func(t *testing.T) { + credsPath := withFile(t, fmt.Sprintf(`{"client_id": "foo","private_key_file": "%s"}`, withFile(t, fakePrivKeyPEM))) + _, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: "http://localhost:8080" + period: 1h + venafi-cloud: + uploader_id: test-agent + cluster_id: "the cluster name" + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath)) + require.EqualError(t, err, "1 error occurred:\n\t* the venafi-cloud.upload_path field is required when using the Venafi Cloud Key Pair Service Account mode\n\n") + }) - t.Run("server field can be left empty in venconn mode", func(t *testing.T) { - _, _, err := getConfiguration(discardLogs(t), - withConfig(testutil.Undent(` - server: "" - period: 1h`, - )), - withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi")) - assert.NoError(t, err) - }) + t.Run("jetstack-secure-oauth-auth: --credential-file alone means jetstack-secure oauth auth", func(t *testing.T) { + // `client_id`, `client_secret`, and `auth_server_domain` are usually + // injected at build time, but we can't do that in tests, so we need to + // provide them in the credentials file. + path := withFile(t, `{"user_id":"fpp2624799349@affectionate-hertz6.platform.jetstack.io","user_secret":"foo","client_id": "k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo","client_secret": "f39w_3KT9Vp0VhzcPzvh-uVbudzqCFmHER3Huj0dvHgJwVrjxsoOQPIw_1SDiCfa","auth_server_domain":"auth.jetstack.io"}`) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--credentials-file", path)) + require.NoError(t, err) + assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, OrganizationID: "foo", ClusterID: "bar", AuthMode: JetstackSecureOAuth}, got) + assert.IsType(t, &client.OAuthClient{}, cl) + }) + + t.Run("jetstack-secure-oauth-auth: --credential-file used but file is missing", func(t *testing.T) { + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--credentials-file", "credentials.json")) + assert.EqualError(t, err, testutil.Undent(` + validating creds: failed loading config using the Jetstack Secure OAuth mode: 1 error occurred: + * credentials file: failed to load credentials from file credentials.json: open credentials.json: no such file or directory + + `)) + assert.Equal(t, CombinedConfig{}, got) + }) + + t.Run("jetstack-secure-oauth-auth: shows helpful err messages", func(t *testing.T) { + credsPath := withFile(t, `{"user_id":""}`) + _, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--credentials-file", credsPath)) + assert.EqualError(t, err, testutil.Undent(` + validating creds: failed loading config using the Jetstack Secure OAuth mode: 2 errors occurred: + * credentials file: user_id cannot be empty + * credentials file: user_secret cannot be empty + + `)) + }) + + t.Run("venafi-cloud-keypair-auth: --client-id cannot be used alone, it needs --private-key-path", func(t *testing.T) { + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + `)), + withCmdLineFlags("--client-id", "test-client-id")) + assert.EqualError(t, err, "if --client-id is specified, --private-key-path must also be specified") + assert.Equal(t, CombinedConfig{}, got) + }) + + t.Run("venafi-cloud-keypair-auth: --private-key-path cannot be used alone, it needs --client-id", func(t *testing.T) { + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + `)), + withCmdLineFlags("--private-key-path", "foo")) + assert.EqualError(t, err, "--private-key-path is specified, --client-id must also be specified") + assert.Equal(t, CombinedConfig{}, got) + }) + + // When --client-id is used, --venafi-cloud is implied. + t.Run("venafi-cloud-keypair-auth: valid --client-id and --private-key-path", func(t *testing.T) { + path := withFile(t, fakePrivKeyPEM) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + cluster_id: the cluster name + venafi-cloud: + upload_path: /foo/bar + `)), + withCmdLineFlags("--client-id", "5bc7d07c-45da-11ef-a878-523f1e1d7de1", "--private-key-path", path)) + require.NoError(t, err) + assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", UploadPath: "/foo/bar"}, got) + assert.IsType(t, &client.VenafiCloudClient{}, cl) + }) + + // --credentials-file + --venafi-cloud can be used instead of + // --client-id and --private-key-path. Unfortunately, --credentials-file + // can't contain the private key material, just a path to it, so you + // still need to have the private key file somewhere one the filesystem. + t.Run("venafi-cloud-keypair-auth: valid --venafi-cloud + --credential-file + private key stored to disk", func(t *testing.T) { + privKeyPath := withFile(t, fakePrivKeyPEM) + credsPath := withFile(t, fmt.Sprintf(`{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1","private_key_file": "%s"}`, privKeyPath)) + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + cluster_id: the cluster name + venafi-cloud: + upload_path: /foo/bar + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath)) + require.NoError(t, err) + assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", UploadPath: "/foo/bar"}, got) + }) + + t.Run("venafi-cloud-keypair-auth: venafi-cloud.upload_path field is required", func(t *testing.T) { + privKeyPath := withFile(t, fakePrivKeyPEM) + credsPath := withFile(t, fmt.Sprintf(`{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1","private_key_file": "%s"}`, privKeyPath)) + _, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + cluster_id: the cluster name + venafi-cloud: + upload_path: "" # <-- Cannot be left empty + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath)) + require.EqualError(t, err, testutil.Undent(` + 1 error occurred: + * the venafi-cloud.upload_path field is required when using the Venafi Cloud Key Pair Service Account mode + + `)) + }) + + t.Run("venafi-cloud-keypair-auth: --private-key-file can be passed with --credential-file", func(t *testing.T) { + privKeyPath := withFile(t, fakePrivKeyPEM) + credsPath := withFile(t, `{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1"}`) + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + cluster_id: the cluster name + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath, "--private-key-path", privKeyPath)) + require.EqualError(t, err, testutil.Undent(` + 1 error occurred: + * the venafi-cloud.upload_path field is required when using the Venafi Cloud Key Pair Service Account mode + + `)) + assert.Equal(t, CombinedConfig{}, got) + }) + + t.Run("venafi-cloud-keypair-auth: config.venafi-cloud", func(t *testing.T) { + privKeyPath := withFile(t, fakePrivKeyPEM) + credsPath := withFile(t, `{"client_id": "5bc7d07c-45da-11ef-a878-523f1e1d7de1"}`) + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + venafi-cloud: + uploader_id: test-agent + upload_path: /testing/path + `)), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath, "--private-key-path", privKeyPath)) + require.EqualError(t, err, testutil.Undent(` + 1 error occurred: + * cluster_id is required in Venafi Cloud Key Pair Service Account mode + + `)) + assert.Equal(t, CombinedConfig{}, got) + }) + + t.Run("venafi-cloud-workload-identity-auth: valid --venafi-connection", func(t *testing.T) { + t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: http://should-be-ignored + period: 1h + cluster_id: the cluster name + `)), + withCmdLineFlags("--install-namespace", "venafi", "--venafi-connection", "venafi-components")) + require.NoError(t, err) + assert.Equal(t, CombinedConfig{ + Period: 1 * time.Hour, + ClusterID: "the cluster name", + AuthMode: VenafiCloudVenafiConnection, + VenConnName: "venafi-components", + VenConnNS: "venafi", + InstallNS: "venafi", + }, got) + assert.IsType(t, &client.VenConnClient{}, cl) + }) + + t.Run("venafi-cloud-workload-identity-auth: namespace can't be read from disk", func(t *testing.T) { + t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + `)), + withCmdLineFlags("--venafi-connection", "venafi-components")) + assert.EqualError(t, err, testutil.Undent(` + 2 errors occurred: + * cluster_id is required in Venafi Cloud VenafiConnection mode + * could not guess which namespace the agent is running in: not running in cluster, please use --install-namespace to specify the namespace in which the agent is running + + `)) + assert.Equal(t, CombinedConfig{}, got) + }) + + t.Run("venafi-cloud-workload-identity-auth: warning about server, venafi-cloud.uploader_id, and venafi-cloud.upload_path being skipped", func(t *testing.T) { + t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) + log, gotLogs := recordLogs() + got, gotCl, err := ValidateAndCombineConfig(log, + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + cluster_id: id + venafi-cloud: + uploader_id: id + upload_path: /path + `)), + withCmdLineFlags("--install-namespace", "venafi", "--venafi-connection", "venafi-components"), + ) + require.NoError(t, err) + assert.Equal(t, testutil.Undent(` + Using the Venafi Cloud VenafiConnection auth mode since --venafi-connection was specified. + ignoring the server field specified in the config file. In Venafi Cloud VenafiConnection mode, this field is not needed. + ignoring the venafi-cloud.upload_path field in the config file. In Venafi Cloud VenafiConnection mode, this field is not needed. + ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in Venafi Cloud VenafiConnection mode. + Using period from config 1h0m0s + `), gotLogs.String()) + assert.Equal(t, VenafiCloudVenafiConnection, got.AuthMode) + assert.IsType(t, &client.VenConnClient{}, gotCl) + }) + + t.Run("venafi-cloud-workload-identity-auth: server field can be left empty in venconn mode", func(t *testing.T) { + t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) + got, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: "" + period: 1h + cluster_id: foo + `)), + withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi")) + require.NoError(t, err) + assert.Equal(t, VenafiCloudVenafiConnection, got.AuthMode) }) } // Slower test cases due to envtest. That's why they are separated from the // other tests. -func Test_getConfiguration_urlWhenVenafiConnection(t *testing.T) { - t.Run("the server field is ignored when VenafiConnection is used", func(t *testing.T) { - _, restCfg, kcl := testutil.WithEnvtest(t) - os.Setenv("KUBECONFIG", testutil.WithKubeconfig(t, restCfg)) - srv, fakeCrt, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) - for _, obj := range testutil.Parse( - testutil.VenConnRBAC + testutil.Undent(fmt.Sprintf(` +func Test_ValidateAndCombineConfig_urlWhenVenafiConnection(t *testing.T) { + _, cfg, kcl := testutil.WithEnvtest(t) + t.Setenv("KUBECONFIG", testutil.WithKubeconfig(t, cfg)) + srv, cert, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) + for _, obj := range testutil.Parse( + testutil.VenConnRBAC + testutil.Undent(fmt.Sprintf(` --- apiVersion: jetstack.io/v1alpha1 kind: VenafiConnection @@ -260,296 +623,152 @@ func Test_getConfiguration_urlWhenVenafiConnection(t *testing.T) { subjects: - kind: ServiceAccount name: venafi-connection - namespace: venafi`, srv.URL))) { - require.NoError(t, kcl.Create(context.Background(), obj)) - } + namespace: venafi + `, srv.URL))) { + require.NoError(t, kcl.Create(context.Background(), obj)) + } - // The URL received by the fake Venafi Cloud server should be the one - // coming from the VenafiConnection, not the one from the config. - setVenafiCloudAssert(func(t testing.TB, r *http.Request) { - assert.Equal(t, srv.URL, "https://"+r.Host) + t.Run("err when cluster_id field is empty", func(t *testing.T) { + expected := srv.URL + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + assert.Equal(t, expected, "https://"+gotReq.Host) }) - cfg, err := ParseConfig([]byte(testutil.Undent(` - server: "http://should-be-ignored" - period: 1h - `)), true) - assert.NoError(t, err) - - _, cl, err := getConfiguration(discardLogs(t), - cfg, - withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi"), - ) - assert.NoError(t, err) - - // `Start(ctx)` needs to be stopped before the apiserver is stopped. - // https://github.com/jetstack/venafi-connection-lib/pull/158#issuecomment-1949002322 - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - go func() { - require.NoError(t, cl.(*client.VenConnClient).Start(ctx)) - }() - certPool := x509.NewCertPool() - certPool.AddCert(fakeCrt) - tr := http.DefaultTransport.(*http.Transport).Clone() - tr.TLSClientConfig.RootCAs = certPool - cl.(*client.VenConnClient).Client.Transport = tr - - err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: "test cluster name"}) - assert.NoError(t, err) + _, _, err := ValidateAndCombineConfig(discardLogs(), + Config{Server: "http://should-be-ignored", Period: 1 * time.Hour}, + AgentCmdFlags{VenConnName: "venafi-components", InstallNS: "venafi"}) + assert.EqualError(t, err, "1 error occurred:\n\t* cluster_id is required in Venafi Cloud VenafiConnection mode\n\n") }) -} - -// Fills in the `server` and `period` as they appear in each and every test -// case. -func fillRequired(c Config) Config { - c.Server = "http://api.venafi.eu" - c.Period = 1 * time.Hour - return c -} - -func TestValidConfigLoad(t *testing.T) { - configFileContents := ` - server: "http://localhost:8080" - period: 1h - organization_id: "example" - cluster_id: "example-cluster" - data-gatherers: - - name: d1 - kind: dummy - config: - always-fail: false - input-path: "/home" - output-path: "/nothome" -` - - loadedConfig, err := ParseConfig([]byte(configFileContents), false) - assert.NoError(t, err) - - expected := Config{ - Server: "http://localhost:8080", - Period: time.Hour, - OrganizationID: "example", - ClusterID: "example-cluster", - DataGatherers: []DataGatherer{ - { - Name: "d1", - Kind: "dummy", - Config: &dummyConfig{ - AlwaysFail: false, - }, - }, - }, - InputPath: "/home", - OutputPath: "/nothome", - } - - assert.Equal(t, expected, loadedConfig) -} - -func TestValidConfigWithEndpointLoad(t *testing.T) { - configFileContents := ` - endpoint: - host: example.com - path: api/v1/data - schedule: "* * * * *" - organization_id: "example" - cluster_id: "example-cluster" - data-gatherers: - - name: d1 - kind: dummy - config: - always-fail: false -` - - loadedConfig, err := ParseConfig([]byte(configFileContents), false) - assert.NoError(t, err) - - expected := Config{ - Endpoint: Endpoint{ - Protocol: "http", - Host: "example.com", - Path: "api/v1/data", - }, - Schedule: "* * * * *", - OrganizationID: "example", - ClusterID: "example-cluster", - DataGatherers: []DataGatherer{ - { - Name: "d1", - Kind: "dummy", - Config: &dummyConfig{ - AlwaysFail: false, - }, - }, - }, - } - - assert.Equal(t, expected, loadedConfig) -} - -func TestValidVenafiCloudConfigLoad(t *testing.T) { - configFileContents := ` - server: "http://localhost:8080" - period: 1h - data-gatherers: - - name: d1 - kind: dummy - config: - always-fail: false - input-path: "/home" - output-path: "/nothome" - venafi-cloud: - uploader_id: test-agent - upload_path: "/testing/path" -` - loadedConfig, err := ParseConfig([]byte(configFileContents), false) - assert.NoError(t, err) - - expected := Config{ - Server: "http://localhost:8080", - Period: time.Hour, - OrganizationID: "", - ClusterID: "", - DataGatherers: []DataGatherer{ - { - Name: "d1", - Kind: "dummy", - Config: &dummyConfig{ - AlwaysFail: false, - }, - }, - }, - InputPath: "/home", - OutputPath: "/nothome", - VenafiCloud: &VenafiCloudConfig{ - UploaderID: "test-agent", - UploadPath: "/testing/path", - }, - } - - assert.Equal(t, expected, loadedConfig) -} - -func TestInvalidConfigError(t *testing.T) { - configFileContents := `data-gatherers: "things"` + t.Run("the server field is ignored when VenafiConnection is used", func(t *testing.T) { + expected := srv.URL + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + assert.Equal(t, expected, "https://"+gotReq.Host) + }) - _, parseError := ParseConfig([]byte(configFileContents), false) + cfg, cl, err := ValidateAndCombineConfig(discardLogs(), + Config{Server: "http://this-url-should-be-ignored", Period: 1 * time.Hour, ClusterID: "test cluster name"}, + AgentCmdFlags{VenConnName: "venafi-components", InstallNS: "venafi"}) + require.NoError(t, err) - expectedError := fmt.Errorf("yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `things` into []agent.DataGatherer") + testutil.VenConnStartWatching(t, cl) + testutil.VenConnTrustCA(t, cl, cert) - if parseError.Error() != expectedError.Error() { - t.Fatalf("got != want;\ngot=%s,\nwant=%s", parseError, expectedError) - } + // TODO(mael): the client should keep track of the cluster ID, we + // shouldn't need to pass it as an option to + // PostDataReadingsWithOptions. + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: cfg.ClusterID}) + require.NoError(t, err) + }) } -func TestMissingConfigError(t *testing.T) { - t.Run("fail to parse config if organization_id or cluster_id are missing (venafi-cloud not enabled)", func(t *testing.T) { - _, parseError := ParseConfig([]byte(""), false) - - if parseError == nil { - t.Fatalf("expected error, got nil") - } - - expectedErrorLines := []string{ - "2 errors occurred:", - "\t* organization_id is required", - "\t* cluster_id is required", - "\n", - } - - expectedError := strings.Join(expectedErrorLines, "\n") - - gotError := parseError.Error() - - if gotError != expectedError { - t.Errorf("\ngot=\n%v\nwant=\n%s\ndiff=\n%s", gotError, expectedError, diff.Diff(gotError, expectedError)) - } +func Test_ParseConfig(t *testing.T) { + t.Run("happy", func(t *testing.T) { + cfg, err := ParseConfig([]byte(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + organization_id: foo + cluster_id: bar + `))) + require.NoError(t, err) + assert.Equal(t, + Config{Server: "https://api.venafi.eu", Period: 1 * time.Hour, OrganizationID: "foo", ClusterID: "bar"}, + cfg) }) - t.Run("successfully parse config if organization_id or cluster_id are missing (venafi-cloud is enabled)", func(t *testing.T) { - _, parseError := ParseConfig([]byte(""), true) - if parseError != nil { - t.Fatalf("unxexpected error, no error should have occured when parsing configuration: %s", parseError) - } + t.Run("unknown data gatherer kind", func(t *testing.T) { + _, err := ParseConfig([]byte(testutil.Undent(` + endpoint: + host: example.com + path: /api/v1/data + schedule: "* * * * *" + data-gatherers: + - kind: "foo" + `))) + assert.EqualError(t, err, `cannot parse data-gatherer configuration, kind "foo" is not supported`) }) -} -func TestPartialMissingConfigError(t *testing.T) { - _, parseError := ParseConfig([]byte(` - endpoint: - host: example.com - path: /api/v1/data - schedule: "* * * * *" - organization_id: "example" - cluster_id: "example-cluster" - data-gatherers: - - kind: dummy`), false) - - if parseError == nil { - t.Fatalf("expected error, got nil") - } - - expectedErrorLines := []string{ - "1 error occurred:", - "\t* datagatherer 1/1 is missing a name", - "\n", - } + t.Run("validates incorrect schema", func(t *testing.T) { + _, gotErr := ParseConfig([]byte(`data-gatherers: "things"`)) + assert.EqualError(t, gotErr, "yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `things` into []agent.DataGatherer") + }) - expectedError := strings.Join(expectedErrorLines, "\n") + t.Run("does not show an error when user provides an unknown field", func(t *testing.T) { + _, gotErr := ParseConfig([]byte(`some-unknown-field: foo`)) + assert.NoError(t, gotErr) + }) - gotError := parseError.Error() + // The only validation that ParseConfig does it to check if the `kind` is + // known. The rest of the validation is done in ValidateDataGatherers and + // ValidateAndCombineConfig. + t.Run("validates that the kind is known", func(t *testing.T) { + _, gotErr := ParseConfig([]byte(testutil.Undent(` + data-gatherers: + - kind: unknown`, + ))) + assert.EqualError(t, gotErr, `cannot parse data-gatherer configuration, kind "unknown" is not supported`) + }) - if gotError != expectedError { - t.Errorf("\ngot=\n%v\nwant=\n%s\ndiff=\n%s", gotError, expectedError, diff.Diff(gotError, expectedError)) - } + // ParseConfig only checks the data-gatherer kind. The rest of the + // validation is done in ValidateDataGatherers and ValidateAndCombineConfig. + t.Run("does not check for missing name", func(t *testing.T) { + _, gotErr := ParseConfig([]byte(testutil.Undent(` + endpoint: + host: example.com + path: /api/v1/data + schedule: "* * * * *" + organization_id: "example" + cluster_id: "example-cluster" + data-gatherers: + - kind: dummy + `))) + assert.NoError(t, gotErr) + }) + t.Run("does not check correct server URL", func(t *testing.T) { + _, gotErr := ParseConfig([]byte(testutil.Undent(` + server: https://api.venafi.eu + `))) + assert.NoError(t, gotErr) + }) } -func TestInvalidServerError(t *testing.T) { - _, parseError := ParseConfig([]byte(` - server: "something not a URL" - organization_id: "my_org" - cluster_id: "my_cluster" - data-gatherers: - - kind: dummy - name: dummy`), false) - - if parseError == nil { - t.Fatalf("expected error, got nil") - } - - expectedErrorLines := []string{ - "1 error occurred:", - "\t* server is not a valid URL", - "\n", - } - - expectedError := strings.Join(expectedErrorLines, "\n") - - gotError := parseError.Error() - - if gotError != expectedError { - t.Errorf("\ngot=\n%v\nwant=\n%s\ndiff=\n%s", gotError, expectedError, diff.Diff(gotError, expectedError)) - } -} +func Test_ValidateDataGatherers(t *testing.T) { + t.Run("happy", func(t *testing.T) { + err := ValidateDataGatherers(withConfig(testutil.Undent(` + data-gatherers: + - kind: "k8s" + name: "k8s/secrets" + - kind: "k8s-discovery" + name: "k8s-discovery" + - kind: "k8s-dynamic" + name: "k8s/secrets" + - kind: "local" + name: "local" + - kind: "dummy" + name: "dummy" + `)).DataGatherers) + require.NoError(t, err) + }) -func TestInvalidDataGathered(t *testing.T) { - _, parseError := ParseConfig([]byte(` - endpoint: - host: example.com - path: /api/v1/data - schedule: "* * * * *" - data-gatherers: - - kind: "foo"`), false) - - if parseError == nil { - t.Fatalf("expected error, got nil") - } + t.Run("missing name", func(t *testing.T) { + gotErr := ValidateDataGatherers(withConfig(testutil.Undent(` + data-gatherers: + - kind: dummy + `)).DataGatherers) + assert.EqualError(t, gotErr, "1 error occurred:\n\t* datagatherer 1/1 is missing a name\n\n") + }) - if got, want := parseError.Error(), `cannot parse data-gatherer configuration, kind "foo" is not supported`; got != want { - t.Errorf("\ngot=\n%v\nwant=\n%s\ndiff=\n%s", got, want, diff.Diff(got, want)) - } + // For context, the custom UnmarshalYAML in ParseConfig already validates + // the kind. That's why ValidateDataGatherers panics: because it would be a + // programmer mistake. + t.Run("missing kind triggers a panic", func(t *testing.T) { + assert.PanicsWithError(t, `cannot parse data-gatherer configuration, kind "unknown" is not supported`, func() { + _ = ValidateDataGatherers(withConfig(testutil.Undent(` + data-gatherers: + - kind: unknown + `)).DataGatherers) + }) + }) } func withFile(t testing.TB, content string) string { @@ -569,21 +788,18 @@ func withFile(t testing.TB, content string) string { return f.Name() } -func withLogs(_ testing.TB) (*log.Logger, *bytes.Buffer) { +func recordLogs() (*log.Logger, *bytes.Buffer) { b := bytes.Buffer{} return log.New(&b, "", 0), &b } -func discardLogs(_ testing.TB) *log.Logger { +func discardLogs() *log.Logger { return log.New(io.Discard, "", 0) } -// ParseConfig does some validation but we don't want this extra validation, so -// this is just using yaml.Unmarshal. +// Shortcut for ParseConfig. func withConfig(s string) Config { - var cfg Config - - err := yaml.Unmarshal([]byte(s), &cfg) + cfg, err := ParseConfig([]byte(s)) if err != nil { panic(err) } @@ -627,3 +843,10 @@ users: client-certificate-data: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURLVENDQWhHZ0F3SUJBZ0lJV1JQVy9Nblo0VnN3RFFZSktvWklodmNOQVFFTEJRQXdGVEVUTUJFR0ExVUUKQXhNS2EzVmlaWEp1WlhSbGN6QWVGdzB5TkRBM01UVXhOREUxTVRSYUZ3MHlOVEEzTVRVeE5ESXdNVFZhTUR3eApIekFkQmdOVkJBb1RGbXQxWW1WaFpHMDZZMngxYzNSbGNpMWhaRzFwYm5NeEdUQVhCZ05WQkFNVEVHdDFZbVZ5CmJtVjBaWE10WVdSdGFXNHdnZ0VpTUEwR0NTcUdTSWIzRFFFQkFRVUFBNElCRHdBd2dnRUtBb0lCQVFDcGpIRW4KY2w3QlVURlJLdTVUeU54TmxEdWxHYittalNLcHdsd2FGa0ZyYUZPMXU0MVRVOE9FalZhNDlheHp1SHZYNTZpWgpLMEJCbkJ5aFdYeGVKNE1CTzRWdXk2K09zYVBHWUgxcDZIcGpmUTBwVW5QODFndTgzMloyWmRaazhmZkJVb0pjCjI4b25Mbjd0UERVdjhHVk9WbndZRzE4RGFDWFFjVGR3VjFNYVFKZCtsNGpveHQ5S0J6aDhZUUhZanJMdnl4RncKd2dPbTNITk5GQ3J3Zno2Wis2bi95bHliaTA3amNHVi9nMTVHaVl6azJNWW5EbFBYUHVQYzY0MVp0NWdBcGFwSgpUbUdsaW95Ym85bUVtZmRFbnd0aDJDSTZTdkx6eXlveTJidlhEVktNRzhZTzE5N25kRUd6TE95T1lYT1RMYUNkCnhaWVVCdlNadkxSK1pzMGpBZ01CQUFHalZqQlVNQTRHQTFVZER3RUIvd1FFQXdJRm9EQVRCZ05WSFNVRUREQUsKQmdnckJnRUZCUWNEQWpBTUJnTlZIUk1CQWY4RUFqQUFNQjhHQTFVZEl3UVlNQmFBRktzSWlFM3l6cUxaMGZRbQprQjd1MVk3K08rdEdNQTBHQ1NxR1NJYjNEUUVCQ3dVQUE0SUJBUUExeXpDdE55Rmp6SHlNZ0FFTVpXalR4OWxWClk2MHRpeTFvYjUvL0thR0MvWmhSbW94NmZ0Sy94dFJDRlptRVYxZ1ZzaXNLc0g2L0YwTEZHRys4V0lrNzVoZXkKVGtoRXUvRVpBdEpRMUNoSmFWMTg4QzNvMmtmSkZOOFlVRlRyS0k3K1NNb0RCTmJJU0VPV3FsZFRiVDdWdkVzNQpsWTRKcS9rU2xnNnNZcWNCRDYzY2pFOHpKU3Y4aDUra3J0d2JVRW90Y0ptN0IvNnpMZksxNWQ5WXBEb0F1anl0CjlVcTVROEhaSGRqWlZ1OWgvNmYvbVMvZkRyek9weDhNOTdPblU1T0MvY2dTNGtUNVhkdVo3SVB3TDJVMkZsTlIKVUdvZ0RndmxDQkFaMDV4WXh4Z2xjNlNYK3JrcURUK3VhWHNtR2dBU21oUjR4OXFkRzA1R2JIdXhoZkJhCi0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K client-key-data: LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFcEFJQkFBS0NBUUVBcVl4eEozSmV3VkV4VVNydVU4amNUWlE3cFJtL3BvMGlxY0pjR2haQmEyaFR0YnVOClUxUERoSTFXdVBXc2M3aDcxK2VvbVN0QVFad2NvVmw4WGllREFUdUZic3V2anJHanhtQjlhZWg2WTMwTktWSnoKL05ZTHZOOW1kbVhXWlBIM3dWS0NYTnZLSnk1KzdUdzFML0JsVGxaOEdCdGZBMmdsMEhFM2NGZFRHa0NYZnBlSQo2TWJmU2djNGZHRUIySTZ5NzhzUmNNSURwdHh6VFJRcThIOCttZnVwLzhwY200dE80M0JsZjROZVJvbU01TmpHCkp3NVQxejdqM091TldiZVlBS1dxU1U1aHBZcU1tNlBaaEpuM1JKOExZZGdpT2tyeTg4c3FNdG03MXcxU2pCdkcKRHRmZTUzUkJzeXpzam1Gemt5MmduY1dXRkFiMG1ieTBmbWJOSXdJREFRQUJBb0lCQUY2dHkzNWdzcU0zYU5mUApwbmpwSUlTOTh6UzJGVHkzY1pUa3NUUHNHNm9UL3pMcndmYTNQdVpsV3ZrOFQ0bnJpbFM5eTN1RkdJUEszbjRICmo1aXdiY3FoWjFqQXE0OStpVnM5Qkt2QW81K3M5RTJQK3E5RkJCYjdsYWNtSlR3SGx2ZkEwSVYwUXdYd1EvYk0KZVZNRTVqMkJ0Qmh1S0hlcGovdy9UTnNTR0pqK2NlNmN2aXVVb2NXWGsxWDl2c1RDaUdtMVdnVkZGQVphVGpMTgpDcEU1dHFpdnpvbEZVbXZIbmVYNTZTOEdFWk01NFA5MFk1enJ3NHBGa0Vud1VMRlBLa1U0cUU0eWVPNVFsWUhCClQ0NklIOVNPcUU5T0pLL3JCSGVzQU45TWNrMTdKblF6Sy95bXh6eHhhcGdPMnk0bVBTcjJaaGk0SENMRHRQV2QKc0ZtRzc2RUNnWUVBeHhQTTJYVFV2bXV5ckZmUVgxblJTSW9jMGhxZFY0MnFaRFlkMzZWVWc1UUVMM0Y4S01aUwptSkNsWlJXYW9IY0NFVUdXakFTWEJaMW9hOHlOMVhSNURTV3ZJMmV5TjE1dnh3NFg1SjV5QzUvY0F4ZW00dUk3CnkzM0VWWktXZXpFQTVVeUFtNlF6ei9lR1R6QkZyNUlxYkJDUitTUldudHRXUHdJTUhkK0VoeEVDZ1lFQTJnY3QKT2h1U0xJeDZZbTFTRHVVT0pSdmtFZFlCazJPQWxRbk5kOVJoaWIxdVlVbjhPTkhYdHBsY2FHZEl3bFdkaEJlcwo4M1F4dXA4MEFydEFtM2FHMXZ6RlZ6Q05KeHA4ZGFxWlFsZk94YlJReUQ0cjdtT2Z5aENFY2VibHAxMkZKRTBQCmNhOFl2TkFuTTdkbnlTSFd0aUo2THFQWDVuMXlRSC9JY1NIaEdQTUNnWUVBa0ZDZFBzSy8rcTZ1SHR1bDFZbVIKK3FrTWpZNzNvdUd5dE9TNk1VZDBCZEtHV2pKRmxIVjRxTnFxMjZXV3ExNjZZL0lOQmNIS0RTcjM2TFduMkNhUQpIbVRFR3NGd1kwMFZjTktacFlUckhkd3NMUjIzUUdCS2dwRFFoRXc0eEdOWXgrRDJsbDJwcGNoRldDQ2hVODU4CjdFdnkxZzV1c01oR05IVHlmYkZzTEZFQ2dZRUF6QXJOVzhVenZuZFZqY25MY3Q4UXBzLzhXR2pVbnJBUFJPdWcKbTlWcDF2TXVXdVJYcElGV0JMQnYxOUZaT1czUWRTK0hEMndkb2c2ZUtUUS9HWDhLWUNhOU5JVGVoTXIzMFZMdwpEVE9KOG1KMiszK2JzNFVPcEpkaXJBb3Z3THI0QUdvUjJ3M0g4K1JGMjlOMzBMYlhieXJDOStVa0I3UTgrWG5kCkIydHljdHNDZ1lCZkxqUTNRUnpQN1Z5Y1VGNkFTYUNYVTJkcE5lckVUbGFpdldIb1FFWVo3NHEyMkFTeFcrMlEKWmtZTEM1RVNGMnZwUU5kZUZhZlRyRm9zR3pLQ1dwYXBUL2QwUC9qaG83TEF1TTJQZEcxSXFoNElRU3FUM3VqNwp4Sm9WUzhIbEg1Ri9sQzZzczZQSm1GWlpsanhFL1FVTDlucDNLYTVCRjFXdXZiZVp0Q2I5Mnc9PQotLS0tLUVORCBSU0EgUFJJVkFURSBLRVktLS0tLQo= ` + +const fakePrivKeyPEM = `-----BEGIN PRIVATE KEY----- +MHcCAQEEIFptpPXOvEWDrYkiMhyEH1+FB1GwtwX2tyXH4KtBO6g7oAoGCCqGSM49 +AwEHoUQDQgAE/BsIwagYc4YUjSSFyqcStj2qliAkdVGlMoJbMuXupzQ9Qs4TX5Pl +dFjz6J/j6Gu4fLPqXmM61Hj6kiuRHx5eHQ== +-----END PRIVATE KEY----- +` diff --git a/pkg/agent/run.go b/pkg/agent/run.go index ec2f7b54..8d7b630d 100644 --- a/pkg/agent/run.go +++ b/pkg/agent/run.go @@ -8,7 +8,6 @@ import ( "fmt" "io" "net/http" - _ "net/http/pprof" "os" "strings" "time" @@ -26,6 +25,8 @@ import ( "github.com/jetstack/preflight/pkg/datagatherer" "github.com/jetstack/preflight/pkg/logs" "github.com/jetstack/preflight/pkg/version" + + _ "net/http/pprof" ) var Flags AgentCmdFlags @@ -55,12 +56,12 @@ func Run(cmd *cobra.Command, args []string) { logs.Log.Fatalf("Failed to read config file: %s", err) } - cfg, err := ParseConfig(b, Flags.StrictMode) + cfg, err := ParseConfig(b) if err != nil { logs.Log.Fatalf("Failed to parse config file: %s", err) } - config, preflightClient, err := getConfiguration(logs.Log, cfg, Flags) + config, preflightClient, err := ValidateAndCombineConfig(logs.Log, cfg, Flags) if err != nil { logs.Log.Fatalf("While evaluating configuration: %v", err) } @@ -165,36 +166,22 @@ func Run(cmd *cobra.Command, args []string) { // configured output using data in datagatherer caches or refreshing from // APIs each cycle depending on datagatherer implementation for { - // if period is set in the config, then use that if not already set - if Flags.Period == 0 && config.Period > 0 { - logs.Log.Printf("Using period from config %s", config.Period) - Flags.Period = config.Period - } - gatherAndOutputData(config, preflightClient, dataGatherers) - if Flags.OneShot { + if config.OneShot { break } - time.Sleep(Flags.Period) + time.Sleep(config.Period) } } -func gatherAndOutputData(config Config, preflightClient client.Client, dataGatherers map[string]datagatherer.DataGatherer) { +func gatherAndOutputData(config CombinedConfig, preflightClient client.Client, dataGatherers map[string]datagatherer.DataGatherer) { var readings []*api.DataReading - // Input/OutputPath flag overwrites agent.yaml configuration - if Flags.InputPath == "" { - Flags.InputPath = config.InputPath - } - if Flags.OutputPath == "" { - Flags.OutputPath = config.OutputPath - } - - if Flags.InputPath != "" { - logs.Log.Printf("Reading data from local file: %s", Flags.InputPath) - data, err := os.ReadFile(Flags.InputPath) + if config.InputPath != "" { + logs.Log.Printf("Reading data from local file: %s", config.InputPath) + data, err := os.ReadFile(config.InputPath) if err != nil { logs.Log.Fatalf("failed to read local data file: %s", err) } @@ -206,21 +193,21 @@ func gatherAndOutputData(config Config, preflightClient client.Client, dataGathe readings = gatherData(config, dataGatherers) } - if Flags.OutputPath != "" { + if config.OutputPath != "" { data, err := json.MarshalIndent(readings, "", " ") if err != nil { logs.Log.Fatal("failed to marshal JSON") } - err = os.WriteFile(Flags.OutputPath, data, 0644) + err = os.WriteFile(config.OutputPath, data, 0644) if err != nil { logs.Log.Fatalf("failed to output to local file: %s", err) } - logs.Log.Printf("Data saved to local file: %s", Flags.OutputPath) + logs.Log.Printf("Data saved to local file: %s", config.OutputPath) } else { backOff := backoff.NewExponentialBackOff() backOff.InitialInterval = 30 * time.Second backOff.MaxInterval = 3 * time.Minute - backOff.MaxElapsedTime = Flags.BackoffMaxTime + backOff.MaxElapsedTime = config.BackoffMaxTime post := func() error { return postData(config, preflightClient, readings) } @@ -233,7 +220,7 @@ func gatherAndOutputData(config Config, preflightClient client.Client, dataGathe } } -func gatherData(config Config, dataGatherers map[string]datagatherer.DataGatherer) []*api.DataReading { +func gatherData(config CombinedConfig, dataGatherers map[string]datagatherer.DataGatherer) []*api.DataReading { var readings []*api.DataReading var dgError *multierror.Error @@ -271,19 +258,19 @@ func gatherData(config Config, dataGatherers map[string]datagatherer.DataGathere } } - if Flags.StrictMode && dgError.ErrorOrNil() != nil { + if config.StrictMode && dgError.ErrorOrNil() != nil { logs.Log.Fatalf("halting datagathering in strict mode due to error: %s", dgError.ErrorOrNil()) } return readings } -func postData(config Config, preflightClient client.Client, readings []*api.DataReading) error { +func postData(config CombinedConfig, preflightClient client.Client, readings []*api.DataReading) error { baseURL := config.Server logs.Log.Println("Posting data to:", baseURL) - if Flags.VenafiCloudMode { + if config.AuthMode == VenafiCloudKeypair || config.AuthMode == VenafiCloudVenafiConnection { // orgID and clusterID are not required for Venafi Cloud auth err := preflightClient.PostDataReadingsWithOptions(readings, client.Options{ ClusterName: config.ClusterID, @@ -309,7 +296,7 @@ func postData(config Config, preflightClient client.Client, readings []*api.Data ) metric.Set(float64(len(data))) logs.Log.Printf("Data readings upload size: %d", len(data)) - path := config.Endpoint.Path + path := config.EndpointPath if path == "" { path = "/api/v1/datareadings" } diff --git a/pkg/client/client.go b/pkg/client/client.go index 6ddcb34d..fef5be65 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -36,7 +36,7 @@ type ( // The Credentials interface describes methods for credential types to implement for verification. Credentials interface { - IsClientSet() bool + IsClientSet() (ok bool, why string) Validate() error } ) diff --git a/pkg/client/client_oauth.go b/pkg/client/client_oauth.go index 5ec4053a..89aca601 100644 --- a/pkg/client/client_oauth.go +++ b/pkg/client/client_oauth.go @@ -12,8 +12,9 @@ import ( "time" "github.com/hashicorp/go-multierror" - "github.com/jetstack/preflight/api" "github.com/pkg/errors" + + "github.com/jetstack/preflight/api" ) type ( @@ -72,17 +73,19 @@ func NewOAuthClient(agentMetadata *api.AgentMetadata, credentials *OAuthCredenti return nil, fmt.Errorf("cannot create OAuthClient: %v", err) } if baseURL == "" { - return nil, fmt.Errorf("cannot create OAuthClient: baseURL cannot be empty") + return nil, fmt.Errorf("programmer mistake: cannot create APITokenClient: baseURL cannot be empty, should have been checked by the caller") } - if !credentials.IsClientSet() { + ok, _ := credentials.IsClientSet() + if !ok { credentials.ClientID = ClientID credentials.ClientSecret = ClientSecret credentials.AuthServerDomain = AuthServerDomain } - if !credentials.IsClientSet() { - return nil, fmt.Errorf("cannot create OAuthClient: invalid OAuth2 client configuration") + ok, why := credentials.IsClientSet() + if !ok { + return nil, fmt.Errorf("%s", why) } return &OAuthClient{ @@ -217,7 +220,9 @@ func (c *OAuthClient) renewAccessToken() error { return nil } -// ParseOAuthCredentials reads credentials into an OAuthCredentials struct. Performs validations. +// Performs validations. Since it may return a multierror.Error, remember to use +// multierror.Prefix(err, "context: ") rather than fmt.Errorf("context: %w", +// err) when wrapping the error. func ParseOAuthCredentials(data []byte) (*OAuthCredentials, error) { var credentials OAuthCredentials @@ -233,9 +238,20 @@ func ParseOAuthCredentials(data []byte) (*OAuthCredentials, error) { return &credentials, nil } -// IsClientSet returns whether the client credentials are set or not. -func (c *OAuthCredentials) IsClientSet() bool { - return c.ClientID != "" && c.ClientSecret != "" && c.AuthServerDomain != "" +// IsClientSet returns whether the client credentials are set or not. `why` is +// only returned when `ok` is false. +func (c *OAuthCredentials) IsClientSet() (ok bool, why string) { + if c.ClientID == "" { + return false, "ClientID is empty" + } + if c.ClientSecret == "" { + return false, "ClientSecret is empty" + } + if c.AuthServerDomain == "" { + return false, "AuthServerDomain is empty" + } + + return true, "" } func (c *OAuthCredentials) Validate() error { diff --git a/pkg/client/client_unauthenticated.go b/pkg/client/client_unauthenticated.go deleted file mode 100644 index d6c7c410..00000000 --- a/pkg/client/client_unauthenticated.go +++ /dev/null @@ -1,85 +0,0 @@ -package client - -import ( - "bytes" - "encoding/json" - "fmt" - "io" - "net/http" - "path/filepath" - "time" - - "github.com/jetstack/preflight/api" -) - -type ( - // The UnauthenticatedClient type is a Client implementation used to upload data readings to the Jetstack Secure - // platform using no authentication method. - UnauthenticatedClient struct { - baseURL string - agentMetadata *api.AgentMetadata - client *http.Client - } -) - -// NewUnauthenticatedClient returns a new instance of the UnauthenticatedClient type that will perform HTTP requests using -// no authentication. -func NewUnauthenticatedClient(agentMetadata *api.AgentMetadata, baseURL string) (*UnauthenticatedClient, error) { - if baseURL == "" { - return nil, fmt.Errorf("cannot create UnauthenticatedClient: baseURL cannot be empty") - } - - return &UnauthenticatedClient{ - agentMetadata: agentMetadata, - baseURL: baseURL, - client: &http.Client{Timeout: time.Minute}, - }, nil -} - -func (c *UnauthenticatedClient) PostDataReadingsWithOptions(readings []*api.DataReading, opts Options) error { - return c.PostDataReadings(opts.OrgID, opts.ClusterID, readings) -} - -// PostDataReadings uploads the slice of api.DataReading to the Jetstack Secure backend to be processed for later -// viewing in the user-interface. -func (c *UnauthenticatedClient) PostDataReadings(orgID, clusterID string, readings []*api.DataReading) error { - payload := api.DataReadingsPost{ - AgentMetadata: c.agentMetadata, - DataGatherTime: time.Now().UTC(), - DataReadings: readings, - } - data, err := json.Marshal(payload) - if err != nil { - return err - } - - res, err := c.Post(filepath.Join("/api/v1/org", orgID, "datareadings", clusterID), bytes.NewBuffer(data)) - if err != nil { - return err - } - defer res.Body.Close() - - if code := res.StatusCode; code < 200 || code >= 300 { - errorContent := "" - body, err := io.ReadAll(res.Body) - if err == nil { - errorContent = string(body) - } - - return fmt.Errorf("received response with status code %d. Body: [%s]", code, errorContent) - } - - return nil -} - -// Post performs an HTTP POST request. -func (c *UnauthenticatedClient) Post(path string, body io.Reader) (*http.Response, error) { - req, err := http.NewRequest(http.MethodPost, fullURL(c.baseURL, path), body) - if err != nil { - return nil, err - } - - req.Header.Set("Content-Type", "application/json") - - return c.client.Do(req) -} diff --git a/pkg/client/client_venafi_cloud.go b/pkg/client/client_venafi_cloud.go index 69e8c1ca..91021da5 100644 --- a/pkg/client/client_venafi_cloud.go +++ b/pkg/client/client_venafi_cloud.go @@ -25,8 +25,9 @@ import ( "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "github.com/hashicorp/go-multierror" - "github.com/jetstack/preflight/api" "github.com/microcosm-cc/bluemonday" + + "github.com/jetstack/preflight/api" ) type ( @@ -93,8 +94,9 @@ func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiS return nil, fmt.Errorf("cannot create VenafiCloudClient: baseURL cannot be empty") } - if !credentials.IsClientSet() { - return nil, fmt.Errorf("cannot create VenafiCloudClient: invalid Venafi Cloud client configuration") + ok, why := credentials.IsClientSet() + if !ok { + return nil, fmt.Errorf("%s", why) } if uploadPath == "" { @@ -149,9 +151,17 @@ func (c *VenafiSvcAccountCredentials) Validate() error { return result.ErrorOrNil() } -// IsClientSet returns whether the client credentials are set or not. -func (c *VenafiSvcAccountCredentials) IsClientSet() bool { - return c.ClientID != "" && c.PrivateKeyFile != "" +// IsClientSet returns whether the client credentials are set or not. `why` is +// only returned when `ok` is false. +func (c *VenafiSvcAccountCredentials) IsClientSet() (ok bool, why string) { + if c.ClientID == "" { + return false, "ClientID is empty" + } + if c.PrivateKeyFile == "" { + return false, "PrivateKeyFile is empty" + } + + return true, "" } // PostDataReadingsWithOptions uploads the slice of api.DataReading to the Venafi Cloud backend to be processed. diff --git a/pkg/client/client_venconn.go b/pkg/client/client_venconn.go index df92ef99..f614ecd0 100644 --- a/pkg/client/client_venconn.go +++ b/pkg/client/client_venconn.go @@ -12,8 +12,6 @@ import ( "time" "github.com/go-logr/logr" - "github.com/jetstack/preflight/api" - "github.com/jetstack/preflight/pkg/version" venapi "github.com/jetstack/venafi-connection-lib/api/v1alpha1" "github.com/jetstack/venafi-connection-lib/venafi_client" "github.com/jetstack/venafi-connection-lib/venafi_client/auth" @@ -24,6 +22,9 @@ import ( "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ctrlruntimelog "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/jetstack/preflight/api" + "github.com/jetstack/preflight/pkg/version" ) type VenConnClient struct { @@ -48,7 +49,8 @@ type VenConnClient struct { // are referring to as its client-go cache will remain empty. // // The http.Client is used for Venafi and Vault, not for Kubernetes. The -// `installNS` is the namespace in which the agent is running in. The passed +// `installNS` is the namespace in which the agent is running in and cannot be +// empty. `venConnName` and `venConnNS` must not be empty either. The passed // `restcfg` is not mutated. `trustedCAs` is only used for connecting to Venafi // Cloud and Vault and can be left nil. func NewVenConnClient(restcfg *rest.Config, agentMetadata *api.AgentMetadata, installNS, venConnName, venConnNS string, trustedCAs *x509.CertPool) (*VenConnClient, error) { diff --git a/pkg/client/client_venconn_test.go b/pkg/client/client_venconn_test.go index d644880b..baa9fcdf 100644 --- a/pkg/client/client_venconn_test.go +++ b/pkg/client/client_venconn_test.go @@ -6,15 +6,15 @@ import ( "strings" "testing" - "github.com/jetstack/preflight/api" - "github.com/jetstack/preflight/pkg/client" - "github.com/jetstack/preflight/pkg/testutil" - "github.com/jetstack/venafi-connection-lib/api/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/types" ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/jetstack/preflight/api" + "github.com/jetstack/preflight/pkg/client" + "github.com/jetstack/preflight/pkg/testutil" ) // These are using envtest (slow) rather than a fake clientset (fast) because @@ -46,7 +46,40 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { accessToken: - secret: name: accesstoken - fields: [accesstoken]`), + fields: [accesstoken] + --- + apiVersion: v1 + kind: Secret + metadata: + name: accesstoken + namespace: venafi + stringData: + accesstoken: VALID_ACCESS_TOKEN + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + metadata: + name: venafi-connection-accesstoken-reader + namespace: venafi + rules: + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] + resourceNames: ["accesstoken"] + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + name: venafi-connection-accesstoken-reader + namespace: venafi + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: venafi-connection-accesstoken-reader + subjects: + - kind: ServiceAccount + name: venafi-connection + namespace: venafi`), expectReadyCondMsg: "ea744d098c2c1c6044e4c4e9d3bf7c2a68ef30553db00f1714886cedf73230f1", })) t.Run("error when the apiKey field is used", run(testcase{ @@ -65,7 +98,40 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { apiKey: - secret: name: apikey - fields: [apikey]`), + fields: [apikey] + --- + apiVersion: v1 + kind: Secret + metadata: + name: apikey + namespace: venafi + stringData: + apikey: VALID_API_KEY + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + metadata: + name: venafi-connection-apikey-reader + namespace: venafi + rules: + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] + resourceNames: ["apikey"] + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + name: venafi-connection-apikey-reader + namespace: venafi + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: venafi-connection-apikey-reader + subjects: + - kind: ServiceAccount + name: venafi-connection + namespace: venafi`), expectReadyCondMsg: "b099d634ccec56556da28028743475dab67f79d079b668bedc3ef544f7eed2f3", expectErr: "VenafiConnection venafi/venafi-components: the agent cannot be used with an API key", })) @@ -92,67 +158,6 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { })) } -// Generated using: -// -// helm template ./deploy/charts/venafi-kubernetes-agent -n venafi --set venafiConnection.include=true --show-only templates/venafi-connection-rbac.yaml | grep -ivE '(helm|\/version)' -const rbac = ` -apiVersion: v1 -kind: Namespace -metadata: - name: venafi ---- -# Source: venafi-kubernetes-agent/templates/venafi-connection-rbac.yaml -# The 'venafi-connection' service account is used by multiple -# controllers. When configuring which resources a VenafiConnection -# can access, the RBAC rules you create manually must point to this SA. -apiVersion: v1 -kind: ServiceAccount -metadata: - name: venafi-connection - namespace: "venafi" - labels: - app.kubernetes.io/name: "venafi-connection" - app.kubernetes.io/instance: release-name ---- -# Source: venafi-kubernetes-agent/templates/venafi-connection-rbac.yaml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: venafi-connection-role - labels: - app.kubernetes.io/name: "venafi-connection" - app.kubernetes.io/instance: release-name -rules: -- apiGroups: [ "" ] - resources: [ "namespaces" ] - verbs: [ "get", "list", "watch" ] - -- apiGroups: [ "jetstack.io" ] - resources: [ "venaficonnections" ] - verbs: [ "get", "list", "watch" ] - -- apiGroups: [ "jetstack.io" ] - resources: [ "venaficonnections/status" ] - verbs: [ "get", "patch" ] ---- -# Source: venafi-kubernetes-agent/templates/venafi-connection-rbac.yaml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding -metadata: - name: venafi-connection-rolebinding - labels: - app.kubernetes.io/name: "venafi-connection" - app.kubernetes.io/instance: release-name -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: venafi-connection-role -subjects: -- kind: ServiceAccount - name: venafi-connection - namespace: "venafi" -` - type testcase struct { given string expectErr string @@ -179,18 +184,7 @@ func run(test testcase) func(t *testing.T) { ) require.NoError(t, err) - // This `cancel` is important because the below func `Start(ctx)` needs - // to be stopped before the apiserver is stopped. Otherwise, the test - // fail with the message "timeout waiting for process kube-apiserver to - // stop". See: - // https://github.com/jetstack/venafi-connection-lib/pull/158#issuecomment-1949002322 - // https://github.com/kubernetes-sigs/controller-runtime/issues/1571#issuecomment-945535598 - ctx, cancel := context.WithCancel(context.Background()) - go func() { - err = cl.Start(ctx) - require.NoError(t, err) - }() - t.Cleanup(cancel) + testutil.VenConnStartWatching(t, cl) // Apply the same RBAC as what you would get from the Venafi // Connection Helm chart, for example after running this: @@ -200,48 +194,7 @@ func run(test testcase) func(t *testing.T) { test.given = strings.ReplaceAll(test.given, "FAKE_TPP_URL", fakeTPP.URL) var given []ctrlruntime.Object - given = append(given, testutil.Parse(rbac)...) - given = append(given, testutil.Parse(testutil.Undent(` - apiVersion: v1 - kind: Secret - metadata: - name: accesstoken - namespace: venafi - stringData: - accesstoken: VALID_ACCESS_TOKEN - --- - apiVersion: v1 - kind: Secret - metadata: - name: apikey - namespace: venafi - stringData: - apikey: VALID_API_KEY - --- - apiVersion: rbac.authorization.k8s.io/v1 - kind: Role - metadata: - name: venafi-connection-secret-reader - namespace: venafi - rules: - - apiGroups: [""] - resources: ["secrets"] - verbs: ["get"] - resourceNames: ["accesstoken", "apikey"] - --- - apiVersion: rbac.authorization.k8s.io/v1 - kind: RoleBinding - metadata: - name: venafi-connection-secret-reader - namespace: venafi - roleRef: - apiGroup: rbac.authorization.k8s.io - kind: Role - name: venafi-connection-secret-reader - subjects: - - kind: ServiceAccount - name: venafi-connection - namespace: venafi`))...) + given = append(given, testutil.Parse(testutil.VenConnRBAC)...) given = append(given, testutil.Parse(test.given)...) for _, obj := range given { require.NoError(t, kclient.Create(context.Background(), obj)) diff --git a/pkg/datagatherer/k8s/cache.go b/pkg/datagatherer/k8s/cache.go index fd89c580..f40c3c09 100644 --- a/pkg/datagatherer/k8s/cache.go +++ b/pkg/datagatherer/k8s/cache.go @@ -3,10 +3,11 @@ package k8s import ( "time" - "github.com/jetstack/preflight/api" - "github.com/jetstack/preflight/pkg/logs" "github.com/pmylund/go-cache" "k8s.io/apimachinery/pkg/types" + + "github.com/jetstack/preflight/api" + "github.com/jetstack/preflight/pkg/logs" ) // time interface, this is used to fetch the current time diff --git a/pkg/datagatherer/k8s/cache_test.go b/pkg/datagatherer/k8s/cache_test.go index 71658471..80ab6a9e 100644 --- a/pkg/datagatherer/k8s/cache_test.go +++ b/pkg/datagatherer/k8s/cache_test.go @@ -6,9 +6,10 @@ import ( "time" "github.com/d4l3k/messagediff" - "github.com/jetstack/preflight/api" "github.com/pmylund/go-cache" "k8s.io/apimachinery/pkg/runtime" + + "github.com/jetstack/preflight/api" ) func makeGatheredResource(obj runtime.Object, deletedAt api.Time) *api.GatheredResource { diff --git a/pkg/datagatherer/k8s/discovery.go b/pkg/datagatherer/k8s/discovery.go index 9eec310e..49ac5324 100644 --- a/pkg/datagatherer/k8s/discovery.go +++ b/pkg/datagatherer/k8s/discovery.go @@ -4,8 +4,9 @@ import ( "context" "fmt" - "github.com/jetstack/preflight/pkg/datagatherer" "k8s.io/client-go/discovery" + + "github.com/jetstack/preflight/pkg/datagatherer" ) // ConfigDiscovery contains the configuration for the k8s-discovery data-gatherer diff --git a/pkg/datagatherer/k8s/dynamic.go b/pkg/datagatherer/k8s/dynamic.go index ca7f8d3a..d24b0629 100644 --- a/pkg/datagatherer/k8s/dynamic.go +++ b/pkg/datagatherer/k8s/dynamic.go @@ -6,7 +6,6 @@ import ( "strings" "time" - "github.com/jetstack/preflight/pkg/logs" "github.com/pkg/errors" "github.com/pmylund/go-cache" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" @@ -27,6 +26,7 @@ import ( "github.com/jetstack/preflight/api" "github.com/jetstack/preflight/pkg/datagatherer" + "github.com/jetstack/preflight/pkg/logs" ) // ConfigDynamic contains the configuration for the data-gatherer. diff --git a/pkg/datagatherer/k8s/dynamic_test.go b/pkg/datagatherer/k8s/dynamic_test.go index fe88ea0c..d64bc876 100644 --- a/pkg/datagatherer/k8s/dynamic_test.go +++ b/pkg/datagatherer/k8s/dynamic_test.go @@ -12,20 +12,20 @@ import ( "time" "github.com/d4l3k/messagediff" - "github.com/jetstack/preflight/api" "gopkg.in/yaml.v2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/dynamic/dynamicinformer" "k8s.io/client-go/dynamic/fake" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" fakeclientset "k8s.io/client-go/kubernetes/fake" k8scache "k8s.io/client-go/tools/cache" + + "github.com/jetstack/preflight/api" ) func getObject(version, kind, name, namespace string, withManagedFields bool) *unstructured.Unstructured { diff --git a/pkg/echo/echo.go b/pkg/echo/echo.go index cb23c450..62fa5f7a 100644 --- a/pkg/echo/echo.go +++ b/pkg/echo/echo.go @@ -6,9 +6,10 @@ import ( "net/http" "github.com/fatih/color" + "github.com/spf13/cobra" + "github.com/jetstack/preflight/api" "github.com/jetstack/preflight/pkg/logs" - "github.com/spf13/cobra" ) var EchoListen string diff --git a/pkg/permissions/generate.go b/pkg/permissions/generate.go index 8894cd05..7e3ab08e 100644 --- a/pkg/permissions/generate.go +++ b/pkg/permissions/generate.go @@ -4,11 +4,12 @@ import ( "fmt" "strings" - "github.com/jetstack/preflight/pkg/agent" - "github.com/jetstack/preflight/pkg/datagatherer/k8s" rbac "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" + + "github.com/jetstack/preflight/pkg/agent" + "github.com/jetstack/preflight/pkg/datagatherer/k8s" ) // AgentRBACManifests is a wrapper around the various RBAC structs needed to grant the agent fine-grained permissions as per its dg configs diff --git a/pkg/permissions/generate_test.go b/pkg/permissions/generate_test.go index 5591e4c1..0a594d86 100644 --- a/pkg/permissions/generate_test.go +++ b/pkg/permissions/generate_test.go @@ -3,12 +3,13 @@ package permissions import ( "testing" - "github.com/jetstack/preflight/pkg/agent" - "github.com/jetstack/preflight/pkg/datagatherer/k8s" "github.com/maxatome/go-testdeep/td" rbac "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/jetstack/preflight/pkg/agent" + "github.com/jetstack/preflight/pkg/datagatherer/k8s" ) func TestGenerateAgentRBACManifestsString(t *testing.T) { diff --git a/pkg/testutil/undent_test.go b/pkg/testutil/undent_test.go index b0a70003..9908a2ea 100644 --- a/pkg/testutil/undent_test.go +++ b/pkg/testutil/undent_test.go @@ -31,7 +31,7 @@ func Test_Undent(t *testing.T) { t.Run("you can also omit the tabs or spaces for empty lines", runTest_Undent(` foo - + bar `, "foo\n\nbar\n")) } From ed9fb59641f7d74e89903ab64e239c3a63c2bd54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 20 Sep 2024 11:31:56 +0200 Subject: [PATCH 03/16] uploader_id: mention that it will be no required needed by the API --- pkg/agent/config.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 47a2a085..80ae6da2 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -433,6 +433,9 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags) // backend. Since the backend requires it for historical reasons (but cannot // be empty), we just ignore whatever the user has set in the config file, // and set it to an arbitrary value in the client since it doesn't matter. + // + // TODO(mael): Remove the arbitrary `/no` path parameter from the Agent once + // https://venafi.atlassian.net/browse/VC-35385 is done. { if res.AuthMode == VenafiCloudVenafiConnection || res.AuthMode == VenafiCloudKeypair { if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploaderID != "" { From 793d855cadc861657c0586232a47706b42c72f5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 20 Sep 2024 11:35:53 +0200 Subject: [PATCH 04/16] cmd-line: clarify the "auth missing" message by adding parentheses Co-authored-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/agent/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 80ae6da2..49a6c770 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -349,7 +349,7 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags) log.Printf("Using the %s auth mode since --credentials-file was specified without --venafi-cloud.", mode) default: return CombinedConfig{}, nil, fmt.Errorf("no auth mode specified. You can use one of four auth modes:\n" + - " - Use --venafi-cloud with --credentials-file or --client-id with --private-key-path to use the " + string(VenafiCloudKeypair) + " mode.\n" + + " - Use (--venafi-cloud with --credentials-file) or (--client-id with --private-key-path) to use the " + string(VenafiCloudKeypair) + " mode.\n" + " - Use --venafi-connection for the " + string(VenafiCloudVenafiConnection) + " mode.\n" + " - Use --credentials-file alone if you want to use the " + string(JetstackSecureOAuth) + " mode.\n" + " - Use --api-token if you want to use the " + string(JetstackSecureAPIToken) + " mode.\n") From 2801ca7063e061d26b79e0de4fb56b213912dc08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 20 Sep 2024 12:06:13 +0200 Subject: [PATCH 05/16] envtest rbac: add a todo to remind us to stop hardcoding VenConnRBAC --- pkg/testutil/envtest.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/testutil/envtest.go b/pkg/testutil/envtest.go index 79a80f4a..6671dcae 100644 --- a/pkg/testutil/envtest.go +++ b/pkg/testutil/envtest.go @@ -244,6 +244,10 @@ func FakeTPP(t testing.TB) (*httptest.Server, *x509.Certificate) { // Generated using: // // helm template ./deploy/charts/venafi-kubernetes-agent -n venafi --set venafiConnection.include=true --show-only templates/venafi-connection-VenConnRBAC.yaml | grep -ivE '(helm|\/version)' +// +// TODO(mael): Once we get the Makefile modules setup, we should generate this +// based on the Helm chart rather than having it hardcoded here. Ticket: +// https://venafi.atlassian.net/browse/VC-36331 const VenConnRBAC = ` apiVersion: v1 kind: Namespace From 7be53429045bbf9443f68ffcb3842b9b5d14bdf4 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 20 Sep 2024 13:58:21 +0200 Subject: [PATCH 06/16] uploader_id: unconditionally show a log line when venafi-cloud.uploader_id is set Co-authored-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/agent/config.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 49a6c770..ca08880d 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -437,10 +437,8 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags) // TODO(mael): Remove the arbitrary `/no` path parameter from the Agent once // https://venafi.atlassian.net/browse/VC-35385 is done. { - if res.AuthMode == VenafiCloudVenafiConnection || res.AuthMode == VenafiCloudKeypair { - if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploaderID != "" { - log.Printf(`ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in %s mode.`, res.AuthMode) - } + if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploaderID != "" { + log.Printf(`ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in %s mode.`, res.AuthMode) } } From b8296724a18b2c5df57b4972730eb2c621725eb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 20 Sep 2024 14:41:59 +0200 Subject: [PATCH 07/16] help: --one-shot, --input-path, and --output-path are meant for testing purposes --- pkg/agent/config.go | 89 ++++++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 34 deletions(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index ca08880d..6dae22cd 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -29,24 +29,34 @@ const ( // Config wraps the options for a run of the agent. type Config struct { + // Deprecated: Schedule doesn't do anything. Use `period` instead. Schedule string `yaml:"schedule"` Period time.Duration `yaml:"period"` - // Deprecated: Endpoint is being replaced with Server. + + // Deprecated: Use `server` instead. Endpoint Endpoint `yaml:"endpoint"` - // Server is the base url for the Preflight server. - // It defaults to https://preflight.jetstack.io. + + // Server is the base URL for the Preflight server. It defaults to + // https://preflight.jetstack.io in Jetstack Secure OAuth and Jetstack + // Secure API Token modes, and https://api.venafi.cloud in Venafi Cloud Key + // Pair Service Account mode. It is ignored in Venafi Cloud VenafiConnection + // mode. Server string `yaml:"server"` - // OrganizationID within Preflight that will receive the data. + + // OrganizationID is only used in Jetstack Secure OAuth and Jetstack Secure + // API Token modes. OrganizationID string `yaml:"organization_id"` - // ClusterID is the cluster that the agent is scanning. - ClusterID string `yaml:"cluster_id"` - ClusterDescription string `yaml:"cluster_description"` - DataGatherers []DataGatherer `yaml:"data-gatherers"` - // InputPath replaces DataGatherers with input data file + + // ClusterID is the cluster that the agent is scanning. Used in all modes. + ClusterID string `yaml:"cluster_id"` + ClusterDescription string `yaml:"cluster_description"` + DataGatherers []DataGatherer `yaml:"data-gatherers"` + VenafiCloud *VenafiCloudConfig `yaml:"venafi-cloud,omitempty"` + + // For testing purposes. InputPath string `yaml:"input-path"` - // OutputPath replaces Server with output data file - OutputPath string `yaml:"output-path"` - VenafiCloud *VenafiCloudConfig `yaml:"venafi-cloud,omitempty"` + // For testing purposes. + OutputPath string `yaml:"output-path"` } type Endpoint struct { @@ -80,11 +90,9 @@ type AgentCmdFlags struct { // precedence over the config field `period`. Period time.Duration - // OneShot (--one-shot) flag causes agent to run once. - OneShot bool - - // VenafiCloudMode (--venafi-cloud) determines which format to load for - // config and credential type. + // VenafiCloudMode (--venafi-cloud) turns on the Venafi Cloud Key Pair + // Service Account mode. Must be used in conjunction with + // --credentials-file. VenafiCloudMode bool // ClientID (--client-id) is the clientID in case of Venafi Cloud Key Pair @@ -95,16 +103,27 @@ type AgentCmdFlags struct { // private key in case of Venafi Cloud Key Pair Service Account mode. PrivateKeyPath string - // CredentialsPath (--credentials-file, -k) is the path to the credentials ) - // is where the agent will try to loads the credentials (Experimental). + // CredentialsPath (--credentials-file, -k) lets you specify the location of + // the credentials file. This is used for the Jetstack Secure OAuth and + // Venafi Cloud Key Pair Service Account modes. In Venafi Cloud Key Pair + // Service Account mode, you also need to pass --venafi-cloud. CredentialsPath string - // OutputPath (--output-path) is where the agent will write data to instead - // of uploading to server. + // OneShot (--one-shot) is used for testing purposes. The agent will run + // once and exit. It is often used in conjunction with --output-path and/or + // --input-path. + OneShot bool + + // OutputPath (--output-path) is used for testing purposes. In conjunction + // with --one-shot, it allows you to write the data readings to a file + // instead uploading them to the Venafi Cloud API. OutputPath string - // InputPath (--input-path) is where the agent will read data from instead - // of gathering data from clusters. + // InputPath (--input-path) is used for testing purposes. In conjunction + // with --one-shot, it allows you to push manually crafted data readings (in + // JSON format) to the Venafi Cloud API without the need to connect to a + // Kubernetes cluster. See the jscp-testing-cli's README for more info: + // https://gitlab.com/venafi/vaas/applications/tls-protect-for-k8s/cloud-services/-/tree/master/jscp-testing-cli InputPath string // BackoffMaxTime (--backoff-max-time) is the maximum time for which data @@ -114,8 +133,8 @@ type AgentCmdFlags struct { // StrictMode (--strict) causes the agent to fail at the first attempt. StrictMode bool - // APIToken (--api-token) is meant for the old Jetstack Secure API and is an - // alternative to OAuth. + // APIToken (--api-token) allows you to use the Jetstack Secure API Token + // mode. Defaults to the value of the env var API_TOKEN. APIToken string // VenConnName (--venafi-connection) is the name of the VenafiConnection @@ -171,7 +190,7 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { "venafi-cloud", "", false, - fmt.Sprintf("Turn on the %s mode. The flag --credentials-file must also be passed.", JetstackSecureOAuth), + fmt.Sprintf("Turns on the %s mode. The flag --credentials-file must also be passed.", JetstackSecureOAuth), ) c.PersistentFlags().StringVarP( &cfg.ClientID, @@ -194,21 +213,21 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { "one-shot", "", false, - "Runs agent a single time if true, or continously if false", + "For testing purposes. The agent will run once and exit. It is often used in conjunction with --output-path and/or --input-path.", ) c.PersistentFlags().StringVarP( &cfg.OutputPath, "output-path", "", "", - "Output file path, if used, it will write data to a local file instead of uploading to the preflight server", + "For testing purposes. In conjunction with --one-shot, it allows you to write the data readings to a file instead of uploading to the server.", ) c.PersistentFlags().StringVarP( &cfg.InputPath, "input-path", "", "", - "Input file path, if used, it will read data from a local file instead of gathering data from clusters", + "For testing purposes. In conjunction with --one-shot, it allows you to push manually crafted data readings (in JSON format) to the Venafi Cloud API without the need to connect to a Kubernetes cluster.", ) c.PersistentFlags().DurationVarP( &cfg.BackoffMaxTime, @@ -228,14 +247,14 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { &cfg.APIToken, "api-token", os.Getenv("API_TOKEN"), - fmt.Sprintf("Turns on the %s mode. Defaults to the value of the env var API_TOKEN.", JetstackSecureAPIToken), + "Turns on the "+string(JetstackSecureAPIToken)+" mode. Defaults to the value of the env var API_TOKEN.", ) c.PersistentFlags().StringVar( &cfg.VenConnName, "venafi-connection", "", - fmt.Sprintf("Turns on the %s mode. This flag configures the name of the "+ - "VenafiConnection to be used.", VenafiCloudVenafiConnection), + "Turns on the "+string(VenafiCloudVenafiConnection)+" mode. "+ + "This flag configures the name of the VenafiConnection to be used.", ) c.PersistentFlags().StringVar( &cfg.VenConnNS, @@ -249,8 +268,9 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { &cfg.InstallNS, "install-namespace", "", - fmt.Sprintf("Namespace in which the agent is running. Only needed with the %s mode"+ - "when running the agent outside of Kubernetes. Used for testing purposes.", VenafiCloudVenafiConnection), + "For testing purposes. Namespace in which the agent is running. "+ + "Only needed with the "+string(VenafiCloudVenafiConnection)+" mode"+ + "when running the agent outside of Kubernetes.", ) c.PersistentFlags().BoolVarP( &cfg.Profiling, @@ -266,6 +286,7 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { false, "Enables Prometheus metrics server on the agent (port: 8081).", ) + } type AuthMode string From fc4d3ae11d3a1ba31216cab547006e4eb981acf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 20 Sep 2024 17:27:44 +0200 Subject: [PATCH 08/16] help: flag --private-key-path is no longer set by default Since --private-key-path isn't set by default, it is now necessary to pass it explicitely. Note that secretKey wasn't used before this commit; with this commit, secretKey becomes useful. --- deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml | 2 ++ pkg/agent/config.go | 1 + 2 files changed, 3 insertions(+) diff --git a/deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml b/deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml index ca3ef98a..772aeaa7 100644 --- a/deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml +++ b/deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml @@ -65,6 +65,8 @@ spec: {{- else }} - "--client-id" - {{ .Values.config.clientId | quote }} + - "--private-key-path" + - "/etc/venafi/agent/key/{{ .Values.authentication.secretKey }}" {{- end }} - "-p" - "0h1m0s" diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 6dae22cd..bd3b2df9 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -192,6 +192,7 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { false, fmt.Sprintf("Turns on the %s mode. The flag --credentials-file must also be passed.", JetstackSecureOAuth), ) + c.PersistentFlags().MarkHidden("venafi-cloud") c.PersistentFlags().StringVarP( &cfg.ClientID, "client-id", From 23f26bd2134ae78e8b2a8372f182801de6cb7cd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 23 Sep 2024 15:17:28 +0200 Subject: [PATCH 09/16] VenConnClient: fix failing tests and use a single envtest process --- pkg/agent/config_test.go | 26 ++--- pkg/client/client_venafi_cloud.go | 10 +- pkg/client/client_venconn_test.go | 167 ++++++++++++++++++++++++------ pkg/testutil/envtest.go | 2 +- 4 files changed, 153 insertions(+), 52 deletions(-) diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index 80278b98..97f1f565 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -155,7 +155,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { ) assert.EqualError(t, err, testutil.Undent(` no auth mode specified. You can use one of four auth modes: - - Use --venafi-cloud with --credentials-file or --client-id with --private-key-path to use the Venafi Cloud Key Pair Service Account mode. + - Use (--venafi-cloud with --credentials-file) or (--client-id with --private-key-path) to use the Venafi Cloud Key Pair Service Account mode. - Use --venafi-connection for the Venafi Cloud VenafiConnection mode. - Use --credentials-file alone if you want to use the Jetstack Secure OAuth mode. - Use --api-token if you want to use the Jetstack Secure API Token mode. @@ -196,6 +196,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { Server: "http://example.com", OrganizationID: "example", EndpointPath: "api/v1/data", + BackoffMaxTime: 10 * time.Minute, } require.NoError(t, err) assert.Equal(t, expect, got) @@ -221,7 +222,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { uploader_id: test-agent upload_path: "/testing/path" `)), - withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath, "--backoff-max-time", "5m"), + withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath, "--backoff-max-time", "99m"), ) expect := CombinedConfig{ Server: "http://localhost:8080", @@ -234,7 +235,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { UploadPath: "/testing/path", AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", - BackoffMaxTime: 5 * time.Minute, + BackoffMaxTime: 99 * time.Minute, } require.NoError(t, err) assert.Equal(t, expect, got) @@ -337,7 +338,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { `)), withCmdLineFlags("--credentials-file", path)) require.NoError(t, err) - assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, OrganizationID: "foo", ClusterID: "bar", AuthMode: JetstackSecureOAuth}, got) + assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, OrganizationID: "foo", ClusterID: "bar", AuthMode: JetstackSecureOAuth, BackoffMaxTime: 10 * time.Minute}, got) assert.IsType(t, &client.OAuthClient{}, cl) }) @@ -411,7 +412,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { `)), withCmdLineFlags("--client-id", "5bc7d07c-45da-11ef-a878-523f1e1d7de1", "--private-key-path", path)) require.NoError(t, err) - assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", UploadPath: "/foo/bar"}, got) + assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", UploadPath: "/foo/bar", BackoffMaxTime: 10 * time.Minute}, got) assert.IsType(t, &client.VenafiCloudClient{}, cl) }) @@ -432,7 +433,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { `)), withCmdLineFlags("--venafi-cloud", "--credentials-file", credsPath)) require.NoError(t, err) - assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", UploadPath: "/foo/bar"}, got) + assert.Equal(t, CombinedConfig{Server: "https://api.venafi.eu", Period: time.Hour, AuthMode: VenafiCloudKeypair, ClusterID: "the cluster name", UploadPath: "/foo/bar", BackoffMaxTime: 10 * time.Minute}, got) }) t.Run("venafi-cloud-keypair-auth: venafi-cloud.upload_path field is required", func(t *testing.T) { @@ -503,12 +504,13 @@ func Test_ValidateAndCombineConfig(t *testing.T) { withCmdLineFlags("--install-namespace", "venafi", "--venafi-connection", "venafi-components")) require.NoError(t, err) assert.Equal(t, CombinedConfig{ - Period: 1 * time.Hour, - ClusterID: "the cluster name", - AuthMode: VenafiCloudVenafiConnection, - VenConnName: "venafi-components", - VenConnNS: "venafi", - InstallNS: "venafi", + Period: 1 * time.Hour, + ClusterID: "the cluster name", + AuthMode: VenafiCloudVenafiConnection, + VenConnName: "venafi-components", + VenConnNS: "venafi", + InstallNS: "venafi", + BackoffMaxTime: 10 * time.Minute, }, got) assert.IsType(t, &client.VenConnClient{}, cl) }) diff --git a/pkg/client/client_venafi_cloud.go b/pkg/client/client_venafi_cloud.go index 91021da5..4faef4ee 100644 --- a/pkg/client/client_venafi_cloud.go +++ b/pkg/client/client_venafi_cloud.go @@ -84,11 +84,11 @@ const ( // to authenticate to the backend API. func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiSvcAccountCredentials, baseURL string, uploaderID string, uploadPath string) (*VenafiCloudClient, error) { if err := credentials.Validate(); err != nil { - return nil, fmt.Errorf("cannot create VenafiCloudClient: %v", err) + return nil, fmt.Errorf("cannot create VenafiCloudClient: %w", err) } privateKey, jwtSigningAlg, err := parsePrivateKeyAndExtractSigningMethod(credentials.PrivateKeyFile) if err != nil { - return nil, fmt.Errorf("error parsing private key file %v", err) + return nil, fmt.Errorf("while parsing private key file: %w", err) } if baseURL == "" { return nil, fmt.Errorf("cannot create VenafiCloudClient: baseURL cannot be empty") @@ -380,7 +380,7 @@ func parsePrivateKeyFromPemFile(privateKeyFilePath string) (crypto.PrivateKey, e der, _ := pem.Decode(pkBytes) if der == nil { - return nil, fmt.Errorf("error decoding private key from pem file %q", privateKeyFilePath) + return nil, fmt.Errorf("while decoding the PEM-encoded private key %v, its content were: %s", privateKeyFilePath, string(pkBytes)) } if key, err := x509.ParsePKCS1PrivateKey(der.Bytes); err == nil { @@ -391,13 +391,13 @@ func parsePrivateKeyFromPemFile(privateKeyFilePath string) (crypto.PrivateKey, e case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey: return key, nil default: - return nil, fmt.Errorf("found unknown private key type in PKCS#8 wrapping") + return nil, fmt.Errorf("found unknown private key type in PKCS#8 wrapping: %T", key) } } if key, err := x509.ParseECPrivateKey(der.Bytes); err == nil { return key, nil } - return nil, fmt.Errorf("failed to parse private key") + return nil, fmt.Errorf("while parsing EC private: %w", err) } func parsePrivateKeyAndExtractSigningMethod(privateKeyFile string) (crypto.PrivateKey, jwt.SigningMethod, error) { diff --git a/pkg/client/client_venconn_test.go b/pkg/client/client_venconn_test.go index baa9fcdf..f765f1ad 100644 --- a/pkg/client/client_venconn_test.go +++ b/pkg/client/client_venconn_test.go @@ -3,6 +3,8 @@ package client_test import ( "context" "crypto/x509" + "net/http" + "regexp" "strings" "testing" @@ -10,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/rest" ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client" "github.com/jetstack/preflight/api" @@ -31,15 +34,19 @@ import ( // // [1] https://github.com/kubernetes-sigs/controller-runtime/issues/2341 func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { + _, restconf, kclient := testutil.WithEnvtest(t) + for _, obj := range testutil.Parse(testutil.VenConnRBAC) { + require.NoError(t, kclient.Create(context.Background(), obj)) + } t.Parallel() - t.Run("valid accessToken", run(testcase{ + t.Run("valid accessToken", run_TestVenConnClient_PostDataReadingsWithOptions(restconf, kclient, testcase{ given: testutil.Undent(` apiVersion: jetstack.io/v1alpha1 kind: VenafiConnection metadata: name: venafi-components - namespace: venafi + namespace: TEST_NAMESPACE spec: vcp: url: FAKE_VENAFI_CLOUD_URL @@ -47,12 +54,15 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { - secret: name: accesstoken fields: [accesstoken] + allowReferencesFrom: + matchExpressions: + - {key: kubernetes.io/metadata.name, operator: In, values: [venafi]} --- apiVersion: v1 kind: Secret metadata: name: accesstoken - namespace: venafi + namespace: TEST_NAMESPACE stringData: accesstoken: VALID_ACCESS_TOKEN --- @@ -60,7 +70,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { kind: Role metadata: name: venafi-connection-accesstoken-reader - namespace: venafi + namespace: TEST_NAMESPACE rules: - apiGroups: [""] resources: ["secrets"] @@ -71,7 +81,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { kind: RoleBinding metadata: name: venafi-connection-accesstoken-reader - namespace: venafi + namespace: TEST_NAMESPACE roleRef: apiGroup: rbac.authorization.k8s.io kind: Role @@ -79,10 +89,11 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { subjects: - kind: ServiceAccount name: venafi-connection - namespace: venafi`), + namespace: venafi + `), expectReadyCondMsg: "ea744d098c2c1c6044e4c4e9d3bf7c2a68ef30553db00f1714886cedf73230f1", })) - t.Run("error when the apiKey field is used", run(testcase{ + t.Run("error when the apiKey field is used", run_TestVenConnClient_PostDataReadingsWithOptions(restconf, kclient, testcase{ // Why isn't it possible to use the 'apiKey' field? Although the // Kubernetes Discovery endpoint works with an API key, we have decided // to not support it because it isn't recommended. @@ -91,7 +102,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { kind: VenafiConnection metadata: name: venafi-components - namespace: venafi + namespace: TEST_NAMESPACE spec: vcp: url: FAKE_VENAFI_CLOUD_URL @@ -99,12 +110,15 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { - secret: name: apikey fields: [apikey] + allowReferencesFrom: + matchExpressions: + - {key: kubernetes.io/metadata.name, operator: In, values: [venafi]} --- apiVersion: v1 kind: Secret metadata: name: apikey - namespace: venafi + namespace: TEST_NAMESPACE stringData: apikey: VALID_API_KEY --- @@ -112,7 +126,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { kind: Role metadata: name: venafi-connection-apikey-reader - namespace: venafi + namespace: TEST_NAMESPACE rules: - apiGroups: [""] resources: ["secrets"] @@ -123,7 +137,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { kind: RoleBinding metadata: name: venafi-connection-apikey-reader - namespace: venafi + namespace: TEST_NAMESPACE roleRef: apiGroup: rbac.authorization.k8s.io kind: Role @@ -131,11 +145,14 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { subjects: - kind: ServiceAccount name: venafi-connection - namespace: venafi`), + namespace: venafi + `), + // PostDataReadingsWithOptions failed, but Get succeeded; that's why the + // condition says the VenafiConnection is ready. expectReadyCondMsg: "b099d634ccec56556da28028743475dab67f79d079b668bedc3ef544f7eed2f3", - expectErr: "VenafiConnection venafi/venafi-components: the agent cannot be used with an API key", + expectErr: "VenafiConnection error-when-the-apikey-field-is-used/venafi-components: the agent cannot be used with an API key", })) - t.Run("error when the tpp field is used", run(testcase{ + t.Run("error when the tpp field is used", run_TestVenConnClient_PostDataReadingsWithOptions(restconf, kclient, testcase{ // IMPORTANT: The user may think they can use 'tpp', spend time // debugging and making the venafi connection work, and then find out // that it doesn't work. The reason is because as of now, we don't first @@ -145,16 +162,53 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { kind: VenafiConnection metadata: name: venafi-components - namespace: venafi + namespace: TEST_NAMESPACE spec: tpp: url: FAKE_TPP_URL accessToken: - secret: name: accesstoken - fields: [accesstoken]`), - expectErr: ``, - expectReadyCondMsg: `ea744d098c2c1c6044e4c4e9d3bf7c2a68ef30553db00f1714886cedf73230f1`, + fields: [accesstoken] + allowReferencesFrom: + matchExpressions: + - {key: kubernetes.io/metadata.name, operator: In, values: [venafi]} + --- + apiVersion: v1 + kind: Secret + metadata: + name: accesstoken + namespace: TEST_NAMESPACE + stringData: + accesstoken: VALID_ACCESS_TOKEN + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + metadata: + name: venafi-connection-accesstoken-reader + namespace: TEST_NAMESPACE + rules: + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] + resourceNames: ["accesstoken"] + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + name: venafi-connection-accesstoken-reader + namespace: TEST_NAMESPACE + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: venafi-connection-accesstoken-reader + subjects: + - kind: ServiceAccount + name: venafi-connection + namespace: venafi + `), + expectReadyCondMsg: "ea744d098c2c1c6044e4c4e9d3bf7c2a68ef30553db00f1714886cedf73230f1", + expectErr: "VenafiConnection error-when-the-tpp-field-is-used/venafi-components: the agent cannot be used with TPP", })) } @@ -164,51 +218,96 @@ type testcase struct { expectReadyCondMsg string } -func run(test testcase) func(t *testing.T) { +// All tests share the same envtest (i.e., the same apiserver and etcd process), +// so each test needs to be contained in its own Kubernetes namespace. +func run_TestVenConnClient_PostDataReadingsWithOptions(restcfg *rest.Config, kclient ctrlruntime.WithWatch, test testcase) func(t *testing.T) { return func(t *testing.T) { - fakeVenafiCloud, certCloud, _ := testutil.FakeVenafiCloud(t) + t.Helper() + + fakeVenafiCloud, certCloud, fakeVenafiAssert := testutil.FakeVenafiCloud(t) fakeTPP, certTPP := testutil.FakeTPP(t) - _, restconf, kclient := testutil.WithEnvtest(t) + fakeVenafiAssert(func(t testing.TB, r *http.Request) { + if r.URL.Path == "/v1/useraccounts" { + return // We only care about /v1/tlspk/upload/clusterdata. + } + // Let's make sure we didn't forget to add the arbitrary "/no" + // (uploader_id) path segment to /v1/tlspk/upload/clusterdata. + assert.Equal(t, "/v1/tlspk/upload/clusterdata/no", r.URL.Path) + }) certPool := x509.NewCertPool() certPool.AddCert(certCloud) certPool.AddCert(certTPP) cl, err := client.NewVenConnClient( - restconf, + restcfg, &api.AgentMetadata{ClusterID: "no"}, - "venafi", // Namespace in which the Agent is running. - "venafi-components", // Name of the VenafiConnection. - "venafi", // Namespace of the VenafiConnection. + "venafi", // Namespace in which the Agent is running. + "venafi-components", // Name of the VenafiConnection. + testNameToNamespace(t), // Namespace of the VenafiConnection. certPool, ) require.NoError(t, err) testutil.VenConnStartWatching(t, cl) - // Apply the same RBAC as what you would get from the Venafi - // Connection Helm chart, for example after running this: - // helm template venafi-connection oci://registry.venafi.cloud/charts/venafi-connection --version v0.1.0 -n venafi --show-only templates/venafi-connection-rbac.yaml - test.given = strings.ReplaceAll(test.given, "FAKE_VENAFI_CLOUD_URL", fakeVenafiCloud.URL) test.given = strings.ReplaceAll(test.given, "FAKE_TPP_URL", fakeTPP.URL) + test.given = strings.ReplaceAll(test.given, "TEST_NAMESPACE", testNameToNamespace(t)) - var given []ctrlruntime.Object - given = append(given, testutil.Parse(testutil.VenConnRBAC)...) - given = append(given, testutil.Parse(test.given)...) - for _, obj := range given { + var givenObjs []ctrlruntime.Object + givenObjs = append(givenObjs, testutil.Parse(testutil.Undent(` + apiVersion: v1 + kind: Namespace + metadata: + name: `+testNameToNamespace(t)))...) + givenObjs = append(givenObjs, testutil.Parse(test.given)...) + for _, obj := range givenObjs { require.NoError(t, kclient.Create(context.Background(), obj)) } err = cl.PostDataReadingsWithOptions([]*api.DataReading{}, client.Options{ClusterName: "test cluster name"}) if test.expectErr != "" { assert.EqualError(t, err, test.expectErr) + } else { + require.NoError(t, err) } got := v1alpha1.VenafiConnection{} - err = kclient.Get(context.Background(), types.NamespacedName{Name: "venafi-components", Namespace: "venafi"}, &got) + err = kclient.Get(context.Background(), types.NamespacedName{Name: "venafi-components", Namespace: testNameToNamespace(t)}, &got) require.NoError(t, err) require.Len(t, got.Status.Conditions, 1) assert.Equal(t, test.expectReadyCondMsg, got.Status.Conditions[0].Message) } } + +// Because we want valid namespaces for each of the tests, this func converts a +// test name into a valid Kubernetes namespace (i.e., a DNS label as per RFC +// 1123, including trimming to 63 chars). +// +// For example, the test name: +// +// Test/sub test has special chars ':"-;@# and is also super super super super long! +// +// will be converted to: +// +// sub-test-has-special-chars-and-is-also-super-super-super-super- +// +// Only the last part of the test name is used. +func testNameToNamespace(t testing.TB) string { + regex := regexp.MustCompile("[^a-zA-Z0-9-]") + + // Only keep the part after the last slash. + parts := strings.Split(t.Name(), "/") + if len(parts) == 0 { + return "" + } + + s := parts[len(parts)-1] + s = strings.ToLower(s) + s = strings.ReplaceAll(s, "_", "-") + s = regex.ReplaceAllString(s, "") + s = strings.TrimLeft(s, "-") + s = strings.TrimRight(s, "-") + return s +} diff --git a/pkg/testutil/envtest.go b/pkg/testutil/envtest.go index 6671dcae..089b909d 100644 --- a/pkg/testutil/envtest.go +++ b/pkg/testutil/envtest.go @@ -243,7 +243,7 @@ func FakeTPP(t testing.TB) (*httptest.Server, *x509.Certificate) { // Generated using: // -// helm template ./deploy/charts/venafi-kubernetes-agent -n venafi --set venafiConnection.include=true --show-only templates/venafi-connection-VenConnRBAC.yaml | grep -ivE '(helm|\/version)' +// helm template ./deploy/charts/venafi-kubernetes-agent -n venafi --set crds.venafiConnection.include=true --show-only templates/venafi-connection-rbac.yaml | grep -ivE '(helm|\/version)' // // TODO(mael): Once we get the Makefile modules setup, we should generate this // based on the Helm chart rather than having it hardcoded here. Ticket: From f00b5fa49144bbff30dad03ab5f982563575e518 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 23 Sep 2024 16:23:26 +0200 Subject: [PATCH 10/16] config_test: add a failing test to showcase that uploader_id missing I found that, in venafi cloud key pair service account mode, the arbitrary path segment "/no" was now missing due to my refactoring. This commit adds a unit test to show this. Test result: --- FAIL: Test_ValidateAndCombineConfig_VenafiCloudKeyPair (0.00s) --- FAIL: Test_ValidateAndCombineConfig_VenafiCloudKeyPair/server,_uploader_id,_and_cluster_name_are_correctly_passed (0.00s) envtest.go:188: fake api.venafi.cloud received request: POST /v1/oauth/token/serviceaccount envtest.go:188: fake api.venafi.cloud received request: POST /v1/tlspk/upload/clusterdata config_test.go:586: Error: Not equal: expected: "/v1/tlspk/upload/clusterdata/no" actual : "/v1/tlspk/upload/clusterdata" Diff: --- Expected +++ Actual @@ -1 +1 @@ -/v1/tlspk/upload/clusterdata/no +/v1/tlspk/upload/clusterdata Test: Test_ValidateAndCombineConfig_VenafiCloudKeyPair/server,_uploader_id,_and_cluster_name_are_correctly_passed --- pkg/agent/config_test.go | 44 +++++++++++++++++++++++++++---- pkg/client/client_venafi_cloud.go | 10 ++++--- pkg/testutil/envtest.go | 42 ++++++++++++++++++++--------- 3 files changed, 74 insertions(+), 22 deletions(-) diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index 97f1f565..c726a598 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -81,7 +81,6 @@ func Test_ValidateAndCombineConfig(t *testing.T) { assert.Equal(t, testutil.Undent(` Using the Jetstack Secure OAuth auth mode since --credentials-file was specified without --venafi-cloud. Both the 'period' field and --period are set. Using the value provided with --period. - `), gotLogs.String()) assert.Equal(t, 99*time.Minute, got.Period) }) @@ -572,9 +571,44 @@ func Test_ValidateAndCombineConfig(t *testing.T) { }) } +func Test_ValidateAndCombineConfig_VenafiCloudKeyPair(t *testing.T) { + t.Run("server, uploader_id, and cluster name are correctly passed", func(t *testing.T) { + srv, cert, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + // Only care about /v1/tlspk/upload/clusterdata/:uploader_id?name= + if gotReq.URL.Path == "/v1/oauth/token/serviceaccount" { + return + } + + assert.Equal(t, srv.URL, "https://"+gotReq.Host) + assert.Equal(t, "test cluster name", gotReq.URL.Query().Get("name")) + assert.Equal(t, "/v1/tlspk/upload/clusterdata/no", gotReq.URL.Path) + }) + + privKeyPath := withFile(t, fakePrivKeyPEM) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: `+srv.URL+` + period: 1h + cluster_id: "test cluster name" + venafi-cloud: + uploader_id: no + upload_path: /v1/tlspk/upload/clusterdata + `)), + withCmdLineFlags("--client-id", "5bc7d07c-45da-11ef-a878-523f1e1d7de1", "--private-key-path", privKeyPath), + ) + testutil.TrustCA(t, cl, cert) + assert.Equal(t, VenafiCloudKeypair, got.AuthMode) + require.NoError(t, err) + + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: "test cluster name"}) + require.NoError(t, err) + }) +} + // Slower test cases due to envtest. That's why they are separated from the // other tests. -func Test_ValidateAndCombineConfig_urlWhenVenafiConnection(t *testing.T) { +func Test_ValidateAndCombineConfig_VenafiConnection(t *testing.T) { _, cfg, kcl := testutil.WithEnvtest(t) t.Setenv("KUBECONFIG", testutil.WithKubeconfig(t, cfg)) srv, cert, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) @@ -588,7 +622,7 @@ func Test_ValidateAndCombineConfig_urlWhenVenafiConnection(t *testing.T) { namespace: venafi spec: vcp: - url: "%s" + url: "`+srv.URL+`" accessToken: - secret: name: accesstoken @@ -626,7 +660,7 @@ func Test_ValidateAndCombineConfig_urlWhenVenafiConnection(t *testing.T) { - kind: ServiceAccount name: venafi-connection namespace: venafi - `, srv.URL))) { + `))) { require.NoError(t, kcl.Create(context.Background(), obj)) } @@ -654,7 +688,7 @@ func Test_ValidateAndCombineConfig_urlWhenVenafiConnection(t *testing.T) { require.NoError(t, err) testutil.VenConnStartWatching(t, cl) - testutil.VenConnTrustCA(t, cl, cert) + testutil.TrustCA(t, cl, cert) // TODO(mael): the client should keep track of the cluster ID, we // shouldn't need to pass it as an option to diff --git a/pkg/client/client_venafi_cloud.go b/pkg/client/client_venafi_cloud.go index 4faef4ee..7e317faf 100644 --- a/pkg/client/client_venafi_cloud.go +++ b/pkg/client/client_venafi_cloud.go @@ -43,13 +43,15 @@ type ( accessToken *venafiCloudAccessToken baseURL string agentMetadata *api.AgentMetadata - client *http.Client uploaderID string uploadPath string privateKey crypto.PrivateKey jwtSigningAlg jwt.SigningMethod lock sync.RWMutex + + // Made public for testing purposes. + Client *http.Client } VenafiSvcAccountCredentials struct { @@ -109,7 +111,7 @@ func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiS credentials: credentials, baseURL: baseURL, accessToken: &venafiCloudAccessToken{}, - client: &http.Client{Timeout: time.Minute}, + Client: &http.Client{Timeout: time.Minute}, uploaderID: uploaderID, uploadPath: uploadPath, privateKey: privateKey, @@ -270,7 +272,7 @@ func (c *VenafiCloudClient) Post(path string, body io.Reader) (*http.Response, e req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token.accessToken)) } - return c.client.Do(req) + return c.Client.Do(req) } // getValidAccessToken returns a valid access token. It will fetch a new access @@ -325,7 +327,7 @@ func (c *VenafiCloudClient) updateAccessToken() error { } func (c *VenafiCloudClient) sendHTTPRequest(request *http.Request, responseObject interface{}) error { - response, err := c.client.Do(request) + response, err := c.Client.Do(request) if err != nil { return err } diff --git a/pkg/testutil/envtest.go b/pkg/testutil/envtest.go index 089b909d..cd93b4a6 100644 --- a/pkg/testutil/envtest.go +++ b/pkg/testutil/envtest.go @@ -12,7 +12,6 @@ import ( "sync" "testing" - "github.com/jetstack/preflight/pkg/client" "github.com/jetstack/venafi-connection-lib/api/v1alpha1" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -25,6 +24,8 @@ import ( clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + + "github.com/jetstack/preflight/pkg/client" ) // To see the API server logs, set: @@ -123,21 +124,31 @@ func VenConnStartWatching(t *testing.T, cl client.Client) { t.Cleanup(cancel) } -func VenConnTrustCA(t *testing.T, cl client.Client, cert *x509.Certificate) { +// Works with VenafiCloudClient and VenConnClient. Allows you to trust a given +// CA. +func TrustCA(t *testing.T, cl client.Client, cert *x509.Certificate) { t.Helper() - require.IsType(t, &client.VenConnClient{}, cl) - vcCl := cl.(*client.VenConnClient) + + var httpClient *http.Client + switch c := cl.(type) { + case *client.VenafiCloudClient: + httpClient = c.Client + case *client.VenConnClient: + httpClient = c.Client + default: + t.Fatalf("unsupported client type: %T", cl) + } pool := x509.NewCertPool() pool.AddCert(cert) - if vcCl.Client.Transport == nil { - vcCl.Client.Transport = http.DefaultTransport + if httpClient.Transport == nil { + httpClient.Transport = http.DefaultTransport } - if vcCl.Client.Transport.(*http.Transport).TLSClientConfig == nil { - vcCl.Client.Transport.(*http.Transport).TLSClientConfig = &tls.Config{} + if httpClient.Transport.(*http.Transport).TLSClientConfig == nil { + httpClient.Transport.(*http.Transport).TLSClientConfig = &tls.Config{} } - vcCl.Client.Transport.(*http.Transport).TLSClientConfig.RootCAs = pool + httpClient.Transport.(*http.Transport).TLSClientConfig.RootCAs = pool } // Parses the YAML manifest. Useful for inlining YAML manifests in Go test @@ -180,10 +191,19 @@ func FakeVenafiCloud(t *testing.T) (_ *httptest.Server, _ *x509.Certificate, set defer assertFnMu.Unlock() assertFn(t, r) + if r.URL.Path == "/v1/oauth2/v2.0/756db001-280e-11ee-84fb-991f3177e2d0/token" { + _, _ = w.Write([]byte(`{"access_token":"VALID_ACCESS_TOKEN","expires_in":900,"token_type":"bearer"}`)) + return + } else if r.URL.Path == "/v1/oauth/token/serviceaccount" { + _, _ = w.Write([]byte(`{"access_token":"VALID_ACCESS_TOKEN","expires_in":900,"token_type":"bearer"}`)) + return + } + accessToken := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") apiKey := r.Header.Get("tppl-api-key") if accessToken != "VALID_ACCESS_TOKEN" && apiKey != "VALID_API_KEY" { w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte(`{"error":"expected header 'Authorization: Bearer VALID_ACCESS_TOKEN' or 'tppl-api-key: VALID_API_KEY', but got Authorization=` + r.Header.Get("Authorization") + ` and tppl-api-key=` + r.Header.Get("tppl-api-key"))) return } if r.URL.Path == "/v1/tlspk/upload/clusterdata/no" { @@ -194,11 +214,7 @@ func FakeVenafiCloud(t *testing.T) (_ *httptest.Server, _ *x509.Certificate, set } _, _ = w.Write([]byte(`{"status":"ok","organization":"756db001-280e-11ee-84fb-991f3177e2d0"}`)) } else if r.URL.Path == "/v1/useraccounts" { - w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`{"user": {"username": "user","id": "76a126f0-280e-11ee-84fb-991f3177e2d0"}}`)) - - } else if r.URL.Path == "/v1/oauth2/v2.0/756db001-280e-11ee-84fb-991f3177e2d0/token" { - _, _ = w.Write([]byte(`{"access_token":"VALID_ACCESS_TOKEN","expires_in":900,"token_type":"bearer"}`)) } else { w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte(`{"error":"unexpected path in the test server","path":"` + r.URL.Path + `"}`)) From d258aca61c3efc570cbdf5c9bb5485a36c0f89d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 23 Sep 2024 16:46:34 +0200 Subject: [PATCH 11/16] config: keep passing down uploader_id, mark uploader_id as deprecated This commit re-introduced the uploader_id. The venafi-cloud.uploader_id field still doesn't do anything. I propose to remove all mentions to uploader_id in the next PR. And since uploader_id is no longer doing anything anymore, I've marked venafi-cloud.uploader_id as "Deprecated". Note that marking this as deprecated doesn't appear to end-users. Instead, end-users will see the warning about uploader_id being ignored if they set it. --- pkg/agent/config.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index bd3b2df9..6f8186cc 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -73,11 +73,14 @@ type DataGatherer struct { } type VenafiCloudConfig struct { + // Deprecated: UploaderID is ignored by the backend and is not needed. // UploaderID is the upload ID that will be used when creating a cluster // connection. This field is ignored by the backend and is often arbitrarily // set to "no". UploaderID string `yaml:"uploader_id,omitempty"` - // UploadPath is the endpoint path for the upload API. + + // UploadPath is the endpoint path for the upload API. Only used in Venafi + // Cloud Key Pair Service Account mode. UploadPath string `yaml:"upload_path,omitempty"` } @@ -702,9 +705,11 @@ func ValidateDataGatherers(dataGatherers []DataGatherer) error { func createCredentialClient(log *log.Logger, credentials client.Credentials, cfg CombinedConfig, agentMetadata *api.AgentMetadata) (client.Client, error) { switch creds := credentials.(type) { case *client.VenafiSvcAccountCredentials: - // check if config has Venafi Cloud data, use config data if it's present - uploaderID := "" // The uploader ID isn't actually used in the backend. - uploadPath := "" + // The uploader ID isn't actually used in the backend, let's use an + // arbitrary value. + uploaderID := "no" + + var uploadPath string if cfg.AuthMode == VenafiCloudKeypair { // We don't do this for the VenafiCloudVenafiConnection mode because // the upload_path field is ignored in that mode. From 161eee9f994ac318f5998146f634f34b52735e48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 23 Sep 2024 17:47:45 +0200 Subject: [PATCH 12/16] test.sh: use --fail-with-body rather than -f to see the err body --- hack/e2e/test.sh | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/hack/e2e/test.sh b/hack/e2e/test.sh index 783073be..ff1e9c00 100755 --- a/hack/e2e/test.sh +++ b/hack/e2e/test.sh @@ -133,11 +133,11 @@ subject="system:serviceaccount:venafi:venafi-components" audience="https://${VEN_API_HOST}" issuerURL="$(kubectl create token -n venafi venafi-components | step crypto jwt inspect --insecure | jq -r '.payload.iss')" openidDiscoveryURL="${issuerURL}/.well-known/openid-configuration" -jwksURI=$(curl -fsSL ${openidDiscoveryURL} | jq -r '.jwks_uri') +jwksURI=$(curl --fail-with-body -sSL ${openidDiscoveryURL} | jq -r '.jwks_uri') # Create the Venafi agent service account if one does not already exist while true; do - tenantID=$(curl -fsSL -H "tppl-api-key: $VEN_API_KEY" https://${VEN_API_HOST}/v1/serviceaccounts \ + tenantID=$(curl --fail-with-body -sSL -H "tppl-api-key: $VEN_API_KEY" https://${VEN_API_HOST}/v1/serviceaccounts \ | jq -r '.[] | select(.issuerURL==$issuerURL and .subject == $subject) | .companyId' \ --arg issuerURL "${issuerURL}" \ --arg subject "${subject}") @@ -163,11 +163,12 @@ while true; do --arg audience "${audience}" \ --arg issuerURL "${issuerURL}" \ --arg jwksURI "${jwksURI}" \ - --argjson teams "$(curl https://${VEN_API_HOST}/v1/teams -fsSL -H tppl-api-key:\ ${VEN_API_KEY})" \ - --argjson applications "$(curl https://${VEN_API_HOST}/outagedetection/v1/applications -fsSL -H tppl-api-key:\ ${VEN_API_KEY})" \ + --argjson teams "$(curl https://${VEN_API_HOST}/v1/teams --fail-with-body -sSL -H tppl-api-key:\ ${VEN_API_KEY})" \ + --argjson applications "$(curl https://${VEN_API_HOST}/outagedetection/v1/applications --fail-with-body -sSL -H tppl-api-key:\ ${VEN_API_KEY})" \ | curl https://${VEN_API_HOST}/v1/serviceaccounts \ -H "tppl-api-key: $VEN_API_KEY" \ - -fsSL --json @- + --fail-with-body \ + -sSL --json @- done kubectl apply -n venafi -f - < Date: Tue, 24 Sep 2024 10:11:03 +0200 Subject: [PATCH 13/16] e2e/test.sh: removed the need for VEN_OWNING_TEAM I spent 3 hours realizing that the reason the script was silently failing was because I was using an ID rather than a name as VEN_OWNING_TEAM. Since the team doesn't really matter in this test, I propose to just pick the first team that is returned by /v1/teams. That should avoid having to find out the name or ID of a team to run this script. --- hack/e2e/test.sh | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/hack/e2e/test.sh b/hack/e2e/test.sh index ff1e9c00..ebd72e60 100755 --- a/hack/e2e/test.sh +++ b/hack/e2e/test.sh @@ -38,10 +38,6 @@ set -o xtrace # doesn't allow you to create registry service accounts. : ${VEN_API_KEY_PULL?} -# The Venafi Cloud team which will be the owner of the generated Venafi service -# accounts. -: ${VEN_OWNING_TEAM?} - # The Venafi Cloud zone (application/issuing_template) which will be used by the # issuer an policy. : ${VEN_ZONE?} @@ -87,12 +83,16 @@ if ! gcloud container clusters get-credentials "${CLUSTER_NAME}"; then fi kubectl create ns venafi || true +# Let's pick the first team as the owning team as it doesn't matter for this +# test. +owningTeamID=$(curl --fail-with-body -sS https://api.venafi.cloud/v1/teams -H "tppl-api-key: $VEN_API_KEY" | jq '.teams[0].id' -r) + # Pull secret for Venafi OCI registry if ! kubectl get secret venafi-image-pull-secret -n venafi; then venctl iam service-accounts registry create \ --api-key "${VEN_API_KEY_PULL}" \ --no-prompts \ - --owning-team "${VEN_OWNING_TEAM}" \ + --owning-team "${owningTeamID}" \ --name "venafi-kubernetes-agent-e2e-registry-${RANDOM}" \ --scopes enterprise-cert-manager,enterprise-venafi-issuer,enterprise-approver-policy \ | jq '{ @@ -129,12 +129,14 @@ venctl components kubernetes apply \ kubectl apply -n venafi -f venafi-components.yaml + subject="system:serviceaccount:venafi:venafi-components" audience="https://${VEN_API_HOST}" issuerURL="$(kubectl create token -n venafi venafi-components | step crypto jwt inspect --insecure | jq -r '.payload.iss')" openidDiscoveryURL="${issuerURL}/.well-known/openid-configuration" jwksURI=$(curl --fail-with-body -sSL ${openidDiscoveryURL} | jq -r '.jwks_uri') + # Create the Venafi agent service account if one does not already exist while true; do tenantID=$(curl --fail-with-body -sSL -H "tppl-api-key: $VEN_API_KEY" https://${VEN_API_HOST}/v1/serviceaccounts \ @@ -155,15 +157,14 @@ while true; do "issuerURL": $issuerURL, "jwksURI": $jwksURI, "applications": [$applications.applications[].id], - "owner": $teams.teams[] | select(.name==$teamName) | .id + "owner": $owningTeamID }' \ --arg random "${RANDOM}" \ - --arg teamName "${VEN_OWNING_TEAM}" \ --arg subject "${subject}" \ --arg audience "${audience}" \ --arg issuerURL "${issuerURL}" \ --arg jwksURI "${jwksURI}" \ - --argjson teams "$(curl https://${VEN_API_HOST}/v1/teams --fail-with-body -sSL -H tppl-api-key:\ ${VEN_API_KEY})" \ + --arg owningTeamID "{$owningTeamID}" \ --argjson applications "$(curl https://${VEN_API_HOST}/outagedetection/v1/applications --fail-with-body -sSL -H tppl-api-key:\ ${VEN_API_KEY})" \ | curl https://${VEN_API_HOST}/v1/serviceaccounts \ -H "tppl-api-key: $VEN_API_KEY" \ From bd73fcf3a2a4d9c4a8b43cb78e1afa6e39314af6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 24 Sep 2024 10:25:34 +0200 Subject: [PATCH 14/16] e2e/test.sh: gather vars at the top and group gcloud/ko commands Grouping gcloud/ko commands makes it much easier to comment out commands that I want to skip when the script fails mid-way. --- hack/e2e/test.sh | 123 +++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 64 deletions(-) diff --git a/hack/e2e/test.sh b/hack/e2e/test.sh index ebd72e60..ab7abaaf 100755 --- a/hack/e2e/test.sh +++ b/hack/e2e/test.sh @@ -31,6 +31,9 @@ set -o nounset set -o errexit set -o pipefail set -o xtrace +script_dir=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd) +root_dir=$(cd "${script_dir}/../.." && pwd) +export TERM=dumb # Your Venafi Cloud API key. : ${VEN_API_KEY?} @@ -51,22 +54,6 @@ set -o xtrace # E.g. ttl.sh/63773370-0bcf-4ac0-bd42-5515616089ff : ${OCI_BASE?} -export VERSION=$(git describe --tags --always --match='v*' --abbrev=14 --dirty) -export KO_DOCKER_REPO=$OCI_BASE/images/venafi-agent -export TERM=dumb - -script_dir=$(cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd) -root_dir=$(cd "${script_dir}/../.." && pwd) - -cd "${script_dir}" - -pushd "${root_dir}" -ko build --bare --tags "${VERSION}" -helm package deploy/charts/venafi-kubernetes-agent --version "${VERSION}" --app-version "${VERSION}" -helm push venafi-kubernetes-agent-${VERSION}.tgz "oci://${OCI_BASE}/charts" -popd - -export USE_GKE_GCLOUD_AUTH_PLUGIN=True # Required gcloud environment variables # https://cloud.google.com/sdk/docs/configurations#setting_configuration_properties : ${CLOUDSDK_CORE_PROJECT?} @@ -75,26 +62,36 @@ export USE_GKE_GCLOUD_AUTH_PLUGIN=True # The name of the cluster to create : ${CLUSTER_NAME?} +# IMPORTANT: we pick the first team as the owning team for the registry and +# workload identity service account as it doesn't matter. + +version=$(git describe --tags --always --match='v*' --abbrev=14 --dirty) + +cd "${script_dir}" + +pushd "${root_dir}" +KO_DOCKER_REPO=$OCI_BASE/images/venafi-agent ko build --bare --tags "${version}" +helm package deploy/charts/venafi-kubernetes-agent --version "${version}" --app-version "${version}" +helm push "venafi-kubernetes-agent-${version}.tgz" "oci://${OCI_BASE}/charts" +popd + +export USE_GKE_GCLOUD_AUTH_PLUGIN=True if ! gcloud container clusters get-credentials "${CLUSTER_NAME}"; then - gcloud container clusters create "${CLUSTER_NAME}" \ - --preemptible \ - --machine-type e2-small \ - --num-nodes 3 + gcloud container clusters create "${CLUSTER_NAME}" \ + --preemptible \ + --machine-type e2-small \ + --num-nodes 3 fi kubectl create ns venafi || true -# Let's pick the first team as the owning team as it doesn't matter for this -# test. -owningTeamID=$(curl --fail-with-body -sS https://api.venafi.cloud/v1/teams -H "tppl-api-key: $VEN_API_KEY" | jq '.teams[0].id' -r) - # Pull secret for Venafi OCI registry if ! kubectl get secret venafi-image-pull-secret -n venafi; then - venctl iam service-accounts registry create \ - --api-key "${VEN_API_KEY_PULL}" \ - --no-prompts \ - --owning-team "${owningTeamID}" \ - --name "venafi-kubernetes-agent-e2e-registry-${RANDOM}" \ - --scopes enterprise-cert-manager,enterprise-venafi-issuer,enterprise-approver-policy \ + venctl iam service-accounts registry create \ + --api-key "${VEN_API_KEY_PULL}" \ + --no-prompts \ + --owning-team "$(curl --fail-with-body -sS "https://${VEN_API_HOST}/v1/teams" -H "tppl-api-key: $VEN_API_KEY_PULL" | jq '.teams[0].id' -r)" \ + --name "venafi-kubernetes-agent-e2e-registry-${RANDOM}" \ + --scopes enterprise-cert-manager,enterprise-venafi-issuer,enterprise-approver-policy \ | jq '{ "apiVersion": "v1", "kind": "Secret", @@ -118,37 +115,35 @@ fi export VENAFI_KUBERNETES_AGENT_CLIENT_ID="not-used-but-required-by-venctl" venctl components kubernetes apply \ - --cert-manager \ - --venafi-enhanced-issuer \ - --approver-policy-enterprise \ - --venafi-kubernetes-agent \ - --venafi-kubernetes-agent-version "${VERSION}" \ - --venafi-kubernetes-agent-values-files "${script_dir}/values.venafi-kubernetes-agent.yaml" \ - --venafi-kubernetes-agent-custom-image-registry "${OCI_BASE}/images" \ - --venafi-kubernetes-agent-custom-chart-repository "oci://${OCI_BASE}/charts" + --cert-manager \ + --venafi-enhanced-issuer \ + --approver-policy-enterprise \ + --venafi-kubernetes-agent \ + --venafi-kubernetes-agent-version "${version}" \ + --venafi-kubernetes-agent-values-files "${script_dir}/values.venafi-kubernetes-agent.yaml" \ + --venafi-kubernetes-agent-custom-image-registry "${OCI_BASE}/images" \ + --venafi-kubernetes-agent-custom-chart-repository "oci://${OCI_BASE}/charts" kubectl apply -n venafi -f venafi-components.yaml - subject="system:serviceaccount:venafi:venafi-components" audience="https://${VEN_API_HOST}" issuerURL="$(kubectl create token -n venafi venafi-components | step crypto jwt inspect --insecure | jq -r '.payload.iss')" openidDiscoveryURL="${issuerURL}/.well-known/openid-configuration" jwksURI=$(curl --fail-with-body -sSL ${openidDiscoveryURL} | jq -r '.jwks_uri') - # Create the Venafi agent service account if one does not already exist while true; do - tenantID=$(curl --fail-with-body -sSL -H "tppl-api-key: $VEN_API_KEY" https://${VEN_API_HOST}/v1/serviceaccounts \ - | jq -r '.[] | select(.issuerURL==$issuerURL and .subject == $subject) | .companyId' \ - --arg issuerURL "${issuerURL}" \ - --arg subject "${subject}") + tenantID=$(curl --fail-with-body -sSL -H "tppl-api-key: $VEN_API_KEY" https://${VEN_API_HOST}/v1/serviceaccounts \ + | jq -r '.[] | select(.issuerURL==$issuerURL and .subject == $subject) | .companyId' \ + --arg issuerURL "${issuerURL}" \ + --arg subject "${subject}") - if [[ "${tenantID}" != "" ]]; then - break - fi + if [[ "${tenantID}" != "" ]]; then + break + fi - jq -n '{ + jq -n '{ "name": "venafi-kubernetes-agent-e2e-agent-\($random)", "authenticationType": "rsaKeyFederated", "scopes": ["kubernetes-discovery-federated", "certificate-issuance"], @@ -159,17 +154,17 @@ while true; do "applications": [$applications.applications[].id], "owner": $owningTeamID }' \ - --arg random "${RANDOM}" \ - --arg subject "${subject}" \ - --arg audience "${audience}" \ - --arg issuerURL "${issuerURL}" \ - --arg jwksURI "${jwksURI}" \ - --arg owningTeamID "{$owningTeamID}" \ - --argjson applications "$(curl https://${VEN_API_HOST}/outagedetection/v1/applications --fail-with-body -sSL -H tppl-api-key:\ ${VEN_API_KEY})" \ - | curl https://${VEN_API_HOST}/v1/serviceaccounts \ - -H "tppl-api-key: $VEN_API_KEY" \ - --fail-with-body \ - -sSL --json @- + --arg random "${RANDOM}" \ + --arg subject "${subject}" \ + --arg audience "${audience}" \ + --arg issuerURL "${issuerURL}" \ + --arg jwksURI "${jwksURI}" \ + --arg owningTeamID "$(curl --fail-with-body -sS "https://${VEN_API_HOST}/v1/teams" -H "tppl-api-key: $VEN_API_KEY" | jq '.teams[0].id' -r)" \ + --argjson applications "$(curl https://${VEN_API_HOST}/outagedetection/v1/applications --fail-with-body -sSL -H tppl-api-key:\ ${VEN_API_KEY})" \ + | curl https://${VEN_API_HOST}/v1/serviceaccounts \ + -H "tppl-api-key: $VEN_API_KEY" \ + --fail-with-body \ + -sSL --json @- done kubectl apply -n venafi -f - <(grep -v -e "reflector\.go" -e "datagatherer" -e "data gatherer" > /dev/stderr) \ - | grep -q "Data sent successfully" + --follow \ + --namespace venafi \ + | tee >(grep -v -e "reflector\.go" -e "datagatherer" -e "data gatherer" >/dev/stderr) \ + | grep -q "Data sent successfully" From 47b99ae9c88c6a5b271bac11159a3f4841f671d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 24 Sep 2024 10:32:04 +0200 Subject: [PATCH 15/16] e2e/test.sh: VenafiConnection CRDs weren't being installed --- hack/e2e/values.venafi-kubernetes-agent.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hack/e2e/values.venafi-kubernetes-agent.yaml b/hack/e2e/values.venafi-kubernetes-agent.yaml index ff55025d..a2545eca 100644 --- a/hack/e2e/values.venafi-kubernetes-agent.yaml +++ b/hack/e2e/values.venafi-kubernetes-agent.yaml @@ -6,3 +6,7 @@ config: authentication: venafiConnection: enabled: true + +crds: + venafiConnection: + include: true From 8c87b6c2f83b2aadac09d6e9782d29a2156bf3fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 24 Sep 2024 15:33:11 +0200 Subject: [PATCH 16/16] Revert "e2e/test.sh: VenafiConnection CRDs weren't being installed" The error that I was trying to fix was: failed to list *v1alpha1.VenafiConnection: venaficonnections.jetstack.io is forbidden: User "system:serviceaccount:venafi:venafi-connection" cannot list resource "venaficonnections" in API group "jetstack.io" at the cluster scope I then added crds.venafiConnection.include=true, thinking that the agent's helm chart wasn't installing the RBAC. But I found that ./hack/e2e/test.sh was installing venafi enhanced issuer along with the Agent, which means the venafi-connection helm chart was being installed and so crds.venafiConnection.include=true isn't needed. --- hack/e2e/values.venafi-kubernetes-agent.yaml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hack/e2e/values.venafi-kubernetes-agent.yaml b/hack/e2e/values.venafi-kubernetes-agent.yaml index a2545eca..ff55025d 100644 --- a/hack/e2e/values.venafi-kubernetes-agent.yaml +++ b/hack/e2e/values.venafi-kubernetes-agent.yaml @@ -6,7 +6,3 @@ config: authentication: venafiConnection: enabled: true - -crds: - venafiConnection: - include: true