Add validation to avoid conflict with service envs (#2687)

api/app_test.go

@@ -3929,16 +3929,10 @@ func (s *S) TestSetEnvHandlerShouldNotChangeValueOfServiceVariables(c *check.C)
 	}
 	c.Assert(envs, check.DeepEquals, expected)
 	c.Assert(eventtest.EventDesc{
-		Target: appTarget(a.Name),
-		Owner:  s.token.GetUserName(),
-		Kind:   "app.update.env.set",
-		StartCustomData: []map[string]interface{}{
-			{"name": ":app", "value": a.Name},
-			{"name": "Envs.0.Name", "value": "DATABASE_HOST"},
-			{"name": "Envs.0.Value", "value": "http://foo.com:8080"},
-			{"name": "NoRestart", "value": ""},
-			{"name": "Private", "value": ""},
-		},
+		Target:       appTarget(a.Name),
+		Owner:        s.token.GetUserName(),
+		Kind:         "app.update.env.set",
+		ErrorMatches: "Environment variable \"DATABASE_HOST\" is already in use by service bind \"srv1/some service\"",
 	}, eventtest.HasEvent)
 }
 

app/app.go

@@ -1621,17 +1621,25 @@ func (app *App) SetEnvs(setEnvs bind.SetEnvArgs) error {
 		return nil
 	}
 
+	envNames := []string{}
 	for _, env := range setEnvs.Envs {
 		err := validateEnv(env.Name)
 		if err != nil {
 			return err
 		}
+		envNames = append(envNames, env.Name)
 	}
 
 	if setEnvs.Writer != nil && len(setEnvs.Envs) > 0 {
 		fmt.Fprintf(setEnvs.Writer, "---- Setting %d new environment variables ----\n", len(setEnvs.Envs))
 	}
 
+	err := validateEnvConflicts(app, envNames)
+	if err != nil {
+		fmt.Fprintf(setEnvs.Writer, "---- environment variables have conflicts with service binds: %s ----\n", err.Error())
+		return err
+	}
+
 	if setEnvs.PruneUnused {
 		for name, value := range app.Env {
 			ok := envInSet(name, setEnvs.Envs)
@@ -1667,6 +1675,23 @@ func (app *App) SetEnvs(setEnvs bind.SetEnvArgs) error {
 	return nil
 }
 
+func validateEnvConflicts(app *App, envNames []string) error {
+	serviceEnvs := map[string]bindTypes.ServiceEnvVar{}
+	for _, env := range app.ServiceEnvs {
+		serviceEnvs[env.Name] = env
+	}
+
+	multiError := &tsuruErrors.MultiError{}
+
+	for _, env := range envNames {
+		if serviceEnv, ok := serviceEnvs[env]; ok {
+			multiError.Add(fmt.Errorf("Environment variable %q is already in use by service bind \"%s/%s\"", env, serviceEnv.ServiceName, serviceEnv.InstanceName))
+		}
+	}
+
+	return multiError.ToError()
+}
+
 // UnsetEnvs removes environment variables from an app, serializing the
 // remaining list of environment variables to all units of the app.
 func (app *App) UnsetEnvs(unsetEnvs bind.UnsetEnvArgs) error {

app/app_test.go

@@ -1130,7 +1130,8 @@ func (s *S) TestSetEnvKeepServiceVariables(c *check.C) {
 					Value:  "localhost",
 					Public: false,
 				},
-				InstanceName: "some service",
+				InstanceName: "instance",
+				ServiceName:  "service",
 			},
 		},
 		TeamOwner: s.team.Name,
@@ -1159,7 +1160,7 @@ func (s *S) TestSetEnvKeepServiceVariables(c *check.C) {
 		ShouldRestart: true,
 		Writer:        &buf,
 	})
-	c.Assert(err, check.IsNil)
+	c.Assert(err, check.ErrorMatches, "Environment variable \"DATABASE_HOST\" is already in use by service bind \"service/instance\"")
 	newApp, err := GetByName(context.TODO(), a.Name)
 	c.Assert(err, check.IsNil)
 	expected := map[string]bindTypes.EnvVar{
@@ -1168,17 +1169,15 @@ func (s *S) TestSetEnvKeepServiceVariables(c *check.C) {
 			Value:  "localhost",
 			Public: false,
 		},
-		"DATABASE_PASSWORD": {
-			Name:   "DATABASE_PASSWORD",
-			Value:  "123",
-			Public: true,
-		},
 	}
 	newAppEnvs := newApp.Envs()
 	delete(newAppEnvs, tsuruEnvs.TsuruServicesEnvVar)
+	delete(newAppEnvs, "TSURU_APPNAME")
+	delete(newAppEnvs, "TSURU_APPDIR")
+
 	c.Assert(newAppEnvs, check.DeepEquals, expected)
-	c.Assert(s.provisioner.Restarts(&a, ""), check.Equals, 1)
-	c.Assert(buf.String(), check.Equals, "---- Setting 2 new environment variables ----\nrestarting app")
+	c.Assert(s.provisioner.Restarts(&a, ""), check.Equals, 0)
+	c.Assert(buf.String(), check.Equals, "---- Setting 2 new environment variables ----\n---- environment variables have conflicts with service binds: Environment variable \"DATABASE_HOST\" is already in use by service bind \"service/instance\" ----\n")
 }
 
 func (s *S) TestSetEnvWithNoRestartFlag(c *check.C) {