apis: move 'Release.Meta.Environment' to 'Release.Spec.Environment'

Kubernetes 1.11 introduces a large set of improvements for CRDs.
However, one of them is stricter validation of the 'metadata' field of
CRD instances: it will drop any unknown keys from the 'metadata' object
before storage.

As a result, Release objects land in the cluster without any
Environment. This Release is useless for the Schedule and Strategy
controllers, and because it does not match the Application template,
Shipper creates a new Release; ultimately this turns into a tight loop
of creating bogus Releases. Suboptimal.

This patch moves the 'environment' portion of Release objects to the
'spec'. We originally did not put 'Environment' into the spec because it
was meant to be immutable, and not part of the user-facing command
language -- we appreciated a 'spec' section that consisted of nothing but
the user control 'targetStep'.

While well-intentioned, this was likely a bad call, since it makes it
awkward to consider Releases generated by hand, without the control of
an Application object. We can solve the immutability by introducing an
admission controller, and trust our users to be smart about which fields
are run-time-editable.

Note: this requires a migration script to run against the management
clusters between shutting down the previous version of Shipper and
starting this one: the script should copy '.metadata.environment' to
'.spec.environment' for all existing Releases.

I decided to do this as a migration script rather than building
a conversion into Shipper itself because there's currently no location
in Shipper which regularly processes and updates all existing Releases.
I expect this to change as we look towards merging the Schedule and
Strategy controllers into the Release controller.

pkg/apis/shipper/v1/types.go

