Merge pull request #566 from jetstack/disable-config-server-field

VC-35630: User can now omit the `server` field in config when using the Venafi Connection mode

Author: maelvls

deploy/charts/venafi-kubernetes-agent/README.md

@@ -26,13 +26,13 @@ If you do not have one, you can sign up for a free trial now at:
 | authentication.venafiConnection.namespace | string | `"venafi"` | The namespace of a VenafiConnection resource which contains the configuration for authenticating to Venafi. |
 | command | list | `[]` | Specify the command to run overriding default binary. |
 | config | object | `{"clientId":"","clusterDescription":"","clusterName":"","configmap":{"key":null,"name":null},"ignoredSecretTypes":["kubernetes.io/service-account-token","kubernetes.io/dockercfg","kubernetes.io/dockerconfigjson","kubernetes.io/basic-auth","kubernetes.io/ssh-auth","bootstrap.kubernetes.io/token","helm.sh/release.v1"],"period":"0h1m0s","server":"https://api.venafi.cloud/"}` | Configuration section for the Venafi Kubernetes Agent itself |
-| config.clientId | string | `""` | The client-id returned from the Venafi Control Plane |
+| config.clientId | string | `""` | The client-id to be used for authenticating with the Venafi Control Plane. Only useful when using a Key Pair Service Account in the Venafi Control Plane. You can obtain the cliend ID by creating a Key Pair Service Account in the Venafi Control Plane. |
 | config.clusterDescription | string | `""` | Description for the cluster resource if it needs to be created in Venafi Control Plane |
 | config.clusterName | string | `""` | Name for the cluster resource if it needs to be created in Venafi Control Plane |
 | config.configmap | object | `{"key":null,"name":null}` | Specify ConfigMap details to load config from an existing resource. This should be blank by default unless you have you own config. |
 | config.ignoredSecretTypes | list | `["kubernetes.io/service-account-token","kubernetes.io/dockercfg","kubernetes.io/dockerconfigjson","kubernetes.io/basic-auth","kubernetes.io/ssh-auth","bootstrap.kubernetes.io/token","helm.sh/release.v1"]` | Reduce the memory usage of the agent and reduce the load on the Kubernetes API server by omitting various common Secret types when listing Secrets. These Secret types will be added to a "type!=<type>" field selector in the agent config. * https://docs.venafi.cloud/vaas/k8s-components/t-cfg-tlspk-agent/#configuration * https://kubernetes.io/docs/concepts/configuration/secret/#secret-types * https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/#list-of-supported-fields |
 | config.period | string | `"0h1m0s"` | Send data back to the platform every minute unless changed |
-| config.server | string | `"https://api.venafi.cloud/"` | Overrides the server if using a proxy in your environment For the EU variant use: https://api.venafi.eu/ |
+| config.server | string | `"https://api.venafi.cloud/"` | API URL of the Venafi Control Plane API. For EU tenants, set this value to https://api.venafi.eu/. If you are using the VenafiConnection authentication method, you must set the API URL using the field `spec.vcp.url` on the VenafiConnection resource instead. |
 | crds.forceRemoveValidationAnnotations | bool | `false` | The 'x-kubernetes-validations' annotation is not supported in Kubernetes 1.22 and below. This annotation is used by CEL, which is a feature introduced in Kubernetes 1.25 that improves how validation is performed. This option allows to force the 'x-kubernetes-validations' annotation to be excluded, even on Kubernetes 1.25+ clusters. |
 | crds.venafiConnection | object | `{"include":false}` | Optionally include the VenafiConnection CRDs |
 | crds.venafiConnection.include | bool | `false` | When set to false, the rendered output does not contain the VenafiConnection CRDs and RBAC. This is useful for when the Venafi Connection resources are already installed separately. |

deploy/charts/venafi-kubernetes-agent/values.yaml

@@ -191,10 +191,15 @@ authentication:
 
 # -- Configuration section for the Venafi Kubernetes Agent itself
 config:
-  # -- Overrides the server if using a proxy in your environment
-  # For the EU variant use: https://api.venafi.eu/
+  # -- API URL of the Venafi Control Plane API. For EU tenants, set this value to
+  # https://api.venafi.eu/. If you are using the VenafiConnection authentication
+  # method, you must set the API URL using the field `spec.vcp.url` on the
+  # VenafiConnection resource instead.
   server: "https://api.venafi.cloud/"
-  # -- The client-id returned from the Venafi Control Plane
+  # -- The client-id to be used for authenticating with the Venafi Control
+  # Plane. Only useful when using a Key Pair Service Account in the Venafi
+  # Control Plane. You can obtain the cliend ID by creating a Key Pair Service
+  # Account in the Venafi Control Plane.
   clientId: ""
   # -- Send data back to the platform every minute unless changed
   period: "0h1m0s"

pkg/agent/config.go

@@ -19,6 +19,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"gopkg.in/yaml.v3"
+	"k8s.io/client-go/rest"
 )
 
 const (
@@ -266,13 +267,22 @@ func getConfiguration(log *log.Logger, cfg Config, flags AgentCmdFlags) (Config,
 		flags.VenafiCloudMode = true
 	}
 
-	baseURL := cfg.Server
-	if baseURL == "" {
-		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)
+	// 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.")
+		}
+	} else {
+		baseURL = cfg.Server
+		if baseURL == "" {
+			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)
+			}
 		}
 	}
 
