Code review updates

hashicorp · Feb 11, 2021 · d9b1268 · d9b1268
1 parent 069ce8b
commit d9b1268
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 12 deletions.
diff --git a/connect-inject/cleanup_resource.go b/connect-inject/cleanup_resource.go
@@ -134,7 +134,7 @@ func (c *CleanupResource) reconcile() {
 func (c *CleanupResource) Delete(key string, obj interface{}) error {
 	pod, ok := obj.(*corev1.Pod)
 	if !ok {
-		return fmt.Errorf("expected pod, got: #%v", obj)
+		return fmt.Errorf("expected pod, got: %#v", obj)
 	}
 	if pod == nil {
 		return fmt.Errorf("object for key %s was nil", key)
@@ -206,6 +206,8 @@ func (c *CleanupResource) Informer() cache.SharedIndexInformer {
 	)
 }
 
+// deregisterInstance deregisters instance from Consul by calling the agent at
+// hostIP's deregister service API.
 func (c *CleanupResource) deregisterInstance(instance *capi.CatalogService, hostIP string) error {
 	fullAddr := fmt.Sprintf("%s://%s:%s", c.ConsulScheme, hostIP, c.ConsulPort)
 	localConfig := capi.DefaultConfig()
@@ -218,10 +220,14 @@ func (c *CleanupResource) deregisterInstance(instance *capi.CatalogService, host
 	return client.Agent().ServiceDeregister(instance.ServiceID)
 }
 
+// currentPodsKey should be used to construct the key to access the
+// currentPods map.
 func currentPodsKey(podName, k8sNamespace string) string {
 	return fmt.Sprintf("%s/%s", k8sNamespace, podName)
 }
 
+// nodeInCluster returns whether nodeName is the name of a node in nodes, i.e.
+// it's the name of a node in this cluster.
 func nodeInCluster(nodes *corev1.NodeList, nodeName string) bool {
 	for _, n := range nodes.Items {
 		if n.Name == nodeName {

diff --git a/connect-inject/cleanup_resource_test.go b/connect-inject/cleanup_resource_test.go
@@ -32,11 +32,11 @@ func TestReconcile(t *testing.T) {
 		},
 		"instance does not have pod-name meta key": {
 			ConsulServices:      []capi.AgentServiceRegistration{consulNoPodNameMetaSvc},
-			ExpConsulServiceIDs: []string{"no-pod-name-meta"},
+			ExpConsulServiceIDs: []string{"foo-abc123-foo"},
 		},
 		"instance does not have k8s-namespace meta key": {
 			ConsulServices:      []capi.AgentServiceRegistration{consulNoK8sNSMetaSvc},
-			ExpConsulServiceIDs: []string{"no-k8s-ns-meta"},
+			ExpConsulServiceIDs: []string{"foo-abc123-foo"},
 		},
 		"out of cluster node": {
 			ConsulServices:      []capi.AgentServiceRegistration{consulFooSvc, consulFooSvcSidecar},
@@ -136,16 +136,21 @@ func TestReconcile(t *testing.T) {
 func TestDelete(t *testing.T) {
 	t.Parallel()
 
+	var nilPod *corev1.Pod
 	cases := map[string]struct {
-		Pod                 *corev1.Pod
+		Pod                 interface{}
 		ConsulServices      []capi.AgentServiceRegistration
 		ExpConsulServiceIDs []string
 		ExpErr              string
 	}{
 		"pod is nil": {
-			Pod:    nil,
+			Pod:    nilPod,
 			ExpErr: "object for key default/foo was nil",
 		},
+		"not expected type": {
+			Pod:    &corev1.Service{},
+			ExpErr: "expected pod, got: &v1.Service",
+		},
 		"pod does not have service-name annotation": {
 			Pod: &corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
@@ -158,6 +163,16 @@ func TestDelete(t *testing.T) {
 			},
 			ExpErr: "pod did not have consul.hashicorp.com/connect-service annotation",
 		},
+		"instance does not have pod-name meta": {
+			Pod:                 fooPod,
+			ConsulServices:      []capi.AgentServiceRegistration{consulNoPodNameMetaSvc},
+			ExpConsulServiceIDs: []string{"foo-abc123-foo"},
+		},
+		"instance does not have k8s-namespace meta": {
+			Pod:                 fooPod,
+			ConsulServices:      []capi.AgentServiceRegistration{consulNoK8sNSMetaSvc},
+			ExpConsulServiceIDs: []string{"foo-abc123-foo"},
+		},
 		"no instances still registered": {
 			Pod:                 fooPod,
 			ConsulServices:      nil,
@@ -223,7 +238,8 @@ func TestDelete(t *testing.T) {
 			// Run Delete.
 			err = cleanupResource.Delete("default/foo", c.Pod)
 			if c.ExpErr != "" {
-				require.EqualError(err, c.ExpErr)
+				require.Error(err)
+				require.Contains(err.Error(), c.ExpErr)
 			} else {
 				require.NoError(err)
 
@@ -288,19 +304,19 @@ var (
 		},
 	}
 	consulNoPodNameMetaSvc = capi.AgentServiceRegistration{
-		ID:      "no-pod-name-meta",
-		Name:    "no-pod-name-meta",
+		ID:      "foo-abc123-foo",
+		Name:    "foo",
 		Address: "127.0.0.1",
 		Meta: map[string]string{
 			MetaKeyKubeNS: "default",
 		},
 	}
 	consulNoK8sNSMetaSvc = capi.AgentServiceRegistration{
-		ID:      "no-k8s-ns-meta",
-		Name:    "no-k8s-ns-meta",
+		ID:      "foo-abc123-foo",
+		Name:    "foo",
 		Address: "127.0.0.1",
 		Meta: map[string]string{
-			MetaKeyPodName: "no-k8s-ns-meta",
+			MetaKeyPodName: "foo-abc123",
 		},
 	}
 	fooPod = &corev1.Pod{

diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go
@@ -133,9 +133,9 @@ func (c *Command) init() {
 		"K8s namespaces to explicitly deny. Takes precedence over allow. May be specified multiple times.")
 	c.flagSet.BoolVar(&c.flagEnableHealthChecks, "enable-health-checks-controller", false,
 		"Enables health checks controller.")
+	c.flagSet.DurationVar(&c.flagHealthChecksReconcilePeriod, "health-checks-reconcile-period", 1*time.Minute, "Reconcile period for health checks controller.")
 	c.flagSet.BoolVar(&c.flagEnableCleanupController, "enable-cleanup-controller", true,
 		"Enables cleanup controller that cleans up stale Consul service instances.")
-	c.flagSet.DurationVar(&c.flagHealthChecksReconcilePeriod, "health-checks-reconcile-period", 1*time.Minute, "Reconcile period for health checks controller.")
 	c.flagSet.DurationVar(&c.flagCleanupControllerReconcilePeriod, "cleanup-controller-reconcile-period", 5*time.Minute, "Reconcile period for cleanup controller.")
 	c.flagSet.BoolVar(&c.flagEnableNamespaces, "enable-namespaces", false,
 		"[Enterprise Only] Enables namespaces, in either a single Consul namespace or mirrored.")