From 02753fa2766725a13c60bcdf0919af2d37dd826e Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Tue, 2 May 2023 04:32:45 -0400 Subject: [PATCH 1/8] Send ClusterIP to Consul --- .../endpoints/endpoints_controller.go | 25 ++- .../endpoints/endpoints_controller_test.go | 51 +++++ .../controllers/configentry_controller.go | 54 +++++ .../configentry_controller_test.go | 209 ++++++++++++++++++ control-plane/go.mod | 6 +- control-plane/go.sum | 12 +- 6 files changed, 347 insertions(+), 10 deletions(-) diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go index f408a21ea1..1d66d0c934 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go @@ -1,6 +1,5 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 - package endpoints import ( @@ -295,6 +294,15 @@ func (r *Controller) registerServicesAndHealthCheck(apiClient *api.Client, pod c return err } + // Add manual ip to the VIP table + r.Log.Info("adding manual ip to virtual ip table in Consul", "name", serviceRegistration.Service.Service, + "id", serviceRegistration.ID) + err = assignServiceVirtualIP(r.Context, apiClient, serviceRegistration.Service) + if err != nil { + r.Log.Error(err, "failed to add ip to virtual ip table", "name", serviceRegistration.Service.Service) + return err + } + // Register the proxy service instance with Consul. r.Log.Info("registering proxy service with Consul", "name", proxyServiceRegistration.Service.Service) _, err = apiClient.Catalog().Register(proxyServiceRegistration, nil) @@ -1258,6 +1266,21 @@ func (r *Controller) appendNodeMeta(registration *api.CatalogRegistration) { } } +// assignServiceVirtualIPs manually assigns the ClusterIP to the virtual IP table so that transparent proxy routing works. +func assignServiceVirtualIP(ctx context.Context, apiClient *api.Client, svc *api.AgentService) error { + _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{svc.TaggedAddresses[clusterIPTaggedAddressName].Address}, &api.WriteOptions{}) + if err != nil { + // Maintain backwards compatibility with older versions of Consul that do not support the VIP improvements. Tproxy + // will not work 100% correctly but the mesh will still work + if strings.Contains(err.Error(), "404") { + return fmt.Errorf("failed to add ip for service %s to virtual ip table. Please upgrade Consul to version 1.16 or higher", svc.Service) + } else { + return err + } + } + return nil +} + // hasBeenInjected checks the value of the status annotation and returns true if the Pod has been injected. func hasBeenInjected(pod corev1.Pod) bool { if anno, ok := pod.Annotations[constants.KeyInjectStatus]; ok && anno == constants.Injected { diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go index 7ea04a4e52..30ff005241 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go @@ -6414,3 +6414,54 @@ func createGatewayPod(name, ip string, annotations map[string]string) *corev1.Po } return pod } + +func TestReconcileAssignServiceVirtualIP(t *testing.T) { + t.Parallel() + ctx := context.Background() + cases := []struct { + name string + service *api.AgentService + expectErr bool + }{ + { + name: "valid service", + service: &api.AgentService{ + ID: "", + Service: "foo", + Port: 80, + Address: "1.2.3.4", + TaggedAddresses: map[string]api.ServiceAddress{ + "virtual": { + Address: "1.2.3.4", + Port: 80, + }, + }, + Meta: map[string]string{constants.MetaKeyKubeNS: "default"}, + }, + expectErr: false, + }, + { + name: "service missing IP should error", + service: &api.AgentService{ + ID: "", + Service: "bar", + Meta: map[string]string{constants.MetaKeyKubeNS: "default"}, + }, + expectErr: true, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + + // Create test consulServer server. + testClient := test.TestServerWithMockConnMgrWatcher(t, nil) + apiClient := testClient.APIClient + err := assignServiceVirtualIP(ctx, apiClient, c.service) + if err != nil { + require.True(t, c.expectErr) + } else { + require.False(t, c.expectErr) + } + }) + } +} diff --git a/control-plane/controllers/configentry_controller.go b/control-plane/controllers/configentry_controller.go index c2c6da9071..86b2af5c4a 100644 --- a/control-plane/controllers/configentry_controller.go +++ b/control-plane/controllers/configentry_controller.go @@ -199,6 +199,7 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont return r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulAgentError, fmt.Errorf("writing config entry to consul: %w", err)) } + logger.Info("config entry created", "request-time", writeMeta.RequestTime) return r.syncSuccessful(ctx, crdCtrl, configEntry) } @@ -267,6 +268,13 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont return r.syncSuccessful(ctx, crdCtrl, configEntry) } + // For resolvers and splitters, we need to set the ClusterIP of the matching service to Consul so that transparent + // proxy works correctly. Do not fail the reconcile if assigning the virtual IP returns an error. + err = assignServiceVirtualIP(ctx, logger, consulClient, crdCtrl, req.NamespacedName, configEntry) + if err != nil { + logger.Error(err, "failed assigning service virtual ip") + } + return ctrl.Result{}, nil } @@ -381,6 +389,52 @@ func (r *ConfigEntryController) nonMatchingMigrationError(kubeEntry common.Confi return fmt.Errorf("migration failed: Kubernetes resource does not match existing Consul config entry: consul=%s, kube=%s", consulJSON, kubeJSON) } +// assignServiceVirtualIPs manually sends the ClusterIP for a matching service for ServiceRouter or ServiceSplitter +// CRDs to Consul so that it can be added to the virtual IP table. The assignment is skipped if the matching service +// does not exist or if an older version of Consul is being used. Endpoints Controller, on service registration, also +// manually sends a ClusterIP when a service is created. This increases the chance of a real IP ending up in the +// discovery chain. +func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClient *capi.Client, crdCtrl Controller, namespacedName types.NamespacedName, configEntry common.ConfigEntryResource) error { + + // Only assign the virtual IP for service routers and splitters. Every other type is skipped. + kubeKind := configEntry.KubeKind() + if kubeKind != common.ServiceRouter && kubeKind != common.ServiceSplitter { + return nil + } + + service := corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: configEntry.KubernetesName(), + Namespace: namespacedName.Namespace, + }, + } + + err := crdCtrl.Get(ctx, namespacedName, &service) + if err != nil { + // It is non-fatal if the service does not exist. The ClusterIP will get added when the service is registered in + // the endpoints controller + if strings.Contains(err.Error(), "not found") { + return nil + } + // Something is really wrong with the service + return err + } + + logger.Info("adding manual ip to virtual ip table in Consul", "name", service.Name) + _, _, err = consulClient.Internal().AssignServiceVirtualIP(ctx, configEntry.KubernetesName(), []string{service.Spec.ClusterIP}, &capi.WriteOptions{}) + if err != nil { + // Maintain backwards compatibility with older versions of Consul that do not support the manual VIP improvements. With the older version, the mesh + // will still work. + if isNotFoundErr(err) { + logger.Error(err, "failed to add ip to virtual ip table. Please upgrade Consul to version 1.16 or higher", "name", service.Name) + return nil + } else { + return err + } + } + return nil +} + func isNotFoundErr(err error) bool { return err != nil && strings.Contains(err.Error(), "404") } diff --git a/control-plane/controllers/configentry_controller_test.go b/control-plane/controllers/configentry_controller_test.go index 494715cf4f..71244baba3 100644 --- a/control-plane/controllers/configentry_controller_test.go +++ b/control-plane/controllers/configentry_controller_test.go @@ -22,6 +22,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -1891,3 +1892,211 @@ func TestConfigEntryController_Migration(t *testing.T) { }) } } + +func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { + /* t.Parallel() */ + kubeNS := "default" + + cases := []struct { + name string + kubeKind string + consulKind string + consulPrereqs []capi.ConfigEntry + configEntryResource common.ConfigEntryResource + service corev1.Service + reconciler func(client.Client, *consul.Config, consul.ServerConnectionManager, logr.Logger) Controller + expectErr bool + }{ + { + name: "ServiceDefaults should have no error because service defaults is not a supported vip CRD", + kubeKind: "ServiceDefaults", + consulKind: capi.ServiceDefaults, + configEntryResource: &v1alpha1.ServiceDefaults{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: kubeNS, + }, + }, + reconciler: func(client client.Client, cfg *consul.Config, watcher consul.ServerConnectionManager, logger logr.Logger) Controller { + return &ServiceDefaultsController{ + Client: client, + Log: logger, + ConfigEntryController: &ConfigEntryController{ + ConsulClientConfig: cfg, + ConsulServerConnMgr: watcher, + DatacenterName: datacenterName, + }, + } + }, + expectErr: false, + }, + { + name: "ServiceRouter no error and vip should be assigned", + kubeKind: "ServiceRouter", + consulKind: capi.ServiceRouter, + consulPrereqs: []capi.ConfigEntry{ + &capi.ServiceConfigEntry{ + Kind: capi.ServiceDefaults, + Name: "foo", + Protocol: "http", + }, + }, + configEntryResource: &v1alpha1.ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: kubeNS, + }, + Spec: v1alpha1.ServiceRouterSpec{ + Routes: []v1alpha1.ServiceRoute{ + { + Match: &v1alpha1.ServiceRouteMatch{ + HTTP: &v1alpha1.ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + }, + }, + }, + }, + service: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: kubeNS, + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + Ports: []corev1.ServicePort{ + { + Port: 8081, + }, + }, + }, + }, + reconciler: func(client client.Client, cfg *consul.Config, watcher consul.ServerConnectionManager, logger logr.Logger) Controller { + return &ServiceRouterController{ + Client: client, + Log: logger, + ConfigEntryController: &ConfigEntryController{ + ConsulClientConfig: cfg, + ConsulServerConnMgr: watcher, + DatacenterName: datacenterName, + }, + } + }, + expectErr: false, + }, + { + name: "ServiceRouter should fail because service does not have a valid IP address", + kubeKind: "ServiceRouter", + consulKind: capi.ServiceRouter, + consulPrereqs: []capi.ConfigEntry{ + &capi.ServiceConfigEntry{ + Kind: capi.ServiceDefaults, + Name: "bar", + Protocol: "http", + }, + }, + configEntryResource: &v1alpha1.ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: kubeNS, + }, + Spec: v1alpha1.ServiceRouterSpec{ + Routes: []v1alpha1.ServiceRoute{ + { + Match: &v1alpha1.ServiceRouteMatch{ + HTTP: &v1alpha1.ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + }, + }, + }, + }, + service: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: kubeNS, + }, + }, + reconciler: func(client client.Client, cfg *consul.Config, watcher consul.ServerConnectionManager, logger logr.Logger) Controller { + return &ServiceRouterController{ + Client: client, + Log: logger, + ConfigEntryController: &ConfigEntryController{ + ConsulClientConfig: cfg, + ConsulServerConnMgr: watcher, + DatacenterName: datacenterName, + }, + } + }, + expectErr: true, + }, + + { + name: "ServiceSplitter no error because a matching service does not exist", + kubeKind: "ServiceSplitter", + consulKind: capi.ServiceSplitter, + consulPrereqs: []capi.ConfigEntry{ + &capi.ServiceConfigEntry{ + Kind: capi.ServiceDefaults, + Name: "foo", + Protocol: "http", + }, + }, + configEntryResource: &v1alpha1.ServiceSplitter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: kubeNS, + }, + Spec: v1alpha1.ServiceSplitterSpec{ + Splits: []v1alpha1.ServiceSplit{ + { + Weight: 100, + }, + }, + }, + }, + reconciler: func(client client.Client, cfg *consul.Config, watcher consul.ServerConnectionManager, logger logr.Logger) Controller { + return &ServiceSplitterController{ + Client: client, + Log: logger, + ConfigEntryController: &ConfigEntryController{ + ConsulClientConfig: cfg, + ConsulServerConnMgr: watcher, + DatacenterName: datacenterName, + }, + } + }, + expectErr: false, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ctx := context.Background() + + s := runtime.NewScheme() + s.AddKnownTypes(v1alpha1.GroupVersion, c.configEntryResource) + s.AddKnownTypes(schema.GroupVersion{Group: "", Version: "v1"}, &corev1.Service{}) + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(&c.service, c.configEntryResource).Build() + + testClient := test.TestServerWithMockConnMgrWatcher(t, nil) + testClient.TestServer.WaitForLeader(t) + consulClient := testClient.APIClient + + ctrl := c.reconciler(fakeClient, testClient.Cfg, testClient.Watcher, logrtest.TestLogger{T: t}) + namespacedName := types.NamespacedName{ + Namespace: kubeNS, + Name: c.configEntryResource.KubernetesName(), + } + + err := assignServiceVirtualIP(ctx, ctrl.Logger(namespacedName), consulClient, ctrl, namespacedName, c.configEntryResource) + if err != nil { + require.True(t, c.expectErr) + } else { + require.False(t, c.expectErr) + } + }) + } +} diff --git a/control-plane/go.mod b/control-plane/go.mod index 4cf30f5816..6a565a080d 100644 --- a/control-plane/go.mod +++ b/control-plane/go.mod @@ -8,9 +8,9 @@ require ( github.com/go-logr/logr v0.4.0 github.com/google/go-cmp v0.5.8 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 - github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20220831174802-b8af65262de8 - github.com/hashicorp/consul-server-connection-manager v0.1.0 - github.com/hashicorp/consul/api v1.10.1-0.20230427155444-391ed069c461 + github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20230511143918-bd16ab83383d + github.com/hashicorp/consul-server-connection-manager v0.1.2 + github.com/hashicorp/consul/api v1.10.1-0.20230512003852-bd0eb07ed3ca github.com/hashicorp/consul/sdk v0.13.1 github.com/hashicorp/go-bexpr v0.1.11 github.com/hashicorp/go-discover v0.0.0-20200812215701-c4b85f6ed31f diff --git a/control-plane/go.sum b/control-plane/go.sum index f49bc4b2d5..9d5bf5e90c 100644 --- a/control-plane/go.sum +++ b/control-plane/go.sum @@ -348,13 +348,13 @@ github.com/grpc-ecosystem/go-grpc-middleware v1.3.0/go.mod h1:z0ButlSOZa5vEBq9m2 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk= github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= -github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20220831174802-b8af65262de8 h1:TQY0oKtLV15UNYWeSkTxi4McBIyLecsEtbc/VfxvbYA= -github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20220831174802-b8af65262de8/go.mod h1:aw35GB76URgbtxaSSMxbOetbG7YEHHPkIX3/SkTBaWc= -github.com/hashicorp/consul-server-connection-manager v0.1.0 h1:XCweGvMHzra88rYv2zxwwuUOjBUdcQmNKVrnQmt/muo= -github.com/hashicorp/consul-server-connection-manager v0.1.0/go.mod h1:XVVlO+Yk7aiRpspiHZkrrFVn9BJIiOPnQIzqytPxGaU= +github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20230511143918-bd16ab83383d h1:RJ1MZ8JKnfgKQ1kR3IBQAMpOpzXrdseZAYN/QR//MFM= +github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20230511143918-bd16ab83383d/go.mod h1:IHIHMzkoMwlv6rLsgwcoFBVYupR7/1pKEOHBMjD4L0k= +github.com/hashicorp/consul-server-connection-manager v0.1.2 h1:tNVQHUPuMbd+cMdD8kd+qkZUYpmLmrHMAV/49f4L53I= +github.com/hashicorp/consul-server-connection-manager v0.1.2/go.mod h1:NzQoVi1KcxGI2SangsDue8+ZPuXZWs+6BKAKrDNyg+w= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= -github.com/hashicorp/consul/api v1.10.1-0.20230427155444-391ed069c461 h1:cbsTR88ShbvcRMqLU8K0atm4GmRr8UH4x4jX4e12RYE= -github.com/hashicorp/consul/api v1.10.1-0.20230427155444-391ed069c461/go.mod h1:tXfrC6o0yFTgAW46xd5Ic8STHc9oIBcRVBcwhX5KNCQ= +github.com/hashicorp/consul/api v1.10.1-0.20230512003852-bd0eb07ed3ca h1:5UPVYOlJg/HBEJ2q82rkkQ3ZLzeMnF5MOpGcw2kh+XU= +github.com/hashicorp/consul/api v1.10.1-0.20230512003852-bd0eb07ed3ca/go.mod h1:tXfrC6o0yFTgAW46xd5Ic8STHc9oIBcRVBcwhX5KNCQ= github.com/hashicorp/consul/proto-public v0.1.0 h1:O0LSmCqydZi363hsqc6n2v5sMz3usQMXZF6ziK3SzXU= github.com/hashicorp/consul/proto-public v0.1.0/go.mod h1:vs2KkuWwtjkIgA5ezp4YKPzQp4GitV+q/+PvksrA92k= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= From 6e12a0def5b1f7ec00af7580d70bebf719b212d9 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Fri, 12 May 2023 14:00:30 -0400 Subject: [PATCH 2/8] Add changelog --- .changelog/2124.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/2124.txt diff --git a/.changelog/2124.txt b/.changelog/2124.txt new file mode 100644 index 0000000000..b65c23db2e --- /dev/null +++ b/.changelog/2124.txt @@ -0,0 +1,3 @@ +``release-note:improvement +control-plane: Transparent proxy enhancements for failover and virtual Services +``` From 4bc3b7dbca0cab2983da3607fb77ce83a8052e78 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Fri, 12 May 2023 21:48:07 -0400 Subject: [PATCH 3/8] Do not have a fatal error if cannot add VIP --- .../connect-inject/controllers/endpoints/endpoints_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go index 1d66d0c934..fdbcbf97f0 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go @@ -300,7 +300,6 @@ func (r *Controller) registerServicesAndHealthCheck(apiClient *api.Client, pod c err = assignServiceVirtualIP(r.Context, apiClient, serviceRegistration.Service) if err != nil { r.Log.Error(err, "failed to add ip to virtual ip table", "name", serviceRegistration.Service.Service) - return err } // Register the proxy service instance with Consul. From ea021945d527c6cf49e64e91c5fcaecc1b005629 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Sat, 13 May 2023 08:11:43 -0400 Subject: [PATCH 4/8] Do not try to assign if IP is empty --- .../controllers/endpoints/endpoints_controller.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go index fdbcbf97f0..7ed99fe434 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go @@ -1267,7 +1267,11 @@ func (r *Controller) appendNodeMeta(registration *api.CatalogRegistration) { // assignServiceVirtualIPs manually assigns the ClusterIP to the virtual IP table so that transparent proxy routing works. func assignServiceVirtualIP(ctx context.Context, apiClient *api.Client, svc *api.AgentService) error { - _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{svc.TaggedAddresses[clusterIPTaggedAddressName].Address}, &api.WriteOptions{}) + IP := svc.TaggedAddresses[clusterIPTaggedAddressName].Address + if IP == "" { + return nil + } + _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{IP}, &api.WriteOptions{}) if err != nil { // Maintain backwards compatibility with older versions of Consul that do not support the VIP improvements. Tproxy // will not work 100% correctly but the mesh will still work From 0d49ac5eb5fd17c106b96a457ad9a778098832a8 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Mon, 15 May 2023 10:10:18 -0400 Subject: [PATCH 5/8] Missing an IP on the service should not error --- .../controllers/endpoints/endpoints_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go index 30ff005241..e9ae243a12 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller_test.go @@ -6441,13 +6441,13 @@ func TestReconcileAssignServiceVirtualIP(t *testing.T) { expectErr: false, }, { - name: "service missing IP should error", + name: "service missing IP should not error", service: &api.AgentService{ ID: "", Service: "bar", Meta: map[string]string{constants.MetaKeyKubeNS: "default"}, }, - expectErr: true, + expectErr: false, }, } for _, c := range cases { From bd54eee7263db551d05cb2dd592f38428a914f8c Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Mon, 15 May 2023 13:47:12 -0400 Subject: [PATCH 6/8] Update based on review feedback --- .../endpoints/endpoints_controller.go | 6 +- .../controllers/configentry_controller.go | 9 ++- .../configentry_controller_test.go | 59 ++++++++++++++++++- 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go index 7ed99fe434..e9f4a62225 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go @@ -1267,11 +1267,11 @@ func (r *Controller) appendNodeMeta(registration *api.CatalogRegistration) { // assignServiceVirtualIPs manually assigns the ClusterIP to the virtual IP table so that transparent proxy routing works. func assignServiceVirtualIP(ctx context.Context, apiClient *api.Client, svc *api.AgentService) error { - IP := svc.TaggedAddresses[clusterIPTaggedAddressName].Address - if IP == "" { + ip := svc.TaggedAddresses[clusterIPTaggedAddressName].Address + if ip == "" { return nil } - _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{IP}, &api.WriteOptions{}) + _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{ip}, &api.WriteOptions{}) if err != nil { // Maintain backwards compatibility with older versions of Consul that do not support the VIP improvements. Tproxy // will not work 100% correctly but the mesh will still work diff --git a/control-plane/controllers/configentry_controller.go b/control-plane/controllers/configentry_controller.go index 86b2af5c4a..0704a6aa23 100644 --- a/control-plane/controllers/configentry_controller.go +++ b/control-plane/controllers/configentry_controller.go @@ -398,7 +398,7 @@ func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClien // Only assign the virtual IP for service routers and splitters. Every other type is skipped. kubeKind := configEntry.KubeKind() - if kubeKind != common.ServiceRouter && kubeKind != common.ServiceSplitter { + if kubeKind != common.ServiceResolver && kubeKind != common.ServiceRouter && kubeKind != common.ServiceSplitter { return nil } @@ -409,11 +409,10 @@ func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClien }, } - err := crdCtrl.Get(ctx, namespacedName, &service) - if err != nil { + if err := crdCtrl.Get(ctx, namespacedName, &service); err != nil { // It is non-fatal if the service does not exist. The ClusterIP will get added when the service is registered in // the endpoints controller - if strings.Contains(err.Error(), "not found") { + if k8serr.IsNotFound(err) { return nil } // Something is really wrong with the service @@ -421,7 +420,7 @@ func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClien } logger.Info("adding manual ip to virtual ip table in Consul", "name", service.Name) - _, _, err = consulClient.Internal().AssignServiceVirtualIP(ctx, configEntry.KubernetesName(), []string{service.Spec.ClusterIP}, &capi.WriteOptions{}) + _, _, err := consulClient.Internal().AssignServiceVirtualIP(ctx, configEntry.KubernetesName(), []string{service.Spec.ClusterIP}, &capi.WriteOptions{}) if err != nil { // Maintain backwards compatibility with older versions of Consul that do not support the manual VIP improvements. With the older version, the mesh // will still work. diff --git a/control-plane/controllers/configentry_controller_test.go b/control-plane/controllers/configentry_controller_test.go index 71244baba3..02ea9d0c95 100644 --- a/control-plane/controllers/configentry_controller_test.go +++ b/control-plane/controllers/configentry_controller_test.go @@ -1931,8 +1931,8 @@ func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { expectErr: false, }, { - name: "ServiceRouter no error and vip should be assigned", - kubeKind: "ServiceRouter", + name: "ServiceResolver no error and vip should be assigned", + kubeKind: "ServiceResolver", consulKind: capi.ServiceRouter, consulPrereqs: []capi.ConfigEntry{ &capi.ServiceConfigEntry{ @@ -1985,6 +1985,61 @@ func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { }, expectErr: false, }, + { + name: "ServiceRouter no error and vip should be assigned", + kubeKind: "ServiceRouter", + consulKind: capi.ServiceRouter, + consulPrereqs: []capi.ConfigEntry{ + &capi.ServiceConfigEntry{ + Kind: capi.ServiceDefaults, + Name: "bar", + Protocol: "http", + }, + }, + configEntryResource: &v1alpha1.ServiceRouter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: kubeNS, + }, + Spec: v1alpha1.ServiceRouterSpec{ + Routes: []v1alpha1.ServiceRoute{ + { + Match: &v1alpha1.ServiceRouteMatch{ + HTTP: &v1alpha1.ServiceRouteHTTPMatch{ + PathPrefix: "/admin", + }, + }, + }, + }, + }, + }, + service: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: kubeNS, + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.2", + Ports: []corev1.ServicePort{ + { + Port: 8081, + }, + }, + }, + }, + reconciler: func(client client.Client, cfg *consul.Config, watcher consul.ServerConnectionManager, logger logr.Logger) Controller { + return &ServiceRouterController{ + Client: client, + Log: logger, + ConfigEntryController: &ConfigEntryController{ + ConsulClientConfig: cfg, + ConsulServerConnMgr: watcher, + DatacenterName: datacenterName, + }, + } + }, + expectErr: false, + }, { name: "ServiceRouter should fail because service does not have a valid IP address", kubeKind: "ServiceRouter", From a74d327e5f2e060107d90ae9be22afc25bd2b25c Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Tue, 16 May 2023 10:25:36 -0400 Subject: [PATCH 7/8] create method to check if a virtualIP needs to be assigned, remove extra code from tests --- .../controllers/configentry_controller.go | 25 +++++++------- .../configentry_controller_test.go | 33 +------------------ 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/control-plane/controllers/configentry_controller.go b/control-plane/controllers/configentry_controller.go index 0704a6aa23..9f3b98f219 100644 --- a/control-plane/controllers/configentry_controller.go +++ b/control-plane/controllers/configentry_controller.go @@ -270,9 +270,11 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont // For resolvers and splitters, we need to set the ClusterIP of the matching service to Consul so that transparent // proxy works correctly. Do not fail the reconcile if assigning the virtual IP returns an error. - err = assignServiceVirtualIP(ctx, logger, consulClient, crdCtrl, req.NamespacedName, configEntry) - if err != nil { - logger.Error(err, "failed assigning service virtual ip") + if needsVirtualIPAssignment(configEntry) { + err = assignServiceVirtualIP(ctx, logger, consulClient, crdCtrl, req.NamespacedName, configEntry) + if err != nil { + logger.Error(err, "failed assigning service virtual ip") + } } return ctrl.Result{}, nil @@ -389,26 +391,27 @@ func (r *ConfigEntryController) nonMatchingMigrationError(kubeEntry common.Confi return fmt.Errorf("migration failed: Kubernetes resource does not match existing Consul config entry: consul=%s, kube=%s", consulJSON, kubeJSON) } +// needsVirtualIPAssignment checks to see if a configEntry type needs to be assigned a virtual IP. +func needsVirtualIPAssignment(configEntry common.ConfigEntryResource) bool { + kubeKind := configEntry.KubeKind() + if kubeKind == common.ServiceResolver || kubeKind == common.ServiceRouter || kubeKind == common.ServiceSplitter { + return true + } + return false +} + // assignServiceVirtualIPs manually sends the ClusterIP for a matching service for ServiceRouter or ServiceSplitter // CRDs to Consul so that it can be added to the virtual IP table. The assignment is skipped if the matching service // does not exist or if an older version of Consul is being used. Endpoints Controller, on service registration, also // manually sends a ClusterIP when a service is created. This increases the chance of a real IP ending up in the // discovery chain. func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClient *capi.Client, crdCtrl Controller, namespacedName types.NamespacedName, configEntry common.ConfigEntryResource) error { - - // Only assign the virtual IP for service routers and splitters. Every other type is skipped. - kubeKind := configEntry.KubeKind() - if kubeKind != common.ServiceResolver && kubeKind != common.ServiceRouter && kubeKind != common.ServiceSplitter { - return nil - } - service := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: configEntry.KubernetesName(), Namespace: namespacedName.Namespace, }, } - if err := crdCtrl.Get(ctx, namespacedName, &service); err != nil { // It is non-fatal if the service does not exist. The ClusterIP will get added when the service is registered in // the endpoints controller diff --git a/control-plane/controllers/configentry_controller_test.go b/control-plane/controllers/configentry_controller_test.go index 02ea9d0c95..1ef6902e0a 100644 --- a/control-plane/controllers/configentry_controller_test.go +++ b/control-plane/controllers/configentry_controller_test.go @@ -1894,7 +1894,7 @@ func TestConfigEntryController_Migration(t *testing.T) { } func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { - /* t.Parallel() */ + t.Parallel() kubeNS := "default" cases := []struct { @@ -1907,40 +1907,10 @@ func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { reconciler func(client.Client, *consul.Config, consul.ServerConnectionManager, logr.Logger) Controller expectErr bool }{ - { - name: "ServiceDefaults should have no error because service defaults is not a supported vip CRD", - kubeKind: "ServiceDefaults", - consulKind: capi.ServiceDefaults, - configEntryResource: &v1alpha1.ServiceDefaults{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: kubeNS, - }, - }, - reconciler: func(client client.Client, cfg *consul.Config, watcher consul.ServerConnectionManager, logger logr.Logger) Controller { - return &ServiceDefaultsController{ - Client: client, - Log: logger, - ConfigEntryController: &ConfigEntryController{ - ConsulClientConfig: cfg, - ConsulServerConnMgr: watcher, - DatacenterName: datacenterName, - }, - } - }, - expectErr: false, - }, { name: "ServiceResolver no error and vip should be assigned", kubeKind: "ServiceResolver", consulKind: capi.ServiceRouter, - consulPrereqs: []capi.ConfigEntry{ - &capi.ServiceConfigEntry{ - Kind: capi.ServiceDefaults, - Name: "foo", - Protocol: "http", - }, - }, configEntryResource: &v1alpha1.ServiceRouter{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -2087,7 +2057,6 @@ func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { }, expectErr: true, }, - { name: "ServiceSplitter no error because a matching service does not exist", kubeKind: "ServiceSplitter", From c82e4c57d9717b7fb59c6acbdbb0bc5488b91a80 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Tue, 16 May 2023 12:57:08 -0400 Subject: [PATCH 8/8] Was missing namespace and partition --- .../controllers/endpoints/endpoints_controller.go | 3 ++- control-plane/controllers/configentry_controller.go | 11 ++++++++--- .../controllers/configentry_controller_test.go | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go index e9f4a62225..13f75f1156 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go @@ -1271,7 +1271,8 @@ func assignServiceVirtualIP(ctx context.Context, apiClient *api.Client, svc *api if ip == "" { return nil } - _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{ip}, &api.WriteOptions{}) + + _, _, err := apiClient.Internal().AssignServiceVirtualIP(ctx, svc.Service, []string{ip}, &api.WriteOptions{Namespace: svc.Namespace, Partition: svc.Partition}) if err != nil { // Maintain backwards compatibility with older versions of Consul that do not support the VIP improvements. Tproxy // will not work 100% correctly but the mesh will still work diff --git a/control-plane/controllers/configentry_controller.go b/control-plane/controllers/configentry_controller.go index 9f3b98f219..f35d2e88e7 100644 --- a/control-plane/controllers/configentry_controller.go +++ b/control-plane/controllers/configentry_controller.go @@ -271,7 +271,7 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont // For resolvers and splitters, we need to set the ClusterIP of the matching service to Consul so that transparent // proxy works correctly. Do not fail the reconcile if assigning the virtual IP returns an error. if needsVirtualIPAssignment(configEntry) { - err = assignServiceVirtualIP(ctx, logger, consulClient, crdCtrl, req.NamespacedName, configEntry) + err = assignServiceVirtualIP(ctx, logger, consulClient, crdCtrl, req.NamespacedName, configEntry, r.DatacenterName) if err != nil { logger.Error(err, "failed assigning service virtual ip") } @@ -405,7 +405,7 @@ func needsVirtualIPAssignment(configEntry common.ConfigEntryResource) bool { // does not exist or if an older version of Consul is being used. Endpoints Controller, on service registration, also // manually sends a ClusterIP when a service is created. This increases the chance of a real IP ending up in the // discovery chain. -func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClient *capi.Client, crdCtrl Controller, namespacedName types.NamespacedName, configEntry common.ConfigEntryResource) error { +func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClient *capi.Client, crdCtrl Controller, namespacedName types.NamespacedName, configEntry common.ConfigEntryResource, datacenter string) error { service := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: configEntry.KubernetesName(), @@ -422,8 +422,13 @@ func assignServiceVirtualIP(ctx context.Context, logger logr.Logger, consulClien return err } + wo := &capi.WriteOptions{ + Namespace: configEntry.ToConsul(datacenter).GetNamespace(), + Partition: configEntry.ToConsul(datacenter).GetPartition(), + } + logger.Info("adding manual ip to virtual ip table in Consul", "name", service.Name) - _, _, err := consulClient.Internal().AssignServiceVirtualIP(ctx, configEntry.KubernetesName(), []string{service.Spec.ClusterIP}, &capi.WriteOptions{}) + _, _, err := consulClient.Internal().AssignServiceVirtualIP(ctx, configEntry.KubernetesName(), []string{service.Spec.ClusterIP}, wo) if err != nil { // Maintain backwards compatibility with older versions of Consul that do not support the manual VIP improvements. With the older version, the mesh // will still work. diff --git a/control-plane/controllers/configentry_controller_test.go b/control-plane/controllers/configentry_controller_test.go index 1ef6902e0a..d548f78069 100644 --- a/control-plane/controllers/configentry_controller_test.go +++ b/control-plane/controllers/configentry_controller_test.go @@ -2115,7 +2115,7 @@ func TestConfigEntryControllers_assignServiceVirtualIP(t *testing.T) { Name: c.configEntryResource.KubernetesName(), } - err := assignServiceVirtualIP(ctx, ctrl.Logger(namespacedName), consulClient, ctrl, namespacedName, c.configEntryResource) + err := assignServiceVirtualIP(ctx, ctrl.Logger(namespacedName), consulClient, ctrl, namespacedName, c.configEntryResource, "dc1") if err != nil { require.True(t, c.expectErr) } else {