Feedback from #552

.github/workflows/release-master.yml

@@ -70,14 +70,14 @@ jobs:
       packages: write
       id-token: write
     steps:
-    - uses: webfactory/ssh-agent@v0.9.0
-      with:
-        ssh-private-key: ${{ secrets.DEPLOY_KEY_READ_VENAFI_CONNECTION_LIB }}
     - name: Install Tools
       # Installing 'bash' because it's required by the 'cosign-installer' action
       # and 'coreutils' because the 'slsa-provenance-action' requires a version
       # of 'base64' that supports the -w flag.
       run: apk add --update make git jq rsync curl bash coreutils go
+    - uses: webfactory/ssh-agent@v0.9.0
+      with:
+        ssh-private-key: ${{ secrets.DEPLOY_KEY_READ_VENAFI_CONNECTION_LIB }}
     - name: Adding github workspace as safe directory
       # See issue https://github.com/actions/checkout/issues/760
       run: git config --global --add safe.directory $GITHUB_WORKSPACE

pkg/agent/run.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"log"
 	"net/http"
 	_ "net/http/pprof"
 	"net/url"
@@ -18,18 +17,17 @@ import (
 
 	"github.com/cenkalti/backoff"
 	"github.com/hashicorp/go-multierror"
-	"github.com/jetstack/preflight/pkg/logs"
 	json "github.com/json-iterator/go"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"github.com/spf13/cobra"
-	"k8s.io/client-go/rest"
-	"k8s.io/client-go/tools/clientcmd"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
 	"github.com/jetstack/preflight/api"
 	"github.com/jetstack/preflight/pkg/client"
 	"github.com/jetstack/preflight/pkg/datagatherer"
+	"github.com/jetstack/preflight/pkg/kubeconfig"
+	"github.com/jetstack/preflight/pkg/logs"
 	"github.com/jetstack/preflight/pkg/version"
 )
 