@@ -354,7 +364,8 @@ func getConfiguration(log *log.Logger, cfg Config, flags AgentCmdFlags) (Config,
 			log.Printf(`ignoring venafi-cloud.uploader_id. In Venafi Connection mode, this field is not needed.`)
 		}
 
-		restCfg, err := kubeconfig.LoadRESTConfig("")
+		var restCfg *rest.Config
+		restCfg, err = kubeconfig.LoadRESTConfig("")
 		if err != nil {
 			return Config{}, nil, fmt.Errorf("failed to load kubeconfig: %w", err)
 		}

pkg/agent/config_test.go

@@ -2,17 +2,24 @@ 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"
 )
 
 func TestGetConfiguration(t *testing.T) {
@@ -171,7 +178,7 @@ func TestGetConfiguration(t *testing.T) {
 			assert.Equal(t, Config{}, got)
 		})
 
-		t.Run("warning about venafi-cloud.uploader_id and venafi-cloud.upload_path being skipped", func(t *testing.T) {
+		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",
@@ -184,7 +191,112 @@ func TestGetConfiguration(t *testing.T) {
 			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("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)
+		})
+	})
+}
+
+// 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(`
+			---
+			apiVersion: jetstack.io/v1alpha1
+			kind: VenafiConnection
+			metadata:
+			  name: venafi-components
+			  namespace: venafi
+			spec:
+			  vcp:
+			    url: "%s"
+			    accessToken:
+			      - secret:
+			          name: 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`, 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)
+		})
+
+		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)
 	})
 }
 
@@ -457,15 +569,43 @@ func withFile(t testing.TB, content string) string {
 	return f.Name()
 }
 
-func withLogs(t testing.TB) (*log.Logger, *bytes.Buffer) {
+func withLogs(_ testing.TB) (*log.Logger, *bytes.Buffer) {
 	b := bytes.Buffer{}
 	return log.New(&b, "", 0), &b
 }
 
-func discardLogs(t testing.TB) *log.Logger {
+func discardLogs(_ testing.TB) *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.
+func withConfig(s string) Config {
+	var cfg Config
+
+	err := yaml.Unmarshal([]byte(s), &cfg)
+	if err != nil {
+		panic(err)
+	}
+	return cfg
+}
+
+func withCmdLineFlags(flags ...string) AgentCmdFlags {
+	parsed := withoutCmdLineFlags()
+	agentCmd := &cobra.Command{}
+	InitAgentCmdFlags(agentCmd, &parsed)
+	err := agentCmd.ParseFlags(flags)
+	if err != nil {
+		panic(err)
+	}
+
+	return parsed
+}
+
+func withoutCmdLineFlags() AgentCmdFlags {
+	return AgentCmdFlags{}
+}
+
 const fakeKubeconfig = `
 apiVersion: v1
 clusters:

pkg/client/client_venafi_cloud.go

@@ -288,9 +288,6 @@ func (c *VenafiCloudClient) updateAccessToken() error {
 	values.Set("assertion", jwtToken)
 
 	tokenURL := fullURL(c.baseURL, accessTokenEndpoint)
-	if err != nil {
-		return err
-	}
 
 	encoded := values.Encode()
 	request, err := http.NewRequest(http.MethodPost, tokenURL, strings.NewReader(encoded))

pkg/client/client_venconn.go

@@ -29,10 +29,15 @@ import (
 type VenConnClient struct {
 	agentMetadata *api.AgentMetadata
 	connHandler   venafi_client.ConnectionHandler
-	installNS     string       // Namespace in which the agent is running in.
-	venConnName   string       // Name of the VenafiConnection resource to use.
-	venConnNS     string       // Namespace of the VenafiConnection resource to use.
-	client        *http.Client // Used to make HTTP requests to Venafi Cloud.
+	installNS     string // Namespace in which the agent is running in.
+	venConnName   string // Name of the VenafiConnection resource to use.
+	venConnNS     string // Namespace of the VenafiConnection resource to use.
+
+	// Used to make HTTP requests to Venafi Cloud. This field is public for
+	// testing purposes so that we can configure trusted CAs; there should be a
+	// way to do that without messing with the client directly (e.g., a flag to
+	// pass a custom CA?), but it's not there yet.
+	Client *http.Client
 }
 
 // NewVenConnClient lets you make requests to the Venafi Cloud backend using the
@@ -111,7 +116,7 @@ func NewVenConnClient(restcfg *rest.Config, agentMetadata *api.AgentMetadata, in
 		installNS:     installNS,
 		venConnName:   venConnName,
 		venConnNS:     venConnNS,
-		client:        vcpClient,
+		Client:        vcpClient,
 	}, nil
 }
 
@@ -180,7 +185,7 @@ func (c *VenConnClient) PostDataReadingsWithOptions(readings []*api.DataReading,
 	}
 	req.URL.RawQuery = q.Encode()
 
-	res, err := c.client.Do(req)
+	res, err := c.Client.Do(req)
 	if err != nil {
 		return err
 	}