Added versioning check to Broker on bootstrap #457

dymurray · 2017-09-27T17:38:27Z

This PR adds version checks to the broker when bootstrapping an APB

Changes proposed in this pull request

Move version.go to its own version pkg
Add version check on imageToSpec conversion
Add unit tests for compatible version checks

Note:
I plan to open an issue to move the version to the spec itself so that this check moves out of the adapter pkg.

dymurray · 2017-09-27T17:42:22Z

pkg/registries/adapters/adapter.go

+	maxMajorVersion, err := strconv.Atoi(strings.Split(maxVersion, ".")[0])
+	maxMinorVersion, err := strconv.Atoi(strings.Split(maxVersion, ".")[1])
+	if err != nil {
+		return false


Oh crap, this is a typo. Let me fix this.

shawn-hurley · 2017-09-27T18:04:47Z

pkg/registries/adapters/adapter.go

+	if err != nil {
+		return false
+	}
+	specMinorVersion, err := strconv.Atoi(strings.Split(specVersion, ".")[1])


This could panic when I have set the version to 1 only.

+1 another good catch

shawn-hurley · 2017-09-27T18:10:40Z

pkg/registries/adapters/adapter.go

+				return true
+			}
+		}
+	} else if minMajorVersion < maxMajorVersion {


I think this is going to return false for isCompatibleVersion("2.5", "1.0", "3.6")
I don't know if this use case make sense or not.

+1 good catch.

shawn-hurley · 2017-09-27T18:16:45Z

pkg/version/apbversion.go

@@ -0,0 +1,4 @@
+package version
+
+var MinAPBVersion = "1.0"


I think that we may want these to be const rather than var.
Also, anything that is exported from the package needs a comment.

dymurray · 2017-09-27T18:55:22Z

@shawn-hurley Thanks for the feedback. Just making it known that this will always fail CI until the APBs are updated with the proper version which I am working on doing today as well.

dymurray · 2017-09-27T19:39:24Z

Confirmed when running catasb with the canary tag for images that are being built off recent commits that they are passing version validation.

rthallisey · 2017-09-27T19:48:47Z

pkg/registries/adapters/adapter.go

@@ -111,6 +114,15 @@ func imageToSpec(log *logging.Logger, req *http.Request, apbtag string) (*apb.Sp
 		log.Infof("Didn't find encoded Spec label. Assuming image is not APB and skiping")
 		return nil, nil
 	}
+	if conf.Config.Label.Version == "" {


You can combine this with if conf.Config.Label.Spec == "" since we now require both be set.

jmrodri · 2017-09-28T03:17:48Z

Linter errors:

pkg/version/apbversion.go:3:1: comment on exported const MinAPBVersion should be of the form "MinAPBVersion ..."

pkg/version/apbversion.go:7:7: exported const MaxAPBVersion should have comment or be unexported

Found 2 lint suggestions; failing.

make: *** [lint] Error 1

jmrodri · 2017-09-28T03:19:22Z

You need to update the version.go.tmpl package as well.

jmrodri

linter failed (see comment above for errors). Run make lint to fix.

The CI failed as well this was the end of the log:

##### CLUSTER SETUP VARIABLE LIST #####
BUILD_ERROR: false
CLUSTER_SETUP_ERROR: false
MAKE_DEPLOY_ERROR: false
##### GATE VARIABLE LIST #####
RESOURCE_ERROR: false
BIND_ERROR: true
PROVISION_ERROR: false
POD_PRESET_ERROR: false
VERIFY_CI_ERROR: false
UNBIND_ERROR: false
DEPROVISION_ERROR: false
DEVAPI_ERROR: false
make: *** [ci] Error 1

jmrodri · 2017-09-28T12:25:01Z

@dymurray forget my CI comment then, I see your comment about being dependent on apbs. But the linter is still broken so that comment still stands.

dymurray · 2017-09-28T14:03:02Z

CI is failing on binding... unsure how my PR affects binding of mediawiki to postgres. I have confirmed the latest images on dockerhub have the proper version and the broker is bootstrapping them.

rthallisey · 2017-09-28T13:25:30Z

pkg/registries/adapters/adapter.go

-	if conf.Config.Label.Spec == "" {
-		log.Infof("Didn't find encoded Spec label. Assuming image is not APB and skiping")
+	if conf.Config.Label.Spec == "" || conf.Config.Label.Version == "" {
+		log.Infof("Didn't find encoded Spec label and version. Assuming image is not APB and skiping")


It should be: Didn't find encoded Spec label or version.

I meant that as in "Unable to find both version and spec label" but I will change it because I can see why thats confusing.

rthallisey · 2017-09-28T13:41:41Z

pkg/version/apbversion.go

+// acceptible APBs.
+
+// MinAPBVersion constant to describe minimum supported spec version
+const MinAPBVersion = "1.0"


From the spec: APB with version 1.y works with broker 1.y.z

I don't think you need two constants to track a valid APB version. I think you really only need one: BrokerVersion=1.0. Using the broker version, you can check if an APB is valid since anything >=1.x and < 2.0 is valid.

One argument I can see for having a MinAPBVersion is backwards compatibility, but we don't have a requirement for that yet.

We do have a requirement for backwards compatibility we just have not gotten to a point where it is a problem yet. Also, I have updated the proposal with the notes we took on the 9/26 meeting. The broker version is not tied directly to the APB spec version. We want brokers to have a range of APB versions they support and not have to bump a release every time something changes in the broker when the APB spec itself has not changed.

rthallisey · 2017-09-28T15:12:57Z

pkg/registries/adapters/adapter.go

+	if err != nil {
+		return false
+	}
+	minMinorVersion, err := strconv.Atoi(strings.Split(minVersion, ".")[1])


How come we have to check minor versions? I thought we accept APBVersions >= 1.0 and < 2.0.

We need to check minor versions because of the scenario where we support beyond 2.0. We need a way to check that the version fits outside of one major version bump.

So you're saying we'd support >= 1.0 and < 2.x? If we do that then we can't really say that only a major version bump will break the API. IMO, that goes against the reason to have major vs minor versioning.

I'm not sure how we can't say that? A broker that only knows how to deploy 1.X APBs will never be able to support 2.X APBs. However, I would expect a broker that knows how to deploy 2.X APBs to know how to deploy 1.X APBs as that would be supporting backwards compatibility. In this case the major version bump does break the API, it's just that we expect the broker to support 1.X APBs even as we move to a 2.X spec.

In this case the major version bump does break the API, it's just that we expect the
broker to support 1.X APBs even as we move to a 2.X spec.

👍

Here's an example of what I'm thinking:

We move to APBVersion 2.3.

APBVersion >= 1.0 and APBVersion < 3.0 are supported

Why would we need to check the minor version when we know that anything greater than or equal to the minimal major version and less than the maximum major version is valid? In other words, all breaking changes are per major version. If we support 1.0 then we also support 1.7 and 1.x so there's no reason to check beyond the major version.

Unless you feel that my example would imply a major version bump... then I could see why it would be trivial to check the minor version. I just worry about not checking the minor version at all and someone putting 1.Z which doesn't exist and has no associated schema.

I just worry about not checking the minor version at all and someone putting 1.Z which doesn't
exist and has no associated schema.

I understand your concern, but if we follow the release plan correctly then we won't have this issue. And if we make a mistake then we can always change the version.

I wouldn't mind having Ryan's logic be the default, but if a user has defined some config then we override? This might just be a feature we add later and for now, just follow Ryan's logic for now.

+1 sounds good to me

+1 @rthallisey example is how I mentally pictured how this would work. The scenario of 2.4 causing a problem means IMO we failed with the change APB spec change.

rthallisey · 2017-09-28T15:57:56Z

pkg/registries/adapters/adapter.go

+	if err != nil {
+		return false
+	}
+	minMinorVersion, err := strconv.Atoi(strings.Split(minVersion, ".")[1])


So you're saying we'd support >= 1.0 and < 2.x? If we do that then we can't really say that only a major version bump will break the API. IMO, that goes against the reason to have major vs minor versioning.

* Added versioning check to Broker on bootstrap * Added error check * Updates based on feedback, more error handling * Fix linting problems * Logging info update * Reverted to just using major version

Added versioning check to Broker on bootstrap

b21850b

dymurray commented Sep 27, 2017

View reviewed changes

dymurray changed the title ~~Added versioning check to Broker on bootstrap~~ [Do Not Merge] Added versioning check to Broker on bootstrap Sep 27, 2017

Added error check

5fafa84

shawn-hurley requested changes Sep 27, 2017

View reviewed changes

Updates based on feedback, more error handling

9cec94e

shawn-hurley approved these changes Sep 27, 2017

View reviewed changes

dymurray changed the title ~~[Do Not Merge] Added versioning check to Broker on bootstrap~~ Added versioning check to Broker on bootstrap Sep 27, 2017

dymurray requested a review from jmrodri September 27, 2017 19:50

rthallisey suggested changes Sep 27, 2017

View reviewed changes

jmrodri suggested changes Sep 28, 2017

View reviewed changes

Fix linting problems

6f66288

rthallisey suggested changes Sep 28, 2017

View reviewed changes

Logging info update

7d694bf

rthallisey suggested changes Sep 28, 2017

View reviewed changes

jmrodri approved these changes Sep 28, 2017

View reviewed changes

Reverted to just using major version

053cd36

rthallisey approved these changes Sep 28, 2017

View reviewed changes

shawn-hurley merged commit 852dde4 into openshift:master Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added versioning check to Broker on bootstrap #457

Added versioning check to Broker on bootstrap #457

dymurray commented Sep 27, 2017

dymurray Sep 27, 2017

shawn-hurley Sep 27, 2017

dymurray Sep 27, 2017

shawn-hurley Sep 27, 2017

dymurray Sep 27, 2017

shawn-hurley Sep 27, 2017

dymurray commented Sep 27, 2017

dymurray commented Sep 27, 2017

rthallisey Sep 27, 2017

jmrodri commented Sep 28, 2017

jmrodri commented Sep 28, 2017

jmrodri left a comment

jmrodri commented Sep 28, 2017

dymurray commented Sep 28, 2017

rthallisey Sep 28, 2017

dymurray Sep 28, 2017

rthallisey Sep 28, 2017

dymurray Sep 28, 2017

rthallisey Sep 28, 2017

dymurray Sep 28, 2017

rthallisey Sep 28, 2017

dymurray Sep 28, 2017

rthallisey Sep 28, 2017

dymurray Sep 28, 2017

rthallisey Sep 28, 2017

shawn-hurley Sep 28, 2017

dymurray Sep 28, 2017

jmrodri Sep 28, 2017

rthallisey Sep 28, 2017

Added versioning check to Broker on bootstrap #457

Added versioning check to Broker on bootstrap #457

Conversation

dymurray commented Sep 27, 2017

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

dymurray commented Sep 27, 2017

dymurray commented Sep 27, 2017

Choose a reason for hiding this comment

jmrodri commented Sep 28, 2017

jmrodri commented Sep 28, 2017

jmrodri left a comment

Choose a reason for hiding this comment

jmrodri commented Sep 28, 2017

dymurray commented Sep 28, 2017

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

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