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.

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

Jump to bottom

OCPBUGS-17249: UPSTREAM: <carry>: openshift-kube-apiserver: pod .spec.nodeName should not override project node selector in podNodeEnvironment admission plugin #1787

Merged
merged 1 commit into from
Jan 11, 2024
Merged

OCPBUGS-17249: UPSTREAM: <carry>: openshift-kube-apiserver: pod .spec.nodeName should not override project node selector in podNodeEnvironment admission plugin #1787

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 31 additions & 5 deletions openshift-kube-apiserver/admission/scheduler/nodeenv/admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/admission/initializer"
"k8s.io/client-go/informers"
Expand All @@ -34,8 +35,10 @@ const (
// podNodeEnvironment is an implementation of admission.MutationInterface.
type podNodeEnvironment struct {
*admission.Handler
nsLister corev1listers.NamespaceLister
nsListerSynced func() bool
nsLister corev1listers.NamespaceLister
nsListerSynced func() bool
nodeLister corev1listers.NodeLister
nodeListerSynced func() bool
// TODO this should become a piece of config passed to the admission plugin
defaultNodeSelector string
}
Expand Down Expand Up @@ -72,7 +75,7 @@ func (p *podNodeEnvironment) admit(ctx context.Context, a admission.Attributes,
return apierrors.NewForbidden(resource, name, err)
}

// If scheduler.alpha.kubernetes.io/node-selector is set on the pod,
// If scheduler.alpha.kubernetes.io/node-selector is set on the namespace,
// do not process the pod further.
if _, ok := namespace.ObjectMeta.Annotations[kubeProjectNodeSelector]; ok {
return nil
Expand All @@ -82,6 +85,7 @@ func (p *podNodeEnvironment) admit(ctx context.Context, a admission.Attributes,
if projectNodeSelector, ok := namespace.ObjectMeta.Annotations[projectv1.ProjectNodeSelector]; ok {
selector = projectNodeSelector
}
// we might consider in the future to allow advanced syntax selectors and use labels.Parse here instead
projectNodeSelector, err := labelselector.Parse(selector)
if err != nil {
return err
Expand All @@ -96,6 +100,20 @@ func (p *podNodeEnvironment) admit(ctx context.Context, a admission.Attributes,
return apierrors.NewForbidden(resource, name, fmt.Errorf("pod node label selector does not extend project node label selector"))
}

if len(pod.Spec.NodeName) > 0 && len(projectNodeSelector) > 0 {
node, err := p.nodeLister.Get(pod.Spec.NodeName)
if err != nil {
return apierrors.NewForbidden(resource, name, fmt.Errorf("cannot validate project node label selector: %v", err))
}
projectNodeSelectorAdvanced, err := labels.Parse(selector)
if err != nil {
return err
}
if !projectNodeSelectorAdvanced.Matches(labels.Set(node.Labels)) {
return apierrors.NewForbidden(resource, name, fmt.Errorf("pod node name conflicts with project node label selector"))
}
}

// modify pod node selector = project node selector + current pod node selector
pod.Spec.NodeSelector = labelselector.Merge(projectNodeSelector, pod.Spec.NodeSelector)

Expand All @@ -117,14 +135,16 @@ func (p *podNodeEnvironment) SetDefaultNodeSelector(in string) {
func (p *podNodeEnvironment) SetExternalKubeInformerFactory(kubeInformers informers.SharedInformerFactory) {
p.nsLister = kubeInformers.Core().V1().Namespaces().Lister()
p.nsListerSynced = kubeInformers.Core().V1().Namespaces().Informer().HasSynced
p.nodeLister = kubeInformers.Core().V1().Nodes().Lister()
p.nodeListerSynced = kubeInformers.Core().V1().Nodes().Informer().HasSynced
}

func (p *podNodeEnvironment) waitForSyncedStore(timeout <-chan time.Time) bool {
for !p.nsListerSynced() {
for !p.nsListerSynced() || !p.nodeListerSynced() {
select {
case <-time.After(100 * time.Millisecond):
case <-timeout:
return p.nsListerSynced()
return p.nsListerSynced() && p.nodeListerSynced()
}
}

Expand All @@ -138,6 +158,12 @@ func (p *podNodeEnvironment) ValidateInitialization() error {
if p.nsListerSynced == nil {
return fmt.Errorf("project node environment plugin needs a namespace lister synced")
}
if p.nodeLister == nil {
return fmt.Errorf("project node environment plugin needs a node lister")
}
if p.nodeListerSynced == nil {
return fmt.Errorf("project node environment plugin needs a node lister synced")
}
return nil
}

Expand Down
53 changes: 52 additions & 1 deletion openshift-kube-apiserver/admission/scheduler/nodeenv/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ func TestPodAdmission(t *testing.T) {
},
}

node := &corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "worker-1",
Namespace: "",
Labels: map[string]string{
"worker": "true",
},
},
}

handler := &podNodeEnvironment{}
pod := &kapi.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "testPod"},
Expand All @@ -34,6 +44,7 @@ func TestPodAdmission(t *testing.T) {
defaultNodeSelector string
projectNodeSelector string
podNodeSelector map[string]string
podNodeName string
mergedNodeSelector map[string]string
ignoreProjectNodeSelector bool
admit bool
Expand Down Expand Up @@ -103,18 +114,58 @@ func TestPodAdmission(t *testing.T) {
admit: false,
testName: "Conflicting pod and project node selector, multiple labels",
},
{
defaultNodeSelector: "",
projectNodeSelector: "worker=true",
podNodeName: "worker-1",
podNodeSelector: nil,
mergedNodeSelector: map[string]string{"worker": "true"},
admit: true,
testName: "node referenced in pod.nodeName does not conflict with project node selector",
},
{
defaultNodeSelector: "",
projectNodeSelector: "",
podNodeName: "worker-1",
podNodeSelector: map[string]string{"worker": "false"},
mergedNodeSelector: map[string]string{"worker": "false"},
admit: true,
// default to kube behavior: let this fail by kubelet
testName: "node referenced in pod spec.nodeName can conflict with its own node selector when no project node selector is specified",
},
{
defaultNodeSelector: "worker = true",
projectNodeSelector: "worker=false",
podNodeName: "worker-1",
podNodeSelector: nil,
mergedNodeSelector: nil,
admit: false,
testName: "node referenced in pod spec.nodeName conflicts with project node selector",
},
{
defaultNodeSelector: "",
projectNodeSelector: "worker=true",
podNodeName: "worker-2",
podNodeSelector: nil,
mergedNodeSelector: nil,
admit: false,
testName: "missing node referenced in pod spec.nodeName does not admit",
},
}
for _, test := range tests {
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
indexer.Add(namespace)
indexer.Add(node)
handler.nsLister = corev1listers.NewNamespaceLister(indexer)
handler.nsListerSynced = func() bool { return true }
handler.nodeLister = corev1listers.NewNodeLister(indexer)
handler.nodeListerSynced = func() bool { return true }
handler.defaultNodeSelector = test.defaultNodeSelector

if !test.ignoreProjectNodeSelector {
namespace.ObjectMeta.Annotations = map[string]string{projectv1.ProjectNodeSelector: test.projectNodeSelector}
}
pod.Spec = kapi.PodSpec{NodeSelector: test.podNodeSelector}
pod.Spec = kapi.PodSpec{NodeSelector: test.podNodeSelector, NodeName: test.podNodeName}

attrs := admission.NewAttributesRecord(pod, nil, kapi.Kind("Pod").WithVersion("version"), "testProject", namespace.ObjectMeta.Name, kapi.Resource("pods").WithVersion("version"), "", admission.Create, nil, false, nil)
err := handler.Admit(context.TODO(), attrs, nil)
Expand Down