Allow the connector to run outside of a kube pod #2613

dnephin · 2022-07-15T22:50:34Z

Summary

Branched from #3220. Best viewed by individual commit, and the third commit benefits from the "Hide Whitespace" view.

This PR adds support for reading the kube API hostname and CA cert from kubectl config when reading the in-cluster files fails. This is the first step to allow us to run the connector outside of kube (primarily for testing).

Also extracts a new type connector struct for the many args being passed to syncWithServer, and extracts a new syncDestination from that function as well. This is the first step to split up the syncWithServer goroutine into its two responsibilities: updating the destination, and updating role bindings.

In the next follow up PR I'll be addressing the remaining in-cluster calls, to allow the connector to run outside of a kube pod.

Related to #2500 (but doesn't get us all the way there yet)

internal/connector/connector.go

mxyng

I'm not sure how I feel about this. Running the connector outside of Kubernetes is a great change but keeping support for Kubernetes while in this mode seems unnecessary. Perhaps what is needed is to decouple the connector from the Kubernetes connector so it can be run separately with possibly other integrations

dnephin · 2022-07-20T17:43:55Z

but keeping support for Kubernetes while in this mode seems unnecessary

The goal here isn't to support different kinds of connectors, but to allow the kubernetes connector to run in different environments. For "getting started" and while developing/contributing, being able to run the connector locally outside of a kube pod is a big improvement. It removes all the complications of helm, and makes it much easier to iterate and try things out.

Even in production environments, having the authentication proxy run inside the environment for which it is authenticating requests could cause problems, especially when trying to recover that environment.

Perhaps what is needed is to decouple the connector from the Kubernetes connector so it can be run separately with possibly other integrations

I think that would also be a good change, but there's not much value in doing that until we are ready to add a different kind of destination.

dnephin

Marking this as ready for review. I've cleaned up the changes, and tested it manually.

I cut out some of the changes required to run locally and I'll re-introduce those in a follow up PR.

dnephin · 2022-09-29T15:49:23Z

internal/kubernetes/kubernetes.go

 	}

-	k.SecretReader = secrets.NewKubernetesSecretProvider(clientset, namespace)


This field was not used anywhere, so I was able to remove it.

dnephin · 2022-09-29T15:51:35Z

internal/connector/connector.go

-	clusterCACert, err := kubernetes.CA()
-	if err != nil {
-		return fmt.Errorf("reading CA file: %w", err)
-	}
-
 	certPool := x509.NewCertPool()
