Bug 1500048 - make plan ids globally unique #480

jmrodri · 2017-10-10T01:18:24Z

Describe what this PR does and why we need it:

With a recent service-catalog change, the serviceplans were moved to top level. We were using the plan.name as the plan.id which was not a problem before because they were namespaced by the serviceclass.

Now that the serviceplans are top level, the names need to be unique. Since we used to rely on the serviceclass name, the new id will be a UUID. We are now storing the id and name in etcd.

/plan_names/UUID: plan.name

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes 1500048

jmrodri · 2017-10-10T01:19:58Z

All of the service plans.

$ oc get serviceplans
NAME                                   KIND
06746960-b828-4cbd-9615-fc5918422acb   ServicePlan.v1alpha1.servicecatalog.k8s.io
089f5e18-4963-4ffb-9df5-3c8cef4fe750   ServicePlan.v1alpha1.servicecatalog.k8s.io
131c8b50-3896-452c-87f1-8f4279413d5c   ServicePlan.v1alpha1.servicecatalog.k8s.io
1a10b15e-17e2-41e6-beca-c4569d67a056   ServicePlan.v1alpha1.servicecatalog.k8s.io
32e465dc-e15b-4882-b00f-b4a818bfeaa7   ServicePlan.v1alpha1.servicecatalog.k8s.io
3994f3f0-e54b-4411-b4e4-1ba25fd43b3c   ServicePlan.v1alpha1.servicecatalog.k8s.io
3b0ed74f-ea9a-454e-b62b-a0d0c6cf7e45   ServicePlan.v1alpha1.servicecatalog.k8s.io
48d6ca7d-e906-41ff-adba-a317759b890f   ServicePlan.v1alpha1.servicecatalog.k8s.io
5f298e84-6b27-4bc2-b307-b4af4be163b3   ServicePlan.v1alpha1.servicecatalog.k8s.io
688cfd4f-bfb4-4b43-b7bc-3784c9dda2e3   ServicePlan.v1alpha1.servicecatalog.k8s.io
73e2ea04-7c94-49bc-9a22-d441dbc78d00   ServicePlan.v1alpha1.servicecatalog.k8s.io
8b78c19d-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8b7c719f-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8b81a9e8-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8b8efc5a-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8ba066b6-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8ba76b34-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8bae8cc2-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8bbb8332-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8bc04c95-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8bc5afc5-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8bc8548b-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
8bcc25e3-aead-11e7-b25b-c85b76145add   ServicePlan.v1alpha1.servicecatalog.k8s.io
90b8fad3-7517-4273-bad3-e6bb2553cd0a   ServicePlan.v1alpha1.servicecatalog.k8s.io
a15dae10-b2af-4457-8e07-989c86691cc4   ServicePlan.v1alpha1.servicecatalog.k8s.io
a3930cae-60c7-4685-854e-5c8b54957e26   ServicePlan.v1alpha1.servicecatalog.k8s.io
a45ecdd4-5de7-4f91-a9df-c537da216477   ServicePlan.v1alpha1.servicecatalog.k8s.io
c0467a67-0ae7-4f28-95c8-2b08ac27807f   ServicePlan.v1alpha1.servicecatalog.k8s.io
daba060c-e74a-4407-9374-25ef66eac8d3   ServicePlan.v1alpha1.servicecatalog.k8s.io
de107e0f-2480-4383-a8d0-90a96f5ef458   ServicePlan.v1alpha1.servicecatalog.k8s.io
e587dd43-fa56-481d-ba2c-4fd933fe5de4   ServicePlan.v1alpha1.servicecatalog.k8s.io
fc4fdcdf-b299-4985-94ac-d13a19116290   ServicePlan.v1alpha1.servicecatalog.k8s.io

jmrodri · 2017-10-10T01:22:42Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1500048

dymurray

Changes make sense and worked for me locally. ACK Pending travis failures.

eriknelson · 2017-10-10T13:11:28Z

Do we need to be concerned about the length of the ids?

cfchase · 2017-10-10T13:25:50Z

