Skip to content

Commit 37af339

Browse files
committed
fix(gcp): merge instead of clobber when setting instance labels
Instance.SetLabelsWithClient sent the caller's labels map directly to the Compute API, which replaces all labels server-side. This is the same bug class as the SetMetadata clobber regression already fixed in v1; calling SetLabels to add a single label silently destroys every other label on the instance. Add a mergeLabels helper paralleling NewMetadata at line 524 and route SetLabelsWithClient through it so existing labels are preserved. TestSetLabelsWithClientMergesExisting verifies the merge. OSS-3455.
1 parent bc047d0 commit 37af339

2 files changed

Lines changed: 54 additions & 1 deletion

File tree

modules/gcp/compute.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,11 +426,14 @@ func (i *Instance) SetLabelsContextE(t testing.TestingT, ctx context.Context, la
426426
}
427427

428428
// SetLabelsWithClient adds the tags to the given Compute Instance using the supplied *compute.Service.
429+
// New labels are merged into the instance's existing labels; passing a key that is already set
430+
// overwrites it, but other existing labels are preserved.
429431
// Prefer this variant in unit tests where the service is backed by an httptest fake server
430432
// (see compute_unit_test.go for the pattern).
431433
// The ctx parameter supports cancellation and timeouts.
432434
func (i *Instance) SetLabelsWithClient(ctx context.Context, service *compute.Service, labels map[string]string) error {
433-
req := compute.InstancesSetLabelsRequest{Labels: labels, LabelFingerprint: i.LabelFingerprint}
435+
merged := mergeLabels(i.Labels, labels)
436+
req := compute.InstancesSetLabelsRequest{Labels: merged, LabelFingerprint: i.LabelFingerprint}
434437

435438
if _, err := service.Instances.SetLabels(i.projectID, ZoneURLToZone(i.Zone), i.Name, &req).Context(ctx).Do(); err != nil {
436439
return fmt.Errorf("Instances.SetLabels(%s) got error: %w", i.Name, err)
@@ -439,6 +442,21 @@ func (i *Instance) SetLabelsWithClient(ctx context.Context, service *compute.Ser
439442
return nil
440443
}
441444

445+
// mergeLabels merges new key-value pairs into existing labels, preserving any keys not in the new set.
446+
// Keys present in newLabels overwrite the existing values.
447+
func mergeLabels(existing, newLabels map[string]string) map[string]string {
448+
merged := make(map[string]string, len(existing)+len(newLabels))
449+
for k, v := range existing {
450+
merged[k] = v
451+
}
452+
453+
for k, v := range newLabels {
454+
merged[k] = v
455+
}
456+
457+
return merged
458+
}
459+
442460
// GetMetadata gets the given Compute Instance's metadata.
443461
//
444462
// Deprecated: Use [Instance.GetMetadataContext] instead.

modules/gcp/compute_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package gcp_test
22

33
import (
44
"context"
5+
"encoding/json"
56
"fmt"
67
"net/http"
78
"net/http/httptest"
@@ -194,6 +195,40 @@ func TestSetLabelsWithClient(t *testing.T) {
194195
require.NoError(t, inst.SetLabelsWithClient(context.Background(), svc, map[string]string{"env": "unit"}))
195196
}
196197

198+
// TestSetLabelsWithClientMergesExisting — regression test for the SetLabels-clobbers-existing bug.
199+
// The instance already carries a label; SetLabels should merge the new label in, not drop the
200+
// existing one.
201+
func TestSetLabelsWithClientMergesExisting(t *testing.T) {
202+
t.Parallel()
203+
204+
zoneURL := "https://www.googleapis.com/compute/v1/projects/p/zones/us-central1-a"
205+
inst := fetchInstanceForTest(t, "p", "i", zoneURL)
206+
inst.Labels = map[string]string{"team": "platform"}
207+
208+
var sentLabels map[string]string
209+
210+
handler := func(w http.ResponseWriter, r *http.Request) {
211+
assert.Equal(t, http.MethodPost, r.Method)
212+
assert.Contains(t, r.URL.Path, "/instances/i/setLabels")
213+
214+
var req compute.InstancesSetLabelsRequest
215+
216+
require.NoError(t, json.NewDecoder(r.Body).Decode(&req))
217+
218+
sentLabels = req.Labels
219+
w.Header().Set("Content-Type", "application/json")
220+
w.WriteHeader(http.StatusOK)
221+
_, _ = w.Write([]byte(`{"name":"op","status":"DONE"}`))
222+
}
223+
224+
svc := newFakeComputeService(t, http.HandlerFunc(handler))
225+
226+
require.NoError(t, inst.SetLabelsWithClient(context.Background(), svc, map[string]string{"env": "unit"}))
227+
228+
assert.Equal(t, "platform", sentLabels["team"], "existing label should be preserved")
229+
assert.Equal(t, "unit", sentLabels["env"], "new label should be set")
230+
}
231+
197232
func TestSetMetadataWithClient(t *testing.T) {
198233
t.Parallel()
199234

0 commit comments

Comments
 (0)