Skip to content

Commit 821ba16

Browse files
Jay BoydJeff Peeler
Jay Boyd
authored and
Jeff Peeler
committed
new admission controller to block updates to service instance updates that (openshift#1210)
change the plan when service class has a non updateable plan
1 parent d69c5e5 commit 821ba16

File tree

5 files changed

+278
-1
lines changed

5 files changed

+278
-1
lines changed

charts/catalog/templates/apiserver-deployment.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ spec:
3838
- {{ .Values.apiserver.audit.logPath }}
3939
{{- end}}
4040
- --admission-control
41-
- "KubernetesNamespaceLifecycle,DefaultServicePlan,ServiceInstanceCredentialsLifecycle"
41+
- "KubernetesNamespaceLifecycle,DefaultServicePlan,ServiceInstanceCredentialsLifecycle,ServicePlanChangeValidator"
4242
- --secure-port
4343
- "8443"
4444
- --storage-type

cmd/apiserver/app/plugins.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,6 @@ import (
2323
// Admission policies
2424
_ "github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/namespace/lifecycle"
2525
_ "github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/serviceinstancecredentials/lifecycle"
26+
_ "github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/serviceplan/changevalidator"
2627
_ "github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/serviceplan/defaultserviceplan"
2728
)

cmd/apiserver/app/server/server.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"github.com/kubernetes-incubator/service-catalog/pkg/registry/servicecatalog/server"
2727
"github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/namespace/lifecycle"
2828
siclifecycle "github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/serviceinstancecredentials/lifecycle"
29+
"github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/serviceplan/changevalidator"
2930
"github.com/kubernetes-incubator/service-catalog/plugin/pkg/admission/serviceplan/defaultserviceplan"
3031
"github.com/spf13/cobra"
3132
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -139,4 +140,5 @@ func registerAllAdmissionPlugins(plugins *admission.Plugins) {
139140
lifecycle.Register(plugins)
140141
defaultserviceplan.Register(plugins)
141142
siclifecycle.Register(plugins)
143+
changevalidator.Register(plugins)
142144
}
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/*
2+
Copyright 2017 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package changevalidator
18+
19+
import (
20+
"errors"
21+
"fmt"
22+
"io"
23+
24+
"github.com/golang/glog"
25+
26+
apierrors "k8s.io/apimachinery/pkg/api/errors"
27+
"k8s.io/apiserver/pkg/admission"
28+
29+
informers "github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/internalversion"
30+
internalversion "github.com/kubernetes-incubator/service-catalog/pkg/client/listers_generated/servicecatalog/internalversion"
31+
32+
"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
33+
scadmission "github.com/kubernetes-incubator/service-catalog/pkg/apiserver/admission"
34+
)
35+
36+
const (
37+
// PluginName is name of admission plug-in
38+
PluginName = "ServicePlanChangeValidator"
39+
)
40+
41+
// Register registers a plugin
42+
func Register(plugins *admission.Plugins) {
43+
plugins.Register(PluginName, func(io.Reader) (admission.Interface, error) {
44+
return NewDenyPlanChangeIfNotUpdatable()
45+
})
46+
}
47+
48+
// denyPlanChangeIfNotUpdatable is an implementation of admission.Interface.
49+
// It checks if the Service Instance is being updated with a Service Plan and
50+
// blocks the operation if the Service Class is set to PlanUpdatable=false
51+
type denyPlanChangeIfNotUpdatable struct {
52+
*admission.Handler
53+
scLister internalversion.ServiceClassLister
54+
instanceLister internalversion.ServiceInstanceLister
55+
}
56+
57+
var _ = scadmission.WantsInternalServiceCatalogInformerFactory(&denyPlanChangeIfNotUpdatable{})
58+
59+
func (d *denyPlanChangeIfNotUpdatable) Admit(a admission.Attributes) error {
60+
// we need to wait for our caches to warm
61+
if !d.WaitForReady() {
62+
return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
63+
}
64+
65+
// We only care about service Instances
66+
if a.GetResource().Group != servicecatalog.GroupName || a.GetResource().GroupResource() != servicecatalog.Resource("serviceinstances") {
67+
return nil
68+
}
69+
instance, ok := a.GetObject().(*servicecatalog.ServiceInstance)
70+
if !ok {
71+
return apierrors.NewBadRequest("Resource was marked with kind Instance but was unable to be converted")
72+
}
73+
74+
sc, err := d.scLister.Get(instance.Spec.ServiceClassName)
75+
if err != nil {
76+
if apierrors.IsNotFound(err) {
77+
glog.V(5).Infof("Could not locate service class %v, can not determine if UpdateablePlan.", instance.Spec.ServiceClassName)
78+
return nil
79+
}
80+
glog.Error(err)
81+
return admission.NewForbidden(a, err)
82+
}
83+
84+
if sc.PlanUpdatable {
85+
return nil
86+
}
87+
88+
if instance.Spec.PlanName != "" {
89+
lister := d.instanceLister.ServiceInstances(instance.Namespace)
90+
origInstance, err := lister.Get(instance.Name)
91+
if err != nil {
92+
glog.Errorf("Error locating instance %v/%v", instance.Namespace, instance.Name)
93+
return err
94+
}
95+
if instance.Spec.PlanName != origInstance.Spec.PlanName {
96+
glog.V(4).Infof("update Service Instance %v/%v request specified Plan Name %v while original instance had %v", instance.Namespace, instance.Name, instance.Spec.PlanName, origInstance.Spec.PlanName)
97+
msg := fmt.Sprintf("The Service Class %v does not allow plan changes.", sc.Name)
98+
glog.Error(msg)
99+
return admission.NewForbidden(a, errors.New(msg))
100+
}
101+
}
102+
103+
return nil
104+
}
105+
106+
// NewDenyPlanChangeIfNotUpdatable creates a new admission control handler that
107+
// blocks updates to an instance service plan if the instance has
108+
// PlanUpdatable=false
109+
// specified Service Class
110+
func NewDenyPlanChangeIfNotUpdatable() (admission.Interface, error) {
111+
return &denyPlanChangeIfNotUpdatable{
112+
Handler: admission.NewHandler(admission.Update),
113+
}, nil
114+
}
115+
116+
func (d *denyPlanChangeIfNotUpdatable) SetInternalServiceCatalogInformerFactory(f informers.SharedInformerFactory) {
117+
scInformer := f.Servicecatalog().InternalVersion().ServiceClasses()
118+
instanceInformer := f.Servicecatalog().InternalVersion().ServiceInstances()
119+
d.instanceLister = instanceInformer.Lister()
120+
d.scLister = scInformer.Lister()
121+
d.SetReadyFunc(scInformer.Informer().HasSynced)
122+
}
123+
124+
func (d *denyPlanChangeIfNotUpdatable) Validate() error {
125+
if d.scLister == nil {
126+
return errors.New("missing service class lister")
127+
}
128+
if d.instanceLister == nil {
129+
return errors.New("missing instance lister")
130+
}
131+
return nil
132+
}
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
Copyright 2017 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package changevalidator
18+
19+
import (
20+
"strings"
21+
"testing"
22+
"time"
23+
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25+
"k8s.io/apimachinery/pkg/runtime"
26+
"k8s.io/apimachinery/pkg/util/wait"
27+
"k8s.io/apiserver/pkg/admission"
28+
29+
"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
30+
scadmission "github.com/kubernetes-incubator/service-catalog/pkg/apiserver/admission"
31+
"github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/internalclientset"
32+
"github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/internalclientset/fake"
33+
informers "github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/internalversion"
34+
core "k8s.io/client-go/testing"
35+
)
36+
37+
// newHandlerForTest returns a configured handler for testing.
38+
func newHandlerForTest(internalClient internalclientset.Interface) (admission.Interface, informers.SharedInformerFactory, error) {
39+
f := informers.NewSharedInformerFactory(internalClient, 5*time.Minute)
40+
handler, err := NewDenyPlanChangeIfNotUpdatable()
41+
if err != nil {
42+
return nil, f, err
43+
}
44+
pluginInitializer := scadmission.NewPluginInitializer(internalClient, f, nil, nil)
45+
pluginInitializer.Initialize(handler)
46+
err = admission.Validate(handler)
47+
return handler, f, err
48+
}
49+
50+
// newFakeServiceCatalogClientForTest creates a fake clientset that returns a
51+
// ServiceClassList with the given ServiceClass as the single list item.
52+
func newFakeServiceCatalogClientForTest(sc *servicecatalog.ServiceClass) *fake.Clientset {
53+
fakeClient := &fake.Clientset{}
54+
55+
scList := &servicecatalog.ServiceClassList{
56+
ListMeta: metav1.ListMeta{
57+
ResourceVersion: "1",
58+
}}
59+
scList.Items = append(scList.Items, *sc)
60+
61+
fakeClient.AddReactor("list", "serviceclasses", func(action core.Action) (bool, runtime.Object, error) {
62+
return true, scList, nil
63+
})
64+
return fakeClient
65+
}
66+
67+
// newServiceInstance returns a new instance for the specified namespace.
68+
func newServiceInstance(namespace string, serviceClassName string, planName string) servicecatalog.ServiceInstance {
69+
instance := servicecatalog.ServiceInstance{
70+
ObjectMeta: metav1.ObjectMeta{Name: "instance", Namespace: namespace},
71+
}
72+
instance.Spec.ServiceClassName = serviceClassName
73+
instance.Spec.PlanName = planName
74+
return instance
75+
}
76+
77+
// newServiceClass returns a new instance with the specified plan and
78+
// UpdateablePlan attribute
79+
func newServiceClass(name string, plan string, updateablePlan bool) *servicecatalog.ServiceClass {
80+
sc := &servicecatalog.ServiceClass{ObjectMeta: metav1.ObjectMeta{Name: name}, PlanUpdatable: updateablePlan}
81+
sc.Plans = append(sc.Plans, servicecatalog.ServicePlan{Name: plan})
82+
return sc
83+
}
84+
85+
// setupInstanceLister creates a Service Instance and sets up a Instance Lister that
86+
// retuns the instance
87+
func setupInstanceLister(fakeClient *fake.Clientset) {
88+
instance := newServiceInstance("dummy", "foo", "original-plan-name")
89+
scList := &servicecatalog.ServiceInstanceList{
90+
ListMeta: metav1.ListMeta{
91+
ResourceVersion: "1",
92+
}}
93+
scList.Items = append(scList.Items, instance)
94+
95+
fakeClient.AddReactor("list", "serviceinstances", func(action core.Action) (bool, runtime.Object, error) {
96+
return true, scList, nil
97+
})
98+
}
99+
100+
// TestServicePlanChangeBlockedByUpdateablePlanSetting tests that the
101+
// Admission Controller will block a request to update an Instance's
102+
// Service Plan
103+
func TestServicePlanChangeBlockedByUpdateablePlanSetting(t *testing.T) {
104+
sc := newServiceClass("foo", "bar", false)
105+
fakeClient := newFakeServiceCatalogClientForTest(sc)
106+
handler, informerFactory, err := newHandlerForTest(fakeClient)
107+
if err != nil {
108+
t.Errorf("unexpected error initializing handler: %v", err)
109+
}
110+
setupInstanceLister(fakeClient)
111+
instance := newServiceInstance("dummy", "foo", "new-plan")
112+
informerFactory.Start(wait.NeverStop)
113+
err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Update, nil))
114+
if err != nil {
115+
if !strings.Contains(err.Error(), "The Service Class foo does not allow plan changes.") {
116+
t.Errorf("unexpected error %q returned from admission handler.", err.Error())
117+
}
118+
} else {
119+
t.Error("This should have been an error")
120+
}
121+
}
122+
123+
// TestServicePlanChangePermittedByUpdateablePlanSetting tests the
124+
// Admission Controller verifying it allows an instance change to the
125+
// plan name if the service class has specified PlanUpdatable=true
126+
func TestServicePlanChangePermittedByUpdateablePlanSetting(t *testing.T) {
127+
sc := newServiceClass("foo", "bar", true)
128+
fakeClient := newFakeServiceCatalogClientForTest(sc)
129+
handler, informerFactory, err := newHandlerForTest(fakeClient)
130+
if err != nil {
131+
t.Errorf("unexpected error initializing handler: %v", err)
132+
}
133+
134+
setupInstanceLister(fakeClient)
135+
136+
instance := newServiceInstance("dummy", "foo", "new-plan")
137+
informerFactory.Start(wait.NeverStop)
138+
err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Update, nil))
139+
if err != nil {
140+
t.Errorf("Unexpected error: %v", err.Error())
141+
}
142+
}

0 commit comments

Comments
 (0)