From 2e4411797a682b2d73ec71a8f318ca7eb5ccffe9 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 28 Mar 2025 10:44:07 -0600 Subject: [PATCH 1/2] CP/DP Split: Openshift support Problem: Now that we have additional pods in the new architecture, we need the proper SecurityContextConstraints for running in Openshift. Solution: Create an SCC for the cert-generator and an SCC for nginx data plane pods on startup. A Role and RoleBinding are created when deploying nginx to link to the SCC. --- charts/nginx-gateway-fabric/README.md | 6 - charts/nginx-gateway-fabric/README.md.gotmpl | 6 - .../templates/certs-job.yaml | 44 +++++++ .../templates/clusterrole.yaml | 11 ++ .../templates/deployment.yaml | 3 + .../nginx-gateway-fabric/templates/scc.yaml | 44 ++++++- cmd/gateway/commands.go | 12 ++ cmd/gateway/commands_test.go | 17 +++ deploy/openshift/deploy.yaml | 100 +++++++++++++++ internal/mode/static/config/config.go | 2 + internal/mode/static/manager.go | 3 + internal/mode/static/provisioner/eventloop.go | 65 ++++++++-- internal/mode/static/provisioner/handler.go | 6 +- internal/mode/static/provisioner/objects.go | 68 +++++++++- .../mode/static/provisioner/objects_test.go | 91 ++++++++++++++ .../static/provisioner/openshift/openshift.go | 38 ++++++ .../openshiftfakes/fake_apichecker.go | 117 ++++++++++++++++++ .../mode/static/provisioner/provisioner.go | 13 ++ .../static/provisioner/provisioner_test.go | 2 + internal/mode/static/provisioner/setter.go | 24 ++++ internal/mode/static/provisioner/store.go | 28 +++++ .../mode/static/provisioner/store_test.go | 53 ++++++++ 22 files changed, 720 insertions(+), 33 deletions(-) create mode 100644 internal/mode/static/provisioner/openshift/openshift.go create mode 100644 internal/mode/static/provisioner/openshift/openshiftfakes/fake_apichecker.go diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index b71ad17677..40030b75ce 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -115,12 +115,6 @@ To use a NodePort Service instead: helm install ngf oci://ghcr.io/nginx/charts/nginx-gateway-fabric --create-namespace -n nginx-gateway --set nginx.service.type=NodePort ``` -To disable the creation of a Service: - -```shell -helm install ngf oci://ghcr.io/nginx/charts/nginx-gateway-fabric --create-namespace -n nginx-gateway --set nginx.service.create=false -``` - ## Upgrading the Chart > [!NOTE] diff --git a/charts/nginx-gateway-fabric/README.md.gotmpl b/charts/nginx-gateway-fabric/README.md.gotmpl index 6306d2a647..f757a7cc8f 100644 --- a/charts/nginx-gateway-fabric/README.md.gotmpl +++ b/charts/nginx-gateway-fabric/README.md.gotmpl @@ -113,12 +113,6 @@ To use a NodePort Service instead: helm install ngf oci://ghcr.io/nginx/charts/nginx-gateway-fabric --create-namespace -n nginx-gateway --set nginx.service.type=NodePort ``` -To disable the creation of a Service: - -```shell -helm install ngf oci://ghcr.io/nginx/charts/nginx-gateway-fabric --create-namespace -n nginx-gateway --set nginx.service.create=false -``` - ## Upgrading the Chart > [!NOTE] diff --git a/charts/nginx-gateway-fabric/templates/certs-job.yaml b/charts/nginx-gateway-fabric/templates/certs-job.yaml index a2b529ae1b..96da6289e2 100644 --- a/charts/nginx-gateway-fabric/templates/certs-job.yaml +++ b/charts/nginx-gateway-fabric/templates/certs-job.yaml @@ -56,6 +56,50 @@ subjects: name: {{ include "nginx-gateway.fullname" . }}-cert-generator namespace: {{ .Release.Namespace }} --- +{{- if .Capabilities.APIVersions.Has "security.openshift.io/v1/SecurityContextConstraints" }} +kind: SecurityContextConstraints +apiVersion: security.openshift.io/v1 +metadata: + name: {{ include "nginx-gateway.scc-name" . }}-cert-generator + labels: + {{- include "nginx-gateway.labels" . | nindent 4 }} + annotations: + "helm.sh/hook-weight": "-1" + "helm.sh/hook": pre-install +allowPrivilegeEscalation: false +allowHostDirVolumePlugin: false +allowHostIPC: false +allowHostNetwork: false +allowHostPID: false +allowHostPorts: false +allowPrivilegedContainer: false +readOnlyRootFilesystem: true +runAsUser: + type: MustRunAsRange + uidRangeMin: 101 + uidRangeMax: 101 +fsGroup: + type: MustRunAs + ranges: + - min: 1001 + max: 1001 +supplementalGroups: + type: MustRunAs + ranges: + - min: 1001 + max: 1001 +seLinuxContext: + type: MustRunAs +seccompProfiles: +- runtime/default +users: +- {{ printf "system:serviceaccount:%s:%s-cert-generator" .Release.Namespace (include "nginx-gateway.fullname" .) }} +requiredDropCapabilities: +- ALL +volumes: +- projected +--- +{{- end }} apiVersion: batch/v1 kind: Job metadata: diff --git a/charts/nginx-gateway-fabric/templates/clusterrole.yaml b/charts/nginx-gateway-fabric/templates/clusterrole.yaml index 479c22adbc..6266134602 100644 --- a/charts/nginx-gateway-fabric/templates/clusterrole.yaml +++ b/charts/nginx-gateway-fabric/templates/clusterrole.yaml @@ -150,8 +150,19 @@ rules: - securitycontextconstraints resourceNames: - {{ include "nginx-gateway.scc-name" . }} + - {{ include "nginx-gateway.scc-name" . }}-nginx verbs: - use +- apiGroups: + - rbac.authorization.k8s.io + resources: + - roles + - rolebindings + verbs: - create + - update + - delete + - list + - get - watch {{- end }} diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index 49898c57b2..1a8b406d90 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -93,6 +93,9 @@ spec: {{- if .Values.nginxGateway.snippetsFilters.enable }} - --snippets-filters {{- end }} + {{- if .Capabilities.APIVersions.Has "security.openshift.io/v1/SecurityContextConstraints" }} + - --nginx-scc={{ include "nginx-gateway.scc-name" . }}-nginx + {{- end}} env: - name: POD_NAMESPACE valueFrom: diff --git a/charts/nginx-gateway-fabric/templates/scc.yaml b/charts/nginx-gateway-fabric/templates/scc.yaml index 1564e84e32..783300c3fe 100644 --- a/charts/nginx-gateway-fabric/templates/scc.yaml +++ b/charts/nginx-gateway-fabric/templates/scc.yaml @@ -1,9 +1,10 @@ -# TODO(sberman): will need an SCC for nginx ServiceAccounts as well. {{- if .Capabilities.APIVersions.Has "security.openshift.io/v1/SecurityContextConstraints" }} kind: SecurityContextConstraints apiVersion: security.openshift.io/v1 metadata: name: {{ include "nginx-gateway.scc-name" . }} + labels: + {{- include "nginx-gateway.labels" . | nindent 4 }} allowPrivilegeEscalation: false allowHostDirVolumePlugin: false allowHostIPC: false @@ -36,4 +37,45 @@ requiredDropCapabilities: - ALL volumes: - secret +--- +kind: SecurityContextConstraints +apiVersion: security.openshift.io/v1 +metadata: + name: {{ include "nginx-gateway.scc-name" . }}-nginx + labels: + {{- include "nginx-gateway.labels" . | nindent 4 }} +allowHostDirVolumePlugin: false +allowHostIPC: false +allowHostNetwork: false +allowHostPID: false +allowHostPorts: false +allowPrivilegedContainer: false +readOnlyRootFilesystem: true +runAsUser: + type: MustRunAsRange + uidRangeMin: 101 + uidRangeMax: 101 +fsGroup: + type: MustRunAs + ranges: + - min: 1001 + max: 1001 +supplementalGroups: + type: MustRunAs + ranges: + - min: 1001 + max: 1001 +seLinuxContext: + type: MustRunAs +seccompProfiles: +- runtime/default +allowedCapabilities: +- NET_BIND_SERVICE +requiredDropCapabilities: +- ALL +volumes: +- emptyDir +- secret +- configMap +- projected {{- end }} diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 4d875cb6ac..853ddd0f72 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -81,6 +81,7 @@ func createControllerCommand() *cobra.Command { usageReportClientSSLSecretFlag = "usage-report-client-ssl-secret" //nolint:gosec // not credentials usageReportCASecretFlag = "usage-report-ca-secret" //nolint:gosec // not credentials snippetsFiltersFlag = "snippets-filters" + nginxSCCFlag = "nginx-scc" ) // flag values @@ -105,6 +106,9 @@ func createControllerCommand() *cobra.Command { validator: validateResourceName, value: agentTLSSecret, } + nginxSCCName = stringValidatingValue{ + validator: validateResourceName, + } disableMetrics bool metricsSecure bool metricsListenPort = intValidatingValue{ @@ -264,6 +268,7 @@ func createControllerCommand() *cobra.Command { SnippetsFilters: snippetsFilters, NginxDockerSecretNames: nginxDockerSecrets.values, AgentTLSSecretName: agentTLSSecretName.value, + NGINXSCCName: nginxSCCName.value, } if err := static.StartManager(conf); err != nil { @@ -457,6 +462,13 @@ func createControllerCommand() *cobra.Command { "generated NGINX config for HTTPRoute and GRPCRoute resources.", ) + cmd.Flags().Var( + &nginxSCCName, + nginxSCCFlag, + `The name of the SecurityContextConstraints to be used with the NGINX data plane Pods.`+ + ` Only applicable in OpenShift.`, + ) + return cmd } diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 8662c4ef0d..0cc031a1c5 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -158,6 +158,7 @@ func TestControllerCmdFlagValidation(t *testing.T) { "--usage-report-ca-secret=ca-secret", "--usage-report-client-ssl-secret=client-secret", "--snippets-filters", + "--nginx-scc=nginx-sscc-name", }, wantErr: false, }, @@ -445,6 +446,22 @@ func TestControllerCmdFlagValidation(t *testing.T) { }, wantErr: true, }, + { + name: "nginx-scc is set to empty string", + args: []string{ + "--nginx-scc=", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "" for "--nginx-scc" flag: must be set`, + }, + { + name: "nginx-scc is invalid", + args: []string{ + "--nginx-scc=!@#$", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "!@#$" for "--nginx-scc" flag: invalid format: `, + }, } // common flags validation is tested separately diff --git a/deploy/openshift/deploy.yaml b/deploy/openshift/deploy.yaml index d61f5e49ac..d5bc82d23f 100644 --- a/deploy/openshift/deploy.yaml +++ b/deploy/openshift/deploy.yaml @@ -175,11 +175,22 @@ rules: - security.openshift.io resourceNames: - nginx-gateway-scc + - nginx-gateway-scc-nginx resources: - securitycontextconstraints verbs: - use +- apiGroups: + - rbac.authorization.k8s.io + resources: + - roles + - rolebindings + verbs: - create + - update + - delete + - list + - get - watch --- apiVersion: rbac.authorization.k8s.io/v1 @@ -272,6 +283,7 @@ spec: - --metrics-port=9113 - --health-port=8081 - --leader-election-lock-name=nginx-gateway-leader-election + - --nginx-scc=nginx-gateway-scc-nginx env: - name: POD_NAMESPACE valueFrom: @@ -442,6 +454,10 @@ fsGroup: type: MustRunAs kind: SecurityContextConstraints metadata: + labels: + app.kubernetes.io/instance: nginx-gateway + app.kubernetes.io/name: nginx-gateway + app.kubernetes.io/version: edge name: nginx-gateway-scc readOnlyRootFilesystem: true requiredDropCapabilities: @@ -463,3 +479,87 @@ users: - system:serviceaccount:nginx-gateway:nginx-gateway volumes: - secret +--- +allowHostDirVolumePlugin: false +allowHostIPC: false +allowHostNetwork: false +allowHostPID: false +allowHostPorts: false +allowPrivilegeEscalation: false +allowPrivilegedContainer: false +apiVersion: security.openshift.io/v1 +fsGroup: + ranges: + - max: 1001 + min: 1001 + type: MustRunAs +kind: SecurityContextConstraints +metadata: + labels: + app.kubernetes.io/instance: nginx-gateway + app.kubernetes.io/name: nginx-gateway + app.kubernetes.io/version: edge + name: nginx-gateway-scc-cert-generator +readOnlyRootFilesystem: true +requiredDropCapabilities: +- ALL +runAsUser: + type: MustRunAsRange + uidRangeMax: 101 + uidRangeMin: 101 +seLinuxContext: + type: MustRunAs +seccompProfiles: +- runtime/default +supplementalGroups: + ranges: + - max: 1001 + min: 1001 + type: MustRunAs +users: +- system:serviceaccount:nginx-gateway:nginx-gateway-cert-generator +volumes: +- projected +--- +allowHostDirVolumePlugin: false +allowHostIPC: false +allowHostNetwork: false +allowHostPID: false +allowHostPorts: false +allowPrivilegedContainer: false +allowedCapabilities: +- NET_BIND_SERVICE +apiVersion: security.openshift.io/v1 +fsGroup: + ranges: + - max: 1001 + min: 1001 + type: MustRunAs +kind: SecurityContextConstraints +metadata: + labels: + app.kubernetes.io/instance: nginx-gateway + app.kubernetes.io/name: nginx-gateway + app.kubernetes.io/version: edge + name: nginx-gateway-scc-nginx +readOnlyRootFilesystem: true +requiredDropCapabilities: +- ALL +runAsUser: + type: MustRunAsRange + uidRangeMax: 101 + uidRangeMin: 101 +seLinuxContext: + type: MustRunAs +seccompProfiles: +- runtime/default +supplementalGroups: + ranges: + - max: 1001 + min: 1001 + type: MustRunAs +volumes: +- emptyDir +- secret +- configMap +- projected diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index d37c6b55f7..9248070e03 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -34,6 +34,8 @@ type Config struct { GatewayClassName string // AgentTLSSecretName is the name of the TLS Secret used by NGINX Agent to communicate with the control plane. AgentTLSSecretName string + // NGINXSCCName is the name of the SecurityContextConstraints for the NGINX Pods. Only applicable in OpenShift. + NGINXSCCName string // NginxDockerSecretNames are the names of any Docker registry Secrets for the NGINX container. NginxDockerSecretNames []string // LeaderElection contains the configuration for leader election. diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index bc68ba5510..7b3d28f140 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -14,6 +14,7 @@ import ( authv1 "k8s.io/api/authentication/v1" apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + rbacv1 "k8s.io/api/rbac/v1" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -93,6 +94,7 @@ func init() { utilruntime.Must(apiext.AddToScheme(scheme)) utilruntime.Must(appsv1.AddToScheme(scheme)) utilruntime.Must(authv1.AddToScheme(scheme)) + utilruntime.Must(rbacv1.AddToScheme(scheme)) } func StartManager(cfg config.Config) error { @@ -216,6 +218,7 @@ func StartManager(cfg config.Config) error { GatewayPodConfig: &cfg.GatewayPodConfig, GCName: cfg.GatewayClassName, AgentTLSSecretName: cfg.AgentTLSSecretName, + NGINXSCCName: cfg.NGINXSCCName, Plus: cfg.Plus, NginxDockerSecretNames: cfg.NginxDockerSecretNames, PlusUsageConfig: &cfg.UsageReportConfig, diff --git a/internal/mode/static/provisioner/eventloop.go b/internal/mode/static/provisioner/eventloop.go index e03a79ade8..5c5d4bea49 100644 --- a/internal/mode/static/provisioner/eventloop.go +++ b/internal/mode/static/provisioner/eventloop.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -31,6 +32,7 @@ func newEventLoop( dockerSecrets []string, agentTLSSecret string, usageConfig *config.UsageReportConfig, + isOpenshift bool, ) (*events.EventLoop, error) { nginxResourceLabelPredicate := predicate.NginxLabelPredicate(selector) @@ -50,11 +52,12 @@ func newEventLoop( } } - controllerRegCfgs := []struct { + type ctlrCfg struct { objectType ngftypes.ObjectType - name string options []controller.Option - }{ + } + + controllerRegCfgs := []ctlrCfg{ { objectType: &gatewayv1.Gateway{}, }, @@ -118,6 +121,33 @@ func newEventLoop( }, } + if isOpenshift { + controllerRegCfgs = append(controllerRegCfgs, + ctlrCfg{ + objectType: &rbacv1.Role{}, + options: []controller.Option{ + controller.WithK8sPredicate( + k8spredicate.And( + k8spredicate.GenerationChangedPredicate{}, + nginxResourceLabelPredicate, + ), + ), + }, + }, + ctlrCfg{ + objectType: &rbacv1.RoleBinding{}, + options: []controller.Option{ + controller.WithK8sPredicate( + k8spredicate.And( + k8spredicate.GenerationChangedPredicate{}, + nginxResourceLabelPredicate, + ), + ), + }, + }, + ) + } + eventCh := make(chan any) for _, regCfg := range controllerRegCfgs { gvk, err := apiutil.GVKForObject(regCfg.objectType, mgr.GetScheme()) @@ -137,19 +167,28 @@ func newEventLoop( } } + objectList := []client.ObjectList{ + // GatewayList MUST be first in this list to ensure that we see it before attempting + // to provision or deprovision any nginx resources. + &gatewayv1.GatewayList{}, + &appsv1.DeploymentList{}, + &corev1.ServiceList{}, + &corev1.ServiceAccountList{}, + &corev1.ConfigMapList{}, + &corev1.SecretList{}, + } + + if isOpenshift { + objectList = append(objectList, + &rbacv1.RoleList{}, + &rbacv1.RoleBindingList{}, + ) + } + firstBatchPreparer := events.NewFirstEventBatchPreparerImpl( mgr.GetCache(), []client.Object{}, - []client.ObjectList{ - // GatewayList MUST be first in this list to ensure that we see it before attempting - // to provision or deprovision any nginx resources. - &gatewayv1.GatewayList{}, - &appsv1.DeploymentList{}, - &corev1.ServiceList{}, - &corev1.ServiceAccountList{}, - &corev1.ConfigMapList{}, - &corev1.SecretList{}, - }, + objectList, ) eventLoop := events.NewEventLoop( diff --git a/internal/mode/static/provisioner/handler.go b/internal/mode/static/provisioner/handler.go index 5058d25771..ef6ba76b82 100644 --- a/internal/mode/static/provisioner/handler.go +++ b/internal/mode/static/provisioner/handler.go @@ -9,6 +9,7 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -59,7 +60,7 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, switch obj := e.Resource.(type) { case *gatewayv1.Gateway: h.store.updateGateway(obj) - case *appsv1.Deployment, *corev1.ServiceAccount, *corev1.ConfigMap: + case *appsv1.Deployment, *corev1.ServiceAccount, *corev1.ConfigMap, *rbacv1.Role, *rbacv1.RoleBinding: objLabels := labels.Set(obj.GetLabels()) if h.labelSelector.Matches(objLabels) { gatewayName := objLabels.Get(controller.GatewayLabel) @@ -114,7 +115,8 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, logger.Error(err, "error deprovisioning nginx resources") } h.store.deleteGateway(e.NamespacedName) - case *appsv1.Deployment, *corev1.Service, *corev1.ServiceAccount, *corev1.ConfigMap: + case *appsv1.Deployment, *corev1.Service, *corev1.ServiceAccount, + *corev1.ConfigMap, *rbacv1.Role, *rbacv1.RoleBinding: if err := h.reprovisionResources(ctx, e); err != nil { logger.Error(err, "error re-provisioning nginx resources") } diff --git a/internal/mode/static/provisioner/objects.go b/internal/mode/static/provisioner/objects.go index b62dc3b1f4..7c058bf784 100644 --- a/internal/mode/static/provisioner/objects.go +++ b/internal/mode/static/provisioner/objects.go @@ -11,6 +11,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -119,6 +120,11 @@ func (p *NginxProvisioner) buildNginxResourceObjects( ObjectMeta: objectMeta, } + var openshiftObjs []client.Object + if p.isOpenshift { + openshiftObjs = p.buildOpenshiftObjects(objectMeta) + } + ports := make(map[int32]struct{}) for _, listener := range gateway.Spec.Listeners { ports[int32(listener.Port)] = struct{}{} @@ -140,17 +146,21 @@ func (p *NginxProvisioner) buildNginxResourceObjects( ) // order to install resources: - // scc (if openshift) // secrets // configmaps // serviceaccount + // role/binding (if openshift) // service // deployment/daemonset - objects := make([]client.Object, 0, len(configmaps)+len(secrets)+3) + objects := make([]client.Object, 0, len(configmaps)+len(secrets)+len(openshiftObjs)+3) objects = append(objects, secrets...) objects = append(objects, configmaps...) - objects = append(objects, serviceAccount, service, deployment) + objects = append(objects, serviceAccount) + if p.isOpenshift { + objects = append(objects, openshiftObjs...) + } + objects = append(objects, service, deployment) return objects, err } @@ -366,6 +376,37 @@ func (p *NginxProvisioner) buildNginxConfigMaps( return []client.Object{bootstrapCM, agentCM} } +func (p *NginxProvisioner) buildOpenshiftObjects(objectMeta metav1.ObjectMeta) []client.Object { + role := &rbacv1.Role{ + ObjectMeta: objectMeta, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{"security.openshift.io"}, + ResourceNames: []string{p.cfg.NGINXSCCName}, + Resources: []string{"securitycontextconstraints"}, + Verbs: []string{"use"}, + }, + }, + } + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: objectMeta, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: objectMeta.Name, + }, + Subjects: []rbacv1.Subject{ + { + Kind: rbacv1.ServiceAccountKind, + Name: objectMeta.Name, + Namespace: objectMeta.Namespace, + }, + }, + } + + return []client.Object{role, roleBinding} +} + func buildNginxService( objectMeta metav1.ObjectMeta, nProxyCfg *graph.EffectiveNginxProxy, @@ -606,6 +647,10 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec( }, ImagePullSecrets: []corev1.LocalObjectReference{}, ServiceAccountName: objectMeta.Name, + SecurityContext: &corev1.PodSecurityContext{ + FSGroup: helpers.GetPointer[int64](1001), + RunAsNonRoot: helpers.GetPointer(true), + }, Volumes: []corev1.Volume{ { Name: "token", @@ -812,10 +857,10 @@ func (p *NginxProvisioner) buildNginxResourceObjectsForDeletion(deploymentNSName // order to delete: // deployment/daemonset // service + // role/binding (if openshift) // serviceaccount // configmaps // secrets - // scc (if openshift) objectMeta := metav1.ObjectMeta{ Name: deploymentNSName.Name, @@ -828,6 +873,19 @@ func (p *NginxProvisioner) buildNginxResourceObjectsForDeletion(deploymentNSName service := &corev1.Service{ ObjectMeta: objectMeta, } + + objects := []client.Object{deployment, service} + + if p.isOpenshift { + role := &rbacv1.Role{ + ObjectMeta: objectMeta, + } + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: objectMeta, + } + objects = append(objects, role, roleBinding) + } + serviceAccount := &corev1.ServiceAccount{ ObjectMeta: objectMeta, } @@ -844,7 +902,7 @@ func (p *NginxProvisioner) buildNginxResourceObjectsForDeletion(deploymentNSName }, } - objects := []client.Object{deployment, service, serviceAccount, bootstrapCM, agentCM} + objects = append(objects, serviceAccount, bootstrapCM, agentCM) agentTLSSecretName := controller.CreateNginxResourceName( deploymentNSName.Name, diff --git a/internal/mode/static/provisioner/objects_test.go b/internal/mode/static/provisioner/objects_test.go index 907a7e9fad..0871f846c9 100644 --- a/internal/mode/static/provisioner/objects_test.go +++ b/internal/mode/static/provisioner/objects_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -592,6 +593,65 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) { })) } +func TestBuildNginxResourceObjects_OpenShift(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + agentTLSSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: agentTLSTestSecretName, + Namespace: ngfNamespace, + }, + Data: map[string][]byte{"tls.crt": []byte("tls")}, + } + fakeClient := fake.NewFakeClient(agentTLSSecret) + + provisioner := &NginxProvisioner{ + isOpenshift: true, + cfg: Config{ + GatewayPodConfig: &config.GatewayPodConfig{ + Namespace: ngfNamespace, + }, + AgentTLSSecretName: agentTLSTestSecretName, + }, + k8sClient: fakeClient, + baseLabelSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "nginx", + }, + }, + } + + gateway := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "default", + }, + } + + resourceName := "gw-nginx" + objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, &graph.EffectiveNginxProxy{}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(objects).To(HaveLen(8)) + + expLabels := map[string]string{ + "app": "nginx", + "gateway.networking.k8s.io/gateway-name": "gw", + "app.kubernetes.io/name": "gw-nginx", + } + + roleObj := objects[4] + role, ok := roleObj.(*rbacv1.Role) + g.Expect(ok).To(BeTrue()) + g.Expect(role.GetLabels()).To(Equal(expLabels)) + + roleBindingObj := objects[5] + roleBinding, ok := roleBindingObj.(*rbacv1.RoleBinding) + g.Expect(ok).To(BeTrue()) + g.Expect(roleBinding.GetLabels()).To(Equal(expLabels)) +} + func TestGetAndUpdateSecret_NotFound(t *testing.T) { t.Parallel() g := NewWithT(t) @@ -754,3 +814,34 @@ func TestBuildNginxResourceObjectsForDeletion_Plus(t *testing.T) { provisioner.cfg.PlusUsageConfig.ClientSSLSecretName, )) } + +func TestBuildNginxResourceObjectsForDeletion_OpenShift(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + provisioner := &NginxProvisioner{isOpenshift: true} + + deploymentNSName := types.NamespacedName{ + Name: "gw-nginx", + Namespace: "default", + } + + objects := provisioner.buildNginxResourceObjectsForDeletion(deploymentNSName) + + g.Expect(objects).To(HaveLen(8)) + + validateMeta := func(obj client.Object, name string) { + g.Expect(obj.GetName()).To(Equal(name)) + g.Expect(obj.GetNamespace()).To(Equal(deploymentNSName.Namespace)) + } + + roleObj := objects[2] + role, ok := roleObj.(*rbacv1.Role) + g.Expect(ok).To(BeTrue()) + validateMeta(role, deploymentNSName.Name) + + roleBindingObj := objects[3] + roleBinding, ok := roleBindingObj.(*rbacv1.RoleBinding) + g.Expect(ok).To(BeTrue()) + validateMeta(roleBinding, deploymentNSName.Name) +} diff --git a/internal/mode/static/provisioner/openshift/openshift.go b/internal/mode/static/provisioner/openshift/openshift.go new file mode 100644 index 0000000000..3c89c4c988 --- /dev/null +++ b/internal/mode/static/provisioner/openshift/openshift.go @@ -0,0 +1,38 @@ +package openshift + +import ( + "fmt" + + "k8s.io/client-go/discovery" + "k8s.io/client-go/rest" +) + +//go:generate go tool counterfeiter -generate + +//counterfeiter:generate . APIChecker + +type APIChecker interface { + IsOpenshift(*rest.Config) (bool, error) +} + +type APICheckerImpl struct{} + +func (o *APICheckerImpl) IsOpenshift(config *rest.Config) (bool, error) { + discoveryClient, err := discovery.NewDiscoveryClientForConfig(config) + if err != nil { + return false, fmt.Errorf("error creating discovery client: %w", err) + } + + apiList, err := discoveryClient.ServerGroups() + if err != nil { + return false, fmt.Errorf("error getting server groups: %w", err) + } + + for _, group := range apiList.Groups { + if group.Name == "security.openshift.io" { + return true, nil + } + } + + return false, nil +} diff --git a/internal/mode/static/provisioner/openshift/openshiftfakes/fake_apichecker.go b/internal/mode/static/provisioner/openshift/openshiftfakes/fake_apichecker.go new file mode 100644 index 0000000000..d1e108544d --- /dev/null +++ b/internal/mode/static/provisioner/openshift/openshiftfakes/fake_apichecker.go @@ -0,0 +1,117 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package openshiftfakes + +import ( + "sync" + + "github.com/nginx/nginx-gateway-fabric/internal/mode/static/provisioner/openshift" + "k8s.io/client-go/rest" +) + +type FakeAPIChecker struct { + IsOpenshiftStub func(*rest.Config) (bool, error) + isOpenshiftMutex sync.RWMutex + isOpenshiftArgsForCall []struct { + arg1 *rest.Config + } + isOpenshiftReturns struct { + result1 bool + result2 error + } + isOpenshiftReturnsOnCall map[int]struct { + result1 bool + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeAPIChecker) IsOpenshift(arg1 *rest.Config) (bool, error) { + fake.isOpenshiftMutex.Lock() + ret, specificReturn := fake.isOpenshiftReturnsOnCall[len(fake.isOpenshiftArgsForCall)] + fake.isOpenshiftArgsForCall = append(fake.isOpenshiftArgsForCall, struct { + arg1 *rest.Config + }{arg1}) + stub := fake.IsOpenshiftStub + fakeReturns := fake.isOpenshiftReturns + fake.recordInvocation("IsOpenshift", []interface{}{arg1}) + fake.isOpenshiftMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeAPIChecker) IsOpenshiftCallCount() int { + fake.isOpenshiftMutex.RLock() + defer fake.isOpenshiftMutex.RUnlock() + return len(fake.isOpenshiftArgsForCall) +} + +func (fake *FakeAPIChecker) IsOpenshiftCalls(stub func(*rest.Config) (bool, error)) { + fake.isOpenshiftMutex.Lock() + defer fake.isOpenshiftMutex.Unlock() + fake.IsOpenshiftStub = stub +} + +func (fake *FakeAPIChecker) IsOpenshiftArgsForCall(i int) *rest.Config { + fake.isOpenshiftMutex.RLock() + defer fake.isOpenshiftMutex.RUnlock() + argsForCall := fake.isOpenshiftArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeAPIChecker) IsOpenshiftReturns(result1 bool, result2 error) { + fake.isOpenshiftMutex.Lock() + defer fake.isOpenshiftMutex.Unlock() + fake.IsOpenshiftStub = nil + fake.isOpenshiftReturns = struct { + result1 bool + result2 error + }{result1, result2} +} + +func (fake *FakeAPIChecker) IsOpenshiftReturnsOnCall(i int, result1 bool, result2 error) { + fake.isOpenshiftMutex.Lock() + defer fake.isOpenshiftMutex.Unlock() + fake.IsOpenshiftStub = nil + if fake.isOpenshiftReturnsOnCall == nil { + fake.isOpenshiftReturnsOnCall = make(map[int]struct { + result1 bool + result2 error + }) + } + fake.isOpenshiftReturnsOnCall[i] = struct { + result1 bool + result2 error + }{result1, result2} +} + +func (fake *FakeAPIChecker) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.isOpenshiftMutex.RLock() + defer fake.isOpenshiftMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeAPIChecker) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ openshift.APIChecker = new(FakeAPIChecker) diff --git a/internal/mode/static/provisioner/provisioner.go b/internal/mode/static/provisioner/provisioner.go index e4a0ce7bee..2eab9fd86f 100644 --- a/internal/mode/static/provisioner/provisioner.go +++ b/internal/mode/static/provisioner/provisioner.go @@ -27,6 +27,7 @@ import ( "github.com/nginx/nginx-gateway-fabric/internal/framework/events" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/nginx/agent" + "github.com/nginx/nginx-gateway-fabric/internal/mode/static/provisioner/openshift" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/status" ) @@ -44,6 +45,7 @@ type Provisioner interface { type Config struct { GCName string AgentTLSSecretName string + NGINXSCCName string DeploymentStore agent.DeploymentStorer StatusQueue *status.Queue @@ -66,10 +68,13 @@ type NginxProvisioner struct { baseLabelSelector metav1.LabelSelector cfg Config leader bool + isOpenshift bool lock sync.RWMutex } +var apiChecker openshift.APIChecker = &openshift.APICheckerImpl{} + // NewNginxProvisioner returns a new instance of a Provisioner that will deploy nginx resources. func NewNginxProvisioner( ctx context.Context, @@ -82,6 +87,7 @@ func NewNginxProvisioner( caSecretName = cfg.PlusUsageConfig.CASecretName clientSSLSecretName = cfg.PlusUsageConfig.ClientSSLSecretName } + store := newStore( cfg.NginxDockerSecretNames, cfg.AgentTLSSecretName, @@ -100,12 +106,18 @@ func NewNginxProvisioner( }, } + isOpenshift, err := apiChecker.IsOpenshift(mgr.GetConfig()) + if err != nil { + return nil, nil, err + } + provisioner := &NginxProvisioner{ k8sClient: mgr.GetClient(), store: store, baseLabelSelector: selector, resourcesToDeleteOnStartup: []types.NamespacedName{}, cfg: cfg, + isOpenshift: isOpenshift, } handler, err := newEventHandler(store, provisioner, selector, cfg.GCName) @@ -123,6 +135,7 @@ func NewNginxProvisioner( cfg.NginxDockerSecretNames, cfg.AgentTLSSecretName, cfg.PlusUsageConfig, + isOpenshift, ) if err != nil { return nil, nil, err diff --git a/internal/mode/static/provisioner/provisioner_test.go b/internal/mode/static/provisioner/provisioner_test.go index 987b835352..d89caefade 100644 --- a/internal/mode/static/provisioner/provisioner_test.go +++ b/internal/mode/static/provisioner/provisioner_test.go @@ -24,6 +24,7 @@ import ( "github.com/nginx/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/nginx/agent/agentfakes" + "github.com/nginx/nginx-gateway-fabric/internal/mode/static/provisioner/openshift/openshiftfakes" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/state/graph" ) @@ -202,6 +203,7 @@ func TestNewNginxProvisioner(t *testing.T) { Logger: logr.Discard(), } + apiChecker = &openshiftfakes.FakeAPIChecker{} provisioner, eventLoop, err := NewNginxProvisioner(context.TODO(), mgr, cfg) g.Expect(err).ToNot(HaveOccurred()) g.Expect(provisioner).NotTo(BeNil()) diff --git a/internal/mode/static/provisioner/setter.go b/internal/mode/static/provisioner/setter.go index dfe42321bc..3d5c84e780 100644 --- a/internal/mode/static/provisioner/setter.go +++ b/internal/mode/static/provisioner/setter.go @@ -3,6 +3,7 @@ package provisioner import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -20,6 +21,10 @@ func objectSpecSetter(object client.Object) controllerutil.MutateFn { return configMapSpecSetter(obj, obj.Data) case *corev1.Secret: return secretSpecSetter(obj, obj.Data) + case *rbacv1.Role: + return roleSpecSetter(obj, obj.Rules) + case *rbacv1.RoleBinding: + return roleBindingSpecSetter(obj, obj.RoleRef, obj.Subjects) } return nil @@ -52,3 +57,22 @@ func secretSpecSetter(secret *corev1.Secret, data map[string][]byte) controlleru return nil } } + +func roleSpecSetter(role *rbacv1.Role, rules []rbacv1.PolicyRule) controllerutil.MutateFn { + return func() error { + role.Rules = rules + return nil + } +} + +func roleBindingSpecSetter( + roleBinding *rbacv1.RoleBinding, + roleRef rbacv1.RoleRef, + subjects []rbacv1.Subject, +) controllerutil.MutateFn { + return func() error { + roleBinding.RoleRef = roleRef + roleBinding.Subjects = subjects + return nil + } +} diff --git a/internal/mode/static/provisioner/store.go b/internal/mode/static/provisioner/store.go index e28a42e103..0af617852c 100644 --- a/internal/mode/static/provisioner/store.go +++ b/internal/mode/static/provisioner/store.go @@ -7,6 +7,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -21,6 +22,8 @@ type NginxResources struct { Deployment metav1.ObjectMeta Service metav1.ObjectMeta ServiceAccount metav1.ObjectMeta + Role metav1.ObjectMeta + RoleBinding metav1.ObjectMeta BootstrapConfigMap metav1.ObjectMeta AgentConfigMap metav1.ObjectMeta AgentTLSSecret metav1.ObjectMeta @@ -143,6 +146,22 @@ func (s *store) registerResourceInGatewayConfig(gatewayNSName types.NamespacedNa } else { cfg.ServiceAccount = obj.ObjectMeta } + case *rbacv1.Role: + if cfg, ok := s.nginxResources[gatewayNSName]; !ok { + s.nginxResources[gatewayNSName] = &NginxResources{ + Role: obj.ObjectMeta, + } + } else { + cfg.Role = obj.ObjectMeta + } + case *rbacv1.RoleBinding: + if cfg, ok := s.nginxResources[gatewayNSName]; !ok { + s.nginxResources[gatewayNSName] = &NginxResources{ + RoleBinding: obj.ObjectMeta, + } + } else { + cfg.RoleBinding = obj.ObjectMeta + } case *corev1.ConfigMap: s.registerConfigMapInGatewayConfig(obj, gatewayNSName) case *corev1.Secret: @@ -260,6 +279,7 @@ func (s *store) deleteResourcesForGateway(nsName types.NamespacedName) { delete(s.nginxResources, nsName) } +//nolint:gocyclo // will refactor at some point func (s *store) gatewayExistsForResource(object client.Object, nsName types.NamespacedName) *graph.Gateway { s.lock.RLock() defer s.lock.RUnlock() @@ -278,6 +298,14 @@ func (s *store) gatewayExistsForResource(object client.Object, nsName types.Name if resourceMatches(resources.ServiceAccount, nsName) { return resources.Gateway } + case *rbacv1.Role: + if resourceMatches(resources.Role, nsName) { + return resources.Gateway + } + case *rbacv1.RoleBinding: + if resourceMatches(resources.RoleBinding, nsName) { + return resources.Gateway + } case *corev1.ConfigMap: if resourceMatches(resources.BootstrapConfigMap, nsName) { return resources.Gateway diff --git a/internal/mode/static/provisioner/store_test.go b/internal/mode/static/provisioner/store_test.go index 814ef1bd79..59e7da1207 100644 --- a/internal/mode/static/provisioner/store_test.go +++ b/internal/mode/static/provisioner/store_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -167,6 +168,30 @@ func TestRegisterResourceInGatewayConfig(t *testing.T) { // clear out resources before next test store.deleteResourcesForGateway(nsName) + // Role + role := &rbacv1.Role{ObjectMeta: defaultMeta} + resources = registerAndGetResources(role) + g.Expect(resources.Role).To(Equal(defaultMeta)) + + // Role again, already exists + resources = registerAndGetResources(role) + g.Expect(resources.Role).To(Equal(defaultMeta)) + + // clear out resources before next test + store.deleteResourcesForGateway(nsName) + + // RoleBinding + roleBinding := &rbacv1.RoleBinding{ObjectMeta: defaultMeta} + resources = registerAndGetResources(roleBinding) + g.Expect(resources.RoleBinding).To(Equal(defaultMeta)) + + // RoleBinding again, already exists + resources = registerAndGetResources(roleBinding) + g.Expect(resources.RoleBinding).To(Equal(defaultMeta)) + + // clear out resources before next test + store.deleteResourcesForGateway(nsName) + // ConfigMap bootstrapCMMeta := metav1.ObjectMeta{ Name: controller.CreateNginxResourceName(defaultMeta.Name, nginxIncludesConfigMapNameSuffix), @@ -406,6 +431,14 @@ func TestGatewayExistsForResource(t *testing.T) { Name: "test-serviceaccount", Namespace: "default", }, + Role: metav1.ObjectMeta{ + Name: "test-role", + Namespace: "default", + }, + RoleBinding: metav1.ObjectMeta{ + Name: "test-rolebinding", + Namespace: "default", + }, BootstrapConfigMap: metav1.ObjectMeta{ Name: "test-bootstrap-configmap", Namespace: "default", @@ -473,6 +506,26 @@ func TestGatewayExistsForResource(t *testing.T) { }, expected: gateway, }, + { + name: "Role exists", + object: &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-role", + Namespace: "default", + }, + }, + expected: gateway, + }, + { + name: "RoleBinding exists", + object: &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rolebinding", + Namespace: "default", + }, + }, + expected: gateway, + }, { name: "Bootstrap ConfigMap exists", object: &corev1.ConfigMap{ From e1276de11ffc55d4bca6b4bb67c3a7c0c2f60ea4 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Wed, 2 Apr 2025 08:17:35 -0600 Subject: [PATCH 2/2] Log instead of error --- internal/mode/static/provisioner/provisioner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/provisioner/provisioner.go b/internal/mode/static/provisioner/provisioner.go index 2eab9fd86f..3151d56c4f 100644 --- a/internal/mode/static/provisioner/provisioner.go +++ b/internal/mode/static/provisioner/provisioner.go @@ -108,7 +108,7 @@ func NewNginxProvisioner( isOpenshift, err := apiChecker.IsOpenshift(mgr.GetConfig()) if err != nil { - return nil, nil, err + cfg.Logger.Error(err, "could not determine if running in openshift, will not create Role/RoleBinding") } provisioner := &NginxProvisioner{