@@ -302,7 +300,7 @@ func getConfiguration() (Config, client.Client) {
 	if venConnMode && InstallNS == "" {
 		InstallNS, err = getInClusterNamespace()
 		if err != nil {
-			log.Fatalf("could not guess which namespace the agent is running in: %s", err)
+			logs.Log.Fatalf("could not guess which namespace the agent is running in: %s", err)
 		}
 	}
 	if venConnMode && VenConnNS == "" {
@@ -323,15 +321,15 @@ func getConfiguration() (Config, client.Client) {
 		// 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.")
+		logs.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 config.VenafiCloud != nil && config.VenafiCloud.UploadPath != "" {
-			log.Printf(`ignoring venafi-cloud.upload_path. In Venafi Connection mode, this field is not needed.`)
+			logs.Log.Printf(`ignoring venafi-cloud.upload_path. In Venafi Connection mode, this field is not needed.`)
 		}
 
 		// Regarding venafi-cloud.uploader_id, we found that it doesn't do
@@ -340,12 +338,12 @@ func getConfiguration() (Config, client.Client) {
 		// set in the config file, and set it to an arbitrary value in the
 		// client since it doesn't matter.
 		if config.VenafiCloud.UploaderID != "" {
-			log.Printf(`ignoring venafi-cloud.uploader_id. In Venafi Connection mode, this field is not needed.`)
+			logs.Log.Printf(`ignoring venafi-cloud.uploader_id. In Venafi Connection mode, this field is not needed.`)
 		}
 
-		cfg, err := loadRESTConfig("")
+		cfg, err := kubeconfig.LoadRESTConfig("")
 		if err != nil {
-			log.Fatalf("failed to load kubeconfig: %v", err)
+			logs.Log.Fatalf("failed to load kubeconfig: %v", err)
 		}
 
 		preflightClient, err = client.NewVenConnClient(cfg, agentMetadata, InstallNS, VenConnName, VenConnNS, nil)
@@ -569,28 +567,3 @@ func getInClusterNamespace() (string, error) {
 	}
 	return string(namespace), nil
 }
-
-func loadRESTConfig(path string) (*rest.Config, error) {
-	switch path {
-	// If the kubeconfig path is not provided, use the default loading rules
-	// so we read the regular KUBECONFIG variable or create a non-interactive
-	// client for agents running in cluster
-	case "":
-		loadingrules := clientcmd.NewDefaultClientConfigLoadingRules()
-		cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-			loadingrules, &clientcmd.ConfigOverrides{}).ClientConfig()
-		if err != nil {
-			return nil, fmt.Errorf("failed to load kubeconfig: %w", err)
-		}
-		return cfg, nil
-	// Otherwise use the explicitly named kubeconfig file.
-	default:
-		cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-			&clientcmd.ClientConfigLoadingRules{ExplicitPath: path},
-			&clientcmd.ConfigOverrides{}).ClientConfig()
-		if err != nil {
-			return nil, fmt.Errorf("failed to load kubeconfig from %s: %w", path, err)
-		}
-		return cfg, nil
-	}
-}

pkg/client/client_venconn_test.go

@@ -17,7 +17,6 @@ import (
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/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/types"
@@ -102,6 +101,67 @@ 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
@@ -144,48 +204,55 @@ func run(test testcase) func(t *testing.T) {
 		// 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
-		require.NoError(t, kclient.Create(context.Background(), &corev1.Namespace{
-			ObjectMeta: metav1.ObjectMeta{Name: "venafi"},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &corev1.ServiceAccount{
-			ObjectMeta: metav1.ObjectMeta{Name: "venafi-connection", Namespace: "venafi"},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &rbacv1.ClusterRole{
-			ObjectMeta: metav1.ObjectMeta{Name: "venafi-connection-role"},
-			Rules: []rbacv1.PolicyRule{
-				{APIGroups: []string{""}, Resources: []string{"namespaces"}, Verbs: []string{"get", "list", "watch"}},
-				{APIGroups: []string{"jetstack.io"}, Resources: []string{"venaficonnections"}, Verbs: []string{"get", "list", "watch"}},
-				{APIGroups: []string{"jetstack.io"}, Resources: []string{"venaficonnections/status"}, Verbs: []string{"get", "patch"}},
-			},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &rbacv1.ClusterRoleBinding{
-			ObjectMeta: metav1.ObjectMeta{Name: "venafi-connection-rolebinding"},
-			RoleRef:    rbacv1.RoleRef{APIGroup: "rbac.authorization.k8s.io", Kind: "ClusterRole", Name: "venafi-connection-role"},
-			Subjects:   []rbacv1.Subject{{Kind: "ServiceAccount", Name: "venafi-connection", Namespace: "venafi"}},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{Name: "accesstoken", Namespace: "venafi"},
-			StringData: map[string]string{"accesstoken": "VALID_ACCESS_TOKEN"},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{Name: "apikey", Namespace: "venafi"},
-			StringData: map[string]string{"apikey": "VALID_API_KEY"},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &rbacv1.Role{
-			ObjectMeta: metav1.ObjectMeta{Name: "venafi-connection-secret-reader", Namespace: "venafi"},
-			Rules: []rbacv1.PolicyRule{
-				{APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}, ResourceNames: []string{"accesstoken", "apikey"}},
-			},
-		}))
-		require.NoError(t, kclient.Create(context.Background(), &rbacv1.RoleBinding{
-			ObjectMeta: metav1.ObjectMeta{Name: "venafi-connection-secret-reader", Namespace: "venafi"},
-			RoleRef:    rbacv1.RoleRef{APIGroup: "rbac.authorization.k8s.io", Kind: "Role", Name: "venafi-connection-secret-reader"},
-			Subjects:   []rbacv1.Subject{{Kind: "ServiceAccount", Name: "venafi-connection", Namespace: "venafi"}},
-		}))
 
 		test.given = strings.ReplaceAll(test.given, "FAKE_VENAFI_CLOUD_URL", fakeVenafiCloud.URL)
 		test.given = strings.ReplaceAll(test.given, "FAKE_TPP_URL", fakeTPP.URL)
-		for _, obj := range parse(test.given) {
+
+		var given []ctrlruntime.Object
+		given = append(given, parse(rbac)...)
+		given = append(given, parse(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, parse(test.given)...)
+		for _, obj := range given {
 			require.NoError(t, kclient.Create(context.Background(), obj))
 		}
 

pkg/datagatherer/k8s/client.go

@@ -6,15 +6,15 @@ import (
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
-	"k8s.io/client-go/tools/clientcmd"
+
+	"github.com/jetstack/preflight/pkg/kubeconfig"
 )
 
 // NewDynamicClient creates a new 'dynamic' clientset using the provided kubeconfig.
 // If kubeconfigPath is not set/empty, it will attempt to load configuration using
 // the default loading rules.
 func NewDynamicClient(kubeconfigPath string) (dynamic.Interface, error) {
-	cfg, err := loadRESTConfig(kubeconfigPath)
+	cfg, err := kubeconfig.LoadRESTConfig(kubeconfigPath)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
@@ -31,7 +31,7 @@ func NewDynamicClient(kubeconfigPath string) (dynamic.Interface, error) {
 func NewDiscoveryClient(kubeconfigPath string) (discovery.DiscoveryClient, error) {
 	var discoveryClient *discovery.DiscoveryClient
 
-	cfg, err := loadRESTConfig(kubeconfigPath)
+	cfg, err := kubeconfig.LoadRESTConfig(kubeconfigPath)
 	if err != nil {
 		return discovery.DiscoveryClient{}, errors.WithStack(err)
 	}
@@ -49,7 +49,7 @@ func NewDiscoveryClient(kubeconfigPath string) (discovery.DiscoveryClient, error
 // the default loading rules.
 func NewClientSet(kubeconfigPath string) (kubernetes.Interface, error) {
 	var clientset *kubernetes.Clientset
-	cfg, err := loadRESTConfig(kubeconfigPath)
+	cfg, err := kubeconfig.LoadRESTConfig(kubeconfigPath)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
@@ -59,28 +59,3 @@ func NewClientSet(kubeconfigPath string) (kubernetes.Interface, error) {
 	}
 	return clientset, nil
 }
-
-func loadRESTConfig(path string) (*rest.Config, error) {
-	switch path {
-	// If the kubeconfig path is not provided, use the default loading rules
-	// so we read the regular KUBECONFIG variable or create a non-interactive
-	// client for agents running in cluster
-	case "":
-		loadingrules := clientcmd.NewDefaultClientConfigLoadingRules()
-		cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-			loadingrules, &clientcmd.ConfigOverrides{}).ClientConfig()
-		if err != nil {
-			return nil, errors.WithStack(err)
-		}
-		return cfg, nil
-	// Otherwise use the explicitly named kubeconfig file.
-	default:
-		cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-			&clientcmd.ClientConfigLoadingRules{ExplicitPath: path},
-			&clientcmd.ConfigOverrides{}).ClientConfig()
-		if err != nil {
-			return nil, errors.WithStack(err)
-		}
-		return cfg, nil
-	}
-}

pkg/kubeconfig/kubeconfig.go

@@ -0,0 +1,35 @@
+package kubeconfig
+
+import (
+	"github.com/pkg/errors"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/clientcmd"
+)
+
+// LoadRESTConfig loads the kube config from the provided path. If the path is
+// empty, the kube config will be loaded from KUBECONFIG, and if KUBECONFIG
+// isn't set, the in-cluster config will be used.
+func LoadRESTConfig(path string) (*rest.Config, error) {
+	switch path {
+	// If the kubeconfig path is not provided, use the default loading rules
+	// so we read the regular KUBECONFIG variable or create a non-interactive
+	// client for agents running in cluster
+	case "":
+		loadingrules := clientcmd.NewDefaultClientConfigLoadingRules()
+		cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+			loadingrules, &clientcmd.ConfigOverrides{}).ClientConfig()
+		if err != nil {
+			return nil, errors.WithStack(err)
+		}
+		return cfg, nil
+	// Otherwise use the explicitly named kubeconfig file.
+	default:
+		cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+			&clientcmd.ClientConfigLoadingRules{ExplicitPath: path},
+			&clientcmd.ConfigOverrides{}).ClientConfig()
+		if err != nil {
+			return nil, errors.WithStack(err)
+		}
+		return cfg, nil
+	}
+}