cockroachdb / cockroachdb/cockroach
roachprod: track private GCE defects shared by master and release-26.3
- 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
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