cockroachdb / cockroachdb/cockroach

roachprod: track private GCE defects shared by master and release-26.3

Open
#173,355 1 comment 0 reactions 0 assignees View on GitHub
A-testeng-infra branch-master C-bug T-testeng
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

## Summary

Track private-GCE feature defects that currently exist on both current master and PR #3365's release-26.3 implementation.

These are not master/backport divergence bugs and are not caused by conflict resolution. They are either source-feature defects carried into the backport or old assumptions exposed by private mode. Fixes should be applied to master and then included in the release backport as appropriate.

## Findings

### GCS SSH-key update race

The GCS-backed shared SSH-key path performs an unconditional read-modify-write:

1. read the complete `ssh-keys` object;
2. append/remove a key locally;
3. overwrite the object without a generation precondition.

Concurrent roachprod processes can lose one another's updates or resurrect stale keys.

Expected behavior: updates should use a generation/ETag precondition, retry on conflict, or be serialized.

The read-modify-write begins here:

```go
func AddUserAuthorizedKey(ak AuthorizedKey) error {
existingKeys, err := Infrastructure.GetUserAuthorizedKeys()
if err != nil {
return err
}
newKeys := append(existingKeys, ak)
return SetUserAuthorizedKeys(newKeys)
}
```

The GCS writer then overwrites the object without an `IfGenerationMatch` (or equivalent) precondition:

```go
func (s *gcsSSHKeysObjectStore) WriteObject(
ctx context.Context, bucket, object string, contents []byte,
) (retErr error) {
writer := s.client.Bucket(bucket).Object(object).NewWriter(ctx)
defer func() { retErr = errors.CombineErrors(retErr, writer.Close()) }()
_, retErr = writer.Write(contents)
return retErr
}
```

### Capacity retry and regional subnet mappings

The `--gce-subnets` plumbing applies a region-to-subnet map, but the capacity retry planner can select provider-suggested or default zones without filtering them against that map.

A retry can therefore select a zone in a region with no configured subnet. The subsequent create fails before resource creation, but the retry is invalid and wastes an attempt.

Expected behavior: constrain retry candidates to configured subnet regions, or disable topology-changing retries when the subnet topology is explicit.

The subnet override stores the configured map on provider options:

```go
gceOpts.Subnets = maps.Clone(subnets)
```

The retry planner chooses provider hints and default candidates without consulting that map:

```go
for _, zone := range capacityErr.SuggestedZones {
if _, ok := p.attemptedZones[zone]; !ok {
return []string{zone}, "provider hint"
}
}
for _, zone := range p.retryCandidates {
if _, ok := p.attemptedZones[zone]; !ok {
return []string{zone}, "untried default zones"
}
}
```

### Related existing tracking

- Private MIG IAP metadata loss is tracked by #173316.
- Classic roachtest private endpoint compatibility, including Jaeger, is tracked by #173116.
- Startup, certificate, and DNS address-mode behavior is tracked by #173115.

## Scope exclusions

This issue does not reopen or duplicate inherited defects such as name-wide cleanup races, SSH setup error handling, duplicate `known_hosts` entries, or certificate-SAN behavior. It also does not treat centralized public-DNS omission of private-only VMs as a defect; that is intentional for the current public-DNS contract.

## Acceptance criteria

- GCS shared-key updates are concurrency-safe and covered by a focused test.
- Capacity retries cannot select a zone whose region lacks a configured subnet.
- Tests cover both failures without creating cloud resources.
- The fixes preserve existing public GCE behavior.
- PR #3365 and the related tracking issues are updated with implementation and test links.

Contributor guide

Open the contributing guide

Research direction

Start at AddUserAuthorizedKey and gcsSSHKeysObjectStore.WriteObject to trace the shared-key read-modify-write, then inspect gceOpts.Subnets and the capacity retry planner. Add focused tests for concurrent GCS updates and subnet-aware retry selection without creating cloud resources. Done means both defects are prevented while existing public GCE behavior remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, google-cloud
Domain
cli, cloud, infrastructure, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.