Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NET-3908: allow configuration of SecurityContextConstraints when running on OpenShift #2184

Merged
merged 42 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
4f5437d
Create SecurityContextConstraints resource to reference when openshif…
nathancoleman May 25, 2023
a529f27
Add rule allowing api-gateway deployments to use SecurityContextConst…
nathancoleman May 25, 2023
3d5e980
Allow controller to use created SecurityContextConstraints resource
nathancoleman May 25, 2023
b176b78
Remove duplicate OpenShift enabled field on Helm config
nathancoleman May 25, 2023
e67f2df
Add changelog entry
nathancoleman May 30, 2023
0bc616a
Use consul.fullname instead of release.name
nathancoleman May 30, 2023
1bd97f9
Fix package import name, add TODO
nathancoleman Jun 2, 2023
11cc9e9
Update ClusterRole for controller to allow management of RoleBindings
nathancoleman Jun 2, 2023
2c4bbee
Separate logic for RoleBinding management from logic for Role
nathancoleman Jun 2, 2023
c6c9a0b
Clean up diff
nathancoleman Jun 2, 2023
31773b6
adding testing statements
missylbytes Jul 20, 2023
52acedd
logging help
missylbytes Jul 20, 2023
3f847e6
added gatekeeper openshift role specifics
missylbytes Jul 21, 2023
283cf75
added rolebinding and serviceaccount updates
missylbytes Jul 21, 2023
5247f0a
logging why we don't own the role
missylbytes Jul 21, 2023
0c93777
fixed ownership issue
missylbytes Jul 21, 2023
cfd9984
needed to update serviceAccountName to account for openshift
missylbytes Jul 24, 2023
c25cb65
did the service account incorrectly
missylbytes Jul 24, 2023
6152c62
some cleanup plus added an openshift test, need to understand owner r…
missylbytes Jul 24, 2023
451a3f7
revert back to one role instead of separate for openshift
missylbytes Jul 26, 2023
67ba54d
missed a call to gatekeeper delete when updating signature
missylbytes Jul 26, 2023
78aa9ce
added template for scc name for easier testing, may or may not leave …
missylbytes Jul 27, 2023
965b2f7
gate security context block for gateway deployments
missylbytes Jul 27, 2023
f101782
Update charts/consul/templates/api-gateway-securitycontextconstraints…
nathancoleman Jul 27, 2023
f8e7525
updated security context constraint for gateway, removed unnecessary …
missylbytes Jul 28, 2023
ffd4a5a
cleaning up
missylbytes Jul 28, 2023
4d7655a
do not know why unit test is failing
missylbytes Jul 28, 2023
dce6bda
forgot name of openshift SCC
missylbytes Jul 28, 2023
d68fbf3
removes custom security context constraint and updates bats
missylbytes Jul 31, 2023
57b7543
cleanup
missylbytes Jul 31, 2023
b945c18
temporary test of port-mapping for envoy
missylbytes Jul 31, 2023
c9dbcce
remove port testing
missylbytes Aug 1, 2023
e44ddff
in the middle of moving openshift scc name parameter to gatewayclassc…
missylbytes Aug 4, 2023
5744687
fix unit tests
missylbytes Aug 4, 2023
4722f9a
cleanup
missylbytes Aug 4, 2023
c3bcb63
fix bats tests
missylbytes Aug 7, 2023
6df3cca
Remove unused configuration fields and arguments
nathancoleman Aug 7, 2023
542522f
Use openshiftSCCName instead of openshiftSccName
nathancoleman Aug 7, 2023
3bf704f
Update charts/consul/templates/crd-gatewayclassconfigs.yaml
nathancoleman Aug 7, 2023
61fb592
Only provide SCC name flag when openshift.enabled, update docs
nathancoleman Aug 7, 2023
ada4579
Update .changelog/2184.txt
nathancoleman Aug 7, 2023
6a6b003
fix bats test
missylbytes Aug 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/2184.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:feature
api-gateway: support deploying to OpenShift
nathancoleman marked this conversation as resolved.
Show resolved Hide resolved
```
10 changes: 10 additions & 0 deletions charts/consul/templates/connect-inject-clusterrole.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -186,4 +186,14 @@ rules:
- "get"
- "list"
- "watch"
{{- if .Values.global.openshift.enabled }}
- apiGroups:
- security.openshift.io
resources:
- securitycontextconstraints
resourceNames:
- {{ .Values.connectInject.apiGateway.managedGatewayClass.openshiftSCCName }}
verbs:
- use
{{- end }}
{{- end }}
4 changes: 4 additions & 0 deletions charts/consul/templates/crd-gatewayclassconfigs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ spec:
type: string
type: object
type: array
openshiftSCCName:
description: The name of an existing SecurityContextConstraints
resource to bind to the managed role when running on OpenShift.
type: string
type: object
type: object
served: true
Expand Down
1 change: 1 addition & 0 deletions charts/consul/templates/gateway-resources-job.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ spec:
- {{- toYaml .Values.connectInject.apiGateway.managedGatewayClass.copyAnnotations.service.annotations | nindent 14 -}}
{{- end }}
- -service-type={{ .Values.connectInject.apiGateway.managedGatewayClass.serviceType }}
- -openshift-scc-name={{ .Values.connectInject.apiGateway.managedGatewayClass.openshiftSCCName }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to gate this to only if global.openshift.enabled = true

{{- end}}
resources:
requests:
Expand Down
24 changes: 24 additions & 0 deletions charts/consul/test/unit/connect-inject-clusterrole.bats
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,27 @@ load _helpers
local actual=$(echo $object | yq -r '.verbs | index("watch")' | tee /dev/stderr)
[ "${actual}" != null ]
}

#--------------------------------------------------------------------
# openshift

@test "connectInject/ClusterRole: adds permission to securitycontextconstraints for Openshift with global.openshift.enabled=true with default apiGateway Openshift SCC Name" {
cd `chart_dir`
local object=$(helm template \
-s templates/connect-inject-clusterrole.yaml \
--set 'global.openshift.enabled=true' \
. | tee /dev/stderr |
yq '.rules[13].resourceNames | index("restricted-v2")' | tee /dev/stderr)
[ "${object}" == 0 ]
}

@test "connectInject/ClusterRole: adds permission to securitycontextconstraints for Openshift with global.openshift.enabled=true and sets apiGateway Openshift SCC Name" {
cd `chart_dir`
local object=$(helm template \
-s templates/connect-inject-clusterrole.yaml \
--set 'global.openshift.enabled=true' \
--set 'connectInject.apiGateway.managedGatewayClass.openshiftSCCName=fakescc' \
. | tee /dev/stderr |
yq '.rules[13].resourceNames | index("fakescc")' | tee /dev/stderr)
[ "${object}" == 0 ]
}
7 changes: 7 additions & 0 deletions charts/consul/test/unit/gateway-resources-job.bats
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ target=templates/gateway-resources-job.yaml
--set 'connectInject.apiGateway.managedGatewayClass.tolerations=- key: bar' \
--set 'connectInject.apiGateway.managedGatewayClass.copyAnnotations.service.annotations=- bingo' \
--set 'connectInject.apiGateway.managedGatewayClass.serviceType=Foo' \
--set 'connectInject.apiGateway.managedGatewayClass.openshiftSCCName=hello' \
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].args' | tee /dev/stderr)

Expand Down Expand Up @@ -121,6 +122,12 @@ target=templates/gateway-resources-job.yaml

local actual=$(echo "$spec" | jq '.[16]')
[ "${actual}" = "\"- bingo\"" ]

local actual=$(echo "$spec" | jq '.[17]')
[ "${actual}" = "\"-service-type=Foo\"" ]

local actual=$(echo "$spec" | jq '.[18]')
[ "${actual}" = "\"-openshift-scc-name=hello\"" ]
}


Expand Down
4 changes: 4 additions & 0 deletions charts/consul/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2201,6 +2201,10 @@ connectInject:
maxInstances: 1
minInstances: 1

# The name of the Openshift SecurityContextConstraints resource to use for Gateways
# @type: string
openshiftSCCName: "restricted-v2"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to include here that this value is only used if global.openshift.enabled = true


# Configuration for the ServiceAccount created for the api-gateway component
serviceAccount:
# This value defines additional annotations for the client service account. This should be formatted as a multi-line
Expand Down
11 changes: 6 additions & 5 deletions cli/helm/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -576,11 +576,12 @@ type CopyAnnotations struct {
}

type ManagedGatewayClass struct {
Enabled bool `yaml:"enabled"`
NodeSelector interface{} `yaml:"nodeSelector"`
ServiceType string `yaml:"serviceType"`
UseHostPorts bool `yaml:"useHostPorts"`
CopyAnnotations CopyAnnotations `yaml:"copyAnnotations"`
Enabled bool `yaml:"enabled"`
NodeSelector interface{} `yaml:"nodeSelector"`
ServiceType string `yaml:"serviceType"`
UseHostPorts bool `yaml:"useHostPorts"`
CopyAnnotations CopyAnnotations `yaml:"copyAnnotations"`
OpenshiftSCCName string `yaml:"openshiftSCCName"`
}

type Service struct {
Expand Down
6 changes: 5 additions & 1 deletion control-plane/api-gateway/common/helm_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ import (
const componentAuthMethod = "k8s-component-auth-method"

// HelmConfig is the configuration of gateways that comes in from the user's Helm values.
// This is a combination of the apiGateway stanza and other settings that impact api-gateways.
type HelmConfig struct {
// ImageDataplane is the Consul Dataplane image to use in gateway deployments.
ImageDataplane string
ImageConsulK8S string
ConsulDestinationNamespace string
NamespaceMirroringPrefix string
EnableNamespaces bool
EnableOpenShift bool
EnableNamespaceMirroring bool
AuthMethod string
// LogLevel is the logging level of the deployed Consul Dataplanes.
Expand All @@ -30,6 +30,10 @@ type HelmConfig struct {
ConsulTLSServerName string
ConsulCACert string
ConsulConfig ConsulConfig

// EnableOpenShift indicates whether we're deploying into an OpenShift environment
// and should create SecurityContextConstraints.
EnableOpenShift bool
}

type ConsulConfig struct {
Expand Down
2 changes: 1 addition & 1 deletion control-plane/api-gateway/gatekeeper/gatekeeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (g *Gatekeeper) namespacedName(gateway gwv1beta1.Gateway) types.NamespacedN
}

func (g *Gatekeeper) serviceAccountName(gateway gwv1beta1.Gateway, config common.HelmConfig) string {
if config.AuthMethod == "" {
if config.AuthMethod == "" && !config.EnableOpenShift {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to think through this again reading over it - a comment explaining why this additional check is here would be super helpful I think

return ""
}
return gateway.Name
Expand Down
72 changes: 64 additions & 8 deletions control-plane/api-gateway/gatekeeper/gatekeeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand All @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -603,6 +603,50 @@ 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{},
OpenshiftSCCName: "test-api-gateway",
},
},
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),
},
roleBindings: []*rbac.RoleBinding{
configureRoleBinding(name, namespace, labels, "1"),
},
services: []*corev1.Service{},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
},
},
},
}

for name, tc := range cases {
Expand Down Expand Up @@ -754,7 +798,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"),
Expand Down Expand Up @@ -1057,7 +1101,19 @@ 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{}

if openshiftEnabled {
rules = []rbac.PolicyRule{
{
APIGroups: []string{"security.openshift.io"},
Resources: []string{"securitycontextconstraints"},
ResourceNames: []string{name + "-api-gateway"},
Verbs: []string{"use"},
},
}
}
return &rbac.Role{
TypeMeta: metav1.TypeMeta{
APIVersion: "rbac.authorization.k8s.io/v1",
Expand All @@ -1078,7 +1134,7 @@ func configureRole(name, namespace string, labels map[string]string, resourceVer
},
},
},
Rules: []rbac.PolicyRule{},
Rules: rules,
}
}

Expand Down
19 changes: 11 additions & 8 deletions control-plane/api-gateway/gatekeeper/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,14 +168,17 @@ func initContainer(config common.HelmConfig, name, namespace string) (corev1.Con
})
}

container.SecurityContext = &corev1.SecurityContext{
RunAsUser: pointer.Int64(initContainersUserAndGroupID),
RunAsGroup: pointer.Int64(initContainersUserAndGroupID),
RunAsNonRoot: pointer.Bool(true),
Privileged: pointer.Bool(false),
Capabilities: &corev1.Capabilities{
Drop: []corev1.Capability{"ALL"},
},
// Openshift Assigns the security context for us, do not enable if it is enabled.
if !config.EnableOpenShift {
container.SecurityContext = &corev1.SecurityContext{
RunAsUser: pointer.Int64(initContainersUserAndGroupID),
RunAsGroup: pointer.Int64(initContainersUserAndGroupID),
RunAsNonRoot: pointer.Bool(true),
Privileged: pointer.Bool(false),
Capabilities: &corev1.Capabilities{
Drop: []corev1.Capability{"ALL"},
},
}
}
nathancoleman marked this conversation as resolved.
Show resolved Hide resolved

return container, nil
Expand Down
15 changes: 12 additions & 3 deletions control-plane/api-gateway/gatekeeper/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,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 @@ -40,7 +40,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 @@ -62,7 +62,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 @@ -81,5 +81,14 @@ func (g *Gatekeeper) role(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassCo
})
}

if config.EnableOpenShift {
role.Rules = append(role.Rules, rbac.PolicyRule{
APIGroups: []string{"security.openshift.io"},
Resources: []string{"securitycontextconstraints"},
ResourceNames: []string{gcc.Spec.OpenshiftSCCName},
Verbs: []string{"use"},
})
}

return role
}
4 changes: 2 additions & 2 deletions control-plane/api-gateway/gatekeeper/rolebinding.go
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 Down
2 changes: 1 addition & 1 deletion control-plane/api-gateway/gatekeeper/serviceaccount.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
)

func (g *Gatekeeper) upsertServiceAccount(ctx context.Context, gateway gwv1beta1.Gateway, config common.HelmConfig) error {
if config.AuthMethod == "" {
if config.AuthMethod == "" && !config.EnableOpenShift {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thought here that a comment would be helpful

return g.deleteServiceAccount(ctx, types.NamespacedName{Namespace: gateway.Namespace, Name: gateway.Name})
}

Expand Down
3 changes: 3 additions & 0 deletions control-plane/api/v1alpha1/api_gateway_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ type GatewayClassConfigSpec struct {

// The name of an existing Kubernetes PodSecurityPolicy to bind to the managed ServiceAccount if ACLs are managed.
PodSecurityPolicy string `json:"podSecurityPolicy,omitempty"`

// The name of the OpenShift SecurityContextConstraints resource for this gateway class to use.
OpenshiftSCCName string `json:"openshiftSCCName,omitempty"`
}

// +k8s:deepcopy-gen=true
Expand Down
1 change: 1 addition & 0 deletions control-plane/api/v1alpha1/api_gateway_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func TestGatewayClassConfigDeepCopy(t *testing.T) {
NodeSelector: map[string]string{
"test": "test",
},
OpenshiftSCCName: "restricted-v2",
}
config := &GatewayClassConfig{
ObjectMeta: metav1.ObjectMeta{
Expand Down
Loading