Add support for specifying plan using K8S names. (#1377)

* Add support for specifying plan using K8S names.

* some wording clarifications and forgot to check in the admission controller file change

* fix e2e tests

Author: Ville Aikas

pkg/apis/servicecatalog/types.go

@@ -431,6 +431,14 @@ type PlanReference struct {
 	// the current name of the ClusterServicePlan, you should follow the
 	// ClusterServicePlanRef below.
 	ExternalClusterServicePlanName string
+
+	// ClusterServiceClassName is the kubernetes name of the
+	// ClusterServiceClass.
+	//
+	// Immutable.
+	ClusterServiceClassName string
+	// ClusterServicePlanName is kubernetes name of the ClusterServicePlan.
+	ClusterServicePlanName string
 }
 
 // ServiceInstanceSpec represents the desired state of an Instance.

pkg/apis/servicecatalog/v1beta1/types.go

@@ -427,6 +427,13 @@ type ServiceInstance struct {
 // ServicePlan and ServiceClass. Because there are multiple ways to
 // specify the desired Class/Plan, this structure specifies the
 // allowed ways to specify the intent.
+//
+// Currently supported ways:
+//  - ExternalClusterServiceClassName and ExternalClusterServicePlanName
+//  - ClusterServiceClassName and ClusterServicePlanName
+//
+// For both of these ways, if a ClusterServiceClass only has one plan
+// then leaving the *ServicePlanName is optional.
 type PlanReference struct {
 	// ExternalClusterServiceClassName is the human-readable name of the
 	// service as reported by the broker. Note that if the broker changes
@@ -442,6 +449,14 @@ type PlanReference struct {
 	// the current name of the ClusterServicePlan, you should follow the
 	// ClusterServicePlanRef below.
 	ExternalClusterServicePlanName string `json:"externalClusterServicePlanName,omitempty"`
+
+	// ClusterServiceClassName is the kubernetes name of the
+	// ClusterServiceClass.
+	//
+	// Immutable.
+	ClusterServiceClassName string `json:"clusterServiceClassName,omitempty"`
+	// ClusterServicePlanName is kubernetes name of the ClusterServicePlan.
+	ClusterServicePlanName string `json:"clusterServicePlanName,omitempty"`
 }
 
 // ServiceInstanceSpec represents the desired state of an Instance.

pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go

@@ -501,6 +501,8 @@ func Convert_servicecatalog_ParametersFromSource_To_v1beta1_ParametersFromSource
 func autoConvert_v1beta1_PlanReference_To_servicecatalog_PlanReference(in *PlanReference, out *servicecatalog.PlanReference, s conversion.Scope) error {
 	out.ExternalClusterServiceClassName = in.ExternalClusterServiceClassName
 	out.ExternalClusterServicePlanName = in.ExternalClusterServicePlanName
+	out.ClusterServiceClassName = in.ClusterServiceClassName
+	out.ClusterServicePlanName = in.ClusterServicePlanName
 	return nil
 }
 
@@ -512,6 +514,8 @@ func Convert_v1beta1_PlanReference_To_servicecatalog_PlanReference(in *PlanRefer
 func autoConvert_servicecatalog_PlanReference_To_v1beta1_PlanReference(in *servicecatalog.PlanReference, out *PlanReference, s conversion.Scope) error {
 	out.ExternalClusterServiceClassName = in.ExternalClusterServiceClassName
 	out.ExternalClusterServicePlanName = in.ExternalClusterServicePlanName
+	out.ClusterServiceClassName = in.ClusterServiceClassName
+	out.ClusterServicePlanName = in.ClusterServicePlanName
 	return nil
 }
 

pkg/apis/servicecatalog/validation/instance.go

@@ -68,19 +68,7 @@ func internalValidateServiceInstance(instance *sc.ServiceInstance, create bool)
 func validateServiceInstanceSpec(spec *sc.ServiceInstanceSpec, fldPath *field.Path, create bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	if "" == spec.ExternalClusterServiceClassName {
-		allErrs = append(allErrs, field.Required(fldPath.Child("externalClusterServiceClassName"), "externalClusterServiceClassName is required"))
-	}
-	for _, msg := range validateServiceClassName(spec.ExternalClusterServiceClassName, false /* prefix */) {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("externalClusterServiceClassName"), spec.ExternalClusterServiceClassName, msg))
-	}
-
-	if "" == spec.ExternalClusterServicePlanName {
-		allErrs = append(allErrs, field.Required(fldPath.Child("externalClusterServicePlanName"), "externalClusterServicePlanName is required"))
-	}
-	for _, msg := range validateServicePlanName(spec.ExternalClusterServicePlanName, false /* prefix */) {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("externalClusterServicePlanName"), spec.ExternalClusterServicePlanName, msg))
-	}
+	allErrs = append(allErrs, validatePlanReference(&spec.PlanReference, fldPath)...)
 
 	if spec.ParametersFrom != nil {
 		for _, paramsFrom := range spec.ParametersFrom {
@@ -252,14 +240,17 @@ func internalValidateServiceInstanceUpdateAllowed(new *sc.ServiceInstance, old *
 func ValidateServiceInstanceUpdate(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList {
 	allErrs := field.ErrorList{}
 
+	specFieldPath := field.NewPath("spec")
+
+	allErrs = append(allErrs, validatePlanReferenceUpdate(&new.Spec.PlanReference, &old.Spec.PlanReference, specFieldPath)...)
 	allErrs = append(allErrs, internalValidateServiceInstanceUpdateAllowed(new, old)...)
 	allErrs = append(allErrs, internalValidateServiceInstance(new, false)...)
 
-	allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalClusterServiceClassName, old.Spec.ExternalClusterServiceClassName, field.NewPath("spec").Child("externalClusterServiceClassName"))...)
-	allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalID, old.Spec.ExternalID, field.NewPath("spec").Child("externalID"))...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalClusterServiceClassName, old.Spec.ExternalClusterServiceClassName, specFieldPath.Child("externalClusterServiceClassName"))...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalID, old.Spec.ExternalID, specFieldPath.Child("externalID"))...)
 
 	if new.Spec.UpdateRequests < old.Spec.UpdateRequests {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("updateRequests"), new.Spec.UpdateRequests, "new updateRequests value must not be less than the old one"))
