Diff actual variable values when updating Gitlab CI vars

controllers/gitrepo/steps.go

@@ -1,7 +1,6 @@
 package gitrepo
 
 import (
-	"bytes"
 	"context"
 	"fmt"
 	"time"
@@ -183,13 +182,6 @@ func ensureAccessToken(ctx context.Context, cli client.Client, instance *synv1al
 }
 
 func ensureCIVariables(ctx context.Context, cli client.Client, instance *synv1alpha1.GitRepo, repo manager.Repo) error {
-	varsJSON, err := json.Marshal(instance.Spec.CIVariables)
-	if err != nil {
-		return fmt.Errorf("error marshalling ci variables: %w", err)
-	}
-	if bytes.Equal(varsJSON, []byte(instance.Status.LastAppliedCIVariables)) {
-		return nil
-	}
 	var prevVars []synv1alpha1.EnvVar
 	if instance.Status.LastAppliedCIVariables != "" {
 		if err := json.Unmarshal([]byte(instance.Status.LastAppliedCIVariables), &prevVars); err != nil {
@@ -228,10 +220,15 @@ func ensureCIVariables(ctx context.Context, cli client.Client, instance *synv1al
 		return fmt.Errorf("error collecting values for env vars: %w", err)
 	}
 
-	if err = repo.EnsureCIVariables(ctx, sets.List(managedVars), vars); err != nil {
+	if err := repo.EnsureCIVariables(ctx, sets.List(managedVars), vars); err != nil {
 		return fmt.Errorf("error ensuring ci variables: %w", err)
 	}
 
+	varsJSON, err := json.Marshal(instance.Spec.CIVariables)
+	if err != nil {
+		return fmt.Errorf("error marshalling ci variables: %w", err)
+	}
+
 	instance.Status.LastAppliedCIVariables = string(varsJSON)
 	return nil
 }

controllers/gitrepo/steps_test.go

@@ -495,15 +495,11 @@ func TestSteps_CIVariables(t *testing.T) {
 		},
 	}, call.vars)
 
-	res2 := steps(repo, pContext, gc)
-	assert.NoError(t, res2.Err)
-	require.Len(t, fr.ensureCIVariablesCalls, 1, "ci variables should not be updated if they did not change")
-
 	// remove first variable. Removed variable should still appear in the managed variables because it was managed before.
 	// it should not appear in the variables to be set.
 	repo.Spec.GitRepoTemplate.CIVariables = repo.Spec.GitRepoTemplate.CIVariables[1:]
-	res3 := steps(repo, pContext, gc)
-	assert.NoError(t, res3.Err)
+	res2 := steps(repo, pContext, gc)
+	assert.NoError(t, res2.Err)
 	require.Len(t, fr.ensureCIVariablesCalls, 2)
 	call = fr.ensureCIVariablesCalls[1]
 	assert.ElementsMatch(t, varNames, call.managed, "managed variables should be remembered from previous run")

git/gitlab/gitlab.go

@@ -15,6 +15,7 @@ import (
 	"go.uber.org/multierr"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	synv1alpha1 "github.com/projectsyn/lieutenant-operator/api/v1alpha1"
 	"github.com/projectsyn/lieutenant-operator/git/helpers"
@@ -541,12 +542,15 @@ func (g *Gitlab) EnsureProjectAccessToken(ctx context.Context, name string, opts
 // Variables that are not managed by the operator will be ignored.
 // Variables that are managed but not in variables will be deleted.
 func (g *Gitlab) EnsureCIVariables(ctx context.Context, managedVariables []string, variables []manager.EnvVar) error {
+	l := log.FromContext(ctx).WithName("EnsureCIVariables")
+
 	var errs []error
 	managed := sets.New(managedVariables...)
 	current := sets.New[string]()
 	for _, v := range variables {
 		current.Insert(v.Name)
 	}
+
 	toDelete := managed.Difference(current)
 	for _, v := range sets.List(toDelete) {
 		_, err := g.client.ProjectVariables.RemoveVariable(g.project.ID, v, &gitlab.RemoveProjectVariableOptions{}, gitlab.WithContext(ctx))
@@ -555,11 +559,35 @@ func (g *Gitlab) EnsureCIVariables(ctx context.Context, managedVariables []strin
 		}
 	}
 
+	remote, _, err := g.client.ProjectVariables.ListVariables(g.project.ID, &gitlab.ListProjectVariablesOptions{}, gitlab.WithContext(ctx))
+	if err != nil {
+		return fmt.Errorf("error listing variables: %w", err)
+	}
+	remoteByName := make(map[string]gitlab.ProjectVariable, len(remote))
+	for _, v := range remote {
+		if v == nil {
+			continue
+		}
+		remoteByName[v.Key] = *v
+	}
+
 	for _, v := range variables {
 		if !managed.Has(v.Name) {
 			continue
 		}
 
+		remote, ok := remoteByName[v.Name]
+		var changed bool
+		if ok {
+			changed = varNeedsUpdate(remote, v)
+		} else {
+			changed = true
+		}
+		if !changed {
+			continue
+		}
+
+		l.Info("updating changed variable", "name", v.Name)
 		err := g.updateOrCreateCIVariable(ctx, v)
 		if err != nil {
 			errs = append(errs, err)
@@ -569,6 +597,27 @@ func (g *Gitlab) EnsureCIVariables(ctx context.Context, managedVariables []strin
 	return multierr.Combine(errs...)
 }
 
+// varNeedsUpdate returns true if the remote variable needs to be updated.
+// Does not check the Key, as the key is not allowed to change.
+func varNeedsUpdate(remote gitlab.ProjectVariable, local manager.EnvVar) bool {
+	if remote.Value != local.Value {
+		return true
+	}
+	if local.GitlabOptions.Description != nil && remote.Description != *local.GitlabOptions.Description {
+		return true
+	}
+	if local.GitlabOptions.Protected != nil && remote.Protected != *local.GitlabOptions.Protected {
+		return true
+	}
+	if local.GitlabOptions.Masked != nil && remote.Masked != *local.GitlabOptions.Masked {
+		return true
+	}
+	if local.GitlabOptions.Raw != nil && remote.Raw != *local.GitlabOptions.Raw {
+		return true
+	}
+	return false
+}
+
 // updateOrCreateCIVariable updates or creates a CI variable in the Gitlab project.
 // It tries to update the variable first and creates it if it does not exist.
 func (g *Gitlab) updateOrCreateCIVariable(ctx context.Context, v manager.EnvVar) error {

git/gitlab/gitlab_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/xanzy/go-gitlab"
+	"go.uber.org/atomic"
 	"go.uber.org/zap"
 	"golang.org/x/exp/maps"
 	"k8s.io/utils/ptr"
@@ -708,7 +709,7 @@ func TestGitlab_EnsureProjectAccessToken(t *testing.T) {
 func TestGitlab_EnsureCIVariables(t *testing.T) {
 	clock := &mockClock{now: time.Now()}
 
-	serv := testProjectProjectVariablesServer(t, clock.Now)
+	serv := newTestProjectProjectVariablesServer(t, clock.Now)
 	defer serv.Close()
 
 	url, err := url.Parse(serv.URL)
@@ -751,6 +752,11 @@ func TestGitlab_EnsureCIVariables(t *testing.T) {
 	assert.Equal(t, "value2", cvs[1].Value)
 	assert.True(t, cvs[1].Protected)
 
+	// no changes should be write noops
+	prevCalls := serv.updateCount.Load()
+	require.NoError(t, g.EnsureCIVariables(context.Background(), []string{"KEY1", "KEY2"}, vars))
+	require.Equal(t, prevCalls, serv.updateCount.Load(), "no changes should be write noops")
+
 	// Update variable value
 	vars[0].Value = "value1.1"
 	vars[1].Value = "value2.1"
@@ -824,23 +830,37 @@ func testProjectAccessTokenServer(t *testing.T, clock func() time.Time) *httptes
 	return httptest.NewServer(mux)
 }
 
-func testProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *httptest.Server {
+type testProjectProjectVariablesServer struct {
+	*httptest.Server
+
+	varsMux sync.Mutex
+	vars    map[string]gitlab.ProjectVariable
+
+	createCount atomic.Int32
+	updateCount atomic.Int32
+	deleteCount atomic.Int32
+}
+
+func newTestProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *testProjectProjectVariablesServer {
 	mux := http.NewServeMux()
 
-	var varsMux sync.Mutex
-	vars := make(map[string]gitlab.ProjectVariable)
+	s := &testProjectProjectVariablesServer{
+		vars: make(map[string]gitlab.ProjectVariable),
+	}
 
 	mux.HandleFunc("GET /api/v4/projects/3/variables", func(res http.ResponseWriter, req *http.Request) {
-		varsMux.Lock()
-		defer varsMux.Unlock()
-		vs := maps.Values(vars)
+		s.varsMux.Lock()
+		defer s.varsMux.Unlock()
+		vs := maps.Values(s.vars)
 		slices.SortFunc(vs, func(a, b gitlab.ProjectVariable) int {
 			return strings.Compare(a.Key, b.Key)
 		})
 		_ = json.NewEncoder(res).Encode(vs)
 	})
 
 	mux.HandleFunc("POST /api/v4/projects/3/variables", func(res http.ResponseWriter, req *http.Request) {
+		s.createCount.Inc()
+
 		var createVar gitlab.CreateProjectVariableOptions
 		if err := json.NewDecoder(req.Body).Decode(&createVar); err != nil {
 			res.WriteHeader(http.StatusInternalServerError)
@@ -856,14 +876,14 @@ func testProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *ht
 		}
 		key := *createVar.Key
 
-		varsMux.Lock()
-		if _, ok := vars[key]; ok {
+		s.varsMux.Lock()
+		if _, ok := s.vars[key]; ok {
 			res.WriteHeader(http.StatusBadRequest)
 			_, _ = res.Write([]byte(`{"error":"variable already exists"}`))
-			varsMux.Unlock()
+			s.varsMux.Unlock()
 			return
 		}
-		defer varsMux.Unlock()
+		defer s.varsMux.Unlock()
 		nVar := gitlab.ProjectVariable{
 			Key:              key,
 			Value:            ptr.Deref(createVar.Value, ""),
@@ -874,11 +894,13 @@ func testProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *ht
 			EnvironmentScope: ptr.Deref(createVar.EnvironmentScope, "*"),
 			Description:      ptr.Deref(createVar.Description, ""),
 		}
-		vars[key] = nVar
+		s.vars[key] = nVar
 		_ = json.NewEncoder(res).Encode(nVar)
 	})
 
 	mux.HandleFunc("PUT /api/v4/projects/3/variables/{key}", func(res http.ResponseWriter, req *http.Request) {
+		s.updateCount.Inc()
+
 		var createVar gitlab.UpdateProjectVariableOptions
 		if err := json.NewDecoder(req.Body).Decode(&createVar); err != nil {
 			res.WriteHeader(http.StatusInternalServerError)
@@ -894,15 +916,15 @@ func testProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *ht
 			return
 		}
 
-		varsMux.Lock()
-		oVar, ok := vars[key]
+		s.varsMux.Lock()
+		oVar, ok := s.vars[key]
 		if !ok {
 			res.WriteHeader(http.StatusNotFound)
 			_, _ = res.Write([]byte(`{"error":"404 not found"}`))
-			varsMux.Unlock()
+			s.varsMux.Unlock()
 			return
 		}
-		defer varsMux.Unlock()
+		defer s.varsMux.Unlock()
 		nVar := gitlab.ProjectVariable{
 			Key:              key,
 			Value:            ptr.Deref(createVar.Value, oVar.Value),
@@ -913,32 +935,35 @@ func testProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *ht
 			EnvironmentScope: ptr.Deref(createVar.EnvironmentScope, oVar.EnvironmentScope),
 			Description:      ptr.Deref(createVar.Description, oVar.Description),
 		}
-		vars[key] = nVar
+		s.vars[key] = nVar
 		_ = json.NewEncoder(res).Encode(nVar)
 	})
 
 	mux.HandleFunc("DELETE /api/v4/projects/3/variables/{key}", func(res http.ResponseWriter, req *http.Request) {
+		s.deleteCount.Inc()
+
 		key := req.PathValue("key")
 		if key == "" {
 			res.WriteHeader(http.StatusBadRequest)
 			_, _ = res.Write([]byte(`{"error":"key is required"}`))
 			return
 		}
 
-		varsMux.Lock()
-		oVar, ok := vars[key]
+		s.varsMux.Lock()
+		oVar, ok := s.vars[key]
 		if !ok {
 			res.WriteHeader(http.StatusNotFound)
 			_, _ = res.Write([]byte(`{"error":"404 not found"}`))
-			varsMux.Unlock()
+			s.varsMux.Unlock()
 			return
 		}
-		defer varsMux.Unlock()
-		delete(vars, key)
+		defer s.varsMux.Unlock()
+		delete(s.vars, key)
 		_ = json.NewEncoder(res).Encode(oVar)
 	})
 
 	mux.HandleFunc("/", testutils.LogNotFoundHandler(t))
 
-	return httptest.NewServer(mux)
+	s.Server = httptest.NewServer(mux)
+	return s
 }

go.mod

@@ -15,7 +15,10 @@ require (
 	github.com/ryankurte/go-structparse v1.2.0
 	github.com/stretchr/testify v1.9.0
 	github.com/xanzy/go-gitlab v0.106.0
+	go.uber.org/atomic v1.11.0
+	go.uber.org/multierr v1.11.0
 	go.uber.org/zap v1.27.0
+	golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842
 	k8s.io/api v0.30.1
 	k8s.io/apimachinery v0.30.1
 	k8s.io/client-go v0.30.1
@@ -119,9 +122,7 @@ require (
 	go.opentelemetry.io/otel/metric v1.27.0 // indirect
 	go.opentelemetry.io/otel/trace v1.27.0 // indirect
 	go.starlark.net v0.0.0-20240520160348-046347dcd104 // indirect
-	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/crypto v0.23.0 // indirect
-	golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 // indirect
 	golang.org/x/mod v0.17.0 // indirect
 	golang.org/x/net v0.25.0 // indirect
 	golang.org/x/oauth2 v0.20.0 // indirect

go.sum

@@ -515,6 +515,8 @@ go.starlark.net v0.0.0-20240520160348-046347dcd104 h1:3qhteRISupnJvaWshOmeqEUs2y
 go.starlark.net v0.0.0-20240520160348-046347dcd104/go.mod h1:YKMCv9b1WrfWmeqdV5MAuEHWsu5iC+fe6kYl2sQjdI8=
 go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
 go.uber.org/atomic v1.9.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
+go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE=
+go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
 go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
 go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
 go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=