@@ -165,18 +165,13 @@ type ClusterStatus struct {
 // contender versions. This is used by the StrategyController to change the
 // state of the cluster to satisfy a single step of a Strategy.
 type Release struct {
-	metav1.TypeMeta `json:",inline"`
-	ReleaseMeta     `json:"metadata,omitempty"`
+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
 
 	Spec   ReleaseSpec   `json:"spec"`
 	Status ReleaseStatus `json:"status"`
 }
 
-type ReleaseMeta struct {
-	metav1.ObjectMeta `json:",inline"`
-	Environment       ReleaseEnvironment `json:"environment"`
-}
-
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
 type ReleaseList struct {
@@ -187,8 +182,8 @@ type ReleaseList struct {
 }
 
 type ReleaseSpec struct {
-	// better indicated with labels?
-	TargetStep int32 `json:"targetStep"`
+	TargetStep  int32              `json:"targetStep"`
+	Environment ReleaseEnvironment `json:"environment"`
 }
 
 // this will likely grow into a struct with interesting fields

pkg/apis/shipper/v1/zz_generated.deepcopy.go

@@ -792,8 +792,8 @@ func (in *RegionRequirement) DeepCopy() *RegionRequirement {
 func (in *Release) DeepCopyInto(out *Release) {
 	*out = *in
 	out.TypeMeta = in.TypeMeta
-	in.ReleaseMeta.DeepCopyInto(&out.ReleaseMeta)
-	out.Spec = in.Spec
+	in.ObjectMeta.DeepCopyInto(&out.ObjectMeta)
+	in.Spec.DeepCopyInto(&out.Spec)
 	in.Status.DeepCopyInto(&out.Status)
 	return
 }
@@ -909,27 +909,10 @@ func (in *ReleaseList) DeepCopyObject() runtime.Object {
 	}
 }
 
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *ReleaseMeta) DeepCopyInto(out *ReleaseMeta) {
-	*out = *in
-	in.ObjectMeta.DeepCopyInto(&out.ObjectMeta)
-	in.Environment.DeepCopyInto(&out.Environment)
-	return
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ReleaseMeta.
-func (in *ReleaseMeta) DeepCopy() *ReleaseMeta {
-	if in == nil {
-		return nil
-	}
-	out := new(ReleaseMeta)
-	in.DeepCopyInto(out)
-	return out
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *ReleaseSpec) DeepCopyInto(out *ReleaseSpec) {
 	*out = *in
+	in.Environment.DeepCopyInto(&out.Environment)
 	return
 }
 

pkg/client/clientset/versioned/typed/shipper/v1/fake/fake_release.go

@@ -19,6 +19,7 @@ package fake
 import (
 	shipper_v1 "github.com/bookingcom/shipper/pkg/apis/shipper/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	labels "k8s.io/apimachinery/pkg/labels"
 	schema "k8s.io/apimachinery/pkg/runtime/schema"
 	types "k8s.io/apimachinery/pkg/types"
 	watch "k8s.io/apimachinery/pkg/watch"
@@ -54,7 +55,18 @@ func (c *FakeReleases) List(opts v1.ListOptions) (result *shipper_v1.ReleaseList
 	if obj == nil {
 		return nil, err
 	}
-	return obj.(*shipper_v1.ReleaseList), err
+
+	label, _, _ := testing.ExtractFromListOptions(opts)
+	if label == nil {
+		label = labels.Everything()
+	}
+	list := &shipper_v1.ReleaseList{}
+	for _, item := range obj.(*shipper_v1.ReleaseList).Items {
+		if label.Matches(labels.Set(item.Labels)) {
+			list.Items = append(list.Items, item)
+		}
+	}
+	return list, err
 }
 
 // Watch returns a watch.Interface that watches the requested releases.

pkg/controller/application/application_controller.go

@@ -409,7 +409,7 @@ func (c *Controller) processApplication(app *shipperv1.Application) error {
 		highestObserved = generation
 	}
 
-	if !identicalEnvironments(app.Spec.Template, contender.Environment) {
+	if !identicalEnvironments(app.Spec.Template, contender.Spec.Environment) {
 		// The application's template has been modified and is different than
 		// the contender's environment. This means that a new release should
 		// be created with the new template.

pkg/controller/application/application_controller_test.go

@@ -38,7 +38,7 @@ func TestHashReleaseEnv(t *testing.T) {
 	rel := newRelease("test-release", app)
 
 	appHash := hashReleaseEnvironment(app.Spec.Template)
-	relHash := hashReleaseEnvironment(rel.Environment)
+	relHash := hashReleaseEnvironment(rel.Spec.Environment)
 	if appHash != relHash {
 		t.Errorf("two identical environments should have hashed to the same value, but they did not: app %q and rel %q", appHash, relHash)
 	}
@@ -89,7 +89,7 @@ func TestCreateFirstRelease(t *testing.T) {
 	// because the testing client does not update listers after Create actions.
 
 	expectedRelease := newRelease(expectedRelName, app)
-	expectedRelease.Environment.Chart.RepoURL = "127.0.0.1"
+	expectedRelease.Spec.Environment.Chart.RepoURL = "127.0.0.1"
 	expectedRelease.Labels[shipperv1.ReleaseEnvironmentHashLabel] = envHash
 	expectedRelease.Annotations[shipperv1.ReleaseTemplateIterationAnnotation] = "0"
 	expectedRelease.Annotations[shipperv1.ReleaseGenerationAnnotation] = "0"
@@ -110,7 +110,7 @@ func TestStatusStableState(t *testing.T) {
 	releaseA.Spec.TargetStep = 2
 	releaseA.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: releaseA.Environment.Strategy.Steps[2].Name,
+		Name: releaseA.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
 	releaseA.Status.Conditions = []shipperv1.ReleaseCondition{
@@ -127,7 +127,7 @@ func TestStatusStableState(t *testing.T) {
 	releaseB.Spec.TargetStep = 2
 	releaseB.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: releaseB.Environment.Strategy.Steps[2].Name,
+		Name: releaseB.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
 	releaseB.Status.Conditions = []shipperv1.ReleaseCondition{
@@ -176,7 +176,7 @@ func TestRevisionHistoryLimit(t *testing.T) {
 	releaseFoo.Spec.TargetStep = 2
 	releaseFoo.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: releaseFoo.Environment.Strategy.Steps[2].Name,
+		Name: releaseFoo.Spec.Environment.Strategy.Steps[2].Name,
 	}
 	releaseutil.SetReleaseCondition(&releaseFoo.Status, *releaseutil.NewReleaseCondition(shipperv1.ReleaseConditionTypeComplete, corev1.ConditionTrue, "", ""))
 	releaseutil.SetGeneration(releaseFoo, 0)
@@ -185,7 +185,7 @@ func TestRevisionHistoryLimit(t *testing.T) {
 	releaseBar.Spec.TargetStep = 2
 	releaseBar.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: releaseBar.Environment.Strategy.Steps[2].Name,
+		Name: releaseBar.Spec.Environment.Strategy.Steps[2].Name,
 	}
 	releaseutil.SetReleaseCondition(&releaseBar.Status, *releaseutil.NewReleaseCondition(shipperv1.ReleaseConditionTypeComplete, corev1.ConditionTrue, "", ""))
 	releaseutil.SetGeneration(releaseBar, 1)
@@ -194,7 +194,7 @@ func TestRevisionHistoryLimit(t *testing.T) {
 	releaseBaz.Spec.TargetStep = 2
 	releaseBaz.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: releaseBaz.Environment.Strategy.Steps[2].Name,
+		Name: releaseBaz.Spec.Environment.Strategy.Steps[2].Name,
 	}
 	releaseutil.SetReleaseCondition(&releaseBaz.Status, *releaseutil.NewReleaseCondition(shipperv1.ReleaseConditionTypeComplete, corev1.ConditionTrue, "", ""))
 	releaseutil.SetGeneration(releaseBaz, 2)
@@ -257,23 +257,23 @@ func TestCreateThirdRelease(t *testing.T) {
 	releaseutil.SetIteration(firstRel, 0)
 	releaseutil.SetGeneration(firstRel, 0)
 	releaseutil.SetReleaseCondition(&firstRel.Status, *releaseutil.NewReleaseCondition(shipperv1.ReleaseConditionTypeComplete, corev1.ConditionTrue, "", ""))
-	firstRel.Environment.Chart.RepoURL = srv.URL()
+	firstRel.Spec.Environment.Chart.RepoURL = srv.URL()
 	firstRel.Spec.TargetStep = 2
 	firstRel.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: firstRel.Environment.Strategy.Steps[2].Name,
+		Name: firstRel.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
 	incumbentRelName := fmt.Sprintf("%s-%s-1", testAppName, incumbentEnvHash)
 	incumbentRel := newRelease(incumbentRelName, app)
 	releaseutil.SetIteration(incumbentRel, 1)
 	releaseutil.SetGeneration(incumbentRel, 1)
 	releaseutil.SetReleaseCondition(&incumbentRel.Status, *releaseutil.NewReleaseCondition(shipperv1.ReleaseConditionTypeComplete, corev1.ConditionTrue, "", ""))
-	incumbentRel.Environment.Chart.RepoURL = srv.URL()
+	incumbentRel.Spec.Environment.Chart.RepoURL = srv.URL()
 	incumbentRel.Spec.TargetStep = 2
 	incumbentRel.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: incumbentRel.Environment.Strategy.Steps[2].Name,
+		Name: incumbentRel.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
 	app.Status.History = []string{firstRelName, incumbentRelName}
@@ -349,11 +349,11 @@ func TestCreateSecondRelease(t *testing.T) {
 	releaseutil.SetGeneration(incumbentRel, 0)
 	releaseutil.SetIteration(incumbentRel, 0)
 	releaseutil.SetReleaseCondition(&incumbentRel.Status, *releaseutil.NewReleaseCondition(shipperv1.ReleaseConditionTypeComplete, corev1.ConditionTrue, "", ""))
-	incumbentRel.Environment.Chart.RepoURL = srv.URL()
+	incumbentRel.Spec.Environment.Chart.RepoURL = srv.URL()
 	incumbentRel.Spec.TargetStep = 2
 	incumbentRel.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: incumbentRel.Environment.Strategy.Steps[2].Name,
+		Name: incumbentRel.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
 	f.objects = append(f.objects, incumbentRel)
@@ -432,15 +432,15 @@ func TestAbort(t *testing.T) {
 	relName := fmt.Sprintf("%s-%s-0", testAppName, envHash)
 
 	release := newRelease(relName, app)
-	release.Environment.Chart.RepoURL = srv.URL()
+	release.Spec.Environment.Chart.RepoURL = srv.URL()
 	release.Annotations[shipperv1.ReleaseGenerationAnnotation] = "0"
 	release.Spec.TargetStep = 2
 	release.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: release.Environment.Strategy.Steps[2].Name,
+		Name: release.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
-	release.Environment.ClusterRequirements = shipperv1.ClusterRequirements{
+	release.Spec.Environment.ClusterRequirements = shipperv1.ClusterRequirements{
 		Regions: []shipperv1.RegionRequirement{{Name: "bar"}},
 	}
 
@@ -451,7 +451,7 @@ func TestAbort(t *testing.T) {
 	expectedApp := app.DeepCopy()
 	expectedApp.Annotations[shipperv1.AppHighestObservedGenerationAnnotation] = "0"
 	// Should have overwritten the old template with the generation 0 one.
-	expectedApp.Spec.Template = release.Environment
+	expectedApp.Spec.Template = release.Spec.Environment
 
 	expectedApp.Status.History = []string{relName}
 
@@ -510,7 +510,7 @@ func TestStateRollingOut(t *testing.T) {
 	contender.Spec.TargetStep = 1
 	contender.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 0,
-		Name: contender.Environment.Strategy.Steps[0].Name,
+		Name: contender.Spec.Environment.Strategy.Steps[0].Name,
 	}
 
 	f.objects = append(f.objects, contender)
@@ -557,7 +557,7 @@ func TestDeletingAbortedReleases(t *testing.T) {
 	releaseBar.Spec.TargetStep = 2
 	releaseBar.Status.AchievedStep = &shipperv1.AchievedStep{
 		Step: 2,
-		Name: releaseBar.Environment.Strategy.Steps[2].Name,
+		Name: releaseBar.Spec.Environment.Strategy.Steps[2].Name,
 	}
 
 	f.objects = append(f.objects, releaseFoo, releaseBar)
@@ -594,23 +594,23 @@ func TestDeletingAbortedReleases(t *testing.T) {
 
 func newRelease(releaseName string, app *shipperv1.Application) *shipperv1.Release {
 	return &shipperv1.Release{
-		ReleaseMeta: shipperv1.ReleaseMeta{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:        releaseName,
-				Namespace:   app.GetNamespace(),
-				Annotations: map[string]string{},
-				Labels: map[string]string{
-					shipperv1.ReleaseLabel: releaseName,
-					shipperv1.AppLabel:     app.GetName(),
-				},
-				OwnerReferences: []metav1.OwnerReference{
-					{
-						APIVersion: "shipper.booking.com/v1",
-						Kind:       "Application",
-						Name:       app.GetName(),
-					},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        releaseName,
+			Namespace:   app.GetNamespace(),
+			Annotations: map[string]string{},
+			Labels: map[string]string{
+				shipperv1.ReleaseLabel: releaseName,
+				shipperv1.AppLabel:     app.GetName(),
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: "shipper.booking.com/v1",
+					Kind:       "Application",
+					Name:       app.GetName(),
 				},
 			},
+		},
+		Spec: shipperv1.ReleaseSpec{
 			Environment: *(app.Spec.Template.DeepCopy()),
 		},
 	}

pkg/controller/application/application_controller_utils.go

@@ -23,26 +23,25 @@ func (c *Controller) createReleaseForApplication(app *shipperv1.Application, rel
 	glog.V(4).Infof("Generated Release name for Application %q: %q", controller.MetaKey(app), releaseName)
 
 	newRelease := &shipperv1.Release{
-		ReleaseMeta: shipperv1.ReleaseMeta{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      releaseName,
-				Namespace: app.Namespace,
-				Labels: map[string]string{
-					shipperv1.ReleaseLabel:                releaseName,
-					shipperv1.AppLabel:                    app.Name,
-					shipperv1.ReleaseEnvironmentHashLabel: hashReleaseEnvironment(app.Spec.Template),
-				},
-				Annotations: map[string]string{
-					shipperv1.ReleaseTemplateIterationAnnotation: strconv.Itoa(iteration),
-					shipperv1.ReleaseGenerationAnnotation:        strconv.Itoa(generation),
-				},
-				OwnerReferences: []metav1.OwnerReference{
-					createOwnerRefFromApplication(app),
-				},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      releaseName,
+			Namespace: app.Namespace,
+			Labels: map[string]string{
+				shipperv1.ReleaseLabel:                releaseName,
+				shipperv1.AppLabel:                    app.Name,
+				shipperv1.ReleaseEnvironmentHashLabel: hashReleaseEnvironment(app.Spec.Template),
 			},
+			Annotations: map[string]string{
+				shipperv1.ReleaseTemplateIterationAnnotation: strconv.Itoa(iteration),
+				shipperv1.ReleaseGenerationAnnotation:        strconv.Itoa(generation),
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				createOwnerRefFromApplication(app),
+			},
+		},
+		Spec: shipperv1.ReleaseSpec{
 			Environment: *(app.Spec.Template.DeepCopy()),
 		},
-		Spec:   shipperv1.ReleaseSpec{},
 		Status: shipperv1.ReleaseStatus{},
 	}
 

pkg/controller/installation/installer.go

@@ -53,7 +53,7 @@ func NewInstaller(chartFetchFunc shipperchart.FetchFunc,
 // cluster, or an error.
 func (i *Installer) renderManifests(_ *shipperv1.Cluster) ([]string, error) {
 	rel := i.Release
-	chart, err := i.fetchChart(rel.Environment.Chart)
+	chart, err := i.fetchChart(rel.Spec.Environment.Chart)
 	if err != nil {
 		return nil, RenderManifestError(err)
 	}
@@ -62,7 +62,7 @@ func (i *Installer) renderManifests(_ *shipperv1.Cluster) ([]string, error) {
 		chart,
 		rel.GetName(),
 		rel.GetNamespace(),
-		rel.Environment.Values,
+		rel.Spec.Environment.Values,
 	)
 
 	if err != nil {

pkg/controller/installation/installer_test.go

@@ -244,7 +244,7 @@ func TestInstallerBrokenChartTarball(t *testing.T) {
 
 	// there is a reviews-api-invalid-tarball.tgz in testdata which contains invalid deployment and service templates
 	release := buildRelease("0.0.1", "reviews-api", "0", "deadbeef", "reviews-api")
-	release.Environment.Chart.Version = "invalid-tarball"
+	release.Spec.Environment.Chart.Version = "invalid-tarball"
 
 	it := buildInstallationTarget(release, "reviews-api", "reviews-api", []string{cluster.Name})
 	installer := newInstaller(release, it)
@@ -266,7 +266,7 @@ func TestInstallerChartTarballBrokenService(t *testing.T) {
 
 	// there is a reviews-api-invalid-tarball.tgz in testdata which contains invalid deployment and service templates
 	release := buildRelease("0.0.1", "reviews-api", "0", "deadbeef", "reviews-api")
-	release.Environment.Chart.Version = "0.0.1-broken-service"
+	release.Spec.Environment.Chart.Version = "0.0.1-broken-service"
 
 	it := buildInstallationTarget(release, "reviews-api", "reviews-api", []string{cluster.Name})
 	installer := newInstaller(release, it)
@@ -289,7 +289,7 @@ func TestInstallerBrokenChartContents(t *testing.T) {
 	// There is a reviews-api-invalid-k8s-objects.tgz in testdata which contains
 	// invalid deployment and service templates.
 	release := buildRelease("0.0.1", "reviews-api", "0", "deadbeef", "reviews-api")
-	release.Environment.Chart.Version = "invalid-k8s-objects"
+	release.Spec.Environment.Chart.Version = "invalid-k8s-objects"
 
 	it := buildInstallationTarget(release, "reviews-api", "reviews-api", []string{cluster.Name})
 	installer := newInstaller(release, it)