Code review fixes.

This commit is contained in:
Victor Lyuboslavsky 2026-04-20 16:40:50 -05:00
parent 33ee0e401a
commit d1b58549b5
No known key found for this signature in database
2 changed files with 69 additions and 8 deletions

View file

@ -50,17 +50,30 @@ func (ds *Datastore) NewAndroidHost(ctx context.Context, host *fleet.AndroidHost
"uuid": host.UUID,
}
var existingID uint
err := sqlx.GetContext(ctx, tx, &existingID,
`SELECT id FROM hosts WHERE uuid = ? AND (platform = 'android' OR platform = '') ORDER BY id LIMIT 1`,
host.UUID,
// Only look up an existing host when we have a non-empty UUID. An empty UUID
// would match every hosts row with uuid='' and falsely dedupe unrelated hosts.
// When multiple rows share this uuid (e.g. one orbit-enrolled with
// node_key=<orbitKey> and one Android with node_key=android/<uuid>), prefer the
// row whose node_key already matches host.NodeKey. Updating that row's node_key
// to itself is a no-op and avoids colliding with the UNIQUE idx_host_unique_nodekey
// constraint that would fire if we picked the other duplicate and tried to flip
// its node_key over to an already-taken value.
var (
existingID uint
foundHost bool
)
notExist := errors.Is(err, sql.ErrNoRows)
if err != nil && !notExist {
return ctxerr.Wrap(ctx, err, "check for existing orbit-enrolled Android host")
if host.UUID != "" {
err := sqlx.GetContext(ctx, tx, &existingID,
`SELECT id FROM hosts WHERE uuid = ? AND platform IN ('android', '') ORDER BY (node_key = ?) DESC, id LIMIT 1`,
host.UUID, host.NodeKey,
)
if err != nil && !errors.Is(err, sql.ErrNoRows) {
return ctxerr.Wrap(ctx, err, "check for existing orbit-enrolled Android host")
}
foundHost = err == nil
}
if notExist {
if !foundHost {
// No orbit-enrolled host for this uuid. Insert as usual.
// We use node_key as a unique identifier for the host table row. It matches: android/{enterpriseSpecificID}.
insertStmt := `

View file

@ -214,6 +214,54 @@ func testNewAndroidHostDedupesOrbitEnrolled(t *testing.T, ds *Datastore) {
require.Equal(t, 1, deviceCount)
})
}
// Two hosts already exist with the same uuid -- one orbit-enrolled
// (node_key=orbitKey) and one Android (node_key=android/<id>). A NewAndroidHost call
// with node_key=android/<id> must pick the Android row (not the orbit-enrolled one),
// otherwise the UPDATE would try to flip the orbit row's node_key to a value already
// held by the Android row and hit idx_host_unique_nodekey.
t.Run("Android orphan duplicates, prefers matching node_key", func(t *testing.T) {
ctx := testCtx()
enterpriseSpecificID := strings.ToUpper(uuid.New().String())
orbitHost, err := ds.EnrollOrbit(ctx,
fleet.WithEnrollOrbitMDMEnabled(true),
fleet.WithEnrollOrbitHostInfo(fleet.OrbitHostInfo{
HardwareUUID: enterpriseSpecificID,
HardwareSerial: enterpriseSpecificID,
Platform: "android",
Hostname: "orbit",
ComputerName: "orbit",
HardwareModel: "TestModel",
}),
fleet.WithEnrollOrbitNodeKey(uuid.New().String()),
)
require.NoError(t, err)
// Insert a second hosts row directly, with the same uuid but the Android-derived
// node_key, to simulate the duplicate state the dedupe must handle.
androidNodeKey := "android/" + enterpriseSpecificID
res, err := ds.writer(ctx).ExecContext(ctx,
`INSERT INTO hosts (node_key, uuid, platform, hostname, computer_name, hardware_serial,
detail_updated_at, label_updated_at, policy_updated_at)
VALUES (?, ?, 'android', 'android-dup', 'android-dup', 'serial-dup', NOW(), NOW(), NOW())`,
androidNodeKey, enterpriseSpecificID,
)
require.NoError(t, err)
androidDupID, err := res.LastInsertId()
require.NoError(t, err)
// NewAndroidHost must pick the existing Android row (not the orbit-enrolled one
// with the lower id). Otherwise the UPDATE would hit the UNIQUE node_key index.
newHost := createAndroidHost(enterpriseSpecificID)
require.Equal(t, androidNodeKey, *newHost.NodeKey,
"createAndroidHost is expected to build node_key=android/<uuid>")
returned, err := ds.NewAndroidHost(ctx, newHost, false)
require.NoError(t, err, "must not violate UNIQUE node_key when duplicate hosts share this uuid")
require.EqualValues(t, androidDupID, returned.Host.ID,
"NewAndroidHost should pick the row whose node_key matches, leaving the orbit-enrolled row alone")
require.NotEqual(t, orbitHost.ID, returned.Host.ID)
})
}
func createAndroidHost(enterpriseSpecificID string) *fleet.AndroidHost {