diff --git a/pkg/deploy/controller/image_change_controller.go b/pkg/deploy/controller/image_change_controller.go index d19ac80c7aed..3e72b8d65720 100644 --- a/pkg/deploy/controller/image_change_controller.go +++ b/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 } diff --git a/pkg/deploy/controller/image_change_controller_test.go b/pkg/deploy/controller/image_change_controller_test.go index eebc0634cf23..15348e084559 100644 --- a/pkg/deploy/controller/image_change_controller_test.go +++ b/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)) + } + } } }