pkg/broker/util.go

 	brokerPlans := make([]Plan, len(apbPlans))
 	i := 0
 	for _, plan := range apbPlans {
+		genID := fmt.Sprintf("%v-%v", fqname, plan.Name)


Is there a character limit on these IDs and how does it compare to the char limit on the unique plan IDs making them not unique. e.g.

serviceclass: dh-cfchase-reallyreallylongfqnameblahblah plans: - plan1 - plan2

turning into

dh-cfchase-reallyreallylongfqnameblahblah-plan1 dh-cfchase-reallyreallylongfqnameblahblah-plan2

and then getting cut short if really long to:

dh-cfchase-reallyreallylongfqnameblahblah-pla dh-cfchase-reallyreallylongfqnameblahblah-pla

and then not being unique.

Perhaps we should make sure we middle truncate or add a generated string and then check for collisions as each is generated

how about
serviceclass name = 32 characters of readable + 8 character generated (40 chars max)
plan name = 16 characters of readable + 8 characters generated (24 chars max)
unique plan name = serviceclass name + plan name (total of 64 characters max, pretty darn unique)

@cfchase as far as serviceclass name is concerned I think we probably want to limit the number of places that have this logic of truncating names. Currently, in the broker:

re := regexp.MustCompile(fqNameRegex) spec.FQName = re.ReplaceAllLiteralString( fmt.Sprintf("%v-%v", registryName, spec.FQName), "-") spec.FQName = fmt.Sprintf("%.51v", spec.FQName) if strings.HasSuffix(spec.FQName, "-") { spec.FQName = spec.FQName[:len(spec.FQName)-1] }

registryName is typically something like dh (I've used dz when I've added my org as an additional registry) so we would have something like dh-etherpad-apb. We are truncating this to 51 characters wide but, I believe, this is sufficiently unique. My only concern is, if we want to shorten this, we should do it here in the broker before we create plan names.

Not really related to this PR

Follow up questions regarding serviceclass names, apb names, fqnames, etc:

Are we doing anything to prevent someone from using the same name 2x in the broker config?

How do we handle multiple APB containers whose apb.yml have the same name?

@eriknelson @djzager @dymurray @cfchase so does the ID need to be human readable? can't I just do a UUID? like f4b72d2a-bf85-4847-a19f-edd8beef0ddd

@jmrodri I don't think there is any requirement that they are human readable. I think a UUID would work fine and in fact that is what the template broker is doing.

+1 to @dymurray comment

rthallisey · 2017-10-11T10:07:47Z

pkg/broker/broker.go

@@ -959,3 +965,11 @@ func ocLogin(log *logging.Logger, args ...string) error {
 	}
 	return nil
 }
