Skip to content

Commit 26986e8

Browse files
author
Kubernetes Submit Queue
authored
Merge pull request kubernetes#55058 from deads2k/admission-11-split-doubles
Automatic merge from submit-queue (batch tested with PRs 53273, 55058, 55237, 50140). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. split some admission plugins into mutation and validation halves Splits the podnodeselector, serviceaccount, and priority admission plugins into validating and mutating admission plugins. @kubernetes/sig-api-machinery-pr-reviews
2 parents d330775 + 2c67161 commit 26986e8

File tree

4 files changed

+105
-48
lines changed

4 files changed

+105
-48
lines changed

plugin/pkg/admission/podnodeselector/admission.go

Lines changed: 70 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ type podNodeSelector struct {
5959
clusterNodeSelectors map[string]string
6060
}
6161

62+
var _ admission.MutationInterface = &podNodeSelector{}
63+
var _ admission.ValidationInterface = &podNodeSelector{}
6264
var _ = kubeapiserveradmission.WantsInternalKubeClientSet(&podNodeSelector{})
6365
var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&podNodeSelector{})
6466

@@ -93,77 +95,105 @@ func readConfig(config io.Reader) *pluginConfig {
9395

9496
// Admit enforces that pod and its namespace node label selectors matches at least a node in the cluster.
9597
func (p *podNodeSelector) Admit(a admission.Attributes) error {
96-
resource := a.GetResource().GroupResource()
97-
if resource != api.Resource("pods") {
98+
if shouldIgnore(a) {
9899
return nil
99100
}
100-
if a.GetSubresource() != "" {
101-
// only run the checks below on pods proper and not subresources
101+
updateInitialized, err := util.IsUpdatingInitializedObject(a)
102+
if err != nil {
103+
return err
104+
}
105+
if updateInitialized {
106+
// node selector of an initialized pod is immutable
102107
return nil
103108
}
109+
if !p.WaitForReady() {
110+
return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
111+
}
104112

105-
obj := a.GetObject()
106-
pod, ok := obj.(*api.Pod)
107-
if !ok {
108-
glog.Errorf("expected pod but got %s", a.GetKind().Kind)
109-
return nil
113+
resource := a.GetResource().GroupResource()
114+
pod := a.GetObject().(*api.Pod)
115+
namespaceNodeSelector, err := p.getNamespaceNodeSelectorMap(a.GetNamespace())
116+
if err != nil {
117+
return err
118+
}
119+
120+
if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
121+
return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
110122
}
111123

124+
// Merge pod node selector = namespace node selector + current pod node selector
125+
// second selector wins
126+
podNodeSelectorLabels := labels.Merge(namespaceNodeSelector, pod.Spec.NodeSelector)
127+
pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels)
128+
return p.Validate(a)
129+
}
130+
131+
// Validate ensures that the pod node selector is allowed
132+
func (p *podNodeSelector) Validate(a admission.Attributes) error {
133+
if shouldIgnore(a) {
134+
return nil
135+
}
112136
if !p.WaitForReady() {
113137
return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
114138
}
115139

116-
updateInitialized, err := util.IsUpdatingInitializedObject(a)
140+
resource := a.GetResource().GroupResource()
141+
pod := a.GetObject().(*api.Pod)
142+
143+
namespaceNodeSelector, err := p.getNamespaceNodeSelectorMap(a.GetNamespace())
117144
if err != nil {
118145
return err
119146
}
120-
if updateInitialized {
121-
// node selector of an initialized pod is immutable
122-
return nil
147+
if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
148+
return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
149+
}
150+
151+
// whitelist verification
152+
whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[a.GetNamespace()])
153+
if err != nil {
154+
return err
155+
}
156+
if !labels.AreLabelsInWhiteList(pod.Spec.NodeSelector, whitelist) {
157+
return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist"))
123158
}
124159

125-
name := pod.Name
126-
nsName := a.GetNamespace()
127-
var namespace *api.Namespace
160+
return nil
161+
}
128162

129-
namespace, err = p.namespaceLister.Get(nsName)
163+
func (p *podNodeSelector) getNamespaceNodeSelectorMap(namespaceName string) (labels.Set, error) {
164+
namespace, err := p.namespaceLister.Get(namespaceName)
130165
if errors.IsNotFound(err) {
131-
namespace, err = p.defaultGetNamespace(nsName)
166+
namespace, err = p.defaultGetNamespace(namespaceName)
132167
if err != nil {
133168
if errors.IsNotFound(err) {
134-
return err
169+
return nil, err
135170
}
136-
return errors.NewInternalError(err)
171+
return nil, errors.NewInternalError(err)
137172
}
138173
} else if err != nil {
139-
return errors.NewInternalError(err)
174+
return nil, errors.NewInternalError(err)
140175
}
141176

142-
namespaceNodeSelector, err := p.getNodeSelectorMap(namespace)
143-
if err != nil {
144-
return err
145-
}
177+
return p.getNodeSelectorMap(namespace)
178+
}
146179

147-
if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
148-
return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
180+
func shouldIgnore(a admission.Attributes) bool {
181+
resource := a.GetResource().GroupResource()
182+
if resource != api.Resource("pods") {
183+
return true
149184
}
150-
151-
whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[namespace.Name])
152-
if err != nil {
153-
return err
185+
if a.GetSubresource() != "" {
186+
// only run the checks below on pods proper and not subresources
187+
return true
154188
}
155189

156-
// Merge pod node selector = namespace node selector + current pod node selector
157-
podNodeSelectorLabels := labels.Merge(namespaceNodeSelector, pod.Spec.NodeSelector)
158-
159-
// whitelist verification
160-
if !labels.AreLabelsInWhiteList(podNodeSelectorLabels, whitelist) {
161-
return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist"))
190+
_, ok := a.GetObject().(*api.Pod)
191+
if !ok {
192+
glog.Errorf("expected pod but got %s", a.GetKind().Kind)
193+
return true
162194
}
163195

164-
// Updated pod node selector = namespace node selector + current pod node selector
165-
pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels)
166-
return nil
196+
return false
167197
}
168198

169199
func NewPodNodeSelector(clusterNodeSelectors map[string]string) *podNodeSelector {

plugin/pkg/admission/podnodeselector/admission_test.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,12 @@ func TestPodAdmission(t *testing.T) {
175175
if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
176176
t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector)
177177
}
178+
err = handler.Validate(admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Create, nil))
179+
if test.admit && err != nil {
180+
t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
181+
} else if !test.admit && err == nil {
182+
t.Errorf("Test: %s, expected an error", test.testName)
183+
}
178184

