gitlab_project_variable: fix nondeterministic reads when using environment_scope (#409)

* gitlab_project_variable: fix nondeterministic reads when using environment_scope

* Respond to PR review
- remove indirection to build and parse the resource id
- link to issue in the resource docs
- small test cleanup by making a isGitLabVersionLessThan SkipFunc

Author: armsnyder

gitlab/resource_gitlab_project_variable.go

@@ -2,10 +2,13 @@ package gitlab
 
 import (
 	"errors"
+	"fmt"
 	"log"
 	"net/http"
+	"net/url"
 	"strings"
 
+	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
 	gitlab "github.com/xanzy/go-gitlab"
 )
@@ -56,7 +59,9 @@ func resourceGitlabProjectVariable() *schema.Resource {
 			"environment_scope": {
 				Type:     schema.TypeString,
 				Optional: true,
-				Default:  false,
+				Default:  "*",
+				// Versions of GitLab prior to 13.4 cannot update environment_scope.
+				ForceNew: true,
 			},
 		},
 	}
@@ -81,30 +86,55 @@ func resourceGitlabProjectVariableCreate(d *schema.ResourceData, meta interface{
 		Masked:           &masked,
 		EnvironmentScope: &environmentScope,
 	}
-	log.Printf("[DEBUG] create gitlab project variable %s/%s", project, key)
+
+	id := strings.Join([]string{project, key, environmentScope}, ":")
+
+	log.Printf("[DEBUG] create gitlab project variable %q", id)
 
 	_, _, err := client.ProjectVariables.CreateVariable(project, &options)
 	if err != nil {
 		return augmentProjectVariableClientError(d, err)
 	}
 
-	d.SetId(buildTwoPartID(&project, &key))
+	d.SetId(id)
 
 	return resourceGitlabProjectVariableRead(d, meta)
 }
 
 func resourceGitlabProjectVariableRead(d *schema.ResourceData, meta interface{}) error {
 	client := meta.(*gitlab.Client)
 
-	project, key, err := parseTwoPartID(d.Id())
-	if err != nil {
-		return err
+	var (
+		project          string
+		key              string
+		environmentScope string
+	)
+
+	// An older version of this resource used the ID format "project:key".
+	// For backwards compatibility we still support the old format.
+	parts := strings.SplitN(d.Id(), ":", 4)
+	switch len(parts) {
+	case 2:
+		project = parts[0]
+		key = parts[1]
+		environmentScope = d.Get("environment_scope").(string)
+	case 3:
+		project = parts[0]
+		key = parts[1]
+		environmentScope = parts[2]
+	default:
+		return fmt.Errorf(`Failed to parse project variable ID %q: expected format project:key or project:key:environment_scope`, d.Id())
 	}
 
-	log.Printf("[DEBUG] read gitlab project variable %s/%s", project, key)
+	log.Printf("[DEBUG] read gitlab project variable %q", d.Id())
 
-	v, _, err := client.ProjectVariables.GetVariable(project, key)
+	v, err := getProjectVariable(client, project, key, environmentScope)
 	if err != nil {
+		if errors.Is(err, errProjectVariableNotExist) {
+			log.Printf("[DEBUG] read gitlab project variable %q was not found", d.Id())
+			d.SetId("")
+			return nil
+		}
 		return augmentProjectVariableClientError(d, err)
 	}
 
@@ -114,9 +144,6 @@ func resourceGitlabProjectVariableRead(d *schema.ResourceData, meta interface{})
 	d.Set("project", project)
 	d.Set("protected", v.Protected)
 	d.Set("masked", v.Masked)
-	//For now I'm ignoring environment_scope when reading back data. (this can cause configuration drift so it is bad).
-	//However I'm unable to stop terraform from gratuitously updating this to values that are unacceptable by Gitlab)
-	//I don't have an enterprise license to properly test this either.
 	d.Set("environment_scope", v.EnvironmentScope)
 	return nil
 }
@@ -139,9 +166,9 @@ func resourceGitlabProjectVariableUpdate(d *schema.ResourceData, meta interface{
 		Masked:           &masked,
 		EnvironmentScope: &environmentScope,
 	}
-	log.Printf("[DEBUG] update gitlab project variable %s/%s", project, key)
+	log.Printf("[DEBUG] update gitlab project variable %q", d.Id())
 
-	_, _, err := client.ProjectVariables.UpdateVariable(project, key, options)
+	_, _, err := client.ProjectVariables.UpdateVariable(project, key, options, withEnvironmentScopeFilter(environmentScope))
 	if err != nil {
 		return augmentProjectVariableClientError(d, err)
 	}
@@ -153,9 +180,14 @@ func resourceGitlabProjectVariableDelete(d *schema.ResourceData, meta interface{
 	client := meta.(*gitlab.Client)
 	project := d.Get("project").(string)
 	key := d.Get("key").(string)
-	log.Printf("[DEBUG] Delete gitlab project variable %s/%s", project, key)
+	environmentScope := d.Get("environment_scope").(string)
+	log.Printf("[DEBUG] Delete gitlab project variable %q", d.Id())
 
-	_, err := client.ProjectVariables.RemoveVariable(project, key)
+	// Note that the environment_scope filter is added here to support GitLab versions >= 13.4,
+	// but it will be ignored in prior versions, causing nondeterministic destroy behavior when
+	// destroying or updating scoped variables.
+	// ref: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39209
+	_, err := client.ProjectVariables.RemoveVariable(project, key, withEnvironmentScopeFilter(environmentScope))
 	return augmentProjectVariableClientError(d, err)
 }
 
@@ -177,3 +209,43 @@ func isInvalidValueError(err error) bool {
 		strings.Contains(httpErr.Message, "value") &&
 		strings.Contains(httpErr.Message, "invalid")
 }
+
+func withEnvironmentScopeFilter(environmentScope string) gitlab.RequestOptionFunc {
+	return func(req *retryablehttp.Request) error {
+		query, err := url.ParseQuery(req.Request.URL.RawQuery)
+		if err != nil {
+			return err
+		}
+		query.Set("filter[environment_scope]", environmentScope)
+		req.Request.URL.RawQuery = query.Encode()
+		return nil
+	}
+}
+
+var errProjectVariableNotExist = errors.New("project variable does not exist")
+
+func getProjectVariable(client *gitlab.Client, project interface{}, key, environmentScope string) (*gitlab.ProjectVariable, error) {
+	// List and filter variables manually to support GitLab versions < v13.4 (2020-08-22)
+	// ref: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39209
+
+	page := 1
+
+	for {
+		projectVariables, resp, err := client.ProjectVariables.ListVariables(project, &gitlab.ListProjectVariablesOptions{Page: page})
+		if err != nil {
+			return nil, err
+		}
+
+		for _, v := range projectVariables {
+			if v.Key == key && v.EnvironmentScope == environmentScope {
+				return v, nil
+			}
+		}
+
+		if resp.NextPage == 0 {
+			return nil, errProjectVariableNotExist
+		}
+
+		page++
+	}
+}