+
+func extractNameFromPlanID(planID string) (string, error) {
+	splitPlan := strings.Split(planID, "-")


Can you get a planID without a '-' in it?

@rthallisey not the way I generated them. There would always be at least one '-' in it.

rthallisey · 2017-10-11T10:10:17Z

pkg/broker/broker.go

+func extractNameFromPlanID(planID string) (string, error) {
+	splitPlan := strings.Split(planID, "-")
+
+	// TOOD: Error Handling


Can planID be just '-'? If neither of these cases can occur or are error checked elsewhere then you don't need any error checking. If they aren't, then they'll need to be checked because they will cause runtime errors.

jmrodri · 2017-10-11T18:02:11Z

I updated this PR with the new uuid and etcd changes. I also updated the output. The 145add are the template-service-brokers, the rest are ours.

jmrodri · 2017-10-11T18:13:27Z

@eriknelson @shawn-hurley @dymurray @rthallisey Please REREVIEW!

eriknelson

Couple small q's

eriknelson · 2017-10-11T18:20:13Z

pkg/apb/types.go

@@ -50,6 +50,7 @@ type ParameterDescriptor struct {

 // Plan - Plan object describing an APB deployment plan and associated parameters
 type Plan struct {
+	ID             string                 `json:"id" yaml:"-"`


Should these values be the same?

I think that we don't want a value to be set from the Spec yaml for the ID field.

👍 to @shawn-hurley we don't want it to get set form the spec yaml. But I want it output via JSON.

eriknelson · 2017-10-11T18:20:16Z

pkg/broker/broker.go

+		// though the names may be the same we want them to be globally unique.
+		for _, p := range s.Plans {
+			if p.ID == "" {
+				a.log.Errorf("We have a plan that did not get its id generated: %v", p.Name)


Should this get continued?

added a continue earlier

eriknelson · 2017-10-11T18:21:41Z

pkg/broker/broker.go

+	planName, err = a.dao.GetPlanName(req.PlanID)
+	if err != nil {
+		// etcd return not found i.e. code 100
+		if client.IsKeyNotFound(err) {


shawn-hurley · 2017-10-11T18:26:44Z

pkg/broker/broker.go

+
+	// need to use the index into the array to actually update the struct.
+	for i := range plans {
+		plans[i].ID = uuid.New()


What will happen here on restarting of the broker? Won't we blow away the spec and therefore get two different entries for the service plans much like what was happening with service instances names awhile ago and the reason we went to a hash of the FQname?

Yes, that's an issue. Good catch. It seems we need the lookup ID to be a similar hash.

Old review

* Prevents duplicate entries on broker reboots and bootstraps. Guarantees the same FQNames will output the same hash

jmrodri

ack @eriknelson changes

djzager

* Bug 1500048 - make plan ids globally unique * continue if plan id is empty * Use md5 hash of FQPlanName instead of UUID * Prevents duplicate entries on broker reboots and bootstraps. Guarantees the same FQNames will output the same hash

jmrodri requested a review from eriknelson October 10, 2017 01:18

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 10, 2017

jmrodri requested a review from dymurray October 10, 2017 01:18

jmrodri added bug needs-review spec-compliance labels Oct 10, 2017

dymurray previously approved these changes Oct 10, 2017

View reviewed changes

cfchase reviewed Oct 10, 2017

View reviewed changes

jmrodri force-pushed the planid branch from 5b570c1 to ac0c294 Compare October 11, 2017 03:53

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 11, 2017

rthallisey reviewed Oct 11, 2017

View reviewed changes

Bug 1500048 - make plan ids globally unique

22e4e06

jmrodri force-pushed the planid branch from ac0c294 to 22e4e06 Compare October 11, 2017 17:43

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2017

eriknelson suggested changes Oct 11, 2017

View reviewed changes

shawn-hurley requested changes Oct 11, 2017

View reviewed changes

continue if plan id is empty

22d0875

Use md5 hash of FQPlanName instead of UUID

5adfffb

* Prevents duplicate entries on broker reboots and bootstraps. Guarantees the same FQNames will output the same hash

eriknelson approved these changes Oct 11, 2017

View reviewed changes

jmrodri commented Oct 11, 2017

View reviewed changes

dymurray approved these changes Oct 11, 2017

View reviewed changes

shawn-hurley approved these changes Oct 11, 2017

View reviewed changes

djzager approved these changes Oct 11, 2017

View reviewed changes

eriknelson merged commit cc99b8c into master Oct 11, 2017

eriknelson deleted the planid branch October 11, 2017 19:28

jwmatthews mentioned this pull request Oct 11, 2017

Fixed RCM for 3.7 fusor/catasb#160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1500048 - make plan ids globally unique #480

Bug 1500048 - make plan ids globally unique #480

jmrodri commented Oct 10, 2017 •

edited

jmrodri commented Oct 10, 2017 •

edited

jmrodri commented Oct 10, 2017

dymurray left a comment

eriknelson commented Oct 10, 2017

cfchase Oct 10, 2017 •

edited

cfchase Oct 10, 2017 •

edited

djzager Oct 10, 2017

jmrodri Oct 10, 2017

dymurray Oct 10, 2017

djzager Oct 11, 2017

rthallisey Oct 11, 2017

jmrodri Oct 11, 2017

rthallisey Oct 11, 2017

jmrodri commented Oct 11, 2017

jmrodri commented Oct 11, 2017

eriknelson left a comment

eriknelson Oct 11, 2017

shawn-hurley Oct 11, 2017

jmrodri Oct 11, 2017

eriknelson Oct 11, 2017

jmrodri Oct 11, 2017

eriknelson Oct 11, 2017

shawn-hurley Oct 11, 2017

eriknelson Oct 11, 2017

jmrodri Oct 11, 2017

jmrodri left a comment

djzager left a comment

Bug 1500048 - make plan ids globally unique #480

Bug 1500048 - make plan ids globally unique #480

Conversation

jmrodri commented Oct 10, 2017 • edited

jmrodri commented Oct 10, 2017 • edited

jmrodri commented Oct 10, 2017

dymurray left a comment

Choose a reason for hiding this comment

eriknelson commented Oct 10, 2017

cfchase Oct 10, 2017 • edited

Choose a reason for hiding this comment

cfchase Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Not really related to this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri commented Oct 11, 2017

jmrodri commented Oct 11, 2017

eriknelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

djzager left a comment

Choose a reason for hiding this comment

jmrodri commented Oct 10, 2017 •

edited

jmrodri commented Oct 10, 2017 •

edited

cfchase Oct 10, 2017 •

edited

cfchase Oct 10, 2017 •

edited