-
-	if ok := certPool.AppendCertsFromPEM(clusterCACert); !ok {
+	if ok := certPool.AppendCertsFromPEM(k8s.Config.CAData); !ok {


The CA data is populated by the rest.InClusterConfig as long as we call rest.LoadTLSFiles (which I added to NewKubernetes). This removes the need to look it up again, and helps keep most the in-cluster logic in the NewKubernetes constructor.

dnephin · 2022-09-29T15:52:36Z

internal/connector/connector.go

+	checkSum := k8s.Checksum()
+	logging.L.Debug().Str("uniqueID", checkSum).Msg("Cluster uniqueID")


I've tested this manually for now, since we don't quite yet have an easy way of testing this outside of kube.

I tested this by running infra connector locally using my kubectl config, and again by running it in a pod. I compared the checksum in both cases to ensure it produced the same value.

dnephin · 2022-09-29T15:55:05Z

internal/kubernetes/kubernetes.go

@@ -501,6 +487,8 @@ func (k *Kubernetes) Name(chksm string) (string, error) {
 	return name, nil
 }

+const podLabelsFilePath = "/etc/podinfo/labels"


Moved this constant to next to where it is used. This helps made code more readable by reducing the need to scroll around the file when reading this function, or looking for functions that use the constant.

I know some styles encourage putting all the constants at the top of the file, but I think often that's not the best choice. It's fine if the constants are for something like an enum that will be used all over the place. For constants like this that are only used by a single function, and are unlikely to ever be used in another place, I think it works much better to keep it close to the function where it is used.

dnephin · 2022-09-29T15:55:57Z

internal/connector/connector.go

@@ -52,6 +51,15 @@ type ListenerOptions struct {
 	Metrics string
 }

+// connector stores all the dependencies for the connector operations.
+type connector struct {


I'm never sure what to call these, so I went with the same pattern we used for Server and called it connector. As the code changes maybe we'll find a better name for this, or find ways to split it into other types.

I'm doing this now to allow us to split up syncWithServer and removing the need to pass around 5+ args to the functions.

dnephin · 2022-09-29T15:57:41Z

internal/connector/connector.go

+func syncDestination(con connector) error {
+	host, port, err := con.k8s.Endpoint()


The next step for allowing this to run locally will be to add a config setting that allows us to skip the endpoint lookup. So before making that change I'm extracting this logic from sycnWithServer to give us some space to add that new logic.

dnephin · 2022-10-03T16:19:34Z

internal/kubernetes/kubernetes.go

+		loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
+		cfg := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, nil)
+		if config, err = cfg.ClientConfig(); err != nil {
+			return nil, fmt.Errorf("load kube config %w", err)
+		}


This ended up not being that useful, so I'm going to delete this "read kube API settings from kubectl config" logic. In #3350 I added support for reading the token from connector options. I think that's a better approach for reading the host and CA cert. I'll add those options in the next follow up, which should also add the tests.

Previously we would only look for in-cluster config. In the future we should be able to specify these values from config options, so that we can run a connector in tests. This change reduces the number of places we use the in-cluser calls by relying on the rest.Config that is populated by the NewKubernetes constructor. Also read the CA cert from the existing config, instead of reading the file again. Also remove the SecretReader field, it was never used. Also add more godoc.

This should allow us to split up the conection logic by using this new type as the function parameter.

jmorganca reviewed Jul 16, 2022

View reviewed changes

internal/connector/connector.go Outdated Show resolved Hide resolved

mxyng reviewed Jul 20, 2022

View reviewed changes

dnephin force-pushed the dnephin/connector-run-local branch from 40f3218 to 9f54b31 Compare July 22, 2022 17:24

dnephin force-pushed the dnephin/connector-run-local branch from 9f54b31 to 296d31e Compare September 12, 2022 16:04

dnephin mentioned this pull request Sep 14, 2022

fix: resolve the access key to the secret value #3207

Merged

dnephin force-pushed the dnephin/connector-run-local branch 3 times, most recently from d699f2f to 68c834e Compare September 29, 2022 15:28

dnephin changed the base branch from main to dnephin/connector-remove-secret-storage September 29, 2022 15:28

dnephin force-pushed the dnephin/connector-remove-secret-storage branch from a1f771a to d342a51 Compare September 29, 2022 15:41

dnephin force-pushed the dnephin/connector-run-local branch from 68c834e to 77dd82c Compare September 29, 2022 15:42

dnephin marked this pull request as ready for review September 29, 2022 15:46

dnephin requested review from BruceMacD and pdevine as code owners September 29, 2022 15:46

dnephin commented Sep 29, 2022

View reviewed changes

dnephin mentioned this pull request Sep 29, 2022

Allow the connector to run outside of a kube pod (part 2) #3350

Merged

mxyng approved these changes Sep 30, 2022

View reviewed changes

dnephin force-pushed the dnephin/connector-remove-secret-storage branch from 1f22999 to 9a6bc74 Compare October 3, 2022 16:00

Base automatically changed from dnephin/connector-remove-secret-storage to main October 3, 2022 16:14

dnephin force-pushed the dnephin/connector-run-local branch from 77dd82c to 21f51cb Compare October 3, 2022 16:16

dnephin commented Oct 3, 2022

View reviewed changes

dnephin added 3 commits October 3, 2022 12:22

maintain: extract type for args

ec245ac

This should allow us to split up the conection logic by using this new type as the function parameter.

maintain: extract syncDestination from syncWithServer

622366f

dnephin force-pushed the dnephin/connector-run-local branch from 21f51cb to 622366f Compare October 3, 2022 16:24

dnephin merged commit f8689e0 into main Oct 3, 2022

dnephin deleted the dnephin/connector-run-local branch October 3, 2022 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the connector to run outside of a kube pod #2613

Allow the connector to run outside of a kube pod #2613

dnephin commented Jul 15, 2022 •

edited

mxyng left a comment

dnephin commented Jul 20, 2022

dnephin left a comment

dnephin Sep 29, 2022

dnephin Sep 29, 2022

dnephin Sep 29, 2022

dnephin Sep 29, 2022

dnephin Sep 29, 2022

dnephin Sep 29, 2022

dnephin Oct 3, 2022

		}

		k.SecretReader = secrets.NewKubernetesSecretProvider(clientset, namespace)

		checkSum := k8s.Checksum()
		logging.L.Debug().Str("uniqueID", checkSum).Msg("Cluster uniqueID")

		func syncDestination(con connector) error {
		host, port, err := con.k8s.Endpoint()

Allow the connector to run outside of a kube pod #2613

Allow the connector to run outside of a kube pod #2613

Conversation

dnephin commented Jul 15, 2022 • edited

Summary

mxyng left a comment

Choose a reason for hiding this comment

dnephin commented Jul 20, 2022

dnephin left a comment

Choose a reason for hiding this comment

dnephin Sep 29, 2022

Choose a reason for hiding this comment

dnephin Sep 29, 2022

Choose a reason for hiding this comment

dnephin Sep 29, 2022

Choose a reason for hiding this comment

dnephin Sep 29, 2022

Choose a reason for hiding this comment

dnephin Sep 29, 2022

Choose a reason for hiding this comment

dnephin Sep 29, 2022

Choose a reason for hiding this comment

dnephin Oct 3, 2022

Choose a reason for hiding this comment

dnephin commented Jul 15, 2022 •

edited