diff --git a/plugin/pkg/admission/podnodeselector/admission.go b/plugin/pkg/admission/podnodeselector/admission.go index 38763d5eac6..a8b6dd947e6 100644 --- a/plugin/pkg/admission/podnodeselector/admission.go +++ b/plugin/pkg/admission/podnodeselector/admission.go @@ -59,6 +59,8 @@ type podNodeSelector struct { clusterNodeSelectors map[string]string } +var _ admission.MutationInterface = &podNodeSelector{} +var _ admission.ValidationInterface = &podNodeSelector{} var _ = kubeapiserveradmission.WantsInternalKubeClientSet(&podNodeSelector{}) var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&podNodeSelector{}) @@ -93,26 +95,9 @@ func readConfig(config io.Reader) *pluginConfig { // Admit enforces that pod and its namespace node label selectors matches at least a node in the cluster. func (p *podNodeSelector) Admit(a admission.Attributes) error { - resource := a.GetResource().GroupResource() - if resource != api.Resource("pods") { + if shouldIgnore(a) { return nil } - if a.GetSubresource() != "" { - // only run the checks below on pods proper and not subresources - return nil - } - - obj := a.GetObject() - pod, ok := obj.(*api.Pod) - if !ok { - glog.Errorf("expected pod but got %s", a.GetKind().Kind) - return nil - } - - if !p.WaitForReady() { - return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request")) - } - updateInitialized, err := util.IsUpdatingInitializedObject(a) if err != nil { return err @@ -121,51 +106,96 @@ func (p *podNodeSelector) Admit(a admission.Attributes) error { // node selector of an initialized pod is immutable return nil } - - name := pod.Name - nsName := a.GetNamespace() - var namespace *api.Namespace - - namespace, err = p.namespaceLister.Get(nsName) - if errors.IsNotFound(err) { - namespace, err = p.defaultGetNamespace(nsName) - if err != nil { - if errors.IsNotFound(err) { - return err - } - return errors.NewInternalError(err) - } - } else if err != nil { - return errors.NewInternalError(err) + if !p.WaitForReady() { + return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request")) } - namespaceNodeSelector, err := p.getNodeSelectorMap(namespace) + resource := a.GetResource().GroupResource() + pod := a.GetObject().(*api.Pod) + namespaceNodeSelector, err := p.getNamespaceNodeSelectorMap(a.GetNamespace()) if err != nil { return err } if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) { - return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector")) - } - - whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[namespace.Name]) - if err != nil { - return err + return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector")) } // Merge pod node selector = namespace node selector + current pod node selector + // second selector wins podNodeSelectorLabels := labels.Merge(namespaceNodeSelector, pod.Spec.NodeSelector) + pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels) + return p.Validate(a) +} - // whitelist verification - if !labels.AreLabelsInWhiteList(podNodeSelectorLabels, whitelist) { - return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist")) +// Validate ensures that the pod node selector is allowed +func (p *podNodeSelector) Validate(a admission.Attributes) error { + if shouldIgnore(a) { + return nil + } + if !p.WaitForReady() { + return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request")) + } + + resource := a.GetResource().GroupResource() + pod := a.GetObject().(*api.Pod) + + namespaceNodeSelector, err := p.getNamespaceNodeSelectorMap(a.GetNamespace()) + if err != nil { + return err + } + if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) { + return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector")) + } + + // whitelist verification + whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[a.GetNamespace()]) + if err != nil { + return err + } + if !labels.AreLabelsInWhiteList(pod.Spec.NodeSelector, whitelist) { + return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist")) } - // Updated pod node selector = namespace node selector + current pod node selector - pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels) return nil } +func (p *podNodeSelector) getNamespaceNodeSelectorMap(namespaceName string) (labels.Set, error) { + namespace, err := p.namespaceLister.Get(namespaceName) + if errors.IsNotFound(err) { + namespace, err = p.defaultGetNamespace(namespaceName) + if err != nil { + if errors.IsNotFound(err) { + return nil, err + } + return nil, errors.NewInternalError(err) + } + } else if err != nil { + return nil, errors.NewInternalError(err) + } + + return p.getNodeSelectorMap(namespace) +} + +func shouldIgnore(a admission.Attributes) bool { + resource := a.GetResource().GroupResource() + if resource != api.Resource("pods") { + return true + } + if a.GetSubresource() != "" { + // only run the checks below on pods proper and not subresources + return true + } + + _, ok := a.GetObject().(*api.Pod) + if !ok { + glog.Errorf("expected pod but got %s", a.GetKind().Kind) + return true + } + + return false +} + func NewPodNodeSelector(clusterNodeSelectors map[string]string) *podNodeSelector { return &podNodeSelector{ Handler: admission.NewHandler(admission.Create, admission.Update), diff --git a/plugin/pkg/admission/podnodeselector/admission_test.go b/plugin/pkg/admission/podnodeselector/admission_test.go index 331d824ee79..c55c0fe778b 100644 --- a/plugin/pkg/admission/podnodeselector/admission_test.go +++ b/plugin/pkg/admission/podnodeselector/admission_test.go @@ -175,6 +175,12 @@ func TestPodAdmission(t *testing.T) { if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) { t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector) } + err = handler.Validate(admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Create, nil)) + if test.admit && err != nil { + t.Errorf("Test: %s, expected no error but got: %s", test.testName, err) + } else if !test.admit && err == nil { + t.Errorf("Test: %s, expected an error", test.testName) + } // handles update of uninitialized pod like it's newly created. err = handler.Admit(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil)) @@ -186,6 +192,12 @@ func TestPodAdmission(t *testing.T) { if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) { t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector) } + err = handler.Validate(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil)) + if test.admit && err != nil { + t.Errorf("Test: %s, expected no error but got: %s", test.testName, err) + } else if !test.admit && err == nil { + t.Errorf("Test: %s, expected an error", test.testName) + } } } diff --git a/plugin/pkg/admission/priority/admission.go b/plugin/pkg/admission/priority/admission.go index eddea6f446a..d67fb518329 100644 --- a/plugin/pkg/admission/priority/admission.go +++ b/plugin/pkg/admission/priority/admission.go @@ -64,6 +64,8 @@ type PriorityPlugin struct { globalDefaultPriority *int32 } +var _ admission.MutationInterface = &PriorityPlugin{} +var _ admission.ValidationInterface = &PriorityPlugin{} var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&PriorityPlugin{}) var _ = kubeapiserveradmission.WantsInternalKubeClientSet(&PriorityPlugin{}) @@ -102,11 +104,10 @@ var ( priorityClassResource = scheduling.Resource("priorityclasses") ) -// Admit checks Pods and PriorityClasses and admits or rejects them. It also resolves the priority of pods based on their PriorityClass. +// Admit checks Pods and admits or rejects them. It also resolves the priority of pods based on their PriorityClass. func (p *PriorityPlugin) Admit(a admission.Attributes) error { operation := a.GetOperation() - // Ignore all calls to subresources or resources other than pods. - // Ignore all operations other than Create and Update. + // Ignore all calls to subresources if len(a.GetSubresource()) != 0 { return nil } @@ -118,9 +119,23 @@ func (p *PriorityPlugin) Admit(a admission.Attributes) error { } return nil + default: + return nil + } +} + +// Validate checks PriorityClasses and admits or rejects them. +func (p *PriorityPlugin) Validate(a admission.Attributes) error { + operation := a.GetOperation() + // Ignore all calls to subresources + if len(a.GetSubresource()) != 0 { + return nil + } + + switch a.GetResource().GroupResource() { case priorityClassResource: if operation == admission.Create || operation == admission.Update { - return p.admitPriorityClass(a) + return p.validatePriorityClass(a) } if operation == admission.Delete { p.invalidateCachedDefaultPriority() @@ -176,8 +191,8 @@ func (p *PriorityPlugin) admitPod(a admission.Attributes) error { return nil } -// admitPriorityClass ensures that the value field is not larger than the highest user definable priority. If the GlobalDefault is set, it ensures that there is no other PriorityClass whose GlobalDefault is set. -func (p *PriorityPlugin) admitPriorityClass(a admission.Attributes) error { +// validatePriorityClass ensures that the value field is not larger than the highest user definable priority. If the GlobalDefault is set, it ensures that there is no other PriorityClass whose GlobalDefault is set. +func (p *PriorityPlugin) validatePriorityClass(a admission.Attributes) error { operation := a.GetOperation() pc, ok := a.GetObject().(*scheduling.PriorityClass) if !ok { diff --git a/plugin/pkg/admission/priority/admission_test.go b/plugin/pkg/admission/priority/admission_test.go index de236f0c18e..7c9a7f8e4f9 100644 --- a/plugin/pkg/admission/priority/admission_test.go +++ b/plugin/pkg/admission/priority/admission_test.go @@ -147,7 +147,7 @@ func TestPriorityClassAdmission(t *testing.T) { admission.Create, nil, ) - err := ctrl.Admit(attrs) + err := ctrl.Validate(attrs) glog.Infof("Got %v", err) if err != nil && !test.expectError { t.Errorf("Test %q: unexpected error received: %v", test.name, err) @@ -219,7 +219,7 @@ func TestDefaultPriority(t *testing.T) { t.Errorf("Test %q: expected default priority %d, but got %d", test.name, test.expectedDefaultBefore, defaultPriority) } if test.attributes != nil { - err := ctrl.Admit(test.attributes) + err := ctrl.Validate(test.attributes) if err != nil { t.Errorf("Test %q: unexpected error received: %v", test.name, err) }