Skip to content

Commit

Permalink
revert back to one role instead of separate for openshift
Browse files Browse the repository at this point in the history
  • Loading branch information
@missylbytes
missylbytes committed Aug 7, 2023
1 parent 6152c62 commit 451a3f7
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 222 deletions.
28 changes: 1 addition & 27 deletions control-plane/api-gateway/gatekeeper/gatekeeper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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"
}
111 changes: 40 additions & 71 deletions control-plane/api-gateway/gatekeeper/gatekeeper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -805,81 +843,14 @@ 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))
})
}
}

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)
Expand Down Expand Up @@ -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{
{
Expand All @@ -1136,15 +1106,14 @@ func configureRole(name, namespace string, labels map[string]string, resourceVer
Verbs: []string{"use"},
},
}
roleName = name + "-openshift"
}
return &rbac.Role{
TypeMeta: metav1.TypeMeta{
APIVersion: "rbac.authorization.k8s.io/v1",
Kind: "Role",
},
ObjectMeta: metav1.ObjectMeta{
Name: roleName,
Name: name,
Namespace: namespace,
Labels: labels,
ResourceVersion: resourceVersion,
Expand Down
73 changes: 13 additions & 60 deletions control-plane/api-gateway/gatekeeper/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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})
}

Expand All @@ -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
}
Expand All @@ -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,
Expand All @@ -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
Expand Down
66 changes: 2 additions & 64 deletions control-plane/api-gateway/gatekeeper/rolebinding.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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})
}

Expand Down Expand Up @@ -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{
Expand All @@ -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,
},
},
}
}

0 comments on commit 451a3f7

Please sign in to comment.