Skip to content

OCPBUGS-58313: Admit sysctls based on the worker node kernel version instead of the current node kernel version #151

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.

Sign up for GitHub

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

Open
jubittajohn wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-58313: Admit sysctls based on the worker node kernel version instead of the current node kernel version #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 11 additions & 2 deletions pkg/securitycontextconstraints/sccadmission/admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type constraint struct {
*admission.Handler
sccLister securityv1listers.SecurityContextConstraintsLister
namespaceLister corev1listers.NamespaceLister
nodeLister corev1listers.NodeLister
listersSynced []cache.InformerSynced
authorizer authorizer.Authorizer
}
Expand Down Expand Up @@ -213,7 +214,7 @@ func (c *constraint) computeSecurityContext(
return nil, nil, nil, admission.NewForbidden(a, err)
}

providers, errs := sccmatching.CreateProvidersFromConstraints(ctx, a.GetNamespace(), constraints, c.namespaceLister)
providers, errs := sccmatching.CreateProvidersFromConstraints(ctx, a.GetNamespace(), constraints, c.namespaceLister, c.nodeLister)

@ibihim ibihim Nov 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From a code / maintenance perspective, I must say that handing the nodeLister 6 levels down doesn't look good. We are coupling all those functions with the nodeLister. E.g.:

func NewSimpleProvider(
  scc *securityv1.SecurityContextConstraints,
  nodeLister corev1listers.NodeLister,
) (SecurityContextConstraintsProvider, error)

It reads well if you transform a scc into a provider, but now you have a scc and a nodeLister?!

Couldn't we check the legit sysctls and extend the constraints.AllowedUnsafeSysctls or adjust the SimpleProvider to hold those specificly, so that we don't pass c.nodeLister down? It would read better like so:

func NewSimpleProvider(
  scc *securityv1.SecurityContextConstraints,
  availableSysCtls []string,
) (SecurityContextConstraintsProvider, error)

or

// Though this taints the clear meaning of what is defined by the user and what is possible by the system.
NewSimpleProvider(
  updateSysctlsBasedOnSafeWhitelist(scc),
)

Everything else might be more effort, like some Factory or so.

WDYT?

@jubittajohn jubittajohn Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestion.

I've refactored NewSimpleProvider to take availableSysCtls []string instead of nodeLister.

