From b570dc137a582c3a1ab65cf112d813d865b46e30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Mi=C5=9Bkiewicz?= Date: Tue, 16 Apr 2019 14:30:21 +0200 Subject: [PATCH] Migrate registry/strategy Updates to webhooks (#17) --- charts/catalog/templates/crds.yaml | 463 +----------------- .../catalog/templates/webhook-register.yaml | 91 ++++ cmd/webhook/server/webhook.go | 20 +- pkg/apis/servicecatalog/validation/binding.go | 2 +- .../servicecatalog/validation/binding_test.go | 2 +- pkg/apis/servicecatalog/validation/broker.go | 11 +- .../servicecatalog/validation/broker_test.go | 2 +- .../servicecatalog/validation/instance.go | 4 +- .../validation/instance_test.go | 3 +- .../servicecatalog/validation/serviceclass.go | 2 +- .../validation/serviceclass_test.go | 2 +- .../servicecatalog/validation/serviceplan.go | 2 +- .../validation/serviceplan_test.go | 2 +- .../servicecatalog/validation/validation.go | 2 +- .../servicecatalog/binding/strategy.go | 2 +- .../servicecatalog/binding/strategy_test.go | 2 +- .../clusterservicebroker/strategy.go | 2 +- .../clusterservicebroker/strategy_test.go | 2 +- .../clusterserviceclass/strategy.go | 2 +- .../clusterserviceplan/strategy.go | 2 +- .../servicecatalog/instance/strategy.go | 2 +- .../servicecatalog/instance/strategy_test.go | 2 +- .../servicecatalog/servicebroker/strategy.go | 2 +- .../servicebroker/strategy_test.go | 2 +- .../servicecatalog/serviceclass/strategy.go | 2 +- .../servicecatalog/serviceplan/strategy.go | 2 +- .../clusterservicebroker/mutation/handler.go | 24 +- .../mutation/handler_test.go | 90 ++++ .../validation/handler.go | 4 +- .../validation/handler_status.go | 71 +++ .../validation/handler_status_test.go | 102 ++++ .../clusterservicebroker/validation/static.go | 69 +++ .../clusterserviceclass/mutation/handler.go | 16 +- .../mutation/handler_test.go | 106 ++++ .../clusterserviceclass/validation/handler.go | 137 ++++++ .../validation/handler_test.go | 36 ++ .../clusterserviceclass/validation/static.go | 69 +++ .../clusterserviceplan/mutation/handler.go | 14 +- .../mutation/handler_test.go | 113 +++++ .../clusterserviceplan/validation/handler.go | 137 ++++++ .../validation/handler_test.go | 36 ++ .../clusterserviceplan/validation/static.go | 69 +++ .../servicebinding/mutation/handler.go | 24 +- .../servicebinding/mutation/handler_test.go | 97 ++++ .../servicebinding/validation/handler.go | 3 +- .../validation/handler_status.go | 72 +++ .../validation/handler_status_test.go | 161 ++++++ .../servicebinding/validation/static.go | 69 +++ .../servicebroker/mutation/handler.go | 18 +- .../servicebroker/mutation/handler_test.go | 90 ++++ .../servicebroker/validation/handler.go | 4 +- .../validation/handler_status.go | 71 +++ .../validation/handler_status_test.go | 102 ++++ .../servicebroker/validation/static.go | 65 +++ .../serviceclass/mutation/handler.go | 17 +- .../serviceclass/mutation/handler_test.go | 106 ++++ .../serviceclass/validation/handler.go | 137 ++++++ .../serviceclass/validation/handler_test.go | 36 ++ .../serviceclass/validation/static.go | 65 +++ .../serviceinstance/mutation/handler.go | 39 +- .../serviceinstance/mutation/handler_test.go | 100 ++++ .../serviceinstance/validation/handler.go | 3 +- .../serviceinstance/validation/static.go | 69 +++ .../serviceplan/mutation/handler.go | 17 +- .../serviceplan/mutation/handler_test.go | 113 +++++ .../serviceplan/validation/handler.go | 137 ++++++ .../serviceplan/validation/handler_test.go | 36 ++ .../serviceplan/validation/static.go | 69 +++ pkg/webhookutil/traced_logger.go | 8 +- 69 files changed, 2904 insertions(+), 549 deletions(-) create mode 100644 pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status.go create mode 100644 pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status_test.go create mode 100644 pkg/webhook/servicecatalog/clusterservicebroker/validation/static.go create mode 100644 pkg/webhook/servicecatalog/clusterserviceclass/validation/handler.go create mode 100644 pkg/webhook/servicecatalog/clusterserviceclass/validation/handler_test.go create mode 100644 pkg/webhook/servicecatalog/clusterserviceclass/validation/static.go create mode 100644 pkg/webhook/servicecatalog/clusterserviceplan/validation/handler.go create mode 100644 pkg/webhook/servicecatalog/clusterserviceplan/validation/handler_test.go create mode 100644 pkg/webhook/servicecatalog/clusterserviceplan/validation/static.go create mode 100644 pkg/webhook/servicecatalog/servicebinding/validation/handler_status.go create mode 100644 pkg/webhook/servicecatalog/servicebinding/validation/handler_status_test.go create mode 100644 pkg/webhook/servicecatalog/servicebinding/validation/static.go create mode 100644 pkg/webhook/servicecatalog/servicebroker/validation/handler_status.go create mode 100644 pkg/webhook/servicecatalog/servicebroker/validation/handler_status_test.go create mode 100644 pkg/webhook/servicecatalog/servicebroker/validation/static.go create mode 100644 pkg/webhook/servicecatalog/serviceclass/validation/handler.go create mode 100644 pkg/webhook/servicecatalog/serviceclass/validation/handler_test.go create mode 100644 pkg/webhook/servicecatalog/serviceclass/validation/static.go create mode 100644 pkg/webhook/servicecatalog/serviceinstance/validation/static.go create mode 100644 pkg/webhook/servicecatalog/serviceplan/validation/handler.go create mode 100644 pkg/webhook/servicecatalog/serviceplan/validation/handler_test.go create mode 100644 pkg/webhook/servicecatalog/serviceplan/validation/static.go diff --git a/charts/catalog/templates/crds.yaml b/charts/catalog/templates/crds.yaml index c565ce248ea..65c23cfcd80 100644 --- a/charts/catalog/templates/crds.yaml +++ b/charts/catalog/templates/crds.yaml @@ -14,55 +14,6 @@ spec: kind: ClusterServiceBroker subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "url" - properties: - url: - type: string - insecureSkipTLSVerify: - type: boolean - caBundle: - type: string - relistBehavior: - type: string - relistDuration: - type: integer - minimum: 1 - relistRequests: - type: integer - minimum: 0 - catalogRestrictions: - type: object - properties: - servicePlan: - type: array - items: - type: string - serviceClass: - type: array - items: - type: string - authInfo: - type: object - properties: - basic: - type: object - required: - - "secretRef" - properties: - secretRef: - type: object - bearer: - type: object - required: - - "secretRef" - properties: - secretRef: - type: object --- @@ -82,55 +33,6 @@ spec: kind: ServiceBroker subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "url" - properties: - url: - type: string - insecureSkipTLSVerify: - type: boolean - caBundle: - type: string - relistBehavior: - type: string - relistDuration: - type: integer - minimum: 1 - relistRequests: - type: integer - minimum: 0 - catalogRestrictions: - type: object - properties: - servicePlan: - type: array - items: - type: string - serviceClass: - type: array - items: - type: string - authInfo: - type: object - properties: - basic: - type: object - required: - - "secretRef" - properties: - secretRef: - type: object - bearer: - type: object - required: - - "secretRef" - properties: - secretRef: - type: object --- @@ -150,44 +52,6 @@ spec: kind: ServiceClass subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "serviceBrokerName" - - "externalID" - - "externalName" - - "description" - properties: - serviceBrokerName: - type: string - externalName: - type: string - maxLength: 63 - externalID: - type: string - maxLength: 63 - description: - type: string - bindable: - type: boolean - bindingRetrievable: - type: boolean - planUpdatable: - type: boolean - tags: - type: array - items: - type: string - requires: - type: array - items: - type: string - externalMetadata: - type: object - defaultProvisionParameters: - type: object --- @@ -207,44 +71,7 @@ spec: kind: ClusterServiceClass subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "clusterServiceBrokerName" - - "externalName" - - "externalID" - - "description" - properties: - clusterServiceBrokerName: - type: string - externalName: - type: string - maxLength: 63 - externalID: - type: string - maxLength: 63 - description: - type: string - bindable: - type: boolean - bindingRetrievable: - type: boolean - planUpdatable: - type: boolean - tags: - type: array - items: - type: string - requires: - type: array - items: - type: string - externalMetadata: - type: object - defaultProvisionParameters: - type: object + --- apiVersion: apiextensions.k8s.io/v1beta1 @@ -263,49 +90,6 @@ spec: kind: ServicePlan subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "serviceClassRef" - - "serviceBrokerName" - - "externalName" - - "externalID" - - "description" - properties: - serviceClassRef: - type: object - required: - - "name" - properties: - name: - type: string - maxLength: 63 - serviceBrokerName: - type: string - externalName: - type: string - maxLength: 63 - externalID: - type: string - maxLength: 63 - description: - type: string - bindable: - type: boolean - free: - type: boolean - instanceCreateParameterSchema: - type: object - instanceUpdateParameterSchema: - type: object - serviceBindingCreateParameterSchema: - type: object - serviceBindingCreateResponseSchema: - type: object - defaultProvisionParameters: - type: object --- @@ -325,49 +109,7 @@ spec: kind: ClusterServicePlan subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "clusterServiceClassRef" - - "clusterServiceBrokerName" - - "externalName" - - "externalID" - - "description" - properties: - clusterServiceClassRef: - type: object - required: - - "name" - properties: - name: - type: string - maxLength: 63 - clusterServiceBrokerName: - type: string - externalName: - type: string - maxLength: 63 - externalID: - type: string - maxLength: 63 - description: - type: string - bindable: - type: boolean - free: - type: boolean - instanceCreateParameterSchema: - type: object - instanceUpdateParameterSchema: - type: object - serviceBindingCreateParameterSchema: - type: object - serviceBindingCreateResponseSchema: - type: object - defaultProvisionParameters: - type: object + --- apiVersion: apiextensions.k8s.io/v1beta1 @@ -386,104 +128,6 @@ spec: kind: ServiceInstance subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - properties: - planReference: - type: object - properties: - clusterServiceClassExternalName: - type: string - clusterServicePlanExternalName: - type: string - clusterServiceClassExternalID: - type: string - clusterServicePlanExternalID: - type: string - clusterServiceClassName: - type: string - clusterServicePlanName: - type: string - serviceClassExternalName: - type: string - servicePlanExternalName: - type: string - serviceClassExternalID: - type: string - servicePlanExternalID: - type: string - serviceClassName: - type: string - servicePlanName: - type: string - clusterServiceClassRef: - type: object - required: - - "name" - properties: - name: - type: string - clusterServicePlanRef: - type: object - required: - - "name" - properties: - name: - type: string - serviceClassRef: - type: object - required: - - "name" - properties: - name: - type: string - servicePlanRef: - type: object - required: - - "name" - properties: - name: - type: string - parameters: - type: object - parametersFromSource: - type: array - items: - type: object - required: - - "name" - - "key" - properties: - name: - type: string - key: - type: string - externalID: - type: string - userInfo: - type: object - properties: - username: - type: string - UID: - type: string - groups: - type: array - items: - type: string - extra: - type: object - properties: - key: - type: string - values: - type: array - items: - type: string - updateRequests: - type: integer --- @@ -503,106 +147,5 @@ spec: kind: ServiceBinding subresources: status: {} - validation: - openAPIV3Schema: - properties: - spec: - required: - - "instanceRef" - properties: - instanceRef: - type: object - required: - - "name" - properties: - name: - type: string - maxLength: 253 - secretName: - type: string - maxLength: 253 - externalID: - type: string - secretTransform: - type: array - items: - type: object - properties: - renameKey: - type: object - required: - - "from" - - "to" - properties: - from: - type: string - to: - type: string - addKey: - type: object - required: - - "key" - properties: - key: - type: string - value: - type: array - items: - type: byte - stringValue: - type: string - JSONPathExpression: - type: string - addKeysFrom: - secretRef: - type: object - required: - - "name" - - "namespace" - properties: - name: - type: string - namespace: - type: string - removeKey: - type: object - required: - - "key" - properties: - key: - type: string - userInfo: - type: object - properties: - username: - type: string - UID: - type: string - groups: - type: array - items: - type: string - extra: - type: object - properties: - key: - type: string - values: - type: array - items: - type: string - parameters: - type: object - parametersFromSource: - type: array - items: - type: object - required: - - "name" - - "key" - properties: - name: - type: string - key: - type: string + --- \ No newline at end of file diff --git a/charts/catalog/templates/webhook-register.yaml b/charts/catalog/templates/webhook-register.yaml index 6b6480bf8cd..a4b79dc81eb 100644 --- a/charts/catalog/templates/webhook-register.yaml +++ b/charts/catalog/templates/webhook-register.yaml @@ -120,6 +120,45 @@ metadata: name: {{ template "fullname" . }}-validating-webhook namespace: "{{ .Release.Namespace }}" webhooks: +- name: validating.status.servicebindings.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-servicebindings/status" + failurePolicy: Fail + rules: + - operations: [ "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["servicebindings/status"] +- name: validating.status.servicbrokers.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-servicebrokers/status" + failurePolicy: Fail + rules: + - operations: [ "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["servicebrokers/status"] +- name: validating.status.clusterservicbrokers.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-clusterservicebrokers/status" + failurePolicy: Fail + rules: + - operations: [ "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["clusterservicebrokers/status"] - name: validating.serviceinstances.servicecatalog.k8s.io clientConfig: caBundle: {{ b64enc $ca.Cert }} @@ -172,6 +211,58 @@ webhooks: apiGroups: ["servicecatalog.k8s.io"] apiVersions: ["v1beta1"] resources: ["servicebrokers"] +- name: validating.serviceclasses.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-serviceclasses" + failurePolicy: Fail + rules: + - operations: [ "CREATE", "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["serviceclasses"] +- name: validating.clusterserviceclasses.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-clusterserviceclasses" + failurePolicy: Fail + rules: + - operations: [ "CREATE", "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["clusterserviceclasses"] +- name: validating.serviceplans.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-serviceplans" + failurePolicy: Fail + rules: + - operations: [ "CREATE", "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["serviceplans"] +- name: validating.clusterserviceplans.servicecatalog.k8s.io + clientConfig: + caBundle: {{ b64enc $ca.Cert }} + service: + name: {{ template "fullname" . }}-webhook + namespace: "{{ .Release.Namespace }}" + path: "/validating-clusterserviceplans" + failurePolicy: Fail + rules: + - operations: [ "CREATE", "UPDATE" ] + apiGroups: ["servicecatalog.k8s.io"] + apiVersions: ["v1beta1"] + resources: ["clusterserviceplans"] --- apiVersion: v1 kind: Secret diff --git a/cmd/webhook/server/webhook.go b/cmd/webhook/server/webhook.go index c38839ea6a3..31b83a76611 100644 --- a/cmd/webhook/server/webhook.go +++ b/cmd/webhook/server/webhook.go @@ -32,9 +32,13 @@ import ( spmutation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/serviceplan/mutation" csbrvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/clusterservicebroker/validation" + cscvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/clusterserviceclass/validation" + cspvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/clusterserviceplan/validation" sbvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/servicebinding/validation" sbrvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/servicebroker/validation" + scvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/serviceclass/validation" sivalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/serviceinstance/validation" + spvalidation "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/serviceplan/validation" "github.com/pkg/errors" "k8s.io/apiserver/pkg/server/healthz" @@ -84,10 +88,18 @@ func run(opts *WebhookServerOptions, stopCh <-chan struct{}) error { "/mutating-serviceplans": &spmutation.CreateUpdateHandler{}, "/mutating-serviceinstances": simutation.New(), - "/validating-clusterservicebrokers": csbrvalidation.NewAdmissionHandler(), - "/validating-servicebindings": sbvalidation.NewAdmissionHandler(), - "/validating-servicebrokers": sbrvalidation.NewAdmissionHandler(), - "/validating-serviceinstances": sivalidation.NewAdmissionHandler(), + "/validating-clusterservicebrokers": csbrvalidation.NewAdmissionHandler(), + "/validating-clusterservicebrokers/status": &csbrvalidation.StatusUpdateHandler{}, + "/validating-clusterserviceclasses": cscvalidation.NewAdmissionHandler(), + "/validating-clusterserviceplans": cspvalidation.NewAdmissionHandler(), + + "/validating-servicebindings": sbvalidation.NewAdmissionHandler(), + "/validating-servicebindings/status": &sbvalidation.StatusUpdateValidationHandler{}, + "/validating-servicebrokers": sbrvalidation.NewAdmissionHandler(), + "/validating-servicebrokers/status": &sbrvalidation.StatusUpdateHandler{}, + "/validating-serviceclasses": scvalidation.NewAdmissionHandler(), + "/validating-serviceplans": spvalidation.NewAdmissionHandler(), + "/validating-serviceinstances": sivalidation.NewAdmissionHandler(), } for path, handler := range webhooks { diff --git a/pkg/apis/servicecatalog/validation/binding.go b/pkg/apis/servicecatalog/validation/binding.go index 269fa4b088a..5ef98c8494d 100644 --- a/pkg/apis/servicecatalog/validation/binding.go +++ b/pkg/apis/servicecatalog/validation/binding.go @@ -17,7 +17,7 @@ limitations under the License. package validation import ( - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features" apivalidation "k8s.io/apimachinery/pkg/api/validation" "k8s.io/apimachinery/pkg/util/validation/field" diff --git a/pkg/apis/servicecatalog/validation/binding_test.go b/pkg/apis/servicecatalog/validation/binding_test.go index 7bb4d01a07a..2b65a89d8c2 100644 --- a/pkg/apis/servicecatalog/validation/binding_test.go +++ b/pkg/apis/servicecatalog/validation/binding_test.go @@ -23,7 +23,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) func validServiceBinding() *servicecatalog.ServiceBinding { diff --git a/pkg/apis/servicecatalog/validation/broker.go b/pkg/apis/servicecatalog/validation/broker.go index 9817c11ef77..115655e55d9 100644 --- a/pkg/apis/servicecatalog/validation/broker.go +++ b/pkg/apis/servicecatalog/validation/broker.go @@ -23,8 +23,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" "github.com/kubernetes-sigs/service-catalog/pkg/filter" ) @@ -218,8 +217,8 @@ func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath * } else { for _, restriction := range spec.CatalogRestrictions.ServiceClass { p := filter.ExtractProperty(restriction) - if !isClusterServiceBroker && !v1beta1.IsValidServiceClassProperty(p) || - isClusterServiceBroker && !v1beta1.IsValidClusterServiceClassProperty(p) { + if !isClusterServiceBroker && !sc.IsValidServiceClassProperty(p) || + isClusterServiceBroker && !sc.IsValidClusterServiceClassProperty(p) { commonErrs = append(commonErrs, field.Invalid(fldPath.Child("catalogRestrictions", "serviceClass"), spec.CatalogRestrictions.ServiceClass, fmt.Sprintf("Invalid property: %s", p))) @@ -237,8 +236,8 @@ func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath * } else { for _, restriction := range spec.CatalogRestrictions.ServicePlan { p := filter.ExtractProperty(restriction) - if !isClusterServiceBroker && !v1beta1.IsValidServicePlanProperty(p) || - isClusterServiceBroker && !v1beta1.IsValidClusterServicePlanProperty(p) { + if !isClusterServiceBroker && !sc.IsValidServicePlanProperty(p) || + isClusterServiceBroker && !sc.IsValidClusterServicePlanProperty(p) { commonErrs = append(commonErrs, field.Invalid(fldPath.Child("catalogRestrictions", "servicePlan"), spec.CatalogRestrictions.ServicePlan, fmt.Sprintf("Invalid property: %s", p))) diff --git a/pkg/apis/servicecatalog/validation/broker_test.go b/pkg/apis/servicecatalog/validation/broker_test.go index d8e039fe7b4..c6e766bef82 100644 --- a/pkg/apis/servicecatalog/validation/broker_test.go +++ b/pkg/apis/servicecatalog/validation/broker_test.go @@ -22,7 +22,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) func TestValidateClusterServiceBroker(t *testing.T) { diff --git a/pkg/apis/servicecatalog/validation/instance.go b/pkg/apis/servicecatalog/validation/instance.go index 5c1bbc4ca43..dc3db62e011 100644 --- a/pkg/apis/servicecatalog/validation/instance.go +++ b/pkg/apis/servicecatalog/validation/instance.go @@ -19,7 +19,7 @@ package validation import ( "fmt" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" "github.com/kubernetes-sigs/service-catalog/pkg/controller" scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features" apivalidation "k8s.io/apimachinery/pkg/api/validation" @@ -78,7 +78,6 @@ func internalValidateServiceInstance(instance *sc.ServiceInstance, create bool) validateServiceInstanceName, field.NewPath("metadata"))...) allErrs = append(allErrs, validateServiceInstanceSpec(&instance.Spec, field.NewPath("spec"), create)...) - allErrs = append(allErrs, validateServiceInstanceStatus(&instance.Status, field.NewPath("status"), create)...) if create { allErrs = append(allErrs, validateServiceInstanceCreate(instance)...) } else { @@ -405,6 +404,7 @@ func internalValidateServiceInstanceReferencesUpdateAllowed(new *sc.ServiceInsta func ValidateServiceInstanceStatusUpdate(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, internalValidateServiceInstanceStatusUpdateAllowed(new, old)...) + allErrs = append(allErrs, validateServiceInstanceStatus(&new.Status, field.NewPath("status"), false)...) allErrs = append(allErrs, internalValidateServiceInstance(new, false)...) return allErrs } diff --git a/pkg/apis/servicecatalog/validation/instance_test.go b/pkg/apis/servicecatalog/validation/instance_test.go index 225b846cbd9..a24f53c3b21 100644 --- a/pkg/apis/servicecatalog/validation/instance_test.go +++ b/pkg/apis/servicecatalog/validation/instance_test.go @@ -26,7 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) const ( @@ -893,6 +893,7 @@ func TestValidateServiceInstance(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { errs := internalValidateServiceInstance(tc.instance, tc.create) + errs = append(errs, validateServiceInstanceStatus(&tc.instance.Status, field.NewPath("status"), tc.create)...) if len(errs) != 0 && tc.valid { t.Errorf("unexpected error: %v", errs) } else if len(errs) == 0 && !tc.valid { diff --git a/pkg/apis/servicecatalog/validation/serviceclass.go b/pkg/apis/servicecatalog/validation/serviceclass.go index 986e12c4e32..eed22f581d1 100644 --- a/pkg/apis/servicecatalog/validation/serviceclass.go +++ b/pkg/apis/servicecatalog/validation/serviceclass.go @@ -21,7 +21,7 @@ import ( utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) const commonServiceClassNameMaxLength int = 63 diff --git a/pkg/apis/servicecatalog/validation/serviceclass_test.go b/pkg/apis/servicecatalog/validation/serviceclass_test.go index 1bfb39a6de7..0fb3f61d0b6 100644 --- a/pkg/apis/servicecatalog/validation/serviceclass_test.go +++ b/pkg/apis/servicecatalog/validation/serviceclass_test.go @@ -21,7 +21,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) func validClusterServiceClass() *servicecatalog.ClusterServiceClass { diff --git a/pkg/apis/servicecatalog/validation/serviceplan.go b/pkg/apis/servicecatalog/validation/serviceplan.go index 6e2b7c0339c..3490341bc39 100644 --- a/pkg/apis/servicecatalog/validation/serviceplan.go +++ b/pkg/apis/servicecatalog/validation/serviceplan.go @@ -23,7 +23,7 @@ import ( utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) const commonServicePlanNameMaxLength int = 63 diff --git a/pkg/apis/servicecatalog/validation/serviceplan_test.go b/pkg/apis/servicecatalog/validation/serviceplan_test.go index 84c2ae54324..9029536196f 100644 --- a/pkg/apis/servicecatalog/validation/serviceplan_test.go +++ b/pkg/apis/servicecatalog/validation/serviceplan_test.go @@ -21,7 +21,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" ) func validClusterServicePlan() *servicecatalog.ClusterServicePlan { diff --git a/pkg/apis/servicecatalog/validation/validation.go b/pkg/apis/servicecatalog/validation/validation.go index ab59a6536cc..1a9d0f890d1 100644 --- a/pkg/apis/servicecatalog/validation/validation.go +++ b/pkg/apis/servicecatalog/validation/validation.go @@ -17,7 +17,7 @@ limitations under the License. package validation import ( - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" "k8s.io/apimachinery/pkg/util/validation/field" "regexp" ) diff --git a/pkg/registry/servicecatalog/binding/strategy.go b/pkg/registry/servicecatalog/binding/strategy.go index dd185fa5677..b7b4f641d2a 100644 --- a/pkg/registry/servicecatalog/binding/strategy.go +++ b/pkg/registry/servicecatalog/binding/strategy.go @@ -32,7 +32,7 @@ import ( "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features" "k8s.io/klog" diff --git a/pkg/registry/servicecatalog/binding/strategy_test.go b/pkg/registry/servicecatalog/binding/strategy_test.go index 45755f59cf4..1fc07685a37 100644 --- a/pkg/registry/servicecatalog/binding/strategy_test.go +++ b/pkg/registry/servicecatalog/binding/strategy_test.go @@ -23,7 +23,7 @@ import ( sctestutil "github.com/kubernetes-sigs/service-catalog/test/util" utilfeature "k8s.io/apiserver/pkg/util/feature" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) diff --git a/pkg/registry/servicecatalog/clusterservicebroker/strategy.go b/pkg/registry/servicecatalog/clusterservicebroker/strategy.go index d7b85029d53..b41f5a563bf 100644 --- a/pkg/registry/servicecatalog/clusterservicebroker/strategy.go +++ b/pkg/registry/servicecatalog/clusterservicebroker/strategy.go @@ -28,7 +28,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" "k8s.io/klog" ) diff --git a/pkg/registry/servicecatalog/clusterservicebroker/strategy_test.go b/pkg/registry/servicecatalog/clusterservicebroker/strategy_test.go index c4decc232b3..773a165152e 100644 --- a/pkg/registry/servicecatalog/clusterservicebroker/strategy_test.go +++ b/pkg/registry/servicecatalog/clusterservicebroker/strategy_test.go @@ -19,7 +19,7 @@ package clusterservicebroker import ( "testing" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" sctestutil "github.com/kubernetes-sigs/service-catalog/test/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) diff --git a/pkg/registry/servicecatalog/clusterserviceclass/strategy.go b/pkg/registry/servicecatalog/clusterserviceclass/strategy.go index 478f433ef42..64874330cbe 100644 --- a/pkg/registry/servicecatalog/clusterserviceclass/strategy.go +++ b/pkg/registry/servicecatalog/clusterserviceclass/strategy.go @@ -27,7 +27,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" "k8s.io/klog" ) diff --git a/pkg/registry/servicecatalog/clusterserviceplan/strategy.go b/pkg/registry/servicecatalog/clusterserviceplan/strategy.go index 53e14cc0c72..5a25f2aee8d 100644 --- a/pkg/registry/servicecatalog/clusterserviceplan/strategy.go +++ b/pkg/registry/servicecatalog/clusterserviceplan/strategy.go @@ -27,7 +27,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" "k8s.io/klog" ) diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index e9e3c752cde..4a0ac817fad 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -32,7 +32,7 @@ import ( "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features" "k8s.io/klog" diff --git a/pkg/registry/servicecatalog/instance/strategy_test.go b/pkg/registry/servicecatalog/instance/strategy_test.go index c5dda6f6b10..d6f44f5548d 100644 --- a/pkg/registry/servicecatalog/instance/strategy_test.go +++ b/pkg/registry/servicecatalog/instance/strategy_test.go @@ -20,7 +20,7 @@ import ( "fmt" "testing" - "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + servicecatalog "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features" sctestutil "github.com/kubernetes-sigs/service-catalog/test/util" diff --git a/pkg/registry/servicecatalog/servicebroker/strategy.go b/pkg/registry/servicecatalog/servicebroker/strategy.go index 04dbef306cb..d8eef573e7a 100644 --- a/pkg/registry/servicecatalog/servicebroker/strategy.go +++ b/pkg/registry/servicecatalog/servicebroker/strategy.go @@ -28,7 +28,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" "k8s.io/klog" ) diff --git a/pkg/registry/servicecatalog/servicebroker/strategy_test.go b/pkg/registry/servicecatalog/servicebroker/strategy_test.go index 628453b7804..86cdad1102a 100644 --- a/pkg/registry/servicecatalog/servicebroker/strategy_test.go +++ b/pkg/registry/servicecatalog/servicebroker/strategy_test.go @@ -19,7 +19,7 @@ package servicebroker import ( "testing" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" sctestutil "github.com/kubernetes-sigs/service-catalog/test/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) diff --git a/pkg/registry/servicecatalog/serviceclass/strategy.go b/pkg/registry/servicecatalog/serviceclass/strategy.go index d0ebd1aa693..0b1c98037f3 100644 --- a/pkg/registry/servicecatalog/serviceclass/strategy.go +++ b/pkg/registry/servicecatalog/serviceclass/strategy.go @@ -27,7 +27,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" "k8s.io/klog" ) diff --git a/pkg/registry/servicecatalog/serviceplan/strategy.go b/pkg/registry/servicecatalog/serviceplan/strategy.go index 45c943a81b1..a203febd532 100644 --- a/pkg/registry/servicecatalog/serviceplan/strategy.go +++ b/pkg/registry/servicecatalog/serviceplan/strategy.go @@ -27,7 +27,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" - sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog" + sc "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1" scv "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/validation" "k8s.io/klog" ) diff --git a/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go b/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go index 53aaf7bffb8..478ba919f87 100644 --- a/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go +++ b/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go @@ -46,7 +46,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) cb := &sc.ClusterServiceBroker{} if err := webhookutil.MatchKinds(cb, req.Kind); err != nil { @@ -64,7 +64,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ClusterServiceBroker{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, oldObj, mutated) default: traced.Infof("ClusterServiceBroker mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -80,14 +85,6 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) return admission.PatchResponseFromRaw(req.Object.Raw, rawMutated) } -//var _ inject.Client = &CreateUpdateHandler{} -// -//// InjectClient injects the client into the CreateUpdateHandler -//func (h *CreateUpdateHandler) InjectClient(c client.Client) error { -// h.client = c -// return nil -//} - var _ admission.DecoderInjector = &CreateUpdateHandler{} // InjectDecoder injects the decoder into the CreateUpdateHandler @@ -106,6 +103,9 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, sb *sc.Cluster } } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ClusterServiceBroker) { - // TODO: implement logic from pkg/registry/servicecatalog/clusterservicebroker/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldClusterServiceBroker, newClusterServiceBroker *sc.ClusterServiceBroker) { + // Ignore the RelistRequests field when it is the default value + if newClusterServiceBroker.Spec.RelistRequests == 0 { + newClusterServiceBroker.Spec.RelistRequests = oldClusterServiceBroker.Spec.RelistRequests + } } diff --git a/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler_test.go b/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler_test.go index e97d8b41660..baa28352d9f 100644 --- a/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler_test.go @@ -136,6 +136,96 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + tests := map[string]struct { + oldRawObject []byte + newRawObj []byte + + expPatches []jsonpatch.Operation + }{ + "Should restore previous relist request, when not provided (set to 0)": { + oldRawObject: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker", + "generation": 1 + }, + "spec": { + "relistRequests": 1, + "relistBehavior": "Duration", + "url": "http://localhost:8081/" + } + }`), + newRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker", + "generation": 1 + }, + "spec": { + "relistRequests": 0, + "relistBehavior": "Duration", + "url": "http://localhost:8081/" + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/relistRequests", + Value: float64(1), + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-broker", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ClusterServiceBroker", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.oldRawObject}, + Object: runtime.RawExtension{Raw: tc.newRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{} + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleDecoderErrors(t *testing.T) { tester.DiscardLoggedMsg() diff --git a/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler.go b/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler.go index 2497bc4db71..0c79a713651 100644 --- a/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler.go +++ b/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler.go @@ -47,8 +47,8 @@ var _ inject.Client = &AdmissionHandler{} // NewAdmissionHandler creates new AdmissionHandler and initializes validators list func NewAdmissionHandler() *AdmissionHandler { return &AdmissionHandler{ - CreateValidators: []Validator{&AccessToBroker{}}, - UpdateValidators: []Validator{&AccessToBroker{}}, + CreateValidators: []Validator{&StaticCreate{}, &AccessToBroker{}}, + UpdateValidators: []Validator{&StaticUpdate{}, &AccessToBroker{}}, } } diff --git a/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status.go b/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status.go new file mode 100644 index 00000000000..5137b9dcad5 --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status.go @@ -0,0 +1,71 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + admissionTypes "k8s.io/api/admission/v1beta1" + + "context" + "net/http" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// StatusUpdateHandler provides status update resource validation handler +type StatusUpdateHandler struct { + decoder *admission.Decoder +} + +// Handle handles admission requests. +func (h *StatusUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name) + + if req.Operation != admissionTypes.Update { + traced.Infof("Operation %s is not validated", req.Operation) + return admission.Allowed("status operation allowed") + } + + newSb := &sc.ClusterServiceBroker{} + if err := h.decoder.Decode(req, newSb); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + oldSb := &sc.ClusterServiceBroker{} + if err := h.decoder.DecodeRaw(req.OldObject, oldSb); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + eList := scv.ValidateClusterServiceBrokerStatusUpdate(newSb, oldSb) + + if err := eList.ToAggregate(); err != nil { + traced.Infof("%s/%s update not allowed: %s", req.Kind.Kind, req.SubResource, err.Error()) + return admission.Denied(err.Error()) + } + + traced.Infof("Completed successfully validation operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name) + return admission.Allowed("status update allowed") +} + +// InjectDecoder injects the decoder into the handlers +func (h *StatusUpdateHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status_test.go b/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status_test.go new file mode 100644 index 00000000000..aa9aa1e0c9e --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterservicebroker/validation/handler_status_test.go @@ -0,0 +1,102 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "context" + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/clusterservicebroker/validation" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + admissionv1beta1 "k8s.io/api/admission/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "testing" +) + +// TestHandlerStatusValidate tests basic cases of ServiceBindingStatus validation. All status validations tests +// are covered by pkg/apis/servicecatalog/v1beta1/validation package +func TestHandlerStatusValidate(t *testing.T) { + tests := map[string]struct { + givenOldRawObj []byte + givenNewRawObj []byte + expectedAllowed bool + }{ + "Should not allow to set wrong unbind status": { + givenOldRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker" + }, + "spec": { + "relistRequests": 1, + "url": "http://localhost:8081/" + } + }`), + givenNewRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker" + }, + "spec": { + "relistRequests": 1, + "url": "http://localhost:8081/" + } + }`), + expectedAllowed: false, + }, + } + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + handler := &validation.StatusUpdateHandler{} + handler.InjectDecoder(decoder) + + req := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-broker", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ClusterServiceBroker", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.givenOldRawObj}, + Object: runtime.RawExtension{Raw: tc.givenNewRawObj}, + SubResource: "status", + }, + } + + // when + resp := handler.Handle(context.Background(), req) + + // then + assert.Equal(t, tc.expectedAllowed, resp.Allowed) + }) + } + +} diff --git a/pkg/webhook/servicecatalog/clusterservicebroker/validation/static.go b/pkg/webhook/servicecatalog/clusterservicebroker/validation/static.go new file mode 100644 index 00000000000..75f8708bb27 --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterservicebroker/validation/static.go @@ -0,0 +1,69 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate performs basic ServiceBroker validation for a Create operation. +type StaticCreate struct { +} + +// StaticUpdate performs basic ServiceBroker validation for an Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +var _ Validator = &StaticCreate{} +var _ Validator = &StaticUpdate{} +var _ admission.DecoderInjector = &StaticUpdate{} + +// Validate validate ClusterServicePlan instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, clusterServiceBroker *sc.ClusterServiceBroker, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateClusterServiceBroker(clusterServiceBroker).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ClusterServicePlan instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, clusterServiceBroker *sc.ClusterServiceBroker, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ClusterServiceBroker{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateClusterServiceBrokerUpdate(clusterServiceBroker, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler.go b/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler.go index a9e55debbad..b51b58715f9 100644 --- a/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler.go +++ b/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler.go @@ -38,7 +38,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) cb := &sc.ClusterServiceClass{} if err := webhookutil.MatchKinds(cb, req.Kind); err != nil { @@ -56,7 +56,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ClusterServiceClass{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, oldObj, mutated) default: traced.Infof("ClusterServiceClass mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -80,12 +85,11 @@ func (h *CreateUpdateHandler) InjectDecoder(d *admission.Decoder) error { return nil } -func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, binding *sc.ClusterServiceClass) { - binding.Finalizers = []string{sc.FinalizerServiceCatalog} +func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, csc *sc.ClusterServiceClass) { } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ClusterServiceClass) { - // TODO: implement logic from pkg/registry/servicecatalog/binding/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldClusterServiceClass, newClusterServiceClass *sc.ClusterServiceClass) { + newClusterServiceClass.Spec.ClusterServiceBrokerName = oldClusterServiceClass.Spec.ClusterServiceBrokerName } func (h *CreateUpdateHandler) syncLabels(obj *sc.ClusterServiceClass) { diff --git a/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler_test.go b/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler_test.go index 751b91ee849..94d600fc50a 100644 --- a/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/clusterserviceclass/mutation/handler_test.go @@ -103,6 +103,112 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + tests := map[string]struct { + oldRawObject []byte + newRawObj []byte + + expPatches []jsonpatch.Operation + }{ + "Should reset broker name to old one and allow to change other fields": { + oldRawObject: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServiceClass", + "metadata": { + "creationTimestamp": null, + "name": "test-class", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.clusterServiceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id" + } + }, + "spec": { + "clusterServiceBrokerName": "test-broker", + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": false, + "bindingRetrievable": false, + "planUpdatable": false + } + }`), + newRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServiceClass", + "metadata": { + "creationTimestamp": null, + "name": "test-class", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.clusterServiceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id" + } + }, + "spec": { + "clusterServiceBrokerName": "test-broker-new", + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": true, + "bindingRetrievable": true, + "planUpdatable": false + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/clusterServiceBrokerName", + Value: "test-broker", + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-class", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ClusterServiceClass", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.oldRawObject}, + Object: runtime.RawExtension{Raw: tc.newRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{} + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleDecoderErrors(t *testing.T) { tester.DiscardLoggedMsg() diff --git a/pkg/webhook/servicecatalog/clusterserviceclass/validation/handler.go b/pkg/webhook/servicecatalog/clusterserviceclass/validation/handler.go new file mode 100644 index 00000000000..06521fa9b77 --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterserviceclass/validation/handler.go @@ -0,0 +1,137 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + "net/http" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + + admissionTypes "k8s.io/api/admission/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// Validator is used to implement new validation logic +type Validator interface { + Validate(context.Context, admission.Request, *sc.ClusterServiceClass, *webhookutil.TracedLogger) *webhookutil.WebhookError +} + +// AdmissionHandler handles ServiceInstance validation +type AdmissionHandler struct { + decoder *admission.Decoder + client client.Client + + CreateValidators []Validator + UpdateValidators []Validator +} + +var _ admission.Handler = &AdmissionHandler{} +var _ admission.DecoderInjector = &AdmissionHandler{} +var _ inject.Client = &AdmissionHandler{} + +// NewAdmissionHandler creates new AdmissionHandler and initializes validators list +func NewAdmissionHandler() *AdmissionHandler { + return &AdmissionHandler{ + CreateValidators: []Validator{&StaticCreate{}}, + UpdateValidators: []Validator{&StaticUpdate{}}, + } +} + +// Handle handles admission requests. +func (h *AdmissionHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + + csc := &sc.ClusterServiceClass{} + if err := webhookutil.MatchKinds(csc, req.Kind); err != nil { + traced.Errorf("Error matching kinds: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + if err := h.decoder.Decode(req, csc); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + traced.Infof("start validation process for %s: %s/%s", csc.Kind, csc.Namespace, csc.Name) + + var err *webhookutil.WebhookError + + switch req.Operation { + case admissionTypes.Create: + for _, v := range h.CreateValidators { + err = v.Validate(ctx, req, csc, traced) + if err != nil { + break + } + } + case admissionTypes.Update: + for _, v := range h.UpdateValidators { + err = v.Validate(ctx, req, csc, traced) + if err != nil { + break + } + } + default: + traced.Infof("ClusterServiceBroker validation wehbook does not support action %q", req.Operation) + return admission.Allowed("action not taken") + } + + if err != nil { + switch err.Code() { + case http.StatusForbidden: + return admission.Denied(err.Error()) + default: + return admission.Errored(err.Code(), err) + } + } + + traced.Infof("Completed successfully validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + return admission.Allowed("ClusterServiceClass AdmissionHandler successful") +} + +// InjectDecoder injects the decoder into the handlers +func (h *AdmissionHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + + for _, v := range h.CreateValidators { + admission.InjectDecoderInto(d, v) + } + for _, v := range h.UpdateValidators { + admission.InjectDecoderInto(d, v) + } + + return nil +} + +// InjectClient injects the client into the handlers +func (h *AdmissionHandler) InjectClient(c client.Client) error { + h.client = c + + for _, v := range h.CreateValidators { + inject.ClientInto(c, v) + } + for _, v := range h.UpdateValidators { + inject.ClientInto(c, v) + } + + return nil +} diff --git a/pkg/webhook/servicecatalog/clusterserviceclass/validation/handler_test.go b/pkg/webhook/servicecatalog/clusterserviceclass/validation/handler_test.go new file mode 100644 index 00000000000..92dc91f530b --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterserviceclass/validation/handler_test.go @@ -0,0 +1,36 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "testing" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/clusterserviceclass/validation" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil/tester" +) + +func TestAdmissionHandlerHandleDecoderErrors(t *testing.T) { + tester.DiscardLoggedMsg() + + for _, fn := range []func(t *testing.T, handler tester.TestDecoderHandler, kind string){ + tester.AssertHandlerReturnErrorIfReqObjIsMalformed, + tester.AssertHandlerReturnErrorIfGVKMismatch, + } { + handler := validation.AdmissionHandler{} + fn(t, &handler, "ClusterServiceClass") + } +} diff --git a/pkg/webhook/servicecatalog/clusterserviceclass/validation/static.go b/pkg/webhook/servicecatalog/clusterserviceclass/validation/static.go new file mode 100644 index 00000000000..eb7de1c4632 --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterserviceclass/validation/static.go @@ -0,0 +1,69 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate performs basic ClusterServiceClass validation for a Create operation. +type StaticCreate struct { +} + +// StaticUpdate performs basic ClusterServiceClass validation for a Create operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +var _ Validator = &StaticCreate{} +var _ Validator = &StaticUpdate{} +var _ admission.DecoderInjector = &StaticUpdate{} + +// Validate validate ClusterServicePlan instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, clusterServiceClass *sc.ClusterServiceClass, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateClusterServiceClass(clusterServiceClass).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ClusterServicePlan instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, clusterServiceClass *sc.ClusterServiceClass, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ClusterServiceClass{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateClusterServiceClassUpdate(clusterServiceClass, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler.go b/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler.go index 06f9bb2f4e9..3a9aaadc8b9 100644 --- a/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler.go +++ b/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler.go @@ -38,7 +38,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) cb := &sc.ClusterServicePlan{} if err := webhookutil.MatchKinds(cb, req.Kind); err != nil { @@ -56,7 +56,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ClusterServicePlan{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, oldObj, mutated) default: traced.Infof("ClusterServicePlan mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -84,8 +89,9 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, binding *sc.Cl } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ClusterServicePlan) { - // TODO: implement logic from pkg/registry/servicecatalog/binding/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldClusterServicePlan, newClusterServicePlan *sc.ClusterServicePlan) { + newClusterServicePlan.Spec.ClusterServiceClassRef = oldClusterServicePlan.Spec.ClusterServiceClassRef + newClusterServicePlan.Spec.ClusterServiceBrokerName = oldClusterServicePlan.Spec.ClusterServiceBrokerName } func (h *CreateUpdateHandler) syncLabels(obj *sc.ClusterServicePlan) { diff --git a/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler_test.go b/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler_test.go index 1c8000eceb8..c9a9bc44fdb 100644 --- a/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler_test.go @@ -106,6 +106,119 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + tests := map[string]struct { + oldRawObject []byte + newRawObj []byte + + expPatches []jsonpatch.Operation + }{ + "Should reset broker name and class ref to old one and allow to change other fields": { + oldRawObject: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServicePlan", + "metadata": { + "creationTimestamp": null, + "name": "test-plan", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.clusterServiceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id", + "servicecatalog.k8s.io/spec.clusterServiceClassRef.name":"external-class" + } + }, + "spec": { + "clusterServiceBrokerName": "test-broker", + "clusterServiceClassRef": {"name":"external-class"}, + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": false, + "free": false + } + }`), + newRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ClusterServicePlan", + "metadata": { + "creationTimestamp": null, + "name": "test-plan", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.clusterServiceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id", + "servicecatalog.k8s.io/spec.clusterServiceClassRef.name":"external-class" + } + }, + "spec": { + "clusterServiceBrokerName": "test-broker-changed", + "clusterServiceClassRef": {"name":"external-class-changed"}, + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": true, + "free": true + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/clusterServiceBrokerName", + Value: "test-broker", + }, + { + Operation: "replace", + Path: "/spec/clusterServiceClassRef/name", + Value: "external-class", + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-plan", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ClusterServicePlan", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.oldRawObject}, + Object: runtime.RawExtension{Raw: tc.newRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{} + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleDecoderErrors(t *testing.T) { tester.DiscardLoggedMsg() diff --git a/pkg/webhook/servicecatalog/clusterserviceplan/validation/handler.go b/pkg/webhook/servicecatalog/clusterserviceplan/validation/handler.go new file mode 100644 index 00000000000..0b2cc0d1b0a --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterserviceplan/validation/handler.go @@ -0,0 +1,137 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + "net/http" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + + admissionTypes "k8s.io/api/admission/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// Validator is used to implement new validation logic +type Validator interface { + Validate(context.Context, admission.Request, *sc.ClusterServicePlan, *webhookutil.TracedLogger) *webhookutil.WebhookError +} + +// AdmissionHandler handles ServiceInstance validation +type AdmissionHandler struct { + decoder *admission.Decoder + client client.Client + + CreateValidators []Validator + UpdateValidators []Validator +} + +var _ admission.Handler = &AdmissionHandler{} +var _ admission.DecoderInjector = &AdmissionHandler{} +var _ inject.Client = &AdmissionHandler{} + +// NewAdmissionHandler creates new AdmissionHandler and initializes validators list +func NewAdmissionHandler() *AdmissionHandler { + return &AdmissionHandler{ + CreateValidators: []Validator{&StaticCreate{}}, + UpdateValidators: []Validator{&StaticUpdate{}}, + } +} + +// Handle handles admission requests. +func (h *AdmissionHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + + csp := &sc.ClusterServicePlan{} + if err := webhookutil.MatchKinds(csp, req.Kind); err != nil { + traced.Errorf("Error matching kinds: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + if err := h.decoder.Decode(req, csp); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + traced.Infof("start validation process for %s: %s/%s", csp.Kind, csp.Namespace, csp.Name) + + var err *webhookutil.WebhookError + + switch req.Operation { + case admissionTypes.Create: + for _, v := range h.CreateValidators { + err = v.Validate(ctx, req, csp, traced) + if err != nil { + break + } + } + case admissionTypes.Update: + for _, v := range h.UpdateValidators { + err = v.Validate(ctx, req, csp, traced) + if err != nil { + break + } + } + default: + traced.Infof("ClusterServiceBroker validation wehbook does not support action %q", req.Operation) + return admission.Allowed("action not taken") + } + + if err != nil { + switch err.Code() { + case http.StatusForbidden: + return admission.Denied(err.Error()) + default: + return admission.Errored(err.Code(), err) + } + } + + traced.Infof("Completed successfully validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + return admission.Allowed("ClusterServicePlan AdmissionHandler successful") +} + +// InjectDecoder injects the decoder into the handlers +func (h *AdmissionHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + + for _, v := range h.CreateValidators { + admission.InjectDecoderInto(d, v) + } + for _, v := range h.UpdateValidators { + admission.InjectDecoderInto(d, v) + } + + return nil +} + +// InjectClient injects the client into the handlers +func (h *AdmissionHandler) InjectClient(c client.Client) error { + h.client = c + + for _, v := range h.CreateValidators { + inject.ClientInto(c, v) + } + for _, v := range h.UpdateValidators { + inject.ClientInto(c, v) + } + + return nil +} diff --git a/pkg/webhook/servicecatalog/clusterserviceplan/validation/handler_test.go b/pkg/webhook/servicecatalog/clusterserviceplan/validation/handler_test.go new file mode 100644 index 00000000000..653d31e04d4 --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterserviceplan/validation/handler_test.go @@ -0,0 +1,36 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "testing" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/clusterserviceplan/validation" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil/tester" +) + +func TestAdmissionHandlerHandleDecoderErrors(t *testing.T) { + tester.DiscardLoggedMsg() + + for _, fn := range []func(t *testing.T, handler tester.TestDecoderHandler, kind string){ + tester.AssertHandlerReturnErrorIfReqObjIsMalformed, + tester.AssertHandlerReturnErrorIfGVKMismatch, + } { + handler := validation.AdmissionHandler{} + fn(t, &handler, "ClusterServicePlan") + } +} diff --git a/pkg/webhook/servicecatalog/clusterserviceplan/validation/static.go b/pkg/webhook/servicecatalog/clusterserviceplan/validation/static.go new file mode 100644 index 00000000000..58d4ab8c5c6 --- /dev/null +++ b/pkg/webhook/servicecatalog/clusterserviceplan/validation/static.go @@ -0,0 +1,69 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate performs basic ClusterServicePlan validation for a Create operation. +type StaticCreate struct { +} + +// StaticUpdate performs basic ClusterServicePlan validation for an Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +var _ Validator = &StaticCreate{} +var _ Validator = &StaticUpdate{} +var _ admission.DecoderInjector = &StaticUpdate{} + +// Validate validates ClusterServicePlan instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, clusterServicePlan *sc.ClusterServicePlan, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateClusterServicePlan(clusterServicePlan).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validates ClusterServicePlan instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, clusterServicePlan *sc.ClusterServicePlan, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ClusterServicePlan{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateClusterServicePlanUpdate(clusterServicePlan, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/servicebinding/mutation/handler.go b/pkg/webhook/servicecatalog/servicebinding/mutation/handler.go index 78151fffca8..575958d93fd 100644 --- a/pkg/webhook/servicecatalog/servicebinding/mutation/handler.go +++ b/pkg/webhook/servicecatalog/servicebinding/mutation/handler.go @@ -41,7 +41,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) sb := &sc.ServiceBinding{} if err := webhookutil.MatchKinds(sb, req.Kind); err != nil { @@ -59,7 +59,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, req, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ServiceBinding{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, req, oldObj, mutated) default: traced.Infof("ServiceBinding mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -84,7 +89,8 @@ func (h *CreateUpdateHandler) InjectDecoder(d *admission.Decoder) error { } func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission.Request, binding *sc.ServiceBinding) { - binding.Finalizers = []string{sc.FinalizerServiceCatalog} + // This feature was copied from Service Catalog registry: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/registry/servicecatalog/binding/strategy.go + // If you want to track previous changes please check there. if binding.Spec.ExternalID == "" { binding.Spec.ExternalID = string(h.UUID.New()) @@ -97,10 +103,18 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission. if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) { setServiceBindingUserInfo(req, binding) } + + binding.Finalizers = []string{sc.FinalizerServiceCatalog} } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServiceBinding) { - // TODO: implement logic from pkg/registry/servicecatalog/binding/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, req admission.Request, oldServiceBinding, newServiceBinding *sc.ServiceBinding) { + // TODO: We currently don't handle any changes to the spec in the + // reconciler. Once we do that, this check needs to be removed and + // proper validation of allowed changes needs to be implemented in + // ValidateUpdate. Also, the check for whether the generation needs + // to be updated needs to be un-commented. + // If the Spec change is allowed do not forget to update UserInfo (setServiceBindingUserInfo function) + newServiceBinding.Spec = oldServiceBinding.Spec } // setServiceBindingUserInfo injects user.Info from the request context diff --git a/pkg/webhook/servicecatalog/servicebinding/mutation/handler_test.go b/pkg/webhook/servicecatalog/servicebinding/mutation/handler_test.go index aafed468c8f..edb63c1ddd5 100644 --- a/pkg/webhook/servicecatalog/servicebinding/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/servicebinding/mutation/handler_test.go @@ -156,6 +156,103 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + const fixUUID = "mocked-uuid-123-abc" + tests := map[string]struct { + givenOldRawObj []byte + givenNewRawObj []byte + expPatches []jsonpatch.Operation + }{ + "Should replace spec changes by existing one": { + givenOldRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBinding", + "metadata": { + "creationTimestamp": null, + "name": "test-binding" + }, + "spec": { + "externalID": "id-0123", + "instanceRef": { + "name": "some-instance" + } + } + }`), + givenNewRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBinding", + "metadata": { + "creationTimestamp": null, + "name": "test-binding" + }, + "spec": { + "externalID": "id-0123", + "instanceRef": { + "name": "some-instance-1" + } + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/instanceRef/name", + Value: "some-instance", + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + err = utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%v=false", scfeatures.OriginatingIdentity)) + require.NoError(t, err, "cannot disable OriginatingIdentity feature") + // restore default state + defer utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%v=true", scfeatures.OriginatingIdentity)) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-binding", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServiceBinding", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + Object: runtime.RawExtension{Raw: tc.givenNewRawObj}, + OldObject: runtime.RawExtension{Raw: tc.givenOldRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{ + UUID: func() types.UID { return fixUUID }, + } + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleSetUserInfoIfOriginatingIdentityIsEnabled(t *testing.T) { // given sc.AddToScheme(scheme.Scheme) diff --git a/pkg/webhook/servicecatalog/servicebinding/validation/handler.go b/pkg/webhook/servicecatalog/servicebinding/validation/handler.go index 55b32014632..d700356bb14 100644 --- a/pkg/webhook/servicecatalog/servicebinding/validation/handler.go +++ b/pkg/webhook/servicecatalog/servicebinding/validation/handler.go @@ -47,7 +47,8 @@ var _ inject.Client = &AdmissionHandler{} // NewAdmissionHandler creates new AdmissionHandler and initializes validators list func NewAdmissionHandler() *AdmissionHandler { return &AdmissionHandler{ - CreateValidators: []Validator{&ReferenceDeletion{}}, + CreateValidators: []Validator{&ReferenceDeletion{}, &StaticCreate{}}, + UpdateValidators: []Validator{&StaticUpdate{}}, } } diff --git a/pkg/webhook/servicecatalog/servicebinding/validation/handler_status.go b/pkg/webhook/servicecatalog/servicebinding/validation/handler_status.go new file mode 100644 index 00000000000..352c3838566 --- /dev/null +++ b/pkg/webhook/servicecatalog/servicebinding/validation/handler_status.go @@ -0,0 +1,72 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + "net/http" + + admissionTypes "k8s.io/api/admission/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + webhookutil "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" +) + +// StatusUpdateValidationHandler provides status resource validation +type StatusUpdateValidationHandler struct { + decoder *admission.Decoder +} + +// Handle handles admission requests. +func (h *StatusUpdateValidationHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name) + + if req.Operation != admissionTypes.Update { + traced.Infof("Operation %s is not validated", req.Operation) + return admission.Allowed("status operation allowed") + } + + newSb := &sc.ServiceBinding{} + if err := h.decoder.Decode(req, newSb); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + oldSb := &sc.ServiceBinding{} + if err := h.decoder.DecodeRaw(req.OldObject, oldSb); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + eList := scv.ValidateServiceBindingStatusUpdate(newSb, oldSb) + + if err := eList.ToAggregate(); err != nil { + traced.Infof("%s/%s update not allowed: %s", req.Kind.Kind, req.SubResource, err.Error()) + return admission.Denied(err.Error()) + } + + traced.Infof("Completed successfully validation operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name) + return admission.Allowed("status update allowed") +} + +// InjectDecoder injects the decoder into the handlers +func (h *StatusUpdateValidationHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/servicebinding/validation/handler_status_test.go b/pkg/webhook/servicecatalog/servicebinding/validation/handler_status_test.go new file mode 100644 index 00000000000..93c0bc34a29 --- /dev/null +++ b/pkg/webhook/servicecatalog/servicebinding/validation/handler_status_test.go @@ -0,0 +1,161 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "context" + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/servicebinding/validation" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + admissionv1beta1 "k8s.io/api/admission/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "testing" +) + +// TestHandlerStatusValidate tests basic cases of ServiceBindingStatus validation. All status validations tests +// are covered by pkg/apis/servicecatalog/v1beta1/validation package +func TestHandlerStatusValidate(t *testing.T) { + tests := map[string]struct { + givenOldRawObj []byte + givenNewRawObj []byte + expectedAllowed bool + }{ + "Should not allow to set wrong unbind status": { + givenOldRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBinding", + "metadata": { + "creationTimestamp": null, + "name": "test-binding", + "namespace": "default" + }, + "spec": { + "externalID": "id-0123", + "instanceRef": { + "name": "some-instance" + }, + "secretName": "test-binding" + }, + "status": { + "conditions": [], + "unbindStatus": "NotRequired" + } + }`), + givenNewRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBinding", + "metadata": { + "creationTimestamp": null, + "name": "test-binding", + "namespace": "default" + }, + "spec": { + "externalID": "id-0123", + "instanceRef": { + "name": "some-instance" + }, + "secretName": "test-binding" + }, + "status": { + "conditions": [], + "unbindStatus": "not-allowed" + } + }`), + expectedAllowed: false, + }, + "Should not allow to set correct unbind status": { + givenOldRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBinding", + "metadata": { + "creationTimestamp": null, + "name": "test-binding", + "namespace": "default" + }, + "spec": { + "externalID": "id-0123", + "instanceRef": { + "name": "some-instance" + }, + "secretName": "test-binding" + }, + "status": { + "conditions": [], + "unbindStatus": "NotRequired" + } + }`), + givenNewRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBinding", + "metadata": { + "creationTimestamp": null, + "name": "test-binding", + "namespace": "default" + }, + "spec": { + "externalID": "id-0123", + "instanceRef": { + "name": "some-instance" + }, + "secretName": "test-binding" + }, + "status": { + "conditions": [], + "unbindStatus": "Succeeded" + } + }`), + expectedAllowed: true, + }, + } + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + handler := &validation.StatusUpdateValidationHandler{} + handler.InjectDecoder(decoder) + + req := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-binding", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServiceBinding", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.givenOldRawObj}, + Object: runtime.RawExtension{Raw: tc.givenNewRawObj}, + SubResource: "status", + }, + } + + // when + resp := handler.Handle(context.Background(), req) + + // then + assert.Equal(t, tc.expectedAllowed, resp.Allowed) + }) + } + +} diff --git a/pkg/webhook/servicecatalog/servicebinding/validation/static.go b/pkg/webhook/servicecatalog/servicebinding/validation/static.go new file mode 100644 index 00000000000..cb7f0539bd2 --- /dev/null +++ b/pkg/webhook/servicecatalog/servicebinding/validation/static.go @@ -0,0 +1,69 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate performs basic ServiceBroker validation for a Create operation. +type StaticCreate struct { +} + +// StaticUpdate performs basic ServiceBroker validation for an Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +var _ Validator = &StaticCreate{} +var _ Validator = &StaticUpdate{} +var _ admission.DecoderInjector = &StaticUpdate{} + +// Validate validate ServiceBinding instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, serviceBinding *sc.ServiceBinding, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateServiceBinding(serviceBinding).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ServiceBinding instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, serviceBinding *sc.ServiceBinding, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ServiceBinding{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateServiceBindingUpdate(serviceBinding, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/servicebroker/mutation/handler.go b/pkg/webhook/servicecatalog/servicebroker/mutation/handler.go index 709b73c29e5..0aa2a05b256 100644 --- a/pkg/webhook/servicecatalog/servicebroker/mutation/handler.go +++ b/pkg/webhook/servicecatalog/servicebroker/mutation/handler.go @@ -38,7 +38,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) sb := &sc.ServiceBroker{} if err := webhookutil.MatchKinds(sb, req.Kind); err != nil { @@ -56,7 +56,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ServiceBroker{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, oldObj, mutated) default: traced.Infof("ServiceBroker mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -88,6 +93,11 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, sb *sc.Service } } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServiceBroker) { - // TODO: implement logic from pkg/registry/servicecatalog/servicebroker/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldSb, newSb *sc.ServiceBroker) { + // This feature was copied from Service Catalog registry: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/registry/servicecatalog/servicebroker/strategy.go + // If you want to track previous changes please check there. + + if newSb.Spec.RelistRequests == 0 { + newSb.Spec.RelistRequests = oldSb.Spec.RelistRequests + } } diff --git a/pkg/webhook/servicecatalog/servicebroker/mutation/handler_test.go b/pkg/webhook/servicecatalog/servicebroker/mutation/handler_test.go index 57de26254a3..5419a6043d7 100644 --- a/pkg/webhook/servicecatalog/servicebroker/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/servicebroker/mutation/handler_test.go @@ -136,6 +136,96 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + tests := map[string]struct { + oldRawObject []byte + newRawObj []byte + + expPatches []jsonpatch.Operation + }{ + "Should override relist request": { + oldRawObject: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker", + "generation": 1 + }, + "spec": { + "relistRequests": 1, + "relistBehavior": "Duration", + "url": "http://localhost:8081/" + } + }`), + newRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker", + "generation": 1 + }, + "spec": { + "relistRequests": 0, + "relistBehavior": "Duration", + "url": "http://localhost:8081/" + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/relistRequests", + Value: float64(1), + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-broker", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServiceBroker", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.oldRawObject}, + Object: runtime.RawExtension{Raw: tc.newRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{} + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleDecoderErrors(t *testing.T) { tester.DiscardLoggedMsg() diff --git a/pkg/webhook/servicecatalog/servicebroker/validation/handler.go b/pkg/webhook/servicecatalog/servicebroker/validation/handler.go index 47e4662e7f2..c0200616e09 100644 --- a/pkg/webhook/servicecatalog/servicebroker/validation/handler.go +++ b/pkg/webhook/servicecatalog/servicebroker/validation/handler.go @@ -47,8 +47,8 @@ var _ inject.Client = &AdmissionHandler{} // NewAdmissionHandler creates new AdmissionHandler and initializes validators list func NewAdmissionHandler() *AdmissionHandler { return &AdmissionHandler{ - CreateValidators: []Validator{&AccessToBroker{}}, - UpdateValidators: []Validator{&AccessToBroker{}}, + CreateValidators: []Validator{&StaticCreate{}, &AccessToBroker{}}, + UpdateValidators: []Validator{&StaticUpdate{}, &AccessToBroker{}}, } } diff --git a/pkg/webhook/servicecatalog/servicebroker/validation/handler_status.go b/pkg/webhook/servicecatalog/servicebroker/validation/handler_status.go new file mode 100644 index 00000000000..c736ebffa39 --- /dev/null +++ b/pkg/webhook/servicecatalog/servicebroker/validation/handler_status.go @@ -0,0 +1,71 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + admissionTypes "k8s.io/api/admission/v1beta1" + + "context" + "net/http" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// StatusUpdateHandler provides status update resource validation handler +type StatusUpdateHandler struct { + decoder *admission.Decoder +} + +// Handle handles admission requests. +func (h *StatusUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name) + + if req.Operation != admissionTypes.Update { + traced.Infof("Operation %s is not validated", req.Operation) + return admission.Allowed("status operation allowed") + } + + newSb := &sc.ServiceBroker{} + if err := h.decoder.Decode(req, newSb); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + oldSb := &sc.ServiceBroker{} + if err := h.decoder.DecodeRaw(req.OldObject, oldSb); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + eList := scv.ValidateServiceBrokerStatusUpdate(newSb, oldSb) + + if err := eList.ToAggregate(); err != nil { + traced.Infof("%s/%s update not allowed: %s", req.Kind.Kind, req.SubResource, err.Error()) + return admission.Denied(err.Error()) + } + + traced.Infof("Completed successfully validation operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name) + return admission.Allowed("status update allowed") +} + +// InjectDecoder injects the decoder into the handlers +func (h *StatusUpdateHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/servicebroker/validation/handler_status_test.go b/pkg/webhook/servicecatalog/servicebroker/validation/handler_status_test.go new file mode 100644 index 00000000000..8a78dd885d7 --- /dev/null +++ b/pkg/webhook/servicecatalog/servicebroker/validation/handler_status_test.go @@ -0,0 +1,102 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "context" + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/servicebroker/validation" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + admissionv1beta1 "k8s.io/api/admission/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "testing" +) + +// TestHandlerStatusValidate tests basic cases of ServiceBindingStatus validation. All status validations tests +// are covered by pkg/apis/servicecatalog/v1beta1/validation package +func TestHandlerStatusValidate(t *testing.T) { + tests := map[string]struct { + givenOldRawObj []byte + givenNewRawObj []byte + expectedAllowed bool + }{ + "Should not allow to set wrong unbind status": { + givenOldRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker" + }, + "spec": { + "relistRequests": 1, + "url": "http://localhost:8081/" + } + }`), + givenNewRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceBroker", + "metadata": { + "creationTimestamp": null, + "name": "test-broker" + }, + "spec": { + "relistRequests": 1, + "url": "http://localhost:8081/" + } + }`), + expectedAllowed: false, + }, + } + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + handler := &validation.StatusUpdateHandler{} + handler.InjectDecoder(decoder) + + req := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-broker", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServiceBroker", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.givenOldRawObj}, + Object: runtime.RawExtension{Raw: tc.givenNewRawObj}, + SubResource: "status", + }, + } + + // when + resp := handler.Handle(context.Background(), req) + + // then + assert.Equal(t, tc.expectedAllowed, resp.Allowed) + }) + } + +} diff --git a/pkg/webhook/servicecatalog/servicebroker/validation/static.go b/pkg/webhook/servicecatalog/servicebroker/validation/static.go new file mode 100644 index 00000000000..c3aa5e9e020 --- /dev/null +++ b/pkg/webhook/servicecatalog/servicebroker/validation/static.go @@ -0,0 +1,65 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate runs basic ServiceBroker validation for a Create operation. +type StaticCreate struct { +} + +// StaticUpdate runs basic ServiceBroker validation for an Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +// Validate validate ServiceBinding instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, serviceBroker *sc.ServiceBroker, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateServiceBroker(serviceBroker).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ServiceBinding instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, serviceBroker *sc.ServiceBroker, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ServiceBroker{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateServiceBrokerUpdate(serviceBroker, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/serviceclass/mutation/handler.go b/pkg/webhook/servicecatalog/serviceclass/mutation/handler.go index 1cd1b07a152..d8554b4d1c1 100644 --- a/pkg/webhook/servicecatalog/serviceclass/mutation/handler.go +++ b/pkg/webhook/servicecatalog/serviceclass/mutation/handler.go @@ -38,7 +38,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) cb := &sc.ServiceClass{} if err := webhookutil.MatchKinds(cb, req.Kind); err != nil { @@ -56,7 +56,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ServiceClass{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, oldObj, mutated) default: traced.Infof("ServiceClass mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -80,12 +85,12 @@ func (h *CreateUpdateHandler) InjectDecoder(d *admission.Decoder) error { return nil } -func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, binding *sc.ServiceClass) { - +func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, serviceClass *sc.ServiceClass) { + serviceClass.Status = sc.ServiceClassStatus{} } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServiceClass) { - // TODO: implement logic from pkg/registry/servicecatalog/binding/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldServiceClass, newServiceClass *sc.ServiceClass) { + newServiceClass.Spec.ServiceBrokerName = oldServiceClass.Spec.ServiceBrokerName } func (h *CreateUpdateHandler) syncLabels(obj *sc.ServiceClass) { diff --git a/pkg/webhook/servicecatalog/serviceclass/mutation/handler_test.go b/pkg/webhook/servicecatalog/serviceclass/mutation/handler_test.go index 3483705c58f..915a6e2fdc5 100644 --- a/pkg/webhook/servicecatalog/serviceclass/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/serviceclass/mutation/handler_test.go @@ -103,6 +103,112 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + tests := map[string]struct { + oldRawObject []byte + newRawObj []byte + + expPatches []jsonpatch.Operation + }{ + "Should reset broker name to old one and allow to change other fields": { + oldRawObject: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceClass", + "metadata": { + "creationTimestamp": null, + "name": "test-class", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.serviceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id" + } + }, + "spec": { + "serviceBrokerName": "test-broker", + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": false, + "bindingRetrievable": false, + "planUpdatable": false + } + }`), + newRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceClass", + "metadata": { + "creationTimestamp": null, + "name": "test-class", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.serviceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id" + } + }, + "spec": { + "serviceBrokerName": "test-broker-new", + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": true, + "bindingRetrievable": true, + "planUpdatable": false + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/serviceBrokerName", + Value: "test-broker", + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-class", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServiceClass", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.oldRawObject}, + Object: runtime.RawExtension{Raw: tc.newRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{} + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleDecoderErrors(t *testing.T) { tester.DiscardLoggedMsg() diff --git a/pkg/webhook/servicecatalog/serviceclass/validation/handler.go b/pkg/webhook/servicecatalog/serviceclass/validation/handler.go new file mode 100644 index 00000000000..6dc1ff78b1e --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceclass/validation/handler.go @@ -0,0 +1,137 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + "net/http" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + + admissionTypes "k8s.io/api/admission/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// Validator is used to implement new validation logic +type Validator interface { + Validate(context.Context, admission.Request, *sc.ServiceClass, *webhookutil.TracedLogger) *webhookutil.WebhookError +} + +// AdmissionHandler handles ServiceInstance validation +type AdmissionHandler struct { + decoder *admission.Decoder + client client.Client + + CreateValidators []Validator + UpdateValidators []Validator +} + +var _ admission.Handler = &AdmissionHandler{} +var _ admission.DecoderInjector = &AdmissionHandler{} +var _ inject.Client = &AdmissionHandler{} + +// NewAdmissionHandler creates new AdmissionHandler and initializes validators list +func NewAdmissionHandler() *AdmissionHandler { + return &AdmissionHandler{ + CreateValidators: []Validator{&StaticCreate{}}, + UpdateValidators: []Validator{&StaticUpdate{}}, + } +} + +// Handle handles admission requests. +func (h *AdmissionHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + + serviceClass := &sc.ServiceClass{} + if err := webhookutil.MatchKinds(serviceClass, req.Kind); err != nil { + traced.Errorf("Error matching kinds: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + if err := h.decoder.Decode(req, serviceClass); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + traced.Infof("start validation process for %s: %s/%s", serviceClass.Kind, serviceClass.Namespace, serviceClass.Name) + + var err *webhookutil.WebhookError + + switch req.Operation { + case admissionTypes.Create: + for _, v := range h.CreateValidators { + err = v.Validate(ctx, req, serviceClass, traced) + if err != nil { + break + } + } + case admissionTypes.Update: + for _, v := range h.UpdateValidators { + err = v.Validate(ctx, req, serviceClass, traced) + if err != nil { + break + } + } + default: + traced.Infof("ServiceInstance validation wehbook does not support action %q", req.Operation) + return admission.Allowed("action not taken") + } + + if err != nil { + switch err.Code() { + case http.StatusForbidden: + return admission.Denied(err.Error()) + default: + return admission.Errored(err.Code(), err) + } + } + + traced.Infof("Completed successfully validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + return admission.Allowed("ServiceClass AdmissionHandler successful") +} + +// InjectDecoder injects the decoder into the handlers +func (h *AdmissionHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + + for _, v := range h.CreateValidators { + admission.InjectDecoderInto(d, v) + } + for _, v := range h.UpdateValidators { + admission.InjectDecoderInto(d, v) + } + + return nil +} + +// InjectClient injects the client into the handlers +func (h *AdmissionHandler) InjectClient(c client.Client) error { + h.client = c + + for _, v := range h.CreateValidators { + inject.ClientInto(c, v) + } + for _, v := range h.UpdateValidators { + inject.ClientInto(c, v) + } + + return nil +} diff --git a/pkg/webhook/servicecatalog/serviceclass/validation/handler_test.go b/pkg/webhook/servicecatalog/serviceclass/validation/handler_test.go new file mode 100644 index 00000000000..4cd48c3e598 --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceclass/validation/handler_test.go @@ -0,0 +1,36 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "testing" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/serviceclass/validation" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil/tester" +) + +func TestAdmissionHandlerHandleDecoderErrors(t *testing.T) { + tester.DiscardLoggedMsg() + + for _, fn := range []func(t *testing.T, handler tester.TestDecoderHandler, kind string){ + tester.AssertHandlerReturnErrorIfReqObjIsMalformed, + tester.AssertHandlerReturnErrorIfGVKMismatch, + } { + handler := validation.AdmissionHandler{} + fn(t, &handler, "ServiceClass") + } +} diff --git a/pkg/webhook/servicecatalog/serviceclass/validation/static.go b/pkg/webhook/servicecatalog/serviceclass/validation/static.go new file mode 100644 index 00000000000..3073af8f99e --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceclass/validation/static.go @@ -0,0 +1,65 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate runs basic ServiceClass validation for Create operation. +type StaticCreate struct { +} + +// StaticUpdate runs basic ServiceClass validation for Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +// Validate validate ServiceBinding instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, serviceClass *sc.ServiceClass, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateServiceClass(serviceClass).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ServiceBinding instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, serviceClass *sc.ServiceClass, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ServiceClass{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateServiceClassUpdate(serviceClass, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/serviceinstance/mutation/handler.go b/pkg/webhook/servicecatalog/serviceinstance/mutation/handler.go index ab4b5dc4350..6d8596e3ec4 100644 --- a/pkg/webhook/servicecatalog/serviceinstance/mutation/handler.go +++ b/pkg/webhook/servicecatalog/serviceinstance/mutation/handler.go @@ -27,6 +27,7 @@ import ( admissionTypes "k8s.io/api/admission/v1beta1" utilfeature "k8s.io/apiserver/pkg/util/feature" + apiequality "k8s.io/apimachinery/pkg/api/equality" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -51,7 +52,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) si := &sc.ServiceInstance{} if err := webhookutil.MatchKinds(si, req.Kind); err != nil { @@ -69,7 +70,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, req, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ServiceInstance{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, req, oldObj, mutated) default: traced.Infof("ServiceInstance mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -104,7 +110,8 @@ func (h *CreateUpdateHandler) InjectDecoder(d *admission.Decoder) error { } func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission.Request, instance *sc.ServiceInstance) { - instance.Finalizers = []string{sc.FinalizerServiceCatalog} + // This feature was copied from Service Catalog registry: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/registry/servicecatalog/instance/strategy.go + // If you want to track previous changes please check there. if instance.Spec.ExternalID == "" { instance.Spec.ExternalID = string(h.UUID.New()) @@ -113,10 +120,32 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission. if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) { setServiceInstanceUserInfo(req, instance) } + + instance.Spec.ClusterServiceClassRef = nil + instance.Spec.ClusterServicePlanRef = nil + instance.Finalizers = []string{sc.FinalizerServiceCatalog} } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServiceInstance) { - // TODO: implement logic from pkg/registry/servicecatalog/instance/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, req admission.Request, oldServiceInstance, newServiceInstance *sc.ServiceInstance) { + + // Clear out the ClusterServicePlanRef so that it is resolved during reconciliation + planUpdated := newServiceInstance.Spec.ClusterServicePlanExternalName != oldServiceInstance.Spec.ClusterServicePlanExternalName || + newServiceInstance.Spec.ClusterServicePlanExternalID != oldServiceInstance.Spec.ClusterServicePlanExternalID || + newServiceInstance.Spec.ClusterServicePlanName != oldServiceInstance.Spec.ClusterServicePlanName + if planUpdated { + newServiceInstance.Spec.ClusterServicePlanRef = nil + } + + // Ignore the UpdateRequests field when it is the default value + if newServiceInstance.Spec.UpdateRequests == 0 { + newServiceInstance.Spec.UpdateRequests = oldServiceInstance.Spec.UpdateRequests + } + + if !apiequality.Semantic.DeepEqual(oldServiceInstance.Spec, newServiceInstance.Spec) { + if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) { + setServiceInstanceUserInfo(req, newServiceInstance) + } + } } // setServiceInstanceUserInfo injects user.Info from the request context diff --git a/pkg/webhook/servicecatalog/serviceinstance/mutation/handler_test.go b/pkg/webhook/servicecatalog/serviceinstance/mutation/handler_test.go index ad02e98bd13..20c53e6b404 100644 --- a/pkg/webhook/servicecatalog/serviceinstance/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/serviceinstance/mutation/handler_test.go @@ -150,6 +150,106 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + const fixUUID = "mocked-uuid-123-abc" + tests := map[string]struct { + givenOldRawObj []byte + givenNewRawObj []byte + expPatches []jsonpatch.Operation + }{ + "Should clear out the ClusterServicePlanRef": { + givenOldRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceInstance", + "metadata": { + "creationTimestamp": null, + "name": "test-instance", + "generation": 1 + }, + "spec": { + "updateRequests": 1, + "clusterServiceClassExternalName": "some-class", + "clusterServicePlanExternalName": "some-plan", + "clusterServicePlanRef": {"name": "plan-01"}, + "externalID": "external-id-001" + } + }`), + givenNewRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServiceInstance", + "metadata": { + "creationTimestamp": null, + "name": "test-instance", + "generation": 1 + }, + "spec": { + "updateRequests": 1, + "clusterServiceClassExternalName": "some-class", + "clusterServicePlanExternalName": "some-plan-changed", + "clusterServicePlanRef": {"name": "plan-01"}, + "externalID": "external-id-001" + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "remove", + Path: "/spec/clusterServicePlanRef", + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + err = utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%v=false", scfeatures.OriginatingIdentity)) + require.NoError(t, err, "cannot disable OriginatingIdentity feature") + // restore default state + defer utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%v=true", scfeatures.OriginatingIdentity)) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-instance", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServiceInstance", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.givenOldRawObj}, + Object: runtime.RawExtension{Raw: tc.givenNewRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{ + UUID: func() types.UID { return fixUUID }, + } + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleSetUserInfoIfOriginatingIdentityIsEnabled(t *testing.T) { // given sc.AddToScheme(scheme.Scheme) diff --git a/pkg/webhook/servicecatalog/serviceinstance/validation/handler.go b/pkg/webhook/servicecatalog/serviceinstance/validation/handler.go index d2872177c89..dbf9cefa036 100644 --- a/pkg/webhook/servicecatalog/serviceinstance/validation/handler.go +++ b/pkg/webhook/servicecatalog/serviceinstance/validation/handler.go @@ -49,7 +49,8 @@ var _ inject.Client = &AdmissionHandler{} // NewAdmissionHandler creates new AdmissionHandler and initializes validators list func NewAdmissionHandler() *AdmissionHandler { return &AdmissionHandler{ - UpdateValidators: []Validator{&DenyPlanChangeIfNotUpdatable{}}, + UpdateValidators: []Validator{&StaticUpdate{}, &DenyPlanChangeIfNotUpdatable{}}, + CreateValidators: []Validator{&StaticCreate{}}, } } diff --git a/pkg/webhook/servicecatalog/serviceinstance/validation/static.go b/pkg/webhook/servicecatalog/serviceinstance/validation/static.go new file mode 100644 index 00000000000..bed999fe31c --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceinstance/validation/static.go @@ -0,0 +1,69 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + "net/http" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" +) + +// StaticCreate runs basic ServiceInstance validation for Create operation. +type StaticCreate struct { +} + +// StaticUpdate runs basic ServiceInstance validation for Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +var _ Validator = &StaticCreate{} +var _ Validator = &StaticUpdate{} +var _ admission.DecoderInjector = &StaticUpdate{} + +// Validate validate ServiceBinding instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, serviceInstance *sc.ServiceInstance, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateServiceInstance(serviceInstance).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ServiceInstance instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, serviceInstance *sc.ServiceInstance, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ServiceInstance{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateServiceInstanceUpdate(serviceInstance, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/servicecatalog/serviceplan/mutation/handler.go b/pkg/webhook/servicecatalog/serviceplan/mutation/handler.go index ab6afdb9ebf..09ccc746b56 100644 --- a/pkg/webhook/servicecatalog/serviceplan/mutation/handler.go +++ b/pkg/webhook/servicecatalog/serviceplan/mutation/handler.go @@ -38,7 +38,7 @@ var _ admission.Handler = &CreateUpdateHandler{} // Handle handles admission requests. func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { traced := webhookutil.NewTracedLogger(req.UID) - traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + traced.Infof("Start handling mutation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) cb := &sc.ServicePlan{} if err := webhookutil.MatchKinds(cb, req.Kind); err != nil { @@ -56,7 +56,12 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) case admissionTypes.Create: h.mutateOnCreate(ctx, mutated) case admissionTypes.Update: - h.mutateOnUpdate(ctx, mutated) + oldObj := &sc.ServicePlan{} + if err := h.decoder.DecodeRaw(req.OldObject, oldObj); err != nil { + traced.Errorf("Could not decode request old object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + h.mutateOnUpdate(ctx, oldObj, mutated) default: traced.Infof("ServicePlan mutation wehbook does not support action %q", req.Operation) return admission.Allowed("action not taken") @@ -84,8 +89,12 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, binding *sc.Se } -func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServicePlan) { - // TODO: implement logic from pkg/registry/servicecatalog/binding/strategy.go +func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldObj, newObj *sc.ServicePlan) { + // This feature was copied from Service Catalog registry: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/registry/servicecatalog/serviceplan/strategy.go + // If you want to track previous changes please check there. + + newObj.Spec.ServiceClassRef = oldObj.Spec.ServiceClassRef + newObj.Spec.ServiceBrokerName = oldObj.Spec.ServiceBrokerName } func (h *CreateUpdateHandler) syncLabels(obj *sc.ServicePlan) { diff --git a/pkg/webhook/servicecatalog/serviceplan/mutation/handler_test.go b/pkg/webhook/servicecatalog/serviceplan/mutation/handler_test.go index a361ad7d90e..63b29994b9f 100644 --- a/pkg/webhook/servicecatalog/serviceplan/mutation/handler_test.go +++ b/pkg/webhook/servicecatalog/serviceplan/mutation/handler_test.go @@ -108,6 +108,119 @@ func TestCreateUpdateHandlerHandleCreateSuccess(t *testing.T) { } } +func TestCreateUpdateHandlerHandleUpdateSuccess(t *testing.T) { + tests := map[string]struct { + oldRawObject []byte + newRawObj []byte + + expPatches []jsonpatch.Operation + }{ + "Should reset broker name and class ref to old one and allow to change other fields": { + oldRawObject: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServicePlan", + "metadata": { + "creationTimestamp": null, + "name": "test-plan", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.serviceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id", + "servicecatalog.k8s.io/spec.serviceClassRef.name":"external-class" + } + }, + "spec": { + "serviceBrokerName": "test-broker", + "serviceClassRef": {"name":"external-class"}, + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": false, + "free": false + } + }`), + newRawObj: []byte(`{ + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "kind": "ServicePlan", + "metadata": { + "creationTimestamp": null, + "name": "test-plan", + "labels": { + "servicecatalog.k8s.io/spec.externalName":"external-name", + "servicecatalog.k8s.io/spec.serviceBrokerName":"test-broker", + "servicecatalog.k8s.io/spec.externalID": "external-id", + "servicecatalog.k8s.io/spec.serviceClassRef.name":"external-class" + } + }, + "spec": { + "serviceBrokerName": "test-broker-changed", + "serviceClassRef": {"name":"external-class-changed"}, + "externalName": "external-name", + "externalID": "external-id", + "description":"a description", + "bindable": true, + "free": true + } + }`), + expPatches: []jsonpatch.Operation{ + { + Operation: "replace", + Path: "/spec/serviceBrokerName", + Value: "test-broker", + }, + { + Operation: "replace", + Path: "/spec/serviceClassRef/name", + Value: "external-class", + }, + }, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sc.AddToScheme(scheme.Scheme) + decoder, err := admission.NewDecoder(scheme.Scheme) + require.NoError(t, err) + + fixReq := admission.Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Update, + Name: "test-plan", + Namespace: "system", + Kind: metav1.GroupVersionKind{ + Kind: "ServicePlan", + Version: "v1beta1", + Group: "servicecatalog.k8s.io", + }, + OldObject: runtime.RawExtension{Raw: tc.oldRawObject}, + Object: runtime.RawExtension{Raw: tc.newRawObj}, + }, + } + + handler := mutation.CreateUpdateHandler{} + handler.InjectDecoder(decoder) + + // when + resp := handler.Handle(context.Background(), fixReq) + + // then + assert.True(t, resp.Allowed) + require.NotNil(t, resp.PatchType) + assert.Equal(t, admissionv1beta1.PatchTypeJSONPatch, *resp.PatchType) + + // filtering out status cause k8s api-server will discard this too + patches := tester.FilterOutStatusPatch(resp.Patches) + + require.Len(t, patches, len(tc.expPatches)) + for _, expPatch := range tc.expPatches { + assert.Contains(t, patches, expPatch) + } + }) + } +} + func TestCreateUpdateHandlerHandleDecoderErrors(t *testing.T) { tester.DiscardLoggedMsg() diff --git a/pkg/webhook/servicecatalog/serviceplan/validation/handler.go b/pkg/webhook/servicecatalog/serviceplan/validation/handler.go new file mode 100644 index 00000000000..cae97894afd --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceplan/validation/handler.go @@ -0,0 +1,137 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + "net/http" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + + admissionTypes "k8s.io/api/admission/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// Validator is used to implement new validation logic +type Validator interface { + Validate(context.Context, admission.Request, *sc.ServicePlan, *webhookutil.TracedLogger) *webhookutil.WebhookError +} + +// AdmissionHandler handles ServiceInstance validation +type AdmissionHandler struct { + decoder *admission.Decoder + client client.Client + + CreateValidators []Validator + UpdateValidators []Validator +} + +var _ admission.Handler = &AdmissionHandler{} +var _ admission.DecoderInjector = &AdmissionHandler{} +var _ inject.Client = &AdmissionHandler{} + +// NewAdmissionHandler creates new AdmissionHandler and initializes validators list +func NewAdmissionHandler() *AdmissionHandler { + return &AdmissionHandler{ + CreateValidators: []Validator{&StaticCreate{}}, + UpdateValidators: []Validator{&StaticUpdate{}}, + } +} + +// Handle handles admission requests. +func (h *AdmissionHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + traced := webhookutil.NewTracedLogger(req.UID) + traced.Infof("Start handling validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + + servicePlan := &sc.ServicePlan{} + if err := webhookutil.MatchKinds(servicePlan, req.Kind); err != nil { + traced.Errorf("Error matching kinds: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + if err := h.decoder.Decode(req, servicePlan); err != nil { + traced.Errorf("Could not decode request object: %v", err) + return admission.Errored(http.StatusBadRequest, err) + } + + traced.Infof("start validation process for %s: %s/%s", servicePlan.Kind, servicePlan.Namespace, servicePlan.Name) + + var err *webhookutil.WebhookError + + switch req.Operation { + case admissionTypes.Create: + for _, v := range h.CreateValidators { + err = v.Validate(ctx, req, servicePlan, traced) + if err != nil { + break + } + } + case admissionTypes.Update: + for _, v := range h.UpdateValidators { + err = v.Validate(ctx, req, servicePlan, traced) + if err != nil { + break + } + } + default: + traced.Infof("ServicePlan validation wehbook does not support action %q", req.Operation) + return admission.Allowed("action not taken") + } + + if err != nil { + switch err.Code() { + case http.StatusForbidden: + return admission.Denied(err.Error()) + default: + return admission.Errored(err.Code(), err) + } + } + + traced.Infof("Completed successfully validation operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name) + return admission.Allowed("ServiceInstance AdmissionHandler successful") +} + +// InjectDecoder injects the decoder into the handlers +func (h *AdmissionHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + + for _, v := range h.CreateValidators { + admission.InjectDecoderInto(d, v) + } + for _, v := range h.UpdateValidators { + admission.InjectDecoderInto(d, v) + } + + return nil +} + +// InjectClient injects the client into the handlers +func (h *AdmissionHandler) InjectClient(c client.Client) error { + h.client = c + + for _, v := range h.CreateValidators { + inject.ClientInto(c, v) + } + for _, v := range h.UpdateValidators { + inject.ClientInto(c, v) + } + + return nil +} diff --git a/pkg/webhook/servicecatalog/serviceplan/validation/handler_test.go b/pkg/webhook/servicecatalog/serviceplan/validation/handler_test.go new file mode 100644 index 00000000000..6a32ca15065 --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceplan/validation/handler_test.go @@ -0,0 +1,36 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation_test + +import ( + "testing" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhook/servicecatalog/serviceplan/validation" + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil/tester" +) + +func TestAdmissionHandlerHandleDecoderErrors(t *testing.T) { + tester.DiscardLoggedMsg() + + for _, fn := range []func(t *testing.T, handler tester.TestDecoderHandler, kind string){ + tester.AssertHandlerReturnErrorIfReqObjIsMalformed, + tester.AssertHandlerReturnErrorIfGVKMismatch, + } { + handler := validation.AdmissionHandler{} + fn(t, &handler, "ServicePlan") + } +} diff --git a/pkg/webhook/servicecatalog/serviceplan/validation/static.go b/pkg/webhook/servicecatalog/serviceplan/validation/static.go new file mode 100644 index 00000000000..6f34a26008e --- /dev/null +++ b/pkg/webhook/servicecatalog/serviceplan/validation/static.go @@ -0,0 +1,69 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "context" + + "github.com/kubernetes-incubator/service-catalog/pkg/webhookutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation" + "net/http" +) + +// StaticCreate runs basic ServicePlan validation for Update operation. +type StaticCreate struct { +} + +// StaticUpdate runs basic ServicePlan validation for Update operation. +type StaticUpdate struct { + decoder *admission.Decoder +} + +var _ Validator = &StaticCreate{} +var _ Validator = &StaticUpdate{} +var _ admission.DecoderInjector = &StaticUpdate{} + +// Validate validate ServiceBinding instance +func (v *StaticCreate) Validate(ctx context.Context, req admission.Request, servicePlan *sc.ServicePlan, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + err := scv.ValidateServicePlan(servicePlan).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// Validate validate ServicePlan instance +func (v *StaticUpdate) Validate(ctx context.Context, req admission.Request, servicePlan *sc.ServicePlan, traced *webhookutil.TracedLogger) *webhookutil.WebhookError { + originalObj := &sc.ServicePlan{} + if err := v.decoder.DecodeRaw(req.OldObject, originalObj); err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusBadRequest) + } + err := scv.ValidateServicePlanUpdate(servicePlan, originalObj).ToAggregate() + if err != nil { + return webhookutil.NewWebhookError(err.Error(), http.StatusForbidden) + } + return nil +} + +// InjectDecoder injects the decoder +func (v *StaticUpdate) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhookutil/traced_logger.go b/pkg/webhookutil/traced_logger.go index 99825eae373..9b3f50c14dd 100644 --- a/pkg/webhookutil/traced_logger.go +++ b/pkg/webhookutil/traced_logger.go @@ -39,22 +39,22 @@ func NewTracedLogger(uid types.UID) *TracedLogger { // Infof logs to the INFO log. func (l *TracedLogger) Infof(format string, args ...interface{}) { - klog.Infof(l.tracedMsgf(format, args...)) + klog.InfoDepth(1, l.tracedMsgf(format, args...)) } // Errorf logs to the ERROR, WARNING, and INFO logs. func (l *TracedLogger) Errorf(format string, args ...interface{}) { - klog.Errorf(l.tracedMsgf(format, args...)) + klog.ErrorDepth(1, l.tracedMsgf(format, args...)) } // Info logs to the INFO log. func (l *TracedLogger) Info(args ...interface{}) { - klog.Info(l.tracedMsg(args...)) + klog.InfoDepth(1, l.tracedMsg(args...)) } // Error logs to the ERROR, WARNING, and INFO logs. func (l *TracedLogger) Error(args ...interface{}) { - klog.Error(l.tracedMsg(args...)) + klog.ErrorDepth(1, l.tracedMsg(args...)) } func (l *TracedLogger) tracedMsgf(format string, args ...interface{}) string {