+		allErrs = append(allErrs, field.Invalid(specFieldPath.Child("updateRequests"), new.Spec.UpdateRequests, "new updateRequests value must not be less than the old one"))
 	}
 
 	return allErrs
@@ -309,3 +300,62 @@ func ValidateServiceInstanceReferencesUpdate(new *sc.ServiceInstance, old *sc.Se
 	allErrs = append(allErrs, internalValidateServiceInstance(new, false)...)
 	return allErrs
 }
+
+func validatePlanReference(p *sc.PlanReference, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	// Just to make reading of the conditionals in the code easier.
+	externalClassSet := p.ExternalClusterServiceClassName != ""
+	externalPlanSet := p.ExternalClusterServicePlanName != ""
+	k8sClassSet := p.ClusterServiceClassName != ""
+	k8sPlanSet := p.ClusterServicePlanName != ""
+
+	// Can't specify both External and k8s name but must specify one.
+	if externalClassSet == k8sClassSet {
+		allErrs = append(allErrs, field.Required(fldPath.Child("externalClusterServiceClassName"), "exactly one of externalClusterServiceClassName or clusterServiceClassName required"))
+		allErrs = append(allErrs, field.Required(fldPath.Child("clusterServiceClassName"), "exactly one of externalClusterServiceClassName or clusterServiceClassName required"))
+	}
+	// Can't specify both External and k8s name but must specify one.
+	if externalPlanSet == k8sPlanSet {
+		allErrs = append(allErrs, field.Required(fldPath.Child("externalClusterServicePlanName"), "either externalClusterServicePlanName or clusterServicePlanName required"))
+		allErrs = append(allErrs, field.Required(fldPath.Child("clusterServicePlanName"), "exactly one of externalClusterServicePlanName or clusterServicePlanName required"))
+	}
+
+	if externalClassSet {
+		for _, msg := range validateServiceClassName(p.ExternalClusterServiceClassName, false /* prefix */) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("externalClusterServiceClassName"), p.ExternalClusterServiceClassName, msg))
+		}
+
+		// If ExternalClusterServiceClassName given, must use ExternalClusterServicePlanName
+		if !externalPlanSet {
+			allErrs = append(allErrs, field.Required(fldPath.Child("externalClusterServicePlanName"), "must specify externalClusterServicePlanName with externalClusterServiceClassName"))
+		}
+
+		for _, msg := range validateServicePlanName(p.ExternalClusterServicePlanName, false /* prefix */) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("externalClusterServicePlanName"), p.ClusterServicePlanName, msg))
+		}
+	}
+	if k8sClassSet {
+		for _, msg := range validateServiceClassName(p.ClusterServiceClassName, false /* prefix */) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterServiceClassName"), p.ClusterServiceClassName, msg))
+		}
+
+		// If ClusterServiceClassName given, must use ClusterServicePlanName
+		if !k8sPlanSet {
+			allErrs = append(allErrs, field.Required(fldPath.Child("clusterServicePlanName"), "must specify clusterServicePlanName with clusterServiceClassName"))
+		}
+		for _, msg := range validateServicePlanName(p.ClusterServicePlanName, false /* prefix */) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterServicePlanName"), p.ClusterServicePlanName, msg))
+		}
+	}
+	return allErrs
+}
+
+func validatePlanReferenceUpdate(pOld *sc.PlanReference, pNew *sc.PlanReference, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, validatePlanReference(pOld, fldPath)...)
+	allErrs = append(allErrs, validatePlanReference(pNew, fldPath)...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(pNew.ExternalClusterServiceClassName, pOld.ExternalClusterServiceClassName, field.NewPath("spec").Child("externalClusterServiceClassName"))...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(pNew.ClusterServiceClassName, pOld.ClusterServiceClassName, field.NewPath("spec").Child("clusterServiceClassName"))...)
+	return allErrs
+}