From 2df5566e9d77f869609e82a4ad332aa12f672685 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Mon, 6 May 2024 16:57:40 -0400 Subject: [PATCH 01/15] first pass at creating write policy for service and updating term gw acl role --- .../configentries/registrations_controller.go | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index c9137ff63a..1f927ca37b 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -5,6 +5,8 @@ package configentries import ( "context" + "fmt" + "strings" "time" "github.com/go-logr/logr" @@ -79,6 +81,15 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } + // if there is an ACL token then we can assume that `manageSystemACLs` has been set and we should handle + // the acl setup + if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { + err = r.updateTermGWACLRole(ctx, log, client, registration) + } + if err != nil { + return ctrl.Result{}, err + } + err = r.updateStatus(ctx, req.NamespacedName) if err != nil { log.Error(err, "failed to update status") @@ -127,6 +138,62 @@ func (r *RegistrationsController) deregisterService(ctx context.Context, log log return nil } +func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + roles, _, err := client.ACL().RoleList(nil) + if err != nil { + return err + } + + var role *capi.ACLRole + for _, r := range roles { + if strings.HasSuffix(r.Name, "terminating-gateway-acl-role") { + fmt.Printf("Role: %v\n", r) + role = r + break + } + } + + if role == nil { + log.Info("terminating gateway role not found") + return nil + } + + policy := &capi.ACLPolicy{ + Name: fmt.Sprintf("%s-write-policy", registration.Spec.Service.Name), + Description: "Write policy for terminating gateways for external service", + Rules: `service "zoidberg" { policy = "write" }`, + Datacenters: []string{registration.Spec.Datacenter}, + Namespace: registration.Spec.Service.Namespace, + Partition: registration.Spec.Service.Partition, + } + + existingPolicy, _, err := client.ACL().PolicyReadByName(policy.Name, nil) + if err != nil { + log.Error(err, "error reading policy") + return err + } + + if existingPolicy == nil { + policy, _, err = client.ACL().PolicyCreate(policy, nil) + if err != nil { + log.Error(err, "error creating policy") + return err + } + } else { + policy = existingPolicy + } + + role.Policies = append(role.Policies, &capi.ACLRolePolicyLink{Name: policy.Name, ID: policy.ID}) + + _, _, err = client.ACL().RoleUpdate(role, nil) + if err != nil { + log.Error(err, "error updating role") + return err + } + + return nil +} + func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { return r.Log.WithValues("request", name) } From 0a8e9b856b0bce6ab8ffef5a170dcaf379815ae8 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Tue, 7 May 2024 13:50:18 -0400 Subject: [PATCH 02/15] handle deregistering, update tests for registering with acls --- .../configentries/registrations_controller.go | 66 ++++- .../registrations_controller_test.go | 228 ++++++++++++++++-- 2 files changed, 270 insertions(+), 24 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 1f927ca37b..a1e9fc781b 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -5,7 +5,9 @@ package configentries import ( "context" + "errors" "fmt" + "slices" "strings" "time" @@ -66,9 +68,14 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques r.updateStatusError(ctx, registration, "ConsulErrorDeregistration", err) return ctrl.Result{}, err } - err := r.updateStatus(ctx, req.NamespacedName) + + // if there is an ACL token then we can assume that `manageSystemACLs` has been set and we should handle + // the acl setup + if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { + err = r.removeTermGWACLRole(log, client, registration) + } if err != nil { - log.Error(err, "failed to update status") + return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -84,7 +91,7 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques // if there is an ACL token then we can assume that `manageSystemACLs` has been set and we should handle // the acl setup if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { - err = r.updateTermGWACLRole(ctx, log, client, registration) + err = r.updateTermGWACLRole(log, client, registration) } if err != nil { return ctrl.Result{}, err @@ -138,7 +145,7 @@ func (r *RegistrationsController) deregisterService(ctx context.Context, log log return nil } -func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { +func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { roles, _, err := client.ACL().RoleList(nil) if err != nil { return err @@ -147,7 +154,6 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l var role *capi.ACLRole for _, r := range roles { if strings.HasSuffix(r.Name, "terminating-gateway-acl-role") { - fmt.Printf("Role: %v\n", r) role = r break } @@ -155,7 +161,7 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l if role == nil { log.Info("terminating gateway role not found") - return nil + return errors.New("terminating gateway role not found") } policy := &capi.ACLPolicy{ @@ -194,6 +200,54 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l return nil } +func (r *RegistrationsController) removeTermGWACLRole(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + roles, _, err := client.ACL().RoleList(nil) + if err != nil { + return err + } + + var role *capi.ACLRole + for _, r := range roles { + if strings.HasSuffix(r.Name, "terminating-gateway-acl-role") { + fmt.Printf("Role: %v\n", r) + role = r + break + } + } + + if role == nil { + return errors.New("terminating gateway role not found") + } + + var policyID string + + role.Policies = slices.DeleteFunc(role.Policies, func(i *capi.ACLRolePolicyLink) bool { + if i.Name == fmt.Sprintf("%s-write-policy", registration.Spec.Service.Name) { + policyID = i.ID + return true + } + return false + }) + + if policyID == "" { + return errors.New("policy not found") + } + + _, _, err = client.ACL().RoleUpdate(role, nil) + if err != nil { + log.Error(err, "error updating role") + return err + } + + _, err = client.ACL().PolicyDelete(policyID, nil) + if err != nil { + log.Error(err, "error deleting service policy") + return err + } + + return nil +} + func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { return r.Log.WithValues("request", name) } diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 5e2b2721fa..10e23c4815 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -5,6 +5,8 @@ package configentries_test import ( "context" + "encoding/json" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -24,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" capi "github.com/hashicorp/consul/api" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul-k8s/control-plane/consul" @@ -31,11 +34,17 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/helper/test" ) +type serverResponseConfig struct { + registering bool + aclEnabled bool +} + func TestReconcile_Success(tt *testing.T) { deletionTime := metav1.Now() cases := map[string]struct { - registration *v1alpha1.Registration - expectedConditions []v1alpha1.Condition + registration *v1alpha1.Registration + serverResponseConfig serverResponseConfig + expectedConditions []v1alpha1.Condition }{ "success on registration": { registration: &v1alpha1.Registration{ @@ -60,6 +69,41 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + serverResponseConfig: serverResponseConfig{registering: true}, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionTrue, + Reason: "", + Message: "", + }}, + }, + "success on registration -- ACLs enabled": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: true, + aclEnabled: true, + }, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionTrue, @@ -91,6 +135,40 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + serverResponseConfig: serverResponseConfig{ + registering: false, + aclEnabled: false, + }, + expectedConditions: []v1alpha1.Condition{}, + }, + "success on deregistration - ACLs enabled": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + DeletionTimestamp: &deletionTime, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: false, + aclEnabled: true, + }, expectedConditions: []v1alpha1.Condition{}, }, } @@ -103,23 +181,9 @@ func TestReconcile_Success(tt *testing.T) { s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) ctx := context.Background() - consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(200) - })) + consulServer, testClient := fakeConsulServer(t, tc.serverResponseConfig, tc.registration.Spec.Service.Name) defer consulServer.Close() - parsedURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) - host := strings.Split(parsedURL.Host, ":")[0] - - port, err := strconv.Atoi(parsedURL.Port()) - require.NoError(t, err) - - testClient := &test.TestServerClient{ - Cfg: &consul.Config{APIClientConfig: &capi.Config{Address: host}, HTTPPort: port}, - Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), - } - fakeClient := fake.NewClientBuilder(). WithScheme(s). WithRuntimeObjects(tc.registration). @@ -134,7 +198,7 @@ func TestReconcile_Success(tt *testing.T) { ConsulServerConnMgr: testClient.Watcher, } - _, err = controller.Reconcile(ctx, ctrl.Request{ + _, err := controller.Reconcile(ctx, ctrl.Request{ NamespacedName: types.NamespacedName{Name: tc.registration.Name, Namespace: tc.registration.Namespace}, }) require.NoError(t, err) @@ -279,3 +343,131 @@ func TestReconcile_Failure(tt *testing.T) { }) } } + +func fakeConsulServer(t *testing.T, serverResponseConfig serverResponseConfig, serviceName string) (*httptest.Server, *test.TestServerClient) { + t.Helper() + mux := buildMux(t, serverResponseConfig, serviceName) + consulServer := httptest.NewServer(mux) + + parsedURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + host := strings.Split(parsedURL.Host, ":")[0] + + port, err := strconv.Atoi(parsedURL.Port()) + require.NoError(t, err) + + cfg := &consul.Config{APIClientConfig: &capi.Config{Address: host}, HTTPPort: port} + if serverResponseConfig.aclEnabled { + cfg.APIClientConfig.Token = "test-token" + } + + testClient := &test.TestServerClient{ + Cfg: cfg, + Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), + } + + return consulServer, testClient +} + +func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.Handler { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/v1/catalog/register", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }) + + mux.HandleFunc("/v1/catalog/deregister", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }) + + policyID, err := uuid.GenerateUUID() + require.NoError(t, err) + + mux.HandleFunc("/v1/acl/roles", func(w http.ResponseWriter, r *http.Request) { + termGWPolicies := []*capi.ACLLink{ + { + ID: "b7e377d9-5e2b-b99c-3f06-139584cf47f8", + Name: "terminating-gateway-policy", + }, + } + + if !cfg.registering { + termGWPolicies = append(termGWPolicies, &capi.ACLLink{ + ID: policyID, + Name: fmt.Sprintf("%s-write-policy", serviceName), + }) + } + + entries := []*capi.ACLRole{ + { + ID: "754a8717-46e9-9f18-7f76-28dc0afafd19", + Name: "consul-consul-connect-inject-acl-role", + Description: "ACL Role for consul-consul-connect-injector", + Policies: []*capi.ACLLink{ + { + ID: "38511a9f-a309-11e2-7f67-7fea12056e7c", + Name: "connect-inject-policy", + }, + }, + }, + { + ID: "61fc5051-96e9-7b67-69b5-98f7f6682563", + Name: "consul-consul-terminating-gateway-acl-role", + Description: "ACL Role for consul-consul-terminating-gateway", + Policies: termGWPolicies, + }, + } + val, err := json.Marshal(entries) + if err != nil { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + w.Write(val) + }) + + mux.HandleFunc("/v1/acl/role/", func(w http.ResponseWriter, r *http.Request) { + role := &capi.ACLRole{ + ID: "61fc5051-96e9-7b67-69b5-98f7f6682563", + Name: "consul-consul-terminating-gateway-acl-role", + Description: "ACL Role for consul-consul-terminating-gateway", + Policies: []*capi.ACLLink{ + { + ID: "b7e377d9-5e2b-b99c-3f06-139584cf47f8", + Name: "terminating-gateway-policy", + }, + { + ID: policyID, + Name: fmt.Sprintf("%s-write-policy", serviceName), + }, + }, + } + val, err := json.Marshal(role) + if err != nil { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + w.Write(val) + }) + + mux.HandleFunc("/v1/acl/policy/name/", func(w http.ResponseWriter, r *http.Request) { + policy := &capi.ACLPolicy{ + ID: policyID, + Name: fmt.Sprintf("%s-write-policy", serviceName), + } + val, err := json.Marshal(policy) + if err != nil { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + w.Write(val) + }) + + mux.HandleFunc("/v1/acl/policy/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }) + + return mux +} From 3556f855842294be392e618e4d03105de34e19f1 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Tue, 7 May 2024 13:59:31 -0400 Subject: [PATCH 03/15] existing deregister tests passing --- .../registrations_controller_test.go | 58 +++++++++++-------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 10e23c4815..9eb60143b9 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -35,8 +35,10 @@ import ( ) type serverResponseConfig struct { - registering bool - aclEnabled bool + registering bool + aclEnabled bool + errOnRegister bool + errOnDeregister bool } func TestReconcile_Success(tt *testing.T) { @@ -46,7 +48,7 @@ func TestReconcile_Success(tt *testing.T) { serverResponseConfig serverResponseConfig expectedConditions []v1alpha1.Condition }{ - "success on registration": { + "registering - success on registration": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ Kind: "Registration", @@ -77,7 +79,7 @@ func TestReconcile_Success(tt *testing.T) { Message: "", }}, }, - "success on registration -- ACLs enabled": { + "registering -- ACLs enabled and policy does not exist": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ Kind: "Registration", @@ -111,7 +113,7 @@ func TestReconcile_Success(tt *testing.T) { Message: "", }}, }, - "success on deregistration": { + "deregistering": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ Kind: "Registration", @@ -141,7 +143,7 @@ func TestReconcile_Success(tt *testing.T) { }, expectedConditions: []v1alpha1.Condition{}, }, - "success on deregistration - ACLs enabled": { + "deregistering - ACLs enabled": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ Kind: "Registration", @@ -220,10 +222,11 @@ func TestReconcile_Success(tt *testing.T) { func TestReconcile_Failure(tt *testing.T) { deletionTime := metav1.Now() cases := map[string]struct { - registration *v1alpha1.Registration - expectedConditions []v1alpha1.Condition + registration *v1alpha1.Registration + serverResponseConfig serverResponseConfig + expectedConditions []v1alpha1.Condition }{ - "failure on registration": { + "registering - registration call to consul fails": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ Kind: "Registration", @@ -246,6 +249,10 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + serverResponseConfig: serverResponseConfig{ + registering: true, + errOnRegister: true, + }, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionFalse, @@ -253,6 +260,10 @@ func TestReconcile_Failure(tt *testing.T) { Message: "", }}, }, + //"registering - terminating gateway acl role not found": {}, + //"registering - error reading policy": {}, + //"registering - policy does not exist - error creating policy": {}, + //"registering - error updating role": {}, "failure on deregistration": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ @@ -277,6 +288,9 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + serverResponseConfig: serverResponseConfig{ + errOnDeregister: true, + }, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionFalse, @@ -294,23 +308,9 @@ func TestReconcile_Failure(tt *testing.T) { s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) ctx := context.Background() - consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(500) - })) + consulServer, testClient := fakeConsulServer(t, tc.serverResponseConfig, tc.registration.Spec.Service.Name) defer consulServer.Close() - parsedURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) - host := strings.Split(parsedURL.Host, ":")[0] - - port, err := strconv.Atoi(parsedURL.Port()) - require.NoError(t, err) - - testClient := &test.TestServerClient{ - Cfg: &consul.Config{APIClientConfig: &capi.Config{Address: host}, HTTPPort: port}, - Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), - } - fakeClient := fake.NewClientBuilder(). WithScheme(s). WithRuntimeObjects(tc.registration). @@ -325,7 +325,7 @@ func TestReconcile_Failure(tt *testing.T) { ConsulServerConnMgr: testClient.Watcher, } - _, err = controller.Reconcile(ctx, ctrl.Request{ + _, err := controller.Reconcile(ctx, ctrl.Request{ NamespacedName: types.NamespacedName{Name: tc.registration.Name, Namespace: tc.registration.Namespace}, }) require.Error(t, err) @@ -373,10 +373,18 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H t.Helper() mux := http.NewServeMux() mux.HandleFunc("/v1/catalog/register", func(w http.ResponseWriter, r *http.Request) { + if cfg.errOnRegister { + w.WriteHeader(500) + return + } w.WriteHeader(200) }) mux.HandleFunc("/v1/catalog/deregister", func(w http.ResponseWriter, r *http.Request) { + if cfg.errOnDeregister { + w.WriteHeader(500) + return + } w.WriteHeader(200) }) From 74603caa7c072965c13a1a1b6ff89d2afff15020 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Tue, 7 May 2024 15:53:05 -0400 Subject: [PATCH 04/15] failures with term gw role not existing --- .../configentries/registrations_controller.go | 7 +- .../registrations_controller_test.go | 96 ++++++++++++++----- 2 files changed, 77 insertions(+), 26 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index a1e9fc781b..7ed5795ff4 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -92,9 +92,10 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques // the acl setup if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { err = r.updateTermGWACLRole(log, client, registration) - } - if err != nil { - return ctrl.Result{}, err + if err != nil { + r.updateStatusError(ctx, registration, "ConsulErrorACL", err) + return ctrl.Result{}, err + } } err = r.updateStatus(ctx, req.NamespacedName) diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 9eb60143b9..581aa666d1 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -35,10 +35,11 @@ import ( ) type serverResponseConfig struct { - registering bool - aclEnabled bool - errOnRegister bool - errOnDeregister bool + registering bool + aclEnabled bool + errOnRegister bool + errOnDeregister bool + temGWRoleMissing bool } func TestReconcile_Success(tt *testing.T) { @@ -260,7 +261,41 @@ func TestReconcile_Failure(tt *testing.T) { Message: "", }}, }, - //"registering - terminating gateway acl role not found": {}, + "registering - terminating gateway acl role not found": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: true, + aclEnabled: true, + temGWRoleMissing: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: "ConsulErrorACL", + Message: "", + }}, + }, //"registering - error reading policy": {}, //"registering - policy does not exist - error creating policy": {}, //"registering - error updating role": {}, @@ -392,20 +427,6 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H require.NoError(t, err) mux.HandleFunc("/v1/acl/roles", func(w http.ResponseWriter, r *http.Request) { - termGWPolicies := []*capi.ACLLink{ - { - ID: "b7e377d9-5e2b-b99c-3f06-139584cf47f8", - Name: "terminating-gateway-policy", - }, - } - - if !cfg.registering { - termGWPolicies = append(termGWPolicies, &capi.ACLLink{ - ID: policyID, - Name: fmt.Sprintf("%s-write-policy", serviceName), - }) - } - entries := []*capi.ACLRole{ { ID: "754a8717-46e9-9f18-7f76-28dc0afafd19", @@ -418,13 +439,42 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H }, }, }, + } + + if cfg.temGWRoleMissing { + val, err := json.Marshal(entries) + if err != nil { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + w.Write(val) + return + } + + termGWPolicies := []*capi.ACLLink{ { - ID: "61fc5051-96e9-7b67-69b5-98f7f6682563", - Name: "consul-consul-terminating-gateway-acl-role", - Description: "ACL Role for consul-consul-terminating-gateway", - Policies: termGWPolicies, + ID: "b7e377d9-5e2b-b99c-3f06-139584cf47f8", + Name: "terminating-gateway-policy", }, } + + if !cfg.registering { + termGWPolicies = append(termGWPolicies, &capi.ACLLink{ + ID: policyID, + Name: fmt.Sprintf("%s-write-policy", serviceName), + }) + } + + termGWRole := &capi.ACLRole{ + ID: "61fc5051-96e9-7b67-69b5-98f7f6682563", + Name: "consul-consul-terminating-gateway-acl-role", + Description: "ACL Role for consul-consul-terminating-gateway", + Policies: termGWPolicies, + } + + entries = append(entries, termGWRole) + val, err := json.Marshal(entries) if err != nil { w.WriteHeader(500) From fc09192acbfd89a2a609735fe1623235a1f8426c Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 8 May 2024 12:11:31 -0400 Subject: [PATCH 05/15] clean up --- .../api/v1alpha1/registration_types.go | 8 + .../configentries/registrations_controller.go | 112 ++++--- .../registrations_controller_test.go | 282 +++++++++++++++++- 3 files changed, 339 insertions(+), 63 deletions(-) diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index 9ca58e7c23..c253f25aa6 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -13,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" ) func init() { @@ -289,6 +290,13 @@ func (r *Registration) ToCatalogDeregistration() *capi.CatalogDeregistration { } } +func (r *Registration) NamespacedName() types.NamespacedName { + return types.NamespacedName{ + Namespace: r.Namespace, + Name: r.Name, + } +} + // SetSyncedCondition sets the synced condition on the Registration. func (r *Registration) SetSyncedCondition(status corev1.ConditionStatus, reason string, message string) { r.Status.Conditions = Conditions{ diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 7ed5795ff4..db0a982526 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -28,14 +28,21 @@ import ( const RegistrationFinalizer = "registration.finalizers.consul.hashicorp.com" +// Status Reasons +const ( + ConsulErrorRegistration = "ConsulErrorRegistration" + ConsulErrorDeregistration = "ConsulErrorDeregistration" + ConsulErrorACL = "ConsulErrorACL" +) + // RegistrationsController is the controller for Registrations resources. type RegistrationsController struct { client.Client FinalizerPatcher - Log logr.Logger Scheme *runtime.Scheme ConsulClientConfig *consul.Config ConsulServerConnMgr consul.ServerConnectionManager + Log logr.Logger } // +kubebuilder:rbac:groups=consul.hashicorp.com,resources=servicerouters,verbs=get;list;watch;create;update;patch;delete @@ -62,57 +69,66 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques // deletion request if !registration.ObjectMeta.DeletionTimestamp.IsZero() { - log.Info("Deregistering service") - err = r.deregisterService(ctx, log, client, registration) - if err != nil { - r.updateStatusError(ctx, registration, "ConsulErrorDeregistration", err) - return ctrl.Result{}, err - } - - // if there is an ACL token then we can assume that `manageSystemACLs` has been set and we should handle - // the acl setup - if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { - err = r.removeTermGWACLRole(log, client, registration) - } + err := r.handleDeletion(ctx, log, client, registration) if err != nil { return ctrl.Result{}, err } - return ctrl.Result{}, nil } - log.Info("Registering service") - err = r.registerService(ctx, log, client, registration) + // registration request + err = r.handleRegistration(ctx, log, client, registration) if err != nil { - r.updateStatusError(ctx, registration, "ConsulErrorRegistration", err) return ctrl.Result{}, err } + return ctrl.Result{}, nil +} - // if there is an ACL token then we can assume that `manageSystemACLs` has been set and we should handle - // the acl setup +func (r *RegistrationsController) handleDeletion(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + log.Info("Deregistering service") + err := r.deregisterService(log, client, registration) + if err != nil { + r.updateStatusError(ctx, log, registration, ConsulErrorDeregistration, err) + return err + } if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { - err = r.updateTermGWACLRole(log, client, registration) + err = r.removeTermGWACLRole(log, client, registration) if err != nil { - r.updateStatusError(ctx, registration, "ConsulErrorACL", err) - return ctrl.Result{}, err + r.updateStatusError(ctx, log, registration, ConsulErrorACL, err) + return err } } - - err = r.updateStatus(ctx, req.NamespacedName) + patch := r.RemoveFinalizersPatch(registration, RegistrationFinalizer) + err = r.Patch(ctx, registration, patch) if err != nil { - log.Error(err, "failed to update status") + return err } - return ctrl.Result{}, err -} -func (r *RegistrationsController) registerService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - patch := r.AddFinalizersPatch(registration, RegistrationFinalizer) + return nil +} - err := r.Patch(ctx, registration, patch) +func (r *RegistrationsController) handleRegistration(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + log.Info("Registering service") + err := r.registerService(log, client, registration) + if err != nil { + r.updateStatusError(ctx, log, registration, ConsulErrorRegistration, err) + return err + } + if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { + err = r.updateTermGWACLRole(log, client, registration) + if err != nil { + r.updateStatusError(ctx, log, registration, ConsulErrorACL, err) + return err + } + } + err = r.updateStatus(ctx, log, registration.NamespacedName()) if err != nil { return err } + return nil +} +func (r *RegistrationsController) registerService(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { regReq, err := registration.ToCatalogRegistration() if err != nil { return err @@ -128,7 +144,7 @@ func (r *RegistrationsController) registerService(ctx context.Context, log logr. return nil } -func (r *RegistrationsController) deregisterService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { +func (r *RegistrationsController) deregisterService(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { deRegReq := registration.ToCatalogDeregistration() _, err := client.Catalog().Deregister(deRegReq, nil) @@ -137,11 +153,6 @@ func (r *RegistrationsController) deregisterService(ctx context.Context, log log return err } - patch := r.RemoveFinalizersPatch(registration, RegistrationFinalizer) - - if err := r.Patch(ctx, registration, patch); err != nil { - return err - } log.Info("Successfully deregistered service", "svcID", deRegReq.ServiceID) return nil } @@ -166,7 +177,7 @@ func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *c } policy := &capi.ACLPolicy{ - Name: fmt.Sprintf("%s-write-policy", registration.Spec.Service.Name), + Name: servicePolicyName(registration.Spec.Service.Name), Description: "Write policy for terminating gateways for external service", Rules: `service "zoidberg" { policy = "write" }`, Datacenters: []string{registration.Spec.Datacenter}, @@ -180,11 +191,11 @@ func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *c return err } - if existingPolicy == nil { + // existingPolicy will never be nil beause of how PolicyReadByName works so we need to check if the ID is empty + if existingPolicy.ID == "" { policy, _, err = client.ACL().PolicyCreate(policy, nil) if err != nil { - log.Error(err, "error creating policy") - return err + return fmt.Errorf("error creating policy: %w", err) } } else { policy = existingPolicy @@ -217,13 +228,15 @@ func (r *RegistrationsController) removeTermGWACLRole(log logr.Logger, client *c } if role == nil { - return errors.New("terminating gateway role not found") + log.Info("terminating gateway role not found") + return nil } var policyID string + expectedPolicyName := servicePolicyName(registration.Spec.Service.Name) role.Policies = slices.DeleteFunc(role.Policies, func(i *capi.ACLRolePolicyLink) bool { - if i.Name == fmt.Sprintf("%s-write-policy", registration.Spec.Service.Name) { + if i.Name == expectedPolicyName { policyID = i.ID return true } @@ -231,7 +244,8 @@ func (r *RegistrationsController) removeTermGWACLRole(log logr.Logger, client *c }) if policyID == "" { - return errors.New("policy not found") + log.Info("policy not found on terminating gateway role", "policyName", expectedPolicyName) + return nil } _, _, err = client.ACL().RoleUpdate(role, nil) @@ -253,17 +267,17 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger return r.Log.WithValues("request", name) } -func (r *RegistrationsController) updateStatusError(ctx context.Context, registration *v1alpha1.Registration, reason string, reconcileErr error) { +func (r *RegistrationsController) updateStatusError(ctx context.Context, log logr.Logger, registration *v1alpha1.Registration, reason string, reconcileErr error) { registration.SetSyncedCondition(corev1.ConditionFalse, reason, reconcileErr.Error()) registration.Status.LastSyncedTime = &metav1.Time{Time: time.Now()} err := r.Status().Update(ctx, registration) if err != nil { - r.Log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) + log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) } } -func (r *RegistrationsController) updateStatus(ctx context.Context, req types.NamespacedName) error { +func (r *RegistrationsController) updateStatus(ctx context.Context, log logr.Logger, req types.NamespacedName) error { registration := &v1alpha1.Registration{} err := r.Get(ctx, req, registration) @@ -276,7 +290,7 @@ func (r *RegistrationsController) updateStatus(ctx context.Context, req types.Na err = r.Status().Update(ctx, registration) if err != nil { - r.Log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) + log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) return err } return nil @@ -285,3 +299,7 @@ func (r *RegistrationsController) updateStatus(ctx context.Context, req types.Na func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { return setupWithManager(mgr, &v1alpha1.Registration{}, r) } + +func servicePolicyName(name string) string { + return fmt.Sprintf("%s-write-policy", name) +} diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 581aa666d1..ceeaf2dd1f 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -35,11 +35,16 @@ import ( ) type serverResponseConfig struct { - registering bool - aclEnabled bool - errOnRegister bool - errOnDeregister bool - temGWRoleMissing bool + registering bool + aclEnabled bool + errOnRegister bool + errOnDeregister bool + errOnPolicyRead bool + errOnPolicyWrite bool + errOnPolicyDelete bool + errOnRoleUpdate bool + policyExists bool + temGWRoleMissing bool } func TestReconcile_Success(tt *testing.T) { @@ -114,6 +119,41 @@ func TestReconcile_Success(tt *testing.T) { Message: "", }}, }, + "registering -- ACLs enabled and policy does exists": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: true, + aclEnabled: true, + policyExists: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionTrue, + Reason: "", + Message: "", + }}, + }, "deregistering": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ @@ -257,7 +297,7 @@ func TestReconcile_Failure(tt *testing.T) { expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionFalse, - Reason: "ConsulErrorRegistration", + Reason: configentries.ConsulErrorRegistration, Message: "", }}, }, @@ -292,14 +332,117 @@ func TestReconcile_Failure(tt *testing.T) { expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionFalse, - Reason: "ConsulErrorACL", + Reason: configentries.ConsulErrorACL, + Message: "", + }}, + }, + "registering - error reading policy": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: true, + aclEnabled: true, + errOnPolicyRead: true, + policyExists: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: configentries.ConsulErrorACL, + Message: "", + }}, + }, + "registering - policy does not exist - error creating policy": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: true, + aclEnabled: true, + errOnPolicyWrite: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: configentries.ConsulErrorACL, Message: "", }}, }, - //"registering - error reading policy": {}, - //"registering - policy does not exist - error creating policy": {}, - //"registering - error updating role": {}, - "failure on deregistration": { + "registering - error updating role": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + registering: true, + aclEnabled: true, + errOnRoleUpdate: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: configentries.ConsulErrorACL, + Message: "", + }}, + }, + "deregistering": { registration: &v1alpha1.Registration{ TypeMeta: metav1.TypeMeta{ Kind: "Registration", @@ -329,7 +472,77 @@ func TestReconcile_Failure(tt *testing.T) { expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionFalse, - Reason: "ConsulErrorDeregistration", + Reason: configentries.ConsulErrorDeregistration, + Message: "", + }}, + }, + "deregistering - ACLs enabled - terminating-gateway error updating role": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + DeletionTimestamp: &deletionTime, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + aclEnabled: true, + errOnRoleUpdate: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: configentries.ConsulErrorACL, + Message: "", + }}, + }, + "deregistering - ACLs enabled - terminating-gateway error deleting policy": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + DeletionTimestamp: &deletionTime, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + serverResponseConfig: serverResponseConfig{ + aclEnabled: true, + errOnPolicyDelete: true, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: configentries.ConsulErrorACL, Message: "", }}, }, @@ -485,6 +698,11 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H }) mux.HandleFunc("/v1/acl/role/", func(w http.ResponseWriter, r *http.Request) { + if cfg.errOnRoleUpdate { + w.WriteHeader(500) + return + } + role := &capi.ACLRole{ ID: "61fc5051-96e9-7b67-69b5-98f7f6682563", Name: "consul-consul-terminating-gateway-acl-role", @@ -510,10 +728,17 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H }) mux.HandleFunc("/v1/acl/policy/name/", func(w http.ResponseWriter, r *http.Request) { - policy := &capi.ACLPolicy{ - ID: policyID, - Name: fmt.Sprintf("%s-write-policy", serviceName), + if cfg.errOnPolicyRead { + w.WriteHeader(500) + return } + + policy := &capi.ACLPolicy{} + if cfg.policyExists { + policy.ID = policyID + policy.Name = fmt.Sprintf("%s-write-policy", serviceName) + } + val, err := json.Marshal(policy) if err != nil { w.WriteHeader(500) @@ -524,7 +749,32 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H }) mux.HandleFunc("/v1/acl/policy/", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(200) + switch r.Method { + case "GET": + if cfg.errOnPolicyWrite { + w.WriteHeader(500) + return + } + + policy := &capi.ACLPolicy{ + ID: policyID, + Name: fmt.Sprintf("%s-write-policy", serviceName), + } + + val, err := json.Marshal(policy) + if err != nil { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + w.Write(val) + case "DELETE": + if cfg.errOnPolicyDelete { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + } }) return mux From 015353e0f5aaa42ac46c862c2647a93e2f815f3f Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 8 May 2024 12:12:42 -0400 Subject: [PATCH 06/15] reorg code --- .../configentries/registrations_controller.go | 84 +++++++++---------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index db0a982526..31ca5ee1ff 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -84,29 +84,6 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } -func (r *RegistrationsController) handleDeletion(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - log.Info("Deregistering service") - err := r.deregisterService(log, client, registration) - if err != nil { - r.updateStatusError(ctx, log, registration, ConsulErrorDeregistration, err) - return err - } - if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { - err = r.removeTermGWACLRole(log, client, registration) - if err != nil { - r.updateStatusError(ctx, log, registration, ConsulErrorACL, err) - return err - } - } - patch := r.RemoveFinalizersPatch(registration, RegistrationFinalizer) - err = r.Patch(ctx, registration, patch) - if err != nil { - return err - } - - return nil -} - func (r *RegistrationsController) handleRegistration(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { log.Info("Registering service") err := r.registerService(log, client, registration) @@ -144,19 +121,6 @@ func (r *RegistrationsController) registerService(log logr.Logger, client *capi. return nil } -func (r *RegistrationsController) deregisterService(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - deRegReq := registration.ToCatalogDeregistration() - - _, err := client.Catalog().Deregister(deRegReq, nil) - if err != nil { - log.Error(err, "error deregistering service", "svcID", deRegReq.ServiceID) - return err - } - - log.Info("Successfully deregistered service", "svcID", deRegReq.ServiceID) - return nil -} - func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { roles, _, err := client.ACL().RoleList(nil) if err != nil { @@ -212,6 +176,42 @@ func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *c return nil } +func (r *RegistrationsController) handleDeletion(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + log.Info("Deregistering service") + err := r.deregisterService(log, client, registration) + if err != nil { + r.updateStatusError(ctx, log, registration, ConsulErrorDeregistration, err) + return err + } + if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { + err = r.removeTermGWACLRole(log, client, registration) + if err != nil { + r.updateStatusError(ctx, log, registration, ConsulErrorACL, err) + return err + } + } + patch := r.RemoveFinalizersPatch(registration, RegistrationFinalizer) + err = r.Patch(ctx, registration, patch) + if err != nil { + return err + } + + return nil +} + +func (r *RegistrationsController) deregisterService(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + deRegReq := registration.ToCatalogDeregistration() + + _, err := client.Catalog().Deregister(deRegReq, nil) + if err != nil { + log.Error(err, "error deregistering service", "svcID", deRegReq.ServiceID) + return err + } + + log.Info("Successfully deregistered service", "svcID", deRegReq.ServiceID) + return nil +} + func (r *RegistrationsController) removeTermGWACLRole(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { roles, _, err := client.ACL().RoleList(nil) if err != nil { @@ -263,8 +263,8 @@ func (r *RegistrationsController) removeTermGWACLRole(log logr.Logger, client *c return nil } -func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { - return r.Log.WithValues("request", name) +func servicePolicyName(name string) string { + return fmt.Sprintf("%s-write-policy", name) } func (r *RegistrationsController) updateStatusError(ctx context.Context, log logr.Logger, registration *v1alpha1.Registration, reason string, reconcileErr error) { @@ -296,10 +296,10 @@ func (r *RegistrationsController) updateStatus(ctx context.Context, log logr.Log return nil } -func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { - return setupWithManager(mgr, &v1alpha1.Registration{}, r) +func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { + return r.Log.WithValues("request", name) } -func servicePolicyName(name string) string { - return fmt.Sprintf("%s-write-policy", name) +func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { + return setupWithManager(mgr, &v1alpha1.Registration{}, r) } From ed4ae69f6193545f25f5949edf3159635026f7f4 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 8 May 2024 15:59:33 -0400 Subject: [PATCH 07/15] Move to own package --- control-plane/registrations/index.go | 1 + .../registrations_controller.go | 7 ++++--- .../registrations_controller_test.go | 2 +- control-plane/subcommand/inject-connect/v1controllers.go | 3 ++- 4 files changed, 8 insertions(+), 5 deletions(-) create mode 100644 control-plane/registrations/index.go rename control-plane/{controllers/configentries => registrations}/registrations_controller.go (97%) rename control-plane/{controllers/configentries => registrations}/registrations_controller_test.go (99%) diff --git a/control-plane/registrations/index.go b/control-plane/registrations/index.go new file mode 100644 index 0000000000..fdd464d2e5 --- /dev/null +++ b/control-plane/registrations/index.go @@ -0,0 +1 @@ +package registrations diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/registrations/registrations_controller.go similarity index 97% rename from control-plane/controllers/configentries/registrations_controller.go rename to control-plane/registrations/registrations_controller.go index 31ca5ee1ff..b94579b363 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/registrations/registrations_controller.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -package configentries +package registrations import ( "context" @@ -24,6 +24,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul-k8s/control-plane/consul" + "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" ) const RegistrationFinalizer = "registration.finalizers.consul.hashicorp.com" @@ -38,7 +39,7 @@ const ( // RegistrationsController is the controller for Registrations resources. type RegistrationsController struct { client.Client - FinalizerPatcher + configentries.FinalizerPatcher Scheme *runtime.Scheme ConsulClientConfig *consul.Config ConsulServerConnMgr consul.ServerConnectionManager @@ -301,5 +302,5 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger } func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { - return setupWithManager(mgr, &v1alpha1.Registration{}, r) + return ctrl.NewControllerManagedBy(mgr).For(&v1alpha1.Registration{}).Complete(r) } diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/registrations/registrations_controller_test.go similarity index 99% rename from control-plane/controllers/configentries/registrations_controller_test.go rename to control-plane/registrations/registrations_controller_test.go index ceeaf2dd1f..a8d6562709 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/registrations/registrations_controller_test.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -package configentries_test +package registrations_test import ( "context" diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go index 4b1de1e325..8e04fef8ce 100644 --- a/control-plane/subcommand/inject-connect/v1controllers.go +++ b/control-plane/subcommand/inject-connect/v1controllers.go @@ -23,6 +23,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/connect-inject/webhook" controllers "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" webhookconfiguration "github.com/hashicorp/consul-k8s/control-plane/helper/webhook-configuration" + "github.com/hashicorp/consul-k8s/control-plane/registrations" "github.com/hashicorp/consul-k8s/control-plane/subcommand/flags" ) @@ -284,7 +285,7 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage return err } - if err := (&controllers.RegistrationsController{ + if err := (®istrations.RegistrationsController{ Client: mgr.GetClient(), ConsulClientConfig: consulConfig, ConsulServerConnMgr: watcher, From 391148f798efff0ebdce26890d667963fc5640d3 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 8 May 2024 16:47:54 -0400 Subject: [PATCH 08/15] watch for terminating gateways --- .../registrations/registrations_controller.go | 77 ++++++++++++++----- 1 file changed, 59 insertions(+), 18 deletions(-) diff --git a/control-plane/registrations/registrations_controller.go b/control-plane/registrations/registrations_controller.go index b94579b363..5b4eddf171 100644 --- a/control-plane/registrations/registrations_controller.go +++ b/control-plane/registrations/registrations_controller.go @@ -19,6 +19,8 @@ import ( "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" capi "github.com/hashicorp/consul/api" @@ -93,7 +95,7 @@ func (r *RegistrationsController) handleRegistration(ctx context.Context, log lo return err } if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { - err = r.updateTermGWACLRole(log, client, registration) + err = r.updateTermGWACLRole(ctx, log, client, registration) if err != nil { r.updateStatusError(ctx, log, registration, ConsulErrorACL, err) return err @@ -122,23 +124,30 @@ func (r *RegistrationsController) registerService(log logr.Logger, client *capi. return nil } -func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - roles, _, err := client.ACL().RoleList(nil) +func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + termGWList := &v1alpha1.TerminatingGatewayList{} + err := r.Client.List(ctx, termGWList) if err != nil { return err } - var role *capi.ACLRole - for _, r := range roles { - if strings.HasSuffix(r.Name, "terminating-gateway-acl-role") { - role = r - break + termGWsToUpdate := make([]v1alpha1.TerminatingGateway, 0, len(termGWList.Items)) + for _, termGW := range termGWList.Items { + for _, svc := range termGW.Spec.Services { + if svc.Name == registration.Spec.Service.Name { + termGWsToUpdate = append(termGWsToUpdate, termGW) + } } } - if role == nil { - log.Info("terminating gateway role not found") - return errors.New("terminating gateway role not found") + if len(termGWsToUpdate) == 0 { + log.Info("terminating gateway not found") + return nil + } + + roles, _, err := client.ACL().RoleList(nil) + if err != nil { + return err } policy := &capi.ACLPolicy{ @@ -157,7 +166,7 @@ func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *c } // existingPolicy will never be nil beause of how PolicyReadByName works so we need to check if the ID is empty - if existingPolicy.ID == "" { + if existingPolicy == nil { policy, _, err = client.ACL().PolicyCreate(policy, nil) if err != nil { return fmt.Errorf("error creating policy: %w", err) @@ -166,12 +175,27 @@ func (r *RegistrationsController) updateTermGWACLRole(log logr.Logger, client *c policy = existingPolicy } - role.Policies = append(role.Policies, &capi.ACLRolePolicyLink{Name: policy.Name, ID: policy.ID}) + for _, termGW := range termGWsToUpdate { + var role *capi.ACLRole + for _, r := range roles { + if strings.HasSuffix(r.Name, fmt.Sprintf("-%s-acl-role", termGW.Name)) { + role = r + break + } + } - _, _, err = client.ACL().RoleUpdate(role, nil) - if err != nil { - log.Error(err, "error updating role") - return err + if role == nil { + log.Info("terminating gateway role not found") + return errors.New("terminating gateway role not found") + } + + role.Policies = append(role.Policies, &capi.ACLRolePolicyLink{Name: policy.Name, ID: policy.ID}) + + _, _, err = client.ACL().RoleUpdate(role, nil) + if err != nil { + log.Error(err, "error updating role") + return err + } } return nil @@ -302,5 +326,22 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger } func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr).For(&v1alpha1.Registration{}).Complete(r) + return ctrl.NewControllerManagedBy(mgr). + For(&v1alpha1.Registration{}). + Watches(&v1alpha1.TerminatingGateway{}, handler.EnqueueRequestsFromMapFunc(r.transformTerminatingGateways)). + Complete(r) +} + +func (r *RegistrationsController) transformTerminatingGateways(ctx context.Context, o client.Object) []reconcile.Request { + termGW := o.(*v1alpha1.TerminatingGateway) + reqs := make([]reconcile.Request, 0, len(termGW.Spec.Services)) + for _, svc := range termGW.Spec.Services { + reqs = append(reqs, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: svc.Name, + Namespace: svc.Namespace, + }, + }) + } + return reqs } From adde44beca19463c9161a2bed087de71794dac2a Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 9 May 2024 13:43:48 -0400 Subject: [PATCH 09/15] move files back, handle multiple terminating gateways --- .../registrations_controller.go | 103 +++++---- .../registrations_controller_test.go | 210 +++++++++++++++++- 2 files changed, 263 insertions(+), 50 deletions(-) rename control-plane/{registrations => controllers/configentries}/registrations_controller.go (80%) rename control-plane/{registrations => controllers/configentries}/registrations_controller_test.go (81%) diff --git a/control-plane/registrations/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go similarity index 80% rename from control-plane/registrations/registrations_controller.go rename to control-plane/controllers/configentries/registrations_controller.go index 5b4eddf171..79f641da7c 100644 --- a/control-plane/registrations/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -package registrations +package configentries import ( "context" @@ -26,7 +26,6 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul-k8s/control-plane/consul" - "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" ) const RegistrationFinalizer = "registration.finalizers.consul.hashicorp.com" @@ -41,7 +40,7 @@ const ( // RegistrationsController is the controller for Registrations resources. type RegistrationsController struct { client.Client - configentries.FinalizerPatcher + FinalizerPatcher Scheme *runtime.Scheme ConsulClientConfig *consul.Config ConsulServerConnMgr consul.ServerConnectionManager @@ -124,6 +123,12 @@ func (r *RegistrationsController) registerService(log logr.Logger, client *capi. return nil } +func termGWContainsService(registration *v1alpha1.Registration) func(v1alpha1.LinkedService) bool { + return func(svc v1alpha1.LinkedService) bool { + return svc.Name == registration.Spec.Service.Name + } +} + func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { termGWList := &v1alpha1.TerminatingGatewayList{} err := r.Client.List(ctx, termGWList) @@ -133,10 +138,8 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l termGWsToUpdate := make([]v1alpha1.TerminatingGateway, 0, len(termGWList.Items)) for _, termGW := range termGWList.Items { - for _, svc := range termGW.Spec.Services { - if svc.Name == registration.Spec.Service.Name { - termGWsToUpdate = append(termGWsToUpdate, termGW) - } + if slices.ContainsFunc(termGW.Spec.Services, termGWContainsService(registration)) { + termGWsToUpdate = append(termGWsToUpdate, termGW) } } @@ -153,7 +156,7 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l policy := &capi.ACLPolicy{ Name: servicePolicyName(registration.Spec.Service.Name), Description: "Write policy for terminating gateways for external service", - Rules: `service "zoidberg" { policy = "write" }`, + Rules: fmt.Sprintf(`service %q { policy = "write" }`, registration.Spec.Service.Name), Datacenters: []string{registration.Spec.Datacenter}, Namespace: registration.Spec.Service.Namespace, Partition: registration.Spec.Service.Partition, @@ -165,7 +168,6 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l return err } - // existingPolicy will never be nil beause of how PolicyReadByName works so we need to check if the ID is empty if existingPolicy == nil { policy, _, err = client.ACL().PolicyCreate(policy, nil) if err != nil { @@ -209,7 +211,7 @@ func (r *RegistrationsController) handleDeletion(ctx context.Context, log logr.L return err } if r.ConsulClientConfig.APIClientConfig.Token != "" || r.ConsulClientConfig.APIClientConfig.TokenFile != "" { - err = r.removeTermGWACLRole(log, client, registration) + err = r.removeTermGWACLRole(ctx, log, client, registration) if err != nil { r.updateStatusError(ctx, log, registration, ConsulErrorACL, err) return err @@ -237,52 +239,71 @@ func (r *RegistrationsController) deregisterService(log logr.Logger, client *cap return nil } -func (r *RegistrationsController) removeTermGWACLRole(log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - roles, _, err := client.ACL().RoleList(nil) +func (r *RegistrationsController) removeTermGWACLRole(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + termGWList := &v1alpha1.TerminatingGatewayList{} + err := r.Client.List(ctx, termGWList) if err != nil { return err } - var role *capi.ACLRole - for _, r := range roles { - if strings.HasSuffix(r.Name, "terminating-gateway-acl-role") { - fmt.Printf("Role: %v\n", r) - role = r - break + termGWsToUpdate := make([]v1alpha1.TerminatingGateway, 0, len(termGWList.Items)) + for _, termGW := range termGWList.Items { + if slices.ContainsFunc(termGW.Spec.Services, termGWContainsService(registration)) { + termGWsToUpdate = append(termGWsToUpdate, termGW) } } - if role == nil { - log.Info("terminating gateway role not found") + if len(termGWsToUpdate) == 0 { + log.Info("terminating gateway not found") return nil } - var policyID string + roles, _, err := client.ACL().RoleList(nil) + if err != nil { + return err + } - expectedPolicyName := servicePolicyName(registration.Spec.Service.Name) - role.Policies = slices.DeleteFunc(role.Policies, func(i *capi.ACLRolePolicyLink) bool { - if i.Name == expectedPolicyName { - policyID = i.ID - return true + for _, termGW := range termGWsToUpdate { + var role *capi.ACLRole + for _, r := range roles { + if strings.HasSuffix(r.Name, fmt.Sprintf("-%s-acl-role", termGW.Name)) { + role = r + break + } } - return false - }) - if policyID == "" { - log.Info("policy not found on terminating gateway role", "policyName", expectedPolicyName) - return nil - } + if role == nil { + log.Info("terminating gateway role not found") + return errors.New("terminating gateway role not found") + } - _, _, err = client.ACL().RoleUpdate(role, nil) - if err != nil { - log.Error(err, "error updating role") - return err - } + var policyID string - _, err = client.ACL().PolicyDelete(policyID, nil) - if err != nil { - log.Error(err, "error deleting service policy") - return err + expectedPolicyName := servicePolicyName(registration.Spec.Service.Name) + role.Policies = slices.DeleteFunc(role.Policies, func(i *capi.ACLRolePolicyLink) bool { + if i.Name == expectedPolicyName { + policyID = i.ID + return true + } + return false + }) + + if policyID == "" { + log.Info("policy not found on terminating gateway role", "policyName", expectedPolicyName) + return nil + } + + _, _, err = client.ACL().RoleUpdate(role, nil) + if err != nil { + log.Error(err, "error updating role") + return err + } + + _, err = client.ACL().PolicyDelete(policyID, nil) + if err != nil { + log.Error(err, "error deleting service policy") + return err + } } return nil diff --git a/control-plane/registrations/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go similarity index 81% rename from control-plane/registrations/registrations_controller_test.go rename to control-plane/controllers/configentries/registrations_controller_test.go index a8d6562709..fa8bfcd457 100644 --- a/control-plane/registrations/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -package registrations_test +package configentries_test import ( "context" @@ -51,6 +51,7 @@ func TestReconcile_Success(tt *testing.T) { deletionTime := metav1.Now() cases := map[string]struct { registration *v1alpha1.Registration + terminatingGateways []runtime.Object serverResponseConfig serverResponseConfig expectedConditions []v1alpha1.Condition }{ @@ -77,6 +78,20 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{registering: true}, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", @@ -108,6 +123,20 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, aclEnabled: true, @@ -142,6 +171,20 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, aclEnabled: true, @@ -178,6 +221,20 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: false, aclEnabled: false, @@ -208,6 +265,20 @@ func TestReconcile_Success(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: false, aclEnabled: true, @@ -221,15 +292,17 @@ func TestReconcile_Success(tt *testing.T) { tt.Run(name, func(t *testing.T) { t.Parallel() s := runtime.NewScheme() - s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) + s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}, &v1alpha1.TerminatingGateway{}, &v1alpha1.TerminatingGatewayList{}) ctx := context.Background() consulServer, testClient := fakeConsulServer(t, tc.serverResponseConfig, tc.registration.Spec.Service.Name) defer consulServer.Close() + runtimeObjs := []runtime.Object{tc.registration} + runtimeObjs = append(runtimeObjs, tc.terminatingGateways...) fakeClient := fake.NewClientBuilder(). WithScheme(s). - WithRuntimeObjects(tc.registration). + WithRuntimeObjects(runtimeObjs...). WithStatusSubresource(&v1alpha1.Registration{}). Build() @@ -264,6 +337,7 @@ func TestReconcile_Failure(tt *testing.T) { deletionTime := metav1.Now() cases := map[string]struct { registration *v1alpha1.Registration + terminatingGateways []runtime.Object serverResponseConfig serverResponseConfig expectedConditions []v1alpha1.Condition }{ @@ -290,6 +364,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, errOnRegister: true, @@ -324,6 +412,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, aclEnabled: true, @@ -359,6 +461,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, aclEnabled: true, @@ -395,6 +511,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, aclEnabled: true, @@ -430,6 +560,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ registering: true, aclEnabled: true, @@ -466,6 +610,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ errOnDeregister: true, }, @@ -500,6 +658,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ aclEnabled: true, errOnRoleUpdate: true, @@ -535,6 +707,20 @@ func TestReconcile_Failure(tt *testing.T) { }, }, }, + terminatingGateways: []runtime.Object{ + &v1alpha1.TerminatingGateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "terminating-gateway", + }, + Spec: v1alpha1.TerminatingGatewaySpec{ + Services: []v1alpha1.LinkedService{ + { + Name: "service-name", + }, + }, + }, + }, + }, serverResponseConfig: serverResponseConfig{ aclEnabled: true, errOnPolicyDelete: true, @@ -553,15 +739,17 @@ func TestReconcile_Failure(tt *testing.T) { tt.Run(name, func(t *testing.T) { t.Parallel() s := runtime.NewScheme() - s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) + s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}, &v1alpha1.TerminatingGateway{}, &v1alpha1.TerminatingGatewayList{}) ctx := context.Background() consulServer, testClient := fakeConsulServer(t, tc.serverResponseConfig, tc.registration.Spec.Service.Name) defer consulServer.Close() + runtimeObjs := []runtime.Object{tc.registration} + runtimeObjs = append(runtimeObjs, tc.terminatingGateways...) fakeClient := fake.NewClientBuilder(). WithScheme(s). - WithRuntimeObjects(tc.registration). + WithRuntimeObjects(runtimeObjs...). WithStatusSubresource(&v1alpha1.Registration{}). Build() @@ -733,10 +921,14 @@ func buildMux(t *testing.T, cfg serverResponseConfig, serviceName string) http.H return } - policy := &capi.ACLPolicy{} - if cfg.policyExists { - policy.ID = policyID - policy.Name = fmt.Sprintf("%s-write-policy", serviceName) + if !cfg.policyExists { + w.WriteHeader(404) + return + } + + policy := &capi.ACLPolicy{ + ID: policyID, + Name: fmt.Sprintf("%s-write-policy", serviceName), } val, err := json.Marshal(policy) From 28cfb1bab4f7ed077022be038a9ae2b589e28809 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 9 May 2024 15:36:00 -0400 Subject: [PATCH 10/15] handle errors and ensure finalizer is set --- .../configentries/registrations_controller.go | 83 ++++++++++++++----- .../inject-connect/v1controllers.go | 6 +- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 79f641da7c..41d5cb2f87 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -5,7 +5,6 @@ package configentries import ( "context" - "errors" "fmt" "slices" "strings" @@ -23,6 +22,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" capi "github.com/hashicorp/consul/api" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul-k8s/control-plane/consul" @@ -88,7 +88,14 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques func (r *RegistrationsController) handleRegistration(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { log.Info("Registering service") - err := r.registerService(log, client, registration) + + patch := r.AddFinalizersPatch(registration, RegistrationFinalizer) + err := r.Patch(ctx, registration, patch) + if err != nil { + return err + } + + err = r.registerService(log, client, registration) if err != nil { r.updateStatusError(ctx, log, registration, ConsulErrorRegistration, err) return err @@ -133,6 +140,7 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l termGWList := &v1alpha1.TerminatingGatewayList{} err := r.Client.List(ctx, termGWList) if err != nil { + log.Error(err, "error listing terminating gateways") return err } @@ -150,6 +158,7 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l roles, _, err := client.ACL().RoleList(nil) if err != nil { + log.Error(err, "error reading role list") return err } @@ -177,6 +186,8 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l policy = existingPolicy } + mErr := &multierror.Error{} + for _, termGW := range termGWsToUpdate { var role *capi.ACLRole for _, r := range roles { @@ -187,20 +198,22 @@ func (r *RegistrationsController) updateTermGWACLRole(ctx context.Context, log l } if role == nil { - log.Info("terminating gateway role not found") - return errors.New("terminating gateway role not found") + log.Info("terminating gateway role not found", "terminatingGatewayName", termGW.Name) + mErr = multierror.Append(mErr, fmt.Errorf("terminating gateway role not found for %q", termGW.Name)) + continue } role.Policies = append(role.Policies, &capi.ACLRolePolicyLink{Name: policy.Name, ID: policy.ID}) _, _, err = client.ACL().RoleUpdate(role, nil) if err != nil { - log.Error(err, "error updating role") - return err + log.Error(err, "error updating role", "roleName", role.Name) + mErr = multierror.Append(mErr, fmt.Errorf("error updating role %q", role.Name)) + continue } } - return nil + return mErr.ErrorOrNil() } func (r *RegistrationsController) handleDeletion(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { @@ -263,6 +276,7 @@ func (r *RegistrationsController) removeTermGWACLRole(ctx context.Context, log l return err } + mErr := &multierror.Error{} for _, termGW := range termGWsToUpdate { var role *capi.ACLRole for _, r := range roles { @@ -273,8 +287,9 @@ func (r *RegistrationsController) removeTermGWACLRole(ctx context.Context, log l } if role == nil { - log.Info("terminating gateway role not found") - return errors.New("terminating gateway role not found") + log.Info("terminating gateway role not found", "terminatingGatewayName", termGW.Name) + mErr = multierror.Append(mErr, fmt.Errorf("terminating gateway role not found for %q", termGW.Name)) + continue } var policyID string @@ -289,24 +304,26 @@ func (r *RegistrationsController) removeTermGWACLRole(ctx context.Context, log l }) if policyID == "" { - log.Info("policy not found on terminating gateway role", "policyName", expectedPolicyName) - return nil + log.Info("policy not found on terminating gateway role", "policyName", expectedPolicyName, "terminatingGatewayName", termGW.Name) + continue } _, _, err = client.ACL().RoleUpdate(role, nil) if err != nil { - log.Error(err, "error updating role") - return err + log.Error(err, "error updating role", "roleName", role.Name) + mErr = multierror.Append(mErr, fmt.Errorf("error updating role %q", role.Name)) + continue } _, err = client.ACL().PolicyDelete(policyID, nil) if err != nil { - log.Error(err, "error deleting service policy") - return err + log.Error(err, "error deleting service policy", "policyID", policyID, "policyName", expectedPolicyName) + mErr = multierror.Append(mErr, fmt.Errorf("error deleting service ACL policy %q", policyID)) + continue } } - return nil + return mErr.ErrorOrNil() } func servicePolicyName(name string) string { @@ -346,23 +363,43 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger return r.Log.WithValues("request", name) } -func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { +func (r *RegistrationsController) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { + if err := mgr.GetFieldIndexer().IndexField(ctx, &v1alpha1.Registration{}, "registrationName", indexerFn); err != nil { + return err + } + return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.Registration{}). Watches(&v1alpha1.TerminatingGateway{}, handler.EnqueueRequestsFromMapFunc(r.transformTerminatingGateways)). Complete(r) } +func indexerFn(o client.Object) []string { + reg := o.(*v1alpha1.Registration) + return []string{reg.Spec.Service.Name} +} + func (r *RegistrationsController) transformTerminatingGateways(ctx context.Context, o client.Object) []reconcile.Request { termGW := o.(*v1alpha1.TerminatingGateway) reqs := make([]reconcile.Request, 0, len(termGW.Spec.Services)) for _, svc := range termGW.Spec.Services { - reqs = append(reqs, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: svc.Name, - Namespace: svc.Namespace, - }, - }) + // lookup registrationList by service name add add it to the reconcile request + registrationList := &v1alpha1.RegistrationList{} + + err := r.Client.List(ctx, registrationList, client.MatchingFields{"registrationName": svc.Name}) + if err != nil { + r.Log.Error(err, "error listing registrations by service name", "serviceName", svc.Name) + continue + } + + for _, reg := range registrationList.Items { + reqs = append(reqs, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: reg.Name, + Namespace: reg.Namespace, + }, + }) + } } return reqs } diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go index 8e04fef8ce..cb227d964b 100644 --- a/control-plane/subcommand/inject-connect/v1controllers.go +++ b/control-plane/subcommand/inject-connect/v1controllers.go @@ -21,9 +21,9 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/connect-inject/lifecycle" "github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics" "github.com/hashicorp/consul-k8s/control-plane/connect-inject/webhook" + "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" controllers "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" webhookconfiguration "github.com/hashicorp/consul-k8s/control-plane/helper/webhook-configuration" - "github.com/hashicorp/consul-k8s/control-plane/registrations" "github.com/hashicorp/consul-k8s/control-plane/subcommand/flags" ) @@ -285,13 +285,13 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage return err } - if err := (®istrations.RegistrationsController{ + if err := (&configentries.RegistrationsController{ Client: mgr.GetClient(), ConsulClientConfig: consulConfig, ConsulServerConnMgr: watcher, Scheme: mgr.GetScheme(), Log: ctrl.Log.WithName("controller").WithName(apicommon.Registration), - }).SetupWithManager(mgr); err != nil { + }).SetupWithManager(ctx, mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", apicommon.Registration) return err } From 629e68ad68c689629292321155fb73eab526266a Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 9 May 2024 15:41:25 -0400 Subject: [PATCH 11/15] Add tests for finalizers --- .../configentries/registrations_controller_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index fa8bfcd457..805be4a014 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -53,6 +53,7 @@ func TestReconcile_Success(tt *testing.T) { registration *v1alpha1.Registration terminatingGateways []runtime.Object serverResponseConfig serverResponseConfig + expectedFinalizers []string expectedConditions []v1alpha1.Condition }{ "registering - success on registration": { @@ -93,6 +94,7 @@ func TestReconcile_Success(tt *testing.T) { }, }, serverResponseConfig: serverResponseConfig{registering: true}, + expectedFinalizers: []string{configentries.RegistrationFinalizer}, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionTrue, @@ -141,6 +143,7 @@ func TestReconcile_Success(tt *testing.T) { registering: true, aclEnabled: true, }, + expectedFinalizers: []string{configentries.RegistrationFinalizer}, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionTrue, @@ -190,6 +193,7 @@ func TestReconcile_Success(tt *testing.T) { aclEnabled: true, policyExists: true, }, + expectedFinalizers: []string{configentries.RegistrationFinalizer}, expectedConditions: []v1alpha1.Condition{{ Type: "Synced", Status: v1.ConditionTrue, @@ -329,6 +333,8 @@ func TestReconcile_Success(tt *testing.T) { t.Errorf("unexpected condition diff: %s", diff) } } + + require.ElementsMatch(t, fetchedReg.Finalizers, tc.expectedFinalizers) }) } } @@ -776,6 +782,8 @@ func TestReconcile_Failure(tt *testing.T) { t.Errorf("unexpected condition diff: %s", diff) } } + + require.ElementsMatch(t, fetchedReg.Finalizers, []string{configentries.RegistrationFinalizer}) }) } } From 97a43237c69316203597293c41b06623cbf9eb31 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 9 May 2024 15:48:41 -0400 Subject: [PATCH 12/15] remove unused file --- control-plane/registrations/index.go | 1 - 1 file changed, 1 deletion(-) delete mode 100644 control-plane/registrations/index.go diff --git a/control-plane/registrations/index.go b/control-plane/registrations/index.go deleted file mode 100644 index fdd464d2e5..0000000000 --- a/control-plane/registrations/index.go +++ /dev/null @@ -1 +0,0 @@ -package registrations From 677a7284d7ddb82804a2c9d39619a290a3e41f3b Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 9 May 2024 15:49:05 -0400 Subject: [PATCH 13/15] fix import naming --- control-plane/subcommand/inject-connect/v1controllers.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go index cb227d964b..df7fff50c1 100644 --- a/control-plane/subcommand/inject-connect/v1controllers.go +++ b/control-plane/subcommand/inject-connect/v1controllers.go @@ -21,7 +21,6 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/connect-inject/lifecycle" "github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics" "github.com/hashicorp/consul-k8s/control-plane/connect-inject/webhook" - "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" controllers "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" webhookconfiguration "github.com/hashicorp/consul-k8s/control-plane/helper/webhook-configuration" "github.com/hashicorp/consul-k8s/control-plane/subcommand/flags" @@ -285,7 +284,7 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage return err } - if err := (&configentries.RegistrationsController{ + if err := (&controllers.RegistrationsController{ Client: mgr.GetClient(), ConsulClientConfig: consulConfig, ConsulServerConnMgr: watcher, From f6ca32b9e7b64650f1ffe18c3b0535ff86ecadba Mon Sep 17 00:00:00 2001 From: jm96441n Date: Fri, 10 May 2024 11:01:06 -0400 Subject: [PATCH 14/15] linting --- .../controllers/configentries/registrations_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 41d5cb2f87..5eb9ab477c 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -30,7 +30,7 @@ import ( const RegistrationFinalizer = "registration.finalizers.consul.hashicorp.com" -// Status Reasons +// Status Reasons. const ( ConsulErrorRegistration = "ConsulErrorRegistration" ConsulErrorDeregistration = "ConsulErrorDeregistration" From 9086e37078d46bd9a26b1f076cd28ed7ce6508f5 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Mon, 13 May 2024 11:36:42 -0400 Subject: [PATCH 15/15] fix comment, extract constant --- .../configentries/registrations_controller.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 5eb9ab477c..86513ca909 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -363,8 +363,11 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger return r.Log.WithValues("request", name) } +const registrationByServiceNameIndex = "registrationName" + func (r *RegistrationsController) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { - if err := mgr.GetFieldIndexer().IndexField(ctx, &v1alpha1.Registration{}, "registrationName", indexerFn); err != nil { + // setup the index to lookup registrations by service name + if err := mgr.GetFieldIndexer().IndexField(ctx, &v1alpha1.Registration{}, registrationByServiceNameIndex, indexerFn); err != nil { return err } @@ -383,10 +386,10 @@ func (r *RegistrationsController) transformTerminatingGateways(ctx context.Conte termGW := o.(*v1alpha1.TerminatingGateway) reqs := make([]reconcile.Request, 0, len(termGW.Spec.Services)) for _, svc := range termGW.Spec.Services { - // lookup registrationList by service name add add it to the reconcile request + // lookup registrationList by service name and add it to the reconcile request registrationList := &v1alpha1.RegistrationList{} - err := r.Client.List(ctx, registrationList, client.MatchingFields{"registrationName": svc.Name}) + err := r.Client.List(ctx, registrationList, client.MatchingFields{registrationByServiceNameIndex: svc.Name}) if err != nil { r.Log.Error(err, "error listing registrations by service name", "serviceName", svc.Name) continue