179185
// handles update of uninitialized pod like it's newly created.
180186
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) {
186192
if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
187193
t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector)
188194
}
195+
err = handler.Validate(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
196+
if test.admit && err != nil {
197+
t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
198+
} else if !test.admit && err == nil {
199+
t.Errorf("Test: %s, expected an error", test.testName)
200+
}
189201
}
190202
}
191203

plugin/pkg/admission/priority/admission.go

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ type PriorityPlugin struct {
6464
globalDefaultPriority *int32
6565
}
6666

67+
var _ admission.MutationInterface = &PriorityPlugin{}
68+
var _ admission.ValidationInterface = &PriorityPlugin{}
6769
var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&PriorityPlugin{})
6870
var _ = kubeapiserveradmission.WantsInternalKubeClientSet(&PriorityPlugin{})
6971

@@ -102,11 +104,10 @@ var (
102104
priorityClassResource = scheduling.Resource("priorityclasses")
103105
)
104106

105-
// Admit checks Pods and PriorityClasses and admits or rejects them. It also resolves the priority of pods based on their PriorityClass.
107+
// Admit checks Pods and admits or rejects them. It also resolves the priority of pods based on their PriorityClass.
106108
func (p *PriorityPlugin) Admit(a admission.Attributes) error {
107109
operation := a.GetOperation()
108-
// Ignore all calls to subresources or resources other than pods.
109-
// Ignore all operations other than Create and Update.
110+
// Ignore all calls to subresources
110111
if len(a.GetSubresource()) != 0 {
111112
return nil
112113
}
@@ -118,9 +119,23 @@ func (p *PriorityPlugin) Admit(a admission.Attributes) error {
118119
}
119120
return nil
120121

122+
default:
123+
return nil
124+
}
125+
}
126+
127+
// Validate checks PriorityClasses and admits or rejects them.
128+
func (p *PriorityPlugin) Validate(a admission.Attributes) error {
129+
operation := a.GetOperation()
130+
// Ignore all calls to subresources
131+
if len(a.GetSubresource()) != 0 {
132+
return nil
133+
}
134+
135+
switch a.GetResource().GroupResource() {
121136
case priorityClassResource:
122137
if operation == admission.Create || operation == admission.Update {
123-
return p.admitPriorityClass(a)
138+
return p.validatePriorityClass(a)
124139
}
125140
if operation == admission.Delete {
126141
p.invalidateCachedDefaultPriority()
@@ -176,8 +191,8 @@ func (p *PriorityPlugin) admitPod(a admission.Attributes) error {
176191
return nil
177192
}
178193

179-
// 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.
180-
func (p *PriorityPlugin) admitPriorityClass(a admission.Attributes) error {
194+
// 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.
195+
func (p *PriorityPlugin) validatePriorityClass(a admission.Attributes) error {
181196
operation := a.GetOperation()
182197
pc, ok := a.GetObject().(*scheduling.PriorityClass)
183198
if !ok {

plugin/pkg/admission/priority/admission_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ func TestPriorityClassAdmission(t *testing.T) {
147147
admission.Create,
148148
nil,
149149
)
150-
err := ctrl.Admit(attrs)
150+
err := ctrl.Validate(attrs)
151151
glog.Infof("Got %v", err)
152152
if err != nil && !test.expectError {
153153
t.Errorf("Test %q: unexpected error received: %v", test.name, err)
@@ -219,7 +219,7 @@ func TestDefaultPriority(t *testing.T) {
219219
t.Errorf("Test %q: expected default priority %d, but got %d", test.name, test.expectedDefaultBefore, defaultPriority)
220220
}
221221
if test.attributes != nil {
222-
err := ctrl.Admit(test.attributes)
222+
err := ctrl.Validate(test.attributes)
223223
if err != nil {
224224
t.Errorf("Test %q: unexpected error received: %v", test.name, err)
225225
}

0 commit comments

Comments
 (0)