Fail sync if consul delete returns an error

api/v1alpha1/proxydefaults_types.go

@@ -25,6 +25,7 @@ const (
 
 // ProxyDefaults is the Schema for the proxydefaults API
 // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource"
 type ProxyDefaults struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

api/v1alpha1/servicedefaults_types.go

@@ -20,6 +20,7 @@ const (
 
 // ServiceDefaults is the Schema for the servicedefaults API
 // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource"
 type ServiceDefaults struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

api/v1alpha1/serviceintentions_types.go

@@ -72,6 +72,7 @@ type IntentionAction string
 
 // ServiceIntentions is the Schema for the serviceintentions API
 // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource"
 type ServiceIntentions struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

api/v1alpha1/serviceresolver_types.go

@@ -22,6 +22,7 @@ const ServiceResolverKubeKind string = "serviceresolver"
 
 // ServiceResolver is the Schema for the serviceresolvers API
 // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource"
 type ServiceResolver struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
@@ -247,7 +248,7 @@ type ServiceResolverFailover struct {
 	ServiceSubset string `json:"serviceSubset,omitempty"`
 	// Namespace is the namespace to resolve the requested service from to form
 	// the failover group of instances. If empty the current namespace is used.
-	Namespace string `json:"namespaces,omitempty"`
+	Namespace string `json:"namespace,omitempty"`
 	// Datacenters is a fixed list of datacenters to try during failover.
 	Datacenters []string `json:"datacenters,omitempty"`
 }

api/v1alpha1/servicerouter_types.go

@@ -271,6 +271,7 @@ func (in *ServiceRouteDestination) toConsul() *capi.ServiceRouteDestination {
 
 // ServiceRouter is the Schema for the servicerouters API
 // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource"
 type ServiceRouter struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

api/v1alpha1/servicesplitter_types.go

@@ -20,6 +20,7 @@ import (
 
 // ServiceSplitter is the Schema for the servicesplitters API
 // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource"
 type ServiceSplitter struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

config/crd/bases/consul.hashicorp.com_proxydefaults.yaml

@@ -13,6 +13,10 @@ spec:
     description: The sync status of the resource with Consul
     name: Synced
     type: string
+  - JSONPath: .metadata.creationTimestamp
+    description: The age of the resource
+    name: Age
+    type: date
   group: consul.hashicorp.com
   names:
     kind: ProxyDefaults

config/crd/bases/consul.hashicorp.com_servicedefaults.yaml

@@ -13,6 +13,10 @@ spec:
     description: The sync status of the resource with Consul
     name: Synced
     type: string
+  - JSONPath: .metadata.creationTimestamp
+    description: The age of the resource
+    name: Age
+    type: date
   group: consul.hashicorp.com
   names:
     kind: ServiceDefaults

config/crd/bases/consul.hashicorp.com_serviceintentions.yaml

@@ -13,6 +13,10 @@ spec:
     description: The sync status of the resource with Consul
     name: Synced
     type: string
+  - JSONPath: .metadata.creationTimestamp
+    description: The age of the resource
+    name: Age
+    type: date
   group: consul.hashicorp.com
   names:
     kind: ServiceIntentions

config/crd/bases/consul.hashicorp.com_serviceresolvers.yaml

@@ -13,6 +13,10 @@ spec:
     description: The sync status of the resource with Consul
     name: Synced
     type: string
+  - JSONPath: .metadata.creationTimestamp
+    description: The age of the resource
+    name: Age
+    type: date
   group: consul.hashicorp.com
   names:
     kind: ServiceResolver
@@ -52,7 +56,7 @@ spec:
                     items:
                       type: string
                     type: array
-                  namespaces:
+                  namespace:
                     description: Namespace is the namespace to resolve the requested service from to form the failover group of instances. If empty the current namespace is used.
                     type: string
                   service:

config/crd/bases/consul.hashicorp.com_servicerouters.yaml

@@ -13,6 +13,10 @@ spec:
     description: The sync status of the resource with Consul
     name: Synced
     type: string
+  - JSONPath: .metadata.creationTimestamp
+    description: The age of the resource
+    name: Age
+    type: date
   group: consul.hashicorp.com
   names:
     kind: ServiceRouter

config/crd/bases/consul.hashicorp.com_servicesplitters.yaml

@@ -13,6 +13,10 @@ spec:
     description: The sync status of the resource with Consul
     name: Synced
     type: string
+  - JSONPath: .metadata.creationTimestamp
+    description: The age of the resource
+    name: Age
+    type: date
   group: consul.hashicorp.com
   names:
     kind: ServiceSplitter

controller/configentry_controller.go

@@ -126,7 +126,8 @@ func (r *ConfigEntryController) ReconcileEntry(
 						Namespace: r.consulNamespace(configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()),
 					})
 					if err != nil {
-						return ctrl.Result{}, fmt.Errorf("deleting config entry from consul: %w", err)
+						return r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulAgentError,
+							fmt.Errorf("deleting config entry from consul: %w", err))
 					}
 					logger.Info("deletion from Consul successful")
 				} else {

controller/configentry_controller_test.go

@@ -8,6 +8,8 @@ import (
 
 	"github.com/go-logr/logr"
 	logrtest "github.com/go-logr/logr/testing"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/hashicorp/consul-k8s/api/common"
 	"github.com/hashicorp/consul-k8s/api/v1alpha1"
 	capi "github.com/hashicorp/consul/api"
@@ -2073,3 +2075,112 @@ func TestConfigEntryControllers_doesNotDeleteUnownedConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestConfigEntryControllers_updatesStatusWhenDeleteFails(t *testing.T) {
+	ctx := context.Background()
+	kubeNS := "default"
+
+	s := runtime.NewScheme()
+	s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.ServiceDefaults{}, &v1alpha1.ServiceSplitter{})
+
+	defaults := &v1alpha1.ServiceDefaults{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "service",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.ServiceDefaultsSpec{
+			Protocol: "http",
+		},
+	}
+
+	splitter := &v1alpha1.ServiceSplitter{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "service",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.ServiceSplitterSpec{
+			Splits: v1alpha1.ServiceSplits{
+				{
+					Weight:  100,
+					Service: "service",
+				},
+			},
+		},
+	}
+
+	client := fake.NewFakeClientWithScheme(s, defaults, splitter)
+
+	consul, err := testutil.NewTestServerConfigT(t, nil)
+	require.NoError(t, err)
+	defer consul.Stop()
+
+	consul.WaitForServiceIntentions(t)
+	consulClient, err := capi.NewClient(&capi.Config{
+		Address: consul.HTTPAddr,
+	})
+	require.NoError(t, err)
+
+	logger := logrtest.TestLogger{T: t}
+
+	svcDefaultsReconciler := ServiceDefaultsController{
+		Client: client,
+		Log:    logger,
+		ConfigEntryController: &ConfigEntryController{
+			ConsulClient:   consulClient,
+			DatacenterName: datacenterName,
+		},
+	}
+	svcSplitterReconciler := ServiceSplitterController{
+		Client: client,
+		Log:    logger,
+		ConfigEntryController: &ConfigEntryController{
+			ConsulClient:   consulClient,
+			DatacenterName: datacenterName,
+		},
+	}
+
+	defaultsNamespacedName := types.NamespacedName{
+		Namespace: kubeNS,
+		Name:      defaults.Name,
+	}
+
+	splitterNamespacedName := types.NamespacedName{
+		Namespace: kubeNS,
+		Name:      splitter.Name,
+	}
+
+	// Create config entries for service-defaults and service-splitter.
+	resp, err := svcDefaultsReconciler.Reconcile(ctrl.Request{NamespacedName: defaultsNamespacedName})
+	require.NoError(t, err)
+	require.False(t, resp.Requeue)
+
+	resp, err = svcSplitterReconciler.Reconcile(ctrl.Request{NamespacedName: splitterNamespacedName})
+	require.NoError(t, err)
+	require.False(t, resp.Requeue)
+
+	err = client.Get(ctx, defaultsNamespacedName, defaults)
+	require.NoError(t, err)
+
+	// Update service-defaults with deletion timestamp so that it attempts deletion on reconcile.
+	defaults.ObjectMeta.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	err = client.Update(ctx, defaults)
+	require.NoError(t, err)
+
+	// Reconcile should fail as the service-splitter still required the service-defaults causing the delete operation on Consul to fail.
+	resp, err = svcDefaultsReconciler.Reconcile(ctrl.Request{NamespacedName: defaultsNamespacedName})
+	require.EqualError(t, err, "deleting config entry from consul: Unexpected response code: 500 (discovery chain \"service\" uses a protocol \"tcp\" that does not permit advanced routing or splitting behavior)")
+	require.False(t, resp.Requeue)
+
+	err = client.Get(ctx, defaultsNamespacedName, defaults)
+	require.NoError(t, err)
+
+	// Ensure the status of the resource is updated to display failure reason.
+	syncCondition := defaults.GetCondition(v1alpha1.ConditionSynced)
+	expectedCondition := &v1alpha1.Condition{
+		Type:    v1alpha1.ConditionSynced,
+		Status:  corev1.ConditionFalse,
+		Reason:  ConsulAgentError,
+		Message: "deleting config entry from consul: Unexpected response code: 500 (discovery chain \"service\" uses a protocol \"tcp\" that does not permit advanced routing or splitting behavior)",
+	}
+	require.True(t, cmp.Equal(syncCondition, expectedCondition, cmpopts.IgnoreFields(v1alpha1.Condition{}, "LastTransitionTime")))
+}

go.sum

@@ -252,8 +252,6 @@ github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t
 github.com/grpc-ecosystem/grpc-gateway v1.9.5/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY=
 github.com/hashicorp/consul/api v1.4.1-0.20201007080954-aa0f5ff839c5 h1:mHgaDaPdf0z3UG3G2UpCINFMRKhzi1DLNoOp6sIQWnU=
 github.com/hashicorp/consul/api v1.4.1-0.20201007080954-aa0f5ff839c5/go.mod h1:1NSuaUUkFaJzMasbfq/11wKYWSR67Xn6r2DXKhuDNFg=
-github.com/hashicorp/consul/api v1.6.0 h1:SZB2hQW8AcTOpfDmiVblQbijxzsRuiyy0JpHfabvHio=
-github.com/hashicorp/consul/api v1.6.0/go.mod h1:1NSuaUUkFaJzMasbfq/11wKYWSR67Xn6r2DXKhuDNFg=
 github.com/hashicorp/consul/sdk v0.4.1-0.20201006182405-a2a8e9c7839a h1:yclqizoDCodLeiAUg1Siaodz3hvIBxzH8A2GnjY74EU=
 github.com/hashicorp/consul/sdk v0.4.1-0.20201006182405-a2a8e9c7839a/go.mod h1:fY08Y9z5SvJqevyZNy6WWPXiG3KwBPAvlcdx16zZ0fM=
 github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA=