From 451a3f7704db2807a24762ca2cc863e0e527df9e Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jul 2023 16:10:31 -0400 Subject: [PATCH] revert back to one role instead of separate for openshift --- .../api-gateway/gatekeeper/gatekeeper.go | 28 +---- .../api-gateway/gatekeeper/gatekeeper_test.go | 111 +++++++----------- control-plane/api-gateway/gatekeeper/role.go | 73 ++---------- .../api-gateway/gatekeeper/rolebinding.go | 66 +---------- 4 files changed, 56 insertions(+), 222 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper.go b/control-plane/api-gateway/gatekeeper/gatekeeper.go index b79e4f599d..8bbb9147f6 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper.go @@ -41,18 +41,6 @@ func (g *Gatekeeper) Upsert(ctx context.Context, gateway gwv1beta1.Gateway, gcc return err } - if config.EnableOpenShift { - g.Log.Info("Gatekeeper Upsert OpenshiftRole") - - if err := g.upsertOpenshiftRole(ctx, gateway, config); err != nil { - return err - } - - if err := g.upsertOpenshiftRoleBinding(ctx, gateway, gcc, config); err != nil { - return err - } - } - if err := g.upsertRoleBinding(ctx, gateway, gcc, config); err != nil { return err } @@ -70,19 +58,9 @@ func (g *Gatekeeper) Upsert(ctx context.Context, gateway gwv1beta1.Gateway, gcc // Delete removes the resources for handling routing of network traffic. // This is done in the reverse order of Upsert due to dependencies between resources. -func (g *Gatekeeper) Delete(ctx context.Context, gatewayName types.NamespacedName, enableOpenshift bool) error { +func (g *Gatekeeper) Delete(ctx context.Context, gatewayName types.NamespacedName, config common.HelmConfig) error { g.Log.V(1).Info(fmt.Sprintf("Delete Gateway Deployment %s/%s", gatewayName.Namespace, gatewayName.Name)) - if enableOpenshift { - g.Log.Info("Deleting Openshift Role") - if err := g.deleteRole(ctx, types.NamespacedName{ - Namespace: gatewayName.Namespace, - Name: gatewayName.Name + "-openshift", - }); err != nil { - return err - } - } - if err := g.deleteDeployment(ctx, gatewayName); err != nil { return err } @@ -122,7 +100,3 @@ func (g *Gatekeeper) serviceAccountName(gateway gwv1beta1.Gateway, config common } return gateway.Name } - -func getOpenshiftName(gateway gwv1beta1.Gateway) string { - return gateway.Name + "-openshift" -} diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index afb94470a1..a34499b960 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -603,6 +603,44 @@ func TestUpsert(t *testing.T) { serviceAccounts: []*corev1.ServiceAccount{}, }, }, + "create a new gateway with openshift enabled": { + gateway: gwv1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: gwv1beta1.GatewaySpec{ + Listeners: listeners, + }, + }, + gatewayClassConfig: v1alpha1.GatewayClassConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "consul-gatewayclassconfig", + }, + Spec: v1alpha1.GatewayClassConfigSpec{ + DeploymentSpec: v1alpha1.DeploymentSpec{ + DefaultInstances: common.PointerTo(int32(3)), + MaxInstances: common.PointerTo(int32(3)), + MinInstances: common.PointerTo(int32(1)), + }, + CopyAnnotations: v1alpha1.CopyAnnotationsSpec{}, + }, + }, + helmConfig: common.HelmConfig{ + EnableOpenShift: true, + }, + initialResources: resources{}, + finalResources: resources{ + deployments: []*appsv1.Deployment{ + configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), + }, + roles: []*rbac.Role{ + configureRole(name, namespace, labels, "1", true), + }, + services: []*corev1.Service{}, + serviceAccounts: []*corev1.ServiceAccount{}, + }, + }, } for name, tc := range cases { @@ -805,7 +843,7 @@ func TestDelete(t *testing.T) { err := gatekeeper.Delete(context.Background(), types.NamespacedName{ Namespace: tc.gateway.Namespace, Name: tc.gateway.Name, - }, false) + }, tc.helmConfig) require.NoError(t, err) require.NoError(t, validateResourcesExist(t, client, tc.finalResources)) require.NoError(t, validateResourcesAreDeleted(t, client, tc.initialResources)) @@ -813,73 +851,6 @@ func TestDelete(t *testing.T) { } } -func TestUpsertOpenshift(t *testing.T) { - t.Parallel() - - cases := map[string]testCase{ - "create a new gateway with openshift enabled": { - gateway: gwv1beta1.Gateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Spec: gwv1beta1.GatewaySpec{ - Listeners: listeners, - }, - }, - gatewayClassConfig: v1alpha1.GatewayClassConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: "consul-gatewayclassconfig", - }, - Spec: v1alpha1.GatewayClassConfigSpec{ - DeploymentSpec: v1alpha1.DeploymentSpec{ - DefaultInstances: common.PointerTo(int32(3)), - MaxInstances: common.PointerTo(int32(3)), - MinInstances: common.PointerTo(int32(1)), - }, - CopyAnnotations: v1alpha1.CopyAnnotationsSpec{}, - }, - }, - helmConfig: common.HelmConfig{ - EnableOpenShift: true, - }, - initialResources: resources{}, - finalResources: resources{ - deployments: []*appsv1.Deployment{ - configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), - }, - roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1", true), - }, - services: []*corev1.Service{}, - serviceAccounts: []*corev1.ServiceAccount{}, - }, - }, - } - - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - s := runtime.NewScheme() - require.NoError(t, gwv1beta1.Install(s)) - require.NoError(t, v1alpha1.AddToScheme(s)) - require.NoError(t, rbac.AddToScheme(s)) - require.NoError(t, corev1.AddToScheme(s)) - require.NoError(t, appsv1.AddToScheme(s)) - - log := logrtest.New(t) - - objs := append(joinResources(tc.initialResources), &tc.gateway, &tc.gatewayClassConfig) - client := fake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build() - - gatekeeper := New(log, client) - - err := gatekeeper.Upsert(context.Background(), tc.gateway, tc.gatewayClassConfig, tc.helmConfig) - require.NoError(t, err) - require.NoError(t, validateResourcesExist(t, client, tc.finalResources)) - }) - } -} - func joinResources(resources resources) (objs []client.Object) { for _, deployment := range resources.deployments { objs = append(objs, deployment) @@ -1126,7 +1097,6 @@ func configureDeployment(name, namespace string, labels map[string]string, repli func configureRole(name, namespace string, labels map[string]string, resourceVersion string, openshiftEnabled bool) *rbac.Role { rules := []rbac.PolicyRule{} - roleName := name if openshiftEnabled { rules = []rbac.PolicyRule{ { @@ -1136,7 +1106,6 @@ func configureRole(name, namespace string, labels map[string]string, resourceVer Verbs: []string{"use"}, }, } - roleName = name + "-openshift" } return &rbac.Role{ TypeMeta: metav1.TypeMeta{ @@ -1144,7 +1113,7 @@ func configureRole(name, namespace string, labels map[string]string, resourceVer Kind: "Role", }, ObjectMeta: metav1.ObjectMeta{ - Name: roleName, + Name: name, Namespace: namespace, Labels: labels, ResourceVersion: resourceVersion, diff --git a/control-plane/api-gateway/gatekeeper/role.go b/control-plane/api-gateway/gatekeeper/role.go index 27fd45d358..7d3149a1bc 100644 --- a/control-plane/api-gateway/gatekeeper/role.go +++ b/control-plane/api-gateway/gatekeeper/role.go @@ -18,7 +18,7 @@ import ( ) func (g *Gatekeeper) upsertRole(ctx context.Context, gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) error { - if config.AuthMethod == "" { + if config.AuthMethod == "" && !config.EnableOpenShift { return g.deleteRole(ctx, types.NamespacedName{Namespace: gateway.Namespace, Name: gateway.Name}) } @@ -39,7 +39,7 @@ func (g *Gatekeeper) upsertRole(ctx context.Context, gateway gwv1beta1.Gateway, return errors.New("role not owned by controller") } - role = g.role(gateway, gcc) + role = g.role(gateway, gcc, config) if err := ctrl.SetControllerReference(&gateway, role, g.Client.Scheme()); err != nil { return err } @@ -61,7 +61,7 @@ func (g *Gatekeeper) deleteRole(ctx context.Context, gwName types.NamespacedName return nil } -func (g *Gatekeeper) role(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig) *rbac.Role { +func (g *Gatekeeper) role(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) *rbac.Role { role := &rbac.Role{ ObjectMeta: metav1.ObjectMeta{ Name: gateway.Name, @@ -80,63 +80,16 @@ func (g *Gatekeeper) role(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassCo }) } - return role -} - -func (g *Gatekeeper) upsertOpenshiftRole(ctx context.Context, gateway gwv1beta1.Gateway, config common.HelmConfig) error { - role := &rbac.Role{} - - openshiftRoleName := getOpenshiftName(gateway) - - // If the Role already exists, ensure that we own the Role - err := g.Client.Get(ctx, types.NamespacedName{ - Namespace: gateway.Namespace, - Name: openshiftRoleName, - }, role) - if err != nil && !k8serrors.IsNotFound(err) { - return err - } else if !k8serrors.IsNotFound(err) { - // Ensure we own the Role. - for _, ref := range role.GetOwnerReferences() { - if ref.UID == gateway.GetUID() && ref.Name == gateway.Name { - // We found ourselves! - g.Log.Info("We own the role") - - return nil - } - - } - return errors.New("role not owned by controller") - } - - role = g.openshiftRole(gateway, openshiftRoleName, config) - if err := ctrl.SetControllerReference(&gateway, role, g.Client.Scheme()); err != nil { - return err - } - if err := g.Client.Create(ctx, role); err != nil { - return err - } - - return nil -} - -func (g *Gatekeeper) openshiftRole(gateway gwv1beta1.Gateway, roleName string, config common.HelmConfig) *rbac.Role { - role := &rbac.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: roleName, - Namespace: gateway.Namespace, - Labels: common.LabelsForGateway(&gateway), - }, - Rules: []rbac.PolicyRule{ - { - APIGroups: []string{"security.openshift.io"}, - Resources: []string{"securitycontextconstraints"}, - // TODO(nathancoleman) Consider accepting an explicit SCC name. This will make the code - // here less brittle and allow for the user to provide their own SCC if they wish. - ResourceNames: []string{config.ReleaseName + "-api-gateway"}, - Verbs: []string{"use"}, - }, - }, + if config.EnableOpenShift { + role.Rules = append(role.Rules, rbac.PolicyRule{ + APIGroups: []string{"security.openshift.io"}, + Resources: []string{"securitycontextconstraints"}, + // TODO(nathancoleman) Consider accepting an explicit SCC name. This will make the code + // here less brittle and allow for the user to provide their own SCC if they wish. + //ResourceNames: []string{config.ReleaseName + "-api-gateway"}, + ResourceNames: []string{"privileged"}, + Verbs: []string{"use"}, + }) } return role diff --git a/control-plane/api-gateway/gatekeeper/rolebinding.go b/control-plane/api-gateway/gatekeeper/rolebinding.go index c433c78303..1a60e752c8 100644 --- a/control-plane/api-gateway/gatekeeper/rolebinding.go +++ b/control-plane/api-gateway/gatekeeper/rolebinding.go @@ -19,7 +19,7 @@ import ( ) func (g *Gatekeeper) upsertRoleBinding(ctx context.Context, gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) error { - if config.AuthMethod == "" { + if config.AuthMethod == "" && !config.EnableOpenShift { return g.deleteRole(ctx, types.NamespacedName{Namespace: gateway.Namespace, Name: gateway.Name}) } @@ -66,7 +66,7 @@ func (g *Gatekeeper) deleteRoleBinding(ctx context.Context, gwName types.Namespa func (g *Gatekeeper) roleBinding(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) *rbac.RoleBinding { // Create resources for reference. This avoids bugs if naming patterns change. serviceAccount := g.serviceAccount(gateway) - role := g.role(gateway, gcc) + role := g.role(gateway, gcc, config) return &rbac.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ @@ -88,65 +88,3 @@ func (g *Gatekeeper) roleBinding(gateway gwv1beta1.Gateway, gcc v1alpha1.Gateway }, } } - -func (g *Gatekeeper) upsertOpenshiftRoleBinding(ctx context.Context, gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) error { - roleBinding := &rbac.RoleBinding{} - - // If the RoleBinding already exists, ensure that we own the RoleBinding - openshiftRoleName := getOpenshiftName(gateway) - - g.Log.Info("UpsertOpenshiftRoleBinding") - // If the Role already exists, ensure that we own the Role - err := g.Client.Get(ctx, types.NamespacedName{ - Namespace: gateway.Namespace, - Name: openshiftRoleName, - }, roleBinding) - if err != nil && !k8serrors.IsNotFound(err) { - return err - } else if !k8serrors.IsNotFound(err) { - // Ensure we own the Role. - for _, ref := range roleBinding.GetOwnerReferences() { - if ref.UID == gateway.GetUID() && ref.Name == gateway.Name { - // We found ourselves! - return nil - } - } - return errors.New("role not owned by controller") - } - - // Create or update the RoleBinding - roleBinding = g.roleBindingOpenshift(gateway, getOpenshiftName(gateway)) - if err := ctrl.SetControllerReference(&gateway, roleBinding, g.Client.Scheme()); err != nil { - return err - } - if err := g.Client.Create(ctx, roleBinding); err != nil { - return err - } - - return nil -} - -func (g *Gatekeeper) roleBindingOpenshift(gateway gwv1beta1.Gateway, roleName string) *rbac.RoleBinding { - // Create resources for reference. This avoids bugs if naming patterns change. - serviceAccount := g.serviceAccount(gateway) - - return &rbac.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: roleName, - Namespace: gateway.Namespace, - Labels: common.LabelsForGateway(&gateway), - }, - RoleRef: rbac.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: roleName, - }, - Subjects: []rbac.Subject{ - { - Kind: "ServiceAccount", - Name: serviceAccount.Name, - Namespace: serviceAccount.Namespace, - }, - }, - } -}