logProviders(pod, providers, errs)
if len(errs) > 0 {
return nil, nil, nil, kutilerrors.NewAggregate(errs)
Expand Down Expand Up @@ -519,7 +520,12 @@ func (c *constraint) SetSecurityInformers(informers securityv1informer.SecurityC

func (c *constraint) SetExternalKubeInformerFactory(informers informers.SharedInformerFactory) {
c.namespaceLister = informers.Core().V1().Namespaces().Lister()
c.listersSynced = append(c.listersSynced, informers.Core().V1().Namespaces().Informer().HasSynced)
c.nodeLister = informers.Core().V1().Nodes().Lister()
c.listersSynced = append(
c.listersSynced,
informers.Core().V1().Namespaces().Informer().HasSynced,
informers.Core().V1().Nodes().Informer().HasSynced,
)
}

func (c *constraint) SetAuthorizer(authorizer authorizer.Authorizer) {
Expand All @@ -537,6 +543,9 @@ func (c *constraint) ValidateInitialization() error {
if c.namespaceLister == nil {
return fmt.Errorf("%s requires a namespaceLister", PluginName)
}
if c.nodeLister == nil {
return fmt.Errorf("%s requires a nodeLister", PluginName)
}
if c.authorizer == nil {
return fmt.Errorf("%s requires an authorizer", PluginName)
}
Expand Down
25 changes: 23 additions & 2 deletions pkg/securitycontextconstraints/sccadmission/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,29 @@ import (
securityv1listers "github.com/openshift/client-go/security/listers/security/v1"

"github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sccmatching"
"github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sysctl"
sccsort "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/util/sort"
)

// testNodeLister is a simple implementation for testing that returns empty node list
type testNodeLister struct{}

func (f *testNodeLister) List(selector labels.Selector) ([]*corev1.Node, error) {
return []*corev1.Node{}, nil
}

func (f *testNodeLister) Get(name string) (*corev1.Node, error) {
return nil, fmt.Errorf("node %s not found", name)
}

var _ corev1listers.NodeLister = &testNodeLister{}

// getTestSysctls returns a default set of sysctls for testing purposes.
// This mimics the behavior of sysctl.SafeSysctlAllowlist with a testNodeLister.
func getTestSysctls() []string {
return sysctl.SafeSysctlAllowlist(&testNodeLister{})
}

// createSAForTest Build and Initializes a ServiceAccount for tests
func createSAForTest() *corev1.ServiceAccount {
return &corev1.ServiceAccount{
Expand Down Expand Up @@ -60,6 +80,7 @@ func newTestAdmission(sccLister securityv1listers.SecurityContextConstraintsList
return &constraint{
Handler: admission.NewHandler(admission.Create),
namespaceLister: nsLister,
nodeLister: &testNodeLister{},
sccLister: sccLister,
listersSynced: []cache.InformerSynced{func() bool { return true }},
authorizer: authorizer,
Expand Down Expand Up @@ -987,7 +1008,7 @@ func TestCreateProvidersFromConstraints(t *testing.T) {
// let timeout based failures fail fast
ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond)
defer cancel()
_, errs := sccmatching.CreateProvidersFromConstraints(ctx, attributes.GetNamespace(), []*securityv1.SecurityContextConstraints{scc}, nsLister)
_, errs := sccmatching.CreateProvidersFromConstraints(ctx, attributes.GetNamespace(), []*securityv1.SecurityContextConstraints{scc}, nsLister, &testNodeLister{})

if !reflect.DeepEqual(scc, v.scc()) {
diff := diff.Diff(scc, v.scc())
Expand Down Expand Up @@ -1390,7 +1411,7 @@ func TestAdmitPreferNonmutatingWhenPossible(t *testing.T) {
},
}

mutatingProvider, err := sccmatching.NewSimpleProvider(mutatingSCC)
mutatingProvider, err := sccmatching.NewSimpleProvider(mutatingSCC, getTestSysctls())
if err != nil {
t.Fatalf("failed to create a mutating provider: %v", err)
}
Expand Down
21 changes: 19 additions & 2 deletions pkg/securitycontextconstraints/sccadmission/scc_authz_check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,31 @@ package sccadmission

import (
"context"
"fmt"
"testing"

"github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sccmatching"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/authentication/user"
corev1listers "k8s.io/client-go/listers/core/v1"
coreapi "k8s.io/kubernetes/pkg/apis/core"
)

// fakeNodeLister is a simple implementation for testing that returns empty node list
type fakeNodeLister struct{}

func (f *fakeNodeLister) List(selector labels.Selector) ([]*corev1.Node, error) {
return []*corev1.Node{}, nil
}

func (f *fakeNodeLister) Get(name string) (*corev1.Node, error) {
return nil, fmt.Errorf("node %s not found", name)
}

var _ corev1listers.NodeLister = &fakeNodeLister{}

func TestSCCAuthorizationChecker(t *testing.T) {
userSCC := laxSCC()
userSCC.Name = "user-scc"
Expand Down Expand Up @@ -95,9 +112,9 @@ func TestSCCAuthorizationChecker(t *testing.T) {
var provider sccmatching.SecurityContextConstraintsProvider
var err error
if test.scc == "user-scc" {
provider, err = sccmatching.NewSimpleProvider(userSCC)
provider, err = sccmatching.NewSimpleProvider(userSCC, getTestSysctls())
} else {
provider, err = sccmatching.NewSimpleProvider(saSCC)
provider, err = sccmatching.NewSimpleProvider(saSCC, getTestSysctls())
}
if err != nil {
t.Fatalf("Error creating provider: %v", err)
Expand Down
9 changes: 5 additions & 4 deletions pkg/securitycontextconstraints/sccmatching/matcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"github.com/openshift/api/security"
securityv1 "github.com/openshift/api/security/v1"
"github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sysctl"
sccsort "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/util/sort"
securityv1listers "github.com/openshift/client-go/security/listers/security/v1"
"github.com/openshift/library-go/pkg/security/uid"
Expand Down Expand Up @@ -164,7 +165,7 @@ func constraintSupportsGroup(group string, constraintGroups []string) bool {

// CreateProvidersFromConstraints creates providers from the constraints supplied, including
// looking up pre-allocated values if necessary using the pod's namespace.
func CreateProvidersFromConstraints(ctx context.Context, namespaceName string, sccs []*securityv1.SecurityContextConstraints, namespaceLister corev1listers.NamespaceLister) ([]SecurityContextConstraintsProvider, []error) {
func CreateProvidersFromConstraints(ctx context.Context, namespaceName string, sccs []*securityv1.SecurityContextConstraints, namespaceLister corev1listers.NamespaceLister, nodeLister corev1listers.NodeLister) ([]SecurityContextConstraintsProvider, []error) {
var (
// namespace is declared here for reuse but we will not fetch it unless required by the matched constraints
namespace *corev1.Namespace
Expand Down Expand Up @@ -207,7 +208,7 @@ func CreateProvidersFromConstraints(ctx context.Context, namespaceName string, s
provider SecurityContextConstraintsProvider
err error
)
provider, err = CreateProviderFromConstraint(namespace, constraint)
provider, err = CreateProviderFromConstraint(namespace, constraint, nodeLister)
if err != nil {
errs = append(errs, err)
continue
Expand All @@ -218,7 +219,7 @@ func CreateProvidersFromConstraints(ctx context.Context, namespaceName string, s
}

// CreateProviderFromConstraint creates a SecurityContextConstraintProvider from a SecurityContextConstraint
func CreateProviderFromConstraint(namespace *corev1.Namespace, constraint *securityv1.SecurityContextConstraints) (SecurityContextConstraintsProvider, error) {
func CreateProviderFromConstraint(namespace *corev1.Namespace, constraint *securityv1.SecurityContextConstraints, nodeLister corev1listers.NodeLister) (SecurityContextConstraintsProvider, error) {
var err error

// Make a copy of the constraint so we don't mutate the store's cache
Expand Down Expand Up @@ -258,7 +259,7 @@ func CreateProviderFromConstraint(namespace *corev1.Namespace, constraint *secur
}

// Create the provider
provider, err := NewSimpleProvider(constraint)
provider, err := NewSimpleProvider(constraint, sysctl.SafeSysctlAllowlist(nodeLister))

@ibihim ibihim Jan 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This calculation doesn't change per request, right? So we could move it out of the for-loop, right?

Due to the fact that CreateProviderFromConstraint is in a for loop that iterates on sccs, we have:

O(SCCs * Nodes)

But we could have

O(SCCs) + O(Nodes)

If we move the check out of the for-loop.

It would be good to figure out how many nodes big clusters have as we could improve the performance drastically on clusters with plenty of Nodes.
If we cache the result and add an event listener that is being updated on Add, Update and Delete events of the nodeInformer.

This would create a O(SCCs) + O(1) situation per request.

With 10k Nodes and 10 SCCs those numbers change drastically:

  1. Current: 100k
  2. Outside the for-loop: 10,010
  3. Cache: 10 + per Node change with "per Node change" being a lot smaller than "per Pod admission"

if err != nil {
return nil, fmt.Errorf("error creating provider for SCC %s in namespace %s: %v", constraint.Name, namespace.GetName(), err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/securitycontextconstraints/sccmatching/matcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestAssignSecurityContext(t *testing.T) {
Type: securityv1.SupplementalGroupsStrategyRunAsAny,
},
}
provider, err := NewSimpleProvider(scc)
provider, err := NewSimpleProvider(scc, getTestSysctls())
if err != nil {
t.Fatalf("failed to create provider: %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/securitycontextconstraints/sccmatching/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type simpleProvider struct {
var _ SecurityContextConstraintsProvider = &simpleProvider{}

// NewSimpleProvider creates a new SecurityContextConstraintsProvider instance.
func NewSimpleProvider(scc *securityv1.SecurityContextConstraints) (SecurityContextConstraintsProvider, error) {
func NewSimpleProvider(scc *securityv1.SecurityContextConstraints, availableSysCtls []string) (SecurityContextConstraintsProvider, error) {
if scc == nil {
return nil, fmt.Errorf("NewSimpleProvider requires a SecurityContextConstraints")
}
Expand Down Expand Up @@ -79,7 +79,7 @@ func NewSimpleProvider(scc *securityv1.SecurityContextConstraints) (SecurityCont
return nil, err
}

sysctlsStrat, err := createSysctlsStrategy(sysctl.SafeSysctlAllowlist(), scc.AllowedUnsafeSysctls, scc.ForbiddenSysctls)
sysctlsStrat, err := createSysctlsStrategy(availableSysCtls, scc.AllowedUnsafeSysctls, scc.ForbiddenSysctls)
if err != nil {
return nil, err
}
Expand Down
50 changes: 36 additions & 14 deletions pkg/securitycontextconstraints/sccmatching/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,37 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/apimachinery/pkg/util/validation/field"
corev1listers "k8s.io/client-go/listers/core/v1"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/utils/pointer"

securityv1 "github.com/openshift/api/security/v1"
"github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sysctl"
sccutil "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/util"
)

// fakeNodeLister is a simple implementation for testing that returns empty node list
type fakeNodeLister struct{}

func (f *fakeNodeLister) List(selector labels.Selector) ([]*corev1.Node, error) {
return []*corev1.Node{}, nil
}

func (f *fakeNodeLister) Get(name string) (*corev1.Node, error) {
return nil, fmt.Errorf("node %s not found", name)
}

var _ corev1listers.NodeLister = &fakeNodeLister{}

// getTestSysctls returns a default set of sysctls for testing purposes.
// This mimics the behavior of sysctl.SafeSysctlAllowlist with a fakeNodeLister.
func getTestSysctls() []string {
return sysctl.SafeSysctlAllowlist(&fakeNodeLister{})
}

func TestCreatePodSecurityContextNonmutating(t *testing.T) {
// Create a pod with a security context that needs filling in
createPod := func() *api.Pod {
Expand Down Expand Up @@ -57,7 +79,7 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) {
pod := createPod()
scc := createSCC()

provider, err := NewSimpleProvider(scc)
provider, err := NewSimpleProvider(scc, getTestSysctls())
if err != nil {
t.Fatalf("unable to create provider %v", err)
}
Expand Down Expand Up @@ -114,7 +136,7 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) {
pod := createPod()
scc := createSCC()

provider, err := NewSimpleProvider(scc)
provider, err := NewSimpleProvider(scc, getTestSysctls())
if err != nil {
t.Fatalf("unable to create provider %v", err)
}
Expand Down Expand Up @@ -350,7 +372,7 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
},
}
for k, v := range errorCases {
provider, err := NewSimpleProvider(v.scc)
provider, err := NewSimpleProvider(v.scc, getTestSysctls())
if err != nil {
t.Fatalf("unable to create provider %v", err)
}
Expand Down Expand Up @@ -492,7 +514,7 @@ func TestValidateContainerSecurityContextFailures(t *testing.T) {
}

for k, v := range errorCases {
provider, err := NewSimpleProvider(v.scc)
provider, err := NewSimpleProvider(v.scc, getTestSysctls())
if err != nil {
t.Fatalf("unable to create provider %v", err)
}
Expand Down Expand Up @@ -713,7 +735,7 @@ func TestValidatePodSecurityContextSuccess(t *testing.T) {
}

for k, v := range successCases {
provider, err := NewSimpleProvider(v.scc)
provider, err := NewSimpleProvider(v.scc, getTestSysctls())
if err != nil {
t.Fatalf("unable to create provider %v", err)
}
Expand Down Expand Up @@ -885,7 +907,7 @@ func TestValidateContainerSecurityContextSuccess(t *testing.T) {
}

for k, v := range successCases {
provider, err := NewSimpleProvider(v.scc)
provider, err := NewSimpleProvider(v.scc, getTestSysctls())
if err != nil {
t.Fatalf("unable to create provider %v", err)
}
Expand Down Expand Up @@ -952,7 +974,7 @@ func TestGenerateContainerSecurityContextReadOnlyRootFS(t *testing.T) {
}

for k, v := range tests {
provider, err := NewSimpleProvider(v.scc)
provider, err := NewSimpleProvider(v.scc, getTestSysctls())
if err != nil {
t.Errorf("%s unable to create provider %v", k, err)
continue
Expand Down Expand Up @@ -1106,7 +1128,7 @@ func TestGenerateNonRootSecurityContextOnNonZeroRunAsUser(t *testing.T) {
}

for k, v := range tests {
provider, err := NewSimpleProvider(v.scc)
provider, err := NewSimpleProvider(v.scc, getTestSysctls())
if err != nil {
t.Errorf("%s unable to create provider %v", k, err)
continue
Expand Down Expand Up @@ -1218,7 +1240,7 @@ func TestValidateAllowedVolumes(t *testing.T) {
// create an SCC that allows no volumes
scc := defaultSCC()

provider, err := NewSimpleProvider(scc)
provider, err := NewSimpleProvider(scc, getTestSysctls())
if err != nil {
t.Errorf("error creating provider for %s: %s", fieldVal.Name, err.Error())
continue
Expand Down Expand Up @@ -1253,7 +1275,7 @@ func TestValidateAllowedVolumes(t *testing.T) {
func TestValidateProjectedVolume(t *testing.T) {
pod := defaultPod()
scc := defaultSCC()
provider, err := NewSimpleProvider(scc)
provider, err := NewSimpleProvider(scc, getTestSysctls())
require.NoError(t, err, "error creating provider")

tests := []struct {
Expand Down Expand Up @@ -1373,7 +1395,7 @@ func TestValidateAllowPrivilegeEscalation(t *testing.T) {
scc := defaultSCC()
scc.AllowPrivilegeEscalation = &no

provider, err := NewSimpleProvider(scc)
provider, err := NewSimpleProvider(scc, getTestSysctls())
if err != nil {
t.Errorf("error creating provider: %v", err.Error())
}
Expand Down Expand Up @@ -1417,17 +1439,17 @@ func TestValidateAllowPrivilegeEscalation(t *testing.T) {
}

func TestSeccompAnnotationsFieldsGeneration(t *testing.T) {
noSeccompProvider, err := NewSimpleProvider(defaultSCC())
noSeccompProvider, err := NewSimpleProvider(defaultSCC(), getTestSysctls())
require.NoError(t, err)

sccWildcardSeccomp := defaultSCC()
sccWildcardSeccomp.SeccompProfiles = []string{"*"}
wildcardSeccompProvider, err := NewSimpleProvider(sccWildcardSeccomp)
wildcardSeccompProvider, err := NewSimpleProvider(sccWildcardSeccomp, getTestSysctls())
require.NoError(t, err)

sccGenerateSeccomp := defaultSCC()
sccGenerateSeccomp.SeccompProfiles = []string{corev1.SeccompProfileRuntimeDefault}
generateSeccompProvider, err := NewSimpleProvider(sccGenerateSeccomp)
generateSeccompProvider, err := NewSimpleProvider(sccGenerateSeccomp, getTestSysctls())
require.NoError(t, err)

podPodSeccompAnnotation := defaultPod()
Expand Down
Loading