Fix image change trigger imageRepo matching

pkg/deploy/controller/image_change_controller.go

@@ -1,7 +1,7 @@
 package controller
 
 import (
-	"strings"
+	"fmt"
 
 	"github.com/golang/glog"
 
@@ -36,7 +36,7 @@ func (c *ImageChangeController) Run() {
 // HandleImageRepo processes the next ImageRepository event.
 func (c *ImageChangeController) HandleImageRepo() {
 	imageRepo := c.NextImageRepository()
-	configNames := []string{}
+	configsToGenerate := []*deployapi.DeploymentConfig{}
 	firedTriggersForConfig := make(map[string][]deployapi.DeploymentTriggerImageChangeParams)
 
 	for _, c := range c.DeploymentConfigStore.List() {
@@ -46,9 +46,12 @@ func (c *ImageChangeController) HandleImageRepo() {
 		// Extract relevant triggers for this imageRepo for this config
 		triggersForConfig := []deployapi.DeploymentTriggerImageChangeParams{}
 		for _, trigger := range config.Triggers {
-			if trigger.Type == deployapi.DeploymentTriggerOnImageChange &&
-				trigger.ImageChangeParams.Automatic &&
-				trigger.ImageChangeParams.RepositoryName == imageRepo.DockerImageRepository {
+			if trigger.Type != deployapi.DeploymentTriggerOnImageChange ||
+				!trigger.ImageChangeParams.Automatic {
+				continue
+			}
+			if triggerMatchesImage(config, trigger.ImageChangeParams, imageRepo) {
+				glog.V(4).Infof("Found matching %s trigger for deploymentConfig %s: %#v", trigger.Type, config.Name, trigger.ImageChangeParams)
 				triggersForConfig = append(triggersForConfig, *trigger.ImageChangeParams)
 			}
 		}
@@ -62,67 +65,100 @@ func (c *ImageChangeController) HandleImageRepo() {
 				}
 
 				// The container image's tag name is by convention the same as the image ID it references
-				_, containerImageID := parseImage(container.Image)
+				_, _, _, containerImageID, err := imageapi.SplitDockerPullSpec(container.Image)
+				if err != nil {
+					glog.V(4).Infof("Skipping container %s; container's image is invalid: %v", container.Name, err)
+					continue
+				}
+
 				if repoImageID, repoHasTag := imageRepo.Tags[params.Tag]; repoHasTag && repoImageID != containerImageID {
-					configNames = append(configNames, config.Name)
+					configsToGenerate = append(configsToGenerate, config)
 					firedTriggersForConfig[config.Name] = append(firedTriggersForConfig[config.Name], params)
 				}
 			}
 		}
 	}
 
-	for _, configName := range configNames {
-		glog.V(4).Infof("Regenerating deploymentConfig %s", configName)
-		err := c.regenerate(imageRepo.Namespace, configName, firedTriggersForConfig[configName])
+	for _, config := range configsToGenerate {
+		glog.V(4).Infof("Regenerating deploymentConfig %s/%s", config.Namespace, config.Name)
+		err := c.regenerate(imageRepo, config, firedTriggersForConfig[config.Name])
 		if err != nil {
-			glog.V(2).Infof("Error regenerating deploymentConfig %v: %v", configName, err)
+			glog.V(2).Infof("Error regenerating deploymentConfig %s/%s: %v", config.Namespace, config.Name, err)
 		}
 	}
 }
 
-func (c *ImageChangeController) regenerate(namespace, configName string, triggers []deployapi.DeploymentTriggerImageChangeParams) error {
-	newConfig, err := c.DeploymentConfigInterface.GenerateDeploymentConfig(namespace, configName)
-	if err != nil {
-		glog.V(2).Infof("Error generating new version of deploymentConfig %v", configName)
-		return err
-	}
+// triggerMatchesImages decides whether a given trigger for config matches the provided image repo.
+// When matching:
+// - The trigger From field is preferred over the deprecated RepositoryName field.
+// - The namespace of the trigger is preferred over the config's namespace.
+func triggerMatchesImage(config *deployapi.DeploymentConfig, trigger *deployapi.DeploymentTriggerImageChangeParams, repo *imageapi.ImageRepository) bool {
+	if len(trigger.From.Name) > 0 {
+		namespace := trigger.From.Namespace
+		if len(namespace) == 0 {
+			namespace = config.Namespace
+		}
 
-	// update the deployment config with the trigger that resulted in the new config being generated
-	newConfig.Details = generateTriggerDetails(triggers)
+		return repo.Namespace == namespace && repo.Name == trigger.From.Name
+	}
 
-	_, err = c.DeploymentConfigInterface.UpdateDeploymentConfig(newConfig.Namespace, newConfig)
-	if err != nil {
-		glog.V(2).Infof("Error updating deploymentConfig %v", configName)
-		return err
+	// This is an invalid state (as one of From.Name or RepositoryName is required), but
+	// account for it anyway.
+	if len(trigger.RepositoryName) == 0 {
+		return false
 	}
 
-	return nil
+	// If the repo's repository information isn't yet available, we can't assume it'll match.
+	return len(repo.Status.DockerImageRepository) > 0 &&
+		trigger.RepositoryName == repo.Status.DockerImageRepository
 }
 
-func parseImage(name string) (string, string) {
-	index := strings.LastIndex(name, ":")
-	if index == -1 {
-		return "", ""
+func (c *ImageChangeController) regenerate(imageRepo *imageapi.ImageRepository, config *deployapi.DeploymentConfig, triggers []deployapi.DeploymentTriggerImageChangeParams) error {
+	// Get a regenerated config which includes the new image repo references
+	newConfig, err := c.DeploymentConfigInterface.GenerateDeploymentConfig(config.Namespace, config.Name)
+	if err != nil {
+		glog.V(2).Infof("Error generating new version of deploymentConfig %v", config.Name)
+		return err
 	}
 
-	return name[:index], name[index+1:]
-}
-
-func generateTriggerDetails(triggers []deployapi.DeploymentTriggerImageChangeParams) *deployapi.DeploymentDetails {
-	// Generate the DeploymentCause objects from each DeploymentTriggerImageChangeParams object
-	// Using separate structs to ensure flexibility in the future if these structs need to diverge
+	// Update the deployment config with the trigger that resulted in the new config
 	causes := []*deployapi.DeploymentCause{}
 	for _, trigger := range triggers {
+		repoName := trigger.RepositoryName
+
+		if len(repoName) == 0 {
+			if len(imageRepo.Status.DockerImageRepository) == 0 {
+				// If the trigger relies on a image repo reference, and we don't know what docker repo
+				// it points at, we can't build a cause for the reference yet.
+				continue
+			}
+
+			id, ok := imageRepo.Tags[trigger.Tag]
+			if !ok {
+				// TODO: not really sure what to do here
+			}
+			repoName = fmt.Sprintf("%s:%s", imageRepo.Status.DockerImageRepository, id)
+		}
+
 		causes = append(causes,
 			&deployapi.DeploymentCause{
 				Type: deployapi.DeploymentTriggerOnImageChange,
 				ImageTrigger: &deployapi.DeploymentCauseImageTrigger{
-					RepositoryName: trigger.RepositoryName,
+					RepositoryName: repoName,
 					Tag:            trigger.Tag,
 				},
 			})
 	}
-	return &deployapi.DeploymentDetails{
+	newConfig.Details = &deployapi.DeploymentDetails{
 		Causes: causes,
 	}
+
+	// Persist the new config
+	_, err = c.DeploymentConfigInterface.UpdateDeploymentConfig(newConfig.Namespace, newConfig)
+	if err != nil {
+		glog.V(2).Infof("Error updating deploymentConfig %v", newConfig.Name)
+		return err
+	}
+
+	return nil
 }

pkg/deploy/controller/image_change_controller_test.go

@@ -105,61 +105,138 @@ func TestImageChangeForUnregisteredTag(t *testing.T) {
 	controller.HandleImageRepo()
 }
 
-func TestImageChange(t *testing.T) {
-	var (
-		generatedConfig          *deployapi.DeploymentConfig
-		updatedConfig            *deployapi.DeploymentConfig
-		generatedConfigNamespace string
-		updatedConfigNamespace   string
-	)
-
-	controller := &ImageChangeController{
-		DeploymentConfigInterface: &testIcDeploymentConfigInterface{
-			UpdateDeploymentConfigFunc: func(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
-				updatedConfigNamespace = namespace
-				updatedConfig = config
-				return updatedConfig, nil
-			},
-			GenerateDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) {
-				generatedConfigNamespace = namespace
-				generatedConfig = regeneratedConfig(namespace)
-				return generatedConfig, nil
-			},
+func TestImageChangeMatchScenarios(t *testing.T) {
+	params := map[string]*deployapi.DeploymentTriggerImageChangeParams{
+		"params.1": {
+			Automatic:      true,
+			ContainerNames: []string{"container-1"},
+			From:           kapi.ObjectReference{Namespace: kapi.NamespaceDefault, Name: "repoA"},
+			Tag:            "test-tag",
 		},
-		NextImageRepository: func() *imageapi.ImageRepository {
-			return tagUpdate()
+		"params.2": {
+			Automatic:      true,
+			ContainerNames: []string{"container-1"},
+			From:           kapi.ObjectReference{Name: "repoA"},
+			Tag:            "test-tag",
+		},
+		"params.3": {
+			Automatic:      true,
+			ContainerNames: []string{"container-1"},
+			RepositoryName: "registry:8080/openshift/test-image",
+			Tag:            "test-tag",
 		},
-		DeploymentConfigStore: deploytest.NewFakeDeploymentConfigStore(imageChangeDeploymentConfig()),
 	}
 
-	controller.HandleImageRepo()
-
-	if generatedConfig == nil {
-		t.Fatalf("expected config generation to occur")
+	updates := map[string]*imageapi.ImageRepository{
+		"repo.1": {
+			ObjectMeta: kapi.ObjectMeta{Name: "repoA", Namespace: kapi.NamespaceDefault},
+			Status:     imageapi.ImageRepositoryStatus{"registry:8080/openshift/test-image"},
+			Tags:       map[string]string{"test-tag": "ref-2"},
+		},
+		"repo.2": {
+			ObjectMeta: kapi.ObjectMeta{Name: "repoB", Namespace: kapi.NamespaceDefault},
+			Status:     imageapi.ImageRepositoryStatus{"registry:8080/openshift/test-image"},
+			Tags:       map[string]string{"test-tag": "ref-3"},
+		},
+		"repo.3": {
+			ObjectMeta: kapi.ObjectMeta{Name: "repoC", Namespace: kapi.NamespaceDefault},
+			Status:     imageapi.ImageRepositoryStatus{"registry:8080/openshift/test-image-B"},
+			Tags:       map[string]string{"test-tag": "ref-2"},
+		},
+		"repo.4": {
+			ObjectMeta: kapi.ObjectMeta{Name: "repoA", Namespace: kapi.NamespaceDefault},
+			Tags:       map[string]string{"test-tag": "ref-2"},
+		},
 	}
 
-	if updatedConfig == nil {
-		t.Fatalf("expected an updated deployment config")
-	} else if updatedConfig.Details == nil {
-		t.Fatalf("expected config change details to be set")
-	} else if updatedConfig.Details.Causes == nil {
-		t.Fatalf("expected config change causes to be set")
-	} else if updatedConfig.Details.Causes[0].Type != deployapi.DeploymentTriggerOnImageChange {
-		t.Fatalf("expected ChangeLog details to be set to image change trigger, got %s", updatedConfig.Details.Causes[0].Type)
-	}
-	if generatedConfigNamespace != nonDefaultNamespace {
-		t.Errorf("Expected generatedConfigNamespace %v, got %v", nonDefaultNamespace, generatedConfigNamespace)
-	}
-	if updatedConfigNamespace != nonDefaultNamespace {
-		t.Errorf("Expected updatedConfigNamespace %v, got %v", nonDefaultNamespace, updatedConfigNamespace)
+	scenarios := []struct {
+		param   string
+		repo    string
+		matches bool
+		causes  []string
+	}{
+		{"params.1", "repo.1", true, []string{"registry:8080/openshift/test-image:ref-2"}},
+		{"params.1", "repo.2", false, []string{}},
+		{"params.1", "repo.3", false, []string{}},
+		// This case relies on a brittle assumption that we'll sometimes has an empty
+		// imageRepo.Status.DockerImageRepository, but we'll still feed it through the
+		// generator anyway (which could return a config with no diffs).
+		{"params.1", "repo.4", true, []string{}},
+		{"params.2", "repo.1", true, []string{"registry:8080/openshift/test-image:ref-2"}},
+		{"params.2", "repo.2", false, []string{}},
+		{"params.2", "repo.3", false, []string{}},
+		// Same as params.1 -> repo.4, see above
+		{"params.2", "repo.4", true, []string{}},
+		{"params.3", "repo.1", true, []string{"registry:8080/openshift/test-image"}},
+		{"params.3", "repo.2", true, []string{"registry:8080/openshift/test-image"}},
+		{"params.3", "repo.3", false, []string{}},
+		{"params.3", "repo.4", false, []string{}},
 	}
 
-	if e, a := updatedConfig.Name, generatedConfig.Name; e != a {
-		t.Fatalf("expected updated config with id %s, got %s", e, a)
-	}
+	for _, s := range scenarios {
+		config := imageChangeDeploymentConfig()
+		config.Namespace = kapi.NamespaceDefault
+		config.Triggers = []deployapi.DeploymentTriggerPolicy{
+			{
+				Type:              deployapi.DeploymentTriggerOnImageChange,
+				ImageChangeParams: params[s.param],
+			},
+		}
+
+		updated := false
+		generated := false
 
-	if e, a := updatedConfig.Name, generatedConfig.Name; e != a {
-		t.Fatalf("expected updated config with id %s, got %s", e, a)
+		controller := &ImageChangeController{
+			DeploymentConfigInterface: &testIcDeploymentConfigInterface{
+				UpdateDeploymentConfigFunc: func(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
+					if !s.matches {
+						t.Fatalf("unexpected deployment config update for scenario: %v", s)
+					}
+					updated = true
+					return config, nil
+				},
+				GenerateDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) {
+					if !s.matches {
+						t.Fatalf("unexpected generator call for scenario: %v", s)
+					}
+					generated = true
+					return config, nil
+				},
+			},
+			NextImageRepository: func() *imageapi.ImageRepository {
+				return updates[s.repo]
+			},
+			DeploymentConfigStore: deploytest.NewFakeDeploymentConfigStore(config),
+		}
+
+		t.Logf("running scenario: %v", s)
+		controller.HandleImageRepo()
+
+		// assert updates/generations occured
+		if s.matches && !updated {
+			t.Fatalf("expected update for scenario: %v", s)
+		}
+
+		if s.matches && !generated {
+			t.Fatalf("expected generation for scenario: %v", s)
+		}
+
+		// assert causes are correct relative to expected updates
+		if updated {
+			if e, a := len(s.causes), len(config.Details.Causes); e != a {
+				t.Fatalf("expected cause length %d, got %d", e, a)
+			}
+
+			for i, cause := range config.Details.Causes {
+				if e, a := s.causes[i], cause.ImageTrigger.RepositoryName; e != a {
+					t.Fatalf("expected cause repositoryName %s, got %s", e, a)
+				}
+			}
+		} else {
+			if config.Details != nil && len(config.Details.Causes) != 0 {
+				t.Fatalf("expected cause length 0, got %d", len(config.Details.Causes))
+			}
+		}
 	}
 }