From 2c64904fa68a59208febe2ea31c63a5219be9e76 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Wed, 17 Jul 2024 13:23:06 +0200 Subject: [PATCH] Diff actual variable values when updating Gitlab CI vars --- controllers/gitrepo/steps.go | 15 +++---- controllers/gitrepo/steps_test.go | 8 +--- git/gitlab/gitlab.go | 49 +++++++++++++++++++++ git/gitlab/gitlab_test.go | 71 +++++++++++++++++++++---------- go.mod | 5 ++- go.sum | 2 + 6 files changed, 110 insertions(+), 40 deletions(-) diff --git a/controllers/gitrepo/steps.go b/controllers/gitrepo/steps.go index b9e894ea..b761b01a 100644 --- a/controllers/gitrepo/steps.go +++ b/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 } diff --git a/controllers/gitrepo/steps_test.go b/controllers/gitrepo/steps_test.go index f2bf90d8..6c173593 100644 --- a/controllers/gitrepo/steps_test.go +++ b/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") diff --git a/git/gitlab/gitlab.go b/git/gitlab/gitlab.go index 4a1511af..0a5997c7 100644 --- a/git/gitlab/gitlab.go +++ b/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 { diff --git a/git/gitlab/gitlab_test.go b/git/gitlab/gitlab_test.go index 00f7abbd..cfab6be0 100644 --- a/git/gitlab/gitlab_test.go +++ b/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,16 +830,28 @@ 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) }) @@ -841,6 +859,8 @@ func testProjectProjectVariablesServer(t *testing.T, clock func() time.Time) *ht }) 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,11 +935,13 @@ 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) @@ -925,20 +949,21 @@ 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() - 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 } diff --git a/go.mod b/go.mod index 166ecf5e..297b6003 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index cf21a2d0..da02f96f 100644 --- a/go.sum +++ b/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=