From 6152c6277cf12e2c019454b2b7ab9e87f4b950da Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 24 Jul 2023 15:36:17 -0400 Subject: [PATCH] some cleanup plus added an openshift test, need to understand owner references in test --- .../api-gateway/gatekeeper/gatekeeper.go | 2 +- .../api-gateway/gatekeeper/gatekeeper_test.go | 37 +++++++++++++------ control-plane/api-gateway/gatekeeper/role.go | 17 +++------ .../api-gateway/gatekeeper/rolebinding.go | 2 +- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper.go b/control-plane/api-gateway/gatekeeper/gatekeeper.go index 6df5e2b1ab..b79e4f599d 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper.go @@ -44,7 +44,7 @@ func (g *Gatekeeper) Upsert(ctx context.Context, gateway gwv1beta1.Gateway, gcc if config.EnableOpenShift { g.Log.Info("Gatekeeper Upsert OpenshiftRole") - if err := g.upsertOpenshiftRole(ctx, gateway); err != nil { + if err := g.upsertOpenshiftRole(ctx, gateway, config); err != nil { return err } diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index b1006844ab..afb94470a1 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -193,7 +193,7 @@ func TestUpsert(t *testing.T) { configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), }, roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1"), + configureRole(name, namespace, labels, "1", false), }, roleBindings: []*rbac.RoleBinding{ configureRoleBinding(name, namespace, labels, "1"), @@ -319,7 +319,7 @@ func TestUpsert(t *testing.T) { configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), }, roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1"), + configureRole(name, namespace, labels, "1", false), }, roleBindings: []*rbac.RoleBinding{ configureRoleBinding(name, namespace, labels, "1"), @@ -342,7 +342,7 @@ func TestUpsert(t *testing.T) { configureDeployment(name, namespace, labels, 3, nil, nil, "", "2"), }, roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1"), + configureRole(name, namespace, labels, "1", false), }, roleBindings: []*rbac.RoleBinding{ configureRoleBinding(name, namespace, labels, "1"), @@ -400,7 +400,7 @@ func TestUpsert(t *testing.T) { configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), }, roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1"), + configureRole(name, namespace, labels, "1", false), }, roleBindings: []*rbac.RoleBinding{ configureRoleBinding(name, namespace, labels, "1"), @@ -428,7 +428,7 @@ func TestUpsert(t *testing.T) { configureDeployment(name, namespace, labels, 3, nil, nil, "", "2"), }, roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1"), + configureRole(name, namespace, labels, "1", false), }, roleBindings: []*rbac.RoleBinding{ configureRoleBinding(name, namespace, labels, "1"), @@ -754,7 +754,7 @@ func TestDelete(t *testing.T) { configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), }, roles: []*rbac.Role{ - configureRole(name, namespace, labels, "1"), + configureRole(name, namespace, labels, "1", false), }, roleBindings: []*rbac.RoleBinding{ configureRoleBinding(name, namespace, labels, "1"), @@ -817,7 +817,7 @@ func TestUpsertOpenshift(t *testing.T) { t.Parallel() cases := map[string]testCase{ - "create a new gateway deployment with managed Service": { + "create a new gateway with openshift enabled": { gateway: gwv1beta1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -848,7 +848,9 @@ func TestUpsertOpenshift(t *testing.T) { deployments: []*appsv1.Deployment{ configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), }, - roles: []*rbac.Role{}, + roles: []*rbac.Role{ + configureRole(name, namespace, labels, "1", true), + }, services: []*corev1.Service{}, serviceAccounts: []*corev1.ServiceAccount{}, }, @@ -1122,14 +1124,27 @@ func configureDeployment(name, namespace string, labels map[string]string, repli } } -func configureRole(name, namespace string, labels map[string]string, resourceVersion string) *rbac.Role { +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{ + { + APIGroups: []string{"security.openshift.io"}, + Resources: []string{"securitycontextconstraints"}, + ResourceNames: []string{name + "-api-gateway"}, + Verbs: []string{"use"}, + }, + } + roleName = name + "-openshift" + } return &rbac.Role{ TypeMeta: metav1.TypeMeta{ APIVersion: "rbac.authorization.k8s.io/v1", Kind: "Role", }, ObjectMeta: metav1.ObjectMeta{ - Name: name, + Name: roleName, Namespace: namespace, Labels: labels, ResourceVersion: resourceVersion, @@ -1143,7 +1158,7 @@ func configureRole(name, namespace string, labels map[string]string, resourceVer }, }, }, - Rules: []rbac.PolicyRule{}, + Rules: rules, } } diff --git a/control-plane/api-gateway/gatekeeper/role.go b/control-plane/api-gateway/gatekeeper/role.go index c235670d6a..27fd45d358 100644 --- a/control-plane/api-gateway/gatekeeper/role.go +++ b/control-plane/api-gateway/gatekeeper/role.go @@ -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, config) + role = g.role(gateway, gcc) 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, config common.HelmConfig) *rbac.Role { +func (g *Gatekeeper) role(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig) *rbac.Role { role := &rbac.Role{ ObjectMeta: metav1.ObjectMeta{ Name: gateway.Name, @@ -83,12 +83,11 @@ func (g *Gatekeeper) role(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassCo return role } -func (g *Gatekeeper) upsertOpenshiftRole(ctx context.Context, gateway gwv1beta1.Gateway) error { +func (g *Gatekeeper) upsertOpenshiftRole(ctx context.Context, gateway gwv1beta1.Gateway, config common.HelmConfig) error { role := &rbac.Role{} openshiftRoleName := getOpenshiftName(gateway) - g.Log.Info("UpsertOpenshiftRole") // If the Role already exists, ensure that we own the Role err := g.Client.Get(ctx, types.NamespacedName{ Namespace: gateway.Namespace, @@ -99,9 +98,6 @@ func (g *Gatekeeper) upsertOpenshiftRole(ctx context.Context, gateway gwv1beta1. } else if !k8serrors.IsNotFound(err) { // Ensure we own the Role. for _, ref := range role.GetOwnerReferences() { - g.Log.Info("Are we owner?") - g.Log.Info("ref.UID: " + string(ref.UID) + " gateway.GetUID(): " + string(gateway.GetUID())) - g.Log.Info("ref.Name: " + ref.Name + " openshiftRoleName: " + openshiftRoleName) if ref.UID == gateway.GetUID() && ref.Name == gateway.Name { // We found ourselves! g.Log.Info("We own the role") @@ -113,7 +109,7 @@ func (g *Gatekeeper) upsertOpenshiftRole(ctx context.Context, gateway gwv1beta1. return errors.New("role not owned by controller") } - role = g.openshiftRole(gateway, openshiftRoleName) + role = g.openshiftRole(gateway, openshiftRoleName, config) if err := ctrl.SetControllerReference(&gateway, role, g.Client.Scheme()); err != nil { return err } @@ -124,7 +120,7 @@ func (g *Gatekeeper) upsertOpenshiftRole(ctx context.Context, gateway gwv1beta1. return nil } -func (g *Gatekeeper) openshiftRole(gateway gwv1beta1.Gateway, roleName string) *rbac.Role { +func (g *Gatekeeper) openshiftRole(gateway gwv1beta1.Gateway, roleName string, config common.HelmConfig) *rbac.Role { role := &rbac.Role{ ObjectMeta: metav1.ObjectMeta{ Name: roleName, @@ -137,8 +133,7 @@ func (g *Gatekeeper) openshiftRole(gateway gwv1beta1.Gateway, roleName string) * 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"}, + ResourceNames: []string{config.ReleaseName + "-api-gateway"}, Verbs: []string{"use"}, }, }, diff --git a/control-plane/api-gateway/gatekeeper/rolebinding.go b/control-plane/api-gateway/gatekeeper/rolebinding.go index 49321ed3e9..c433c78303 100644 --- a/control-plane/api-gateway/gatekeeper/rolebinding.go +++ b/control-plane/api-gateway/gatekeeper/rolebinding.go @@ -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, config) + role := g.role(gateway, gcc) return &rbac.RoleBinding{ ObjectMeta: metav1.ObjectMeta{