From f6002ef8728926286dfb2a5b2dce08182d85358c Mon Sep 17 00:00:00 2001 From: DanStough Date: Thu, 14 Sep 2023 16:23:13 -0400 Subject: [PATCH] PR feedback --- control-plane/connect-inject/common/common.go | 4 +- .../connect-inject/common/common_test.go | 67 +++++++++++++++++++ .../endpointsv2/endpoints_controller.go | 3 +- .../controllers/pod/pod_controller.go | 13 +++- 4 files changed, 81 insertions(+), 6 deletions(-) diff --git a/control-plane/connect-inject/common/common.go b/control-plane/connect-inject/common/common.go index 76507eebe2..d105901df5 100644 --- a/control-plane/connect-inject/common/common.go +++ b/control-plane/connect-inject/common/common.go @@ -181,8 +181,8 @@ func PortValueFromIntOrString(pod corev1.Pod, port intstr.IntOrString) (uint32, return uint32(portVal), nil } -// ConsulNamespaceIsNotFound checks the gRPC error code and message to determine if the -// error indicates that the namespace of for a resource does not yet exist. +// ConsulNamespaceIsNotFound checks the gRPC error code and message to determine +// if a namespace does not exist. If the namespace exists this function returns false, true otherwise. func ConsulNamespaceIsNotFound(err error) bool { if err == nil { return false diff --git a/control-plane/connect-inject/common/common_test.go b/control-plane/connect-inject/common/common_test.go index a401923010..6227648fc1 100644 --- a/control-plane/connect-inject/common/common_test.go +++ b/control-plane/connect-inject/common/common_test.go @@ -4,11 +4,13 @@ package common import ( + "context" "fmt" "testing" mapset "github.com/deckarep/golang-set" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/consul/sdk/testutil" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -21,6 +23,8 @@ import ( "github.com/hashicorp/consul/proto-public/pbresource" "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" + "github.com/hashicorp/consul-k8s/control-plane/consul" + "github.com/hashicorp/consul-k8s/control-plane/helper/test" "github.com/hashicorp/consul-k8s/control-plane/namespaces" ) @@ -412,6 +416,10 @@ func Test_ConsulNamespaceIsNotFound(t *testing.T) { input error expectMissingNamespace bool }{ + { + name: "nil error", + expectMissingNamespace: false, + }, { name: "random error", input: fmt.Errorf("namespace resource not found"), @@ -441,3 +449,62 @@ func Test_ConsulNamespaceIsNotFound(t *testing.T) { }) } } + +// Test_ConsulNamespaceIsNotFound_ErrorMsg is an integration test that verifies the error message +// associated with a missing namespace while creating a resource doesn't drift. +func Test_ConsulNamespaceIsNotFound_ErrorMsg(t *testing.T) { + t.Parallel() + + // Create test consulServer server. + testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) { + c.Experiments = []string{"resource-apis"} + }) + resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher) + require.NoError(t, err) + + id := &pbresource.ID{ + Name: "foo", + Type: &pbresource.Type{ + Group: "catalog", + GroupVersion: "v1alpha1", + Kind: "Workload", + }, + Tenancy: &pbresource.Tenancy{ + Partition: constants.DefaultConsulPartition, + Namespace: "i-dont-exist-but-its-ok-we-will-meet-again-someday", + + // Because we are explicitly defining NS/partition, this will not default and must be explicit. + // At a future point, this will move out of the Tenancy block. + PeerName: constants.DefaultConsulPeer, + }, + } + + workload := &pbcatalog.Workload{ + Addresses: []*pbcatalog.WorkloadAddress{ + {Host: "10.0.0.1", Ports: []string{"mesh"}}, + }, + Ports: map[string]*pbcatalog.WorkloadPort{ + "mesh": { + Port: constants.ProxyDefaultInboundPort, + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + }, + }, + NodeName: "banana", + Identity: "foo", + } + + data := ToProtoAny(workload) + + resource := &pbresource.Resource{ + Id: id, + Data: data, + } + + _, err = resourceClient.Write(context.Background(), &pbresource.WriteRequest{Resource: resource}) + require.Error(t, err) + + s, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.InvalidArgument, s.Code()) + require.Contains(t, s.Message(), "namespace resource not found") +} diff --git a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go index d18de35701..aa6b377e37 100644 --- a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go @@ -125,7 +125,8 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // Requeue (which includes backoff) to try again. if common.ConsulNamespaceIsNotFound(err) { r.Log.Info("Consul namespace not found; re-queueing request", - "service", service.GetName(), "ns", req.Namespace, "consul-ns", r.getConsulNamespace(req.Namespace)) + "service", service.GetName(), "ns", req.Namespace, + "consul-ns", r.getConsulNamespace(req.Namespace), "err", err.Error()) return ctrl.Result{Requeue: true}, nil } errs = multierror.Append(errs, err) diff --git a/control-plane/connect-inject/controllers/pod/pod_controller.go b/control-plane/connect-inject/controllers/pod/pod_controller.go index 3124165e95..ec515214c8 100644 --- a/control-plane/connect-inject/controllers/pod/pod_controller.go +++ b/control-plane/connect-inject/controllers/pod/pod_controller.go @@ -149,16 +149,20 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // Requeue (which includes backoff) to try again. if common.ConsulNamespaceIsNotFound(err) { r.Log.Info("Consul namespace not found; re-queueing request", - "pod", req.Name, "ns", req.Namespace, "consul-ns", r.getConsulNamespace(req.Namespace)) + "pod", req.Name, "ns", req.Namespace, "consul-ns", + r.getConsulNamespace(req.Namespace), "err", err.Error()) return ctrl.Result{Requeue: true}, nil } errs = multierror.Append(errs, err) } if err := r.writeWorkload(ctx, pod); err != nil { + // Technically this is not needed, but keeping in case this gets refactored in + // a different order if common.ConsulNamespaceIsNotFound(err) { r.Log.Info("Consul namespace not found; re-queueing request", - "pod", req.Name, "ns", req.Namespace, "consul-ns", r.getConsulNamespace(req.Namespace)) + "pod", req.Name, "ns", req.Namespace, "consul-ns", + r.getConsulNamespace(req.Namespace), "err", err.Error()) return ctrl.Result{Requeue: true}, nil } errs = multierror.Append(errs, err) @@ -170,9 +174,12 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu //} if err := r.writeHealthStatus(ctx, pod); err != nil { + // Technically this is not needed, but keeping in case this gets refactored in + // a different order if common.ConsulNamespaceIsNotFound(err) { r.Log.Info("Consul namespace not found; re-queueing request", - "pod", req.Name, "ns", req.Namespace, "consul-ns", r.getConsulNamespace(req.Namespace)) + "pod", req.Name, "ns", req.Namespace, "consul-ns", + r.getConsulNamespace(req.Namespace), "err", err.Error()) return ctrl.Result{Requeue: true}, nil } errs = multierror.Append(errs, err)