some cleanup plus added an openshift test, need to understand owner r…

…eferences in test
hashicorp · Jul 24, 2023 · 29a8947 · 29a8947
1 parent 1c22ba5
commit 29a8947
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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{