From 02fcbcd8bd9159f555557a191ac8a010d73bd4a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ervin=20R=C3=A1cz?= Date: Mon, 12 Jan 2026 22:42:59 +0200 Subject: [PATCH] fix(syncer-updates): separate target from floors in update logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: Syncer received batches with the target even if there were still floor packages waiting to be sent. The target would have been used by the next syncer omaha request and that prevented preparing the remaining floors to be sent. - Update GetUpdatePackagesForSyncer to handle floors and target separately - Fix tests for multi-step-updates floor pagination for syncers - Modify update selection to prioritize floors before target package - Add LIMIT+1 query pattern for floor pagination detection - Update architecture decisions doc Signed-off-by: Ervin Rácz --- backend/pkg/api/packages.go | 2 +- backend/pkg/api/packages_floors.go | 36 ++- backend/pkg/api/packages_floors_test.go | 9 +- backend/pkg/api/updates.go | 127 +++++++--- backend/pkg/omaha/helpers_test.go | 31 +++ backend/pkg/omaha/omaha.go | 59 ++--- backend/pkg/omaha/omaha_floors_test.go | 219 ++++++++++-------- backend/pkg/syncer/syncer.go | 20 +- .../pkg/syncer/syncer_multimanifest_test.go | 153 ++++++++++++ docs/ARD-001-multi-step-updates.md | 82 ------- docs/architecture-decisions.md | 107 ++++++++- 11 files changed, 578 insertions(+), 267 deletions(-) delete mode 100644 docs/ARD-001-multi-step-updates.md diff --git a/backend/pkg/api/packages.go b/backend/pkg/api/packages.go index 4820d6ac1..b0dd89fc9 100644 --- a/backend/pkg/api/packages.go +++ b/backend/pkg/api/packages.go @@ -382,7 +382,7 @@ func (api *API) DeletePackage(pkgID string) error { var exists bool var isFloor bool err = api.db.QueryRow(` - SELECT + SELECT EXISTS(SELECT 1 FROM package WHERE id = $1), EXISTS(SELECT 1 FROM channel_package_floors WHERE package_id = $1) `, pkgID).Scan(&exists, &isFloor) diff --git a/backend/pkg/api/packages_floors.go b/backend/pkg/api/packages_floors.go index a01875fb1..fce12354a 100644 --- a/backend/pkg/api/packages_floors.go +++ b/backend/pkg/api/packages_floors.go @@ -226,13 +226,16 @@ const ( DefaultMaxFloorsPerResponse = 5 ) -// GetRequiredChannelFloors returns floor packages between instance and target versions for a channel -func (api *API) GetRequiredChannelFloors(channel *Channel, instanceVersion string) ([]*Package, error) { +// GetRequiredChannelFloorsWithLimit returns floor packages between instance and target versions, +// along with a boolean indicating if more floors remain beyond the limit. +// This uses LIMIT+1 approach: query for limit+1 rows, if we get more than limit, there are more. +// This is more efficient than a separate COUNT query. +func (api *API) GetRequiredChannelFloorsWithLimit(channel *Channel, instanceVersion string) ([]*Package, bool, error) { if channel == nil || channel.Package == nil { - return nil, ErrNoPackageFound + return nil, false, ErrNoPackageFound } if instanceVersion == "" { - return nil, fmt.Errorf("instance version cannot be empty") + return nil, false, fmt.Errorf("instance version cannot be empty") } targetVersion := channel.Package.Version @@ -245,19 +248,20 @@ func (api *API) GetRequiredChannelFloors(channel *Channel, instanceVersion strin // No blacklist check needed for floors gtExpr, err := versionCompareExpr("p.version", ">", instanceVersion) if err != nil { - return nil, err + return nil, false, err } lteExpr, err := versionCompareExpr("p.version", "<=", targetVersion) if err != nil { - return nil, err + return nil, false, err } semverExpr, err := semverToIntArray("p.version") if err != nil { - return nil, err + return nil, false, err } + // Query for LIMIT+1 to detect if more floors exist query, _, err := goqu.From(goqu.L(` package p JOIN channel_package_floors cpf ON p.id = cpf.package_id @@ -273,14 +277,26 @@ func (api *API) GetRequiredChannelFloors(channel *Channel, instanceVersion strin lteExpr, )). Order(goqu.L(semverExpr).Asc()). - Limit(uint(maxFloorsPerResponse)). + Limit(uint(maxFloorsPerResponse + 1)). ToSQL() if err != nil { - return nil, err + return nil, false, err } - return api.getPackagesFromQuery(query) + floors, err := api.getPackagesFromQuery(query) + if err != nil { + return nil, false, err + } + + // If we got more than the limit, there are more floors remaining + if len(floors) > maxFloorsPerResponse { + // Return only up to the limit, indicate more remain + return floors[:maxFloorsPerResponse], true, nil + } + + // All floors returned + return floors, false, nil } // GetChannelFloorPackagesCount returns the count of floor packages for a channel diff --git a/backend/pkg/api/packages_floors_test.go b/backend/pkg/api/packages_floors_test.go index 52e550099..d975c2e80 100644 --- a/backend/pkg/api/packages_floors_test.go +++ b/backend/pkg/api/packages_floors_test.go @@ -55,7 +55,7 @@ func TestFloorOperations(t *testing.T) { for instance, expected := range testCases { ch, err := a.GetChannel(setup.Channel.ID) assert.NoError(t, err) - floors, err := a.GetRequiredChannelFloors(ch, instance) + floors, _, err := a.GetRequiredChannelFloorsWithLimit(ch, instance) assert.NoError(t, err) assert.Len(t, floors, expected, "instance %s", instance) } @@ -84,9 +84,10 @@ func TestFloorMaxLimit(t *testing.T) { // Should only get 3 floors due to limit ch, err := a.GetChannel(setup.Channel.ID) assert.NoError(t, err) - floors, err := a.GetRequiredChannelFloors(ch, "0.0.0") + floors, hasMore, err := a.GetRequiredChannelFloorsWithLimit(ch, "0.0.0") assert.NoError(t, err) assert.Len(t, floors, 3) + assert.True(t, hasMore, "Should indicate more floors remain beyond limit") } // TestFloorPagination tests paginated floor retrieval @@ -133,7 +134,7 @@ func TestNonStandardVersions(t *testing.T) { for instance, expected := range testCases { ch, err := a.GetChannel(setup.Channel.ID) assert.NoError(t, err) - floors, err := a.GetRequiredChannelFloors(ch, instance) + floors, _, err := a.GetRequiredChannelFloorsWithLimit(ch, instance) assert.NoError(t, err) assert.Len(t, floors, expected, "instance %s", instance) } @@ -232,7 +233,7 @@ func TestTargetAsFloor(t *testing.T) { for instance, expected := range testCases { ch, err := a.GetChannel(setup.Channel.ID) assert.NoError(t, err) - floors, err := a.GetRequiredChannelFloors(ch, instance) + floors, _, err := a.GetRequiredChannelFloorsWithLimit(ch, instance) assert.NoError(t, err) assert.Len(t, floors, expected.expectedCount, "instance %s", instance) if expected.expectedCount > 0 { diff --git a/backend/pkg/api/updates.go b/backend/pkg/api/updates.go index 808b3da6f..d923b3513 100644 --- a/backend/pkg/api/updates.go +++ b/backend/pkg/api/updates.go @@ -120,12 +120,15 @@ func (api *API) GetUpdatePackage(inst Instance, instApp InstanceApplication) (*P // Instance hasn't reached granted version yet - return what's next to install // This will be the first floor/target above current instance version - packages, err := api.getPackagesWithFloorsForUpdate(group, instanceVersion) + floors, target, err := api.getPackagesWithFloorsForUpdate(group, instanceVersion) if err != nil { return nil, err } - // packages[0] should be the granted version since instance < granted - return packages[0], nil + // Return first floor if any, otherwise target + if len(floors) > 0 { + return floors[0], nil + } + return target, nil } // No granted version tracked (old instances) - safer fallback @@ -145,15 +148,22 @@ func (api *API) GetUpdatePackage(inst Instance, instApp InstanceApplication) (*P return nil, ErrNoUpdatePackageAvailable } - packages, err := api.getPackagesWithFloorsForUpdate(group, instanceVersion) + floors, target, err := api.getPackagesWithFloorsForUpdate(group, instanceVersion) if err != nil { return nil, err } + // Determine the next package to return (first floor or target) + var nextPkg *Package + if len(floors) > 0 { + nextPkg = floors[0] + } else { + nextPkg = target + } + // Safety check: verify the next package isn't blacklisted for this channel // This should never happen (floors/targets can't be blacklisted for their own channel) // but we check anyway for data consistency - nextPkg := packages[0] if slices.Contains(nextPkg.ChannelsBlacklist, group.Channel.ID) { l.Error().Str("package", nextPkg.Version).Str("channel", group.Channel.ID). Msg("Package is blacklisted for its own channel - data inconsistency!") @@ -165,7 +175,7 @@ func (api *API) GetUpdatePackage(inst Instance, instApp InstanceApplication) (*P } // Grant the update using the version we're actually returning - version := packages[0].Version + version := nextPkg.Version if err := api.grantUpdate(instance, version); err != nil { l.Error().Err(err).Str("version", version).Str("instance", instance.ID).Msg("GetUpdatePackage - grantUpdate error") return nil, ErrUpdateGrantFailed @@ -190,15 +200,22 @@ func (api *API) GetUpdatePackage(inst Instance, instApp InstanceApplication) (*P } } - return packages[0], nil + return nextPkg, nil } -// GetUpdatePackagesForSyncer returns all packages (floors + target) for a syncer client -func (api *API) GetUpdatePackagesForSyncer(inst Instance, instApp InstanceApplication) ([]*Package, error) { +// GetUpdatePackagesForSyncer returns floor packages and target for a syncer client. +// Returns: +// - floors: Required floor packages (may be empty) +// - target: The target package, or nil if more floors remain beyond the limit +// - error: Any error that occurred +// +// When target is nil, the syncer should request again with the highest floor version. +// When target is not nil, all required floors have been sent and the channel can be updated. +func (api *API) GetUpdatePackagesForSyncer(inst Instance, instApp InstanceApplication) ([]*Package, *Package, error) { instance, err := api.RegisterInstance(inst, instApp) if err != nil { l.Error().Err(err).Msg("GetUpdatePackagesForSyncer - could not register instance") - return nil, ErrRegisterInstanceFailed + return nil, nil, ErrRegisterInstanceFailed } instanceVersion := instApp.Version @@ -208,65 +225,78 @@ func (api *API) GetUpdatePackagesForSyncer(inst Instance, instApp InstanceApplic if instance.Application.Status.Valid { switch int(instance.Application.Status.Int64) { case InstanceStatusDownloading, InstanceStatusDownloaded, InstanceStatusInstalled: - return nil, ErrUpdateInProgressOnInstance + return nil, nil, ErrUpdateInProgressOnInstance } } group, err := api.GetGroup(groupID) if err != nil { - return nil, err + return nil, nil, err } if group.Channel == nil || group.Channel.Package == nil { if err := api.newGroupActivityEntry(activityPackageNotFound, activityWarning, "0.0.0", appID, groupID); err != nil { l.Error().Err(err).Msg("GetUpdatePackagesForSyncer - could not add new group activity entry") } - return nil, ErrNoPackageFound + return nil, nil, ErrNoPackageFound } // Check if update is needed instanceSemver, _ := semver.Make(instanceVersion) packageSemver, _ := semver.Make(group.Channel.Package.Version) if !instanceSemver.LT(packageSemver) { - return nil, ErrNoUpdatePackageAvailable + return nil, nil, ErrNoUpdatePackageAvailable } - packages, err := api.getPackagesWithFloorsForUpdate(group, instanceVersion) + floors, target, err := api.getPackagesWithFloorsForUpdate(group, instanceVersion) if err != nil { - return nil, err + return nil, nil, err } // Safety check: verify no packages are blacklisted for this channel // Syncers need all packages, so if any is blacklisted we can't send a valid manifest // This should never happen (floors/targets can't be blacklisted for their own channel) // but we check anyway for data consistency - for _, pkg := range packages { + for _, pkg := range floors { if slices.Contains(pkg.ChannelsBlacklist, group.Channel.ID) { l.Error().Str("package", pkg.Version).Str("channel", group.Channel.ID). Msg("Package is blacklisted for its own channel - data inconsistency!") - return nil, ErrNoUpdatePackageAvailable + return nil, nil, ErrNoUpdatePackageAvailable } } + if target != nil && slices.Contains(target.ChannelsBlacklist, group.Channel.ID) { + l.Error().Str("package", target.Version).Str("channel", group.Channel.ID). + Msg("Package is blacklisted for its own channel - data inconsistency!") + return nil, nil, ErrNoUpdatePackageAvailable + } if err := api.enforceRolloutPolicy(instance, group); err != nil { - return nil, err + return nil, nil, err } - // Grant the update using target version - targetVersion := packages[len(packages)-1].Version - if err := api.grantUpdate(instance, targetVersion); err != nil { - l.Error().Err(err).Str("version", targetVersion).Str("instance", instance.ID).Msg("GetUpdatePackagesForSyncer - grantUpdate error") - return nil, ErrUpdateGrantFailed + // Grant the update using the highest version we're returning + // (last floor if no target, or target if present) + var grantVersion string + if target != nil { + grantVersion = target.Version + } else if len(floors) > 0 { + grantVersion = floors[len(floors)-1].Version + } else { + return nil, nil, ErrNoUpdatePackageAvailable + } + if err := api.grantUpdate(instance, grantVersion); err != nil { + l.Error().Err(err).Str("version", grantVersion).Str("instance", instance.ID).Msg("GetUpdatePackagesForSyncer - grantUpdate error") + return nil, nil, ErrUpdateGrantFailed } // Record activity if !api.hasRecentActivity(activityRolloutStarted, ActivityQueryParams{ Severity: activityInfo, AppID: appID, - Version: targetVersion, + Version: grantVersion, GroupID: groupID, }) { - if err := api.newGroupActivityEntry(activityRolloutStarted, activityInfo, targetVersion, appID, groupID); err != nil { + if err := api.newGroupActivityEntry(activityRolloutStarted, activityInfo, grantVersion, appID, groupID); err != nil { l.Error().Err(err).Msg("GetUpdatePackagesForSyncer - could not add new group activity entry") } } @@ -278,7 +308,7 @@ func (api *API) GetUpdatePackagesForSyncer(inst Instance, instApp InstanceApplic } } - return packages, nil + return floors, target, nil } // enforceRolloutPolicy validates if an update should be provided to the @@ -363,29 +393,50 @@ func inOfficeHoursNow(tz string) bool { return true } -// getPackagesWithFloorsForUpdate returns floors + target for the given group and instance version -// This is a helper method extracted from the UpdateHandler logic -func (api *API) getPackagesWithFloorsForUpdate(group *Group, instanceVersion string) ([]*Package, error) { +// getPackagesWithFloorsForUpdate returns floors and target for the given group and instance version. +// This is a helper method extracted from the UpdateHandler logic. +// +// Returns: +// - floors: Required floor packages between instance version and target (may be empty) +// - target: The target package, or nil if more floors remain beyond the limit +// +// IMPORTANT: When there are more floors remaining than NEBRASKA_MAX_FLOORS_PER_RESPONSE, +// target will be nil. This signals that the syncer should request again with the highest +// floor version to get remaining floors. +func (api *API) getPackagesWithFloorsForUpdate(group *Group, instanceVersion string) (floors []*Package, target *Package, err error) { if group.Channel == nil || group.Channel.Package == nil { - return nil, ErrNoPackageFound + return nil, nil, ErrNoPackageFound } - // Get required floors using the channel - requiredFloors, err := api.GetRequiredChannelFloors( + // Get required floors using LIMIT+1 to detect if more floors remain + // This is more efficient than a separate COUNT query + requiredFloors, hasMoreFloors, err := api.GetRequiredChannelFloorsWithLimit( group.Channel, instanceVersion, ) if err != nil { - return nil, err + return nil, nil, err } targetPkg := group.Channel.Package + // Check if target is already included (when target is also a floor) - if len(requiredFloors) > 0 && requiredFloors[len(requiredFloors)-1].ID == targetPkg.ID { - return requiredFloors, nil + var lastFloor *Package + if len(requiredFloors) > 0 { + lastFloor = requiredFloors[len(requiredFloors)-1] + } + targetIsLastFloor := lastFloor != nil && lastFloor.ID == targetPkg.ID + if targetIsLastFloor { + return requiredFloors, lastFloor, nil + } + + // If more floors remain, don't include target yet + // Syncer will request again with highest floor version + if hasMoreFloors { + return requiredFloors, nil, nil } - // Append target if not already included - return append(requiredFloors, targetPkg), nil + // All floors sent (or no floors) - include target + return requiredFloors, targetPkg, nil } diff --git a/backend/pkg/omaha/helpers_test.go b/backend/pkg/omaha/helpers_test.go index 6590919e2..219eecad6 100644 --- a/backend/pkg/omaha/helpers_test.go +++ b/backend/pkg/omaha/helpers_test.go @@ -1,8 +1,11 @@ package omaha import ( + "bytes" + "encoding/xml" "testing" + omahaSpec "github.com/flatcar/go-omaha/omaha" "github.com/stretchr/testify/require" "gopkg.in/guregu/null.v4" @@ -68,3 +71,31 @@ func setupOmahaFloorTest(t *testing.T, a *api.API, name string, floorVersions [] return group, pkgs } + +// doSyncerRequest sends a syncer Omaha request and returns the response +func doSyncerRequest(t *testing.T, h *Handler, version, groupID string, multiManifestOK bool) *omahaSpec.Response { + t.Helper() + + req := omahaSpec.NewRequest() + req.OS.Version = "3" + req.OS.Platform = "CoreOS" + req.OS.ServicePack = "linux" + req.OS.Arch = "x64" + req.Version = "CoreOSUpdateEngine-0.1.0.0" + req.InstallSource = "scheduler" + app := req.AddApp(flatcarAppID, version) + app.MachineID = "syncer-" + version + app.Track = groupID + app.AddUpdateCheck() + app.MultiManifestOK = multiManifestOK + + buf := bytes.NewBuffer(nil) + require.NoError(t, xml.NewEncoder(buf).Encode(req)) + + respBuf := bytes.NewBuffer(nil) + require.NoError(t, h.Handle(buf, respBuf, "10.0.0.1")) + + var resp omahaSpec.Response + require.NoError(t, xml.NewDecoder(respBuf).Decode(&resp)) + return &resp +} diff --git a/backend/pkg/omaha/omaha.go b/backend/pkg/omaha/omaha.go index 34901a8a3..b8c1e82de 100644 --- a/backend/pkg/omaha/omaha.go +++ b/backend/pkg/omaha/omaha.go @@ -165,8 +165,9 @@ func (h *Handler) buildOmahaResponse(omahaReq *omahaSpec.Request, ip string) (*o if reqApp.UpdateCheck != nil { if isSyncerClient(omahaReq) { - // Syncer - get all packages - packages, err := h.crAPI.GetUpdatePackagesForSyncer(inst, instApp) + // Syncer - get floors and target separately + // target is nil when more floors remain beyond NEBRASKA_MAX_FLOORS_PER_RESPONSE limit + floors, target, err := h.crAPI.GetUpdatePackagesForSyncer(inst, instApp) if err != nil { if err == api.ErrNoUpdatePackageAvailable || err == api.ErrUpdateGrantFailed { respApp.AddUpdateCheck(omahaSpec.NoUpdate) @@ -177,32 +178,23 @@ func (h *Handler) buildOmahaResponse(omahaReq *omahaSpec.Request, ip string) (*o continue } - // Check if we got any packages - if len(packages) == 0 { + // Check if we got anything to send + if len(floors) == 0 && target == nil { respApp.AddUpdateCheck(omahaSpec.NoUpdate) continue } // Critical safety rule: old syncers without MultiManifestOK cannot skip floors - // Check if ANY package is a floor (including the target which might also be a floor) - hasFloors := false - for _, pkg := range packages { - if pkg.IsFloor { - hasFloors = true - break - } - } - - if hasFloors && !reqApp.MultiManifestOK { + if len(floors) > 0 && !reqApp.MultiManifestOK { l.Warn().Str("instanceID", reqApp.MachineID). - Int("packageCount", len(packages)). + Int("floorCount", len(floors)). Msg("Syncer without multi-manifest support blocked due to floor requirements") respApp.AddUpdateCheck(omahaSpec.NoUpdate) continue } // Either multi-manifest capable syncer or no floors exist - h.prepareMultiManifestUpdateCheck(respApp, packages) + h.prepareMultiManifestUpdateCheck(respApp, floors, target) } else { // Regular client - get single package pkg, err := h.crAPI.GetUpdatePackage(inst, instApp) @@ -346,25 +338,35 @@ func (h *Handler) prepareUpdateCheck(appResp *omahaSpec.AppResponse, pkg *api.Pa updateCheck.AddURL(pkg.URL) } -// prepareMultiManifestUpdateCheck creates a response with multiple manifests -// Each package gets one manifest with appropriate floor/target metadata based on its properties -func (h *Handler) prepareMultiManifestUpdateCheck(appResp *omahaSpec.AppResponse, packages []*api.Package) { - if len(packages) == 0 { +// prepareMultiManifestUpdateCheck creates a response with multiple manifests. +// When target is nil, more floors remain and syncer should request again. +func (h *Handler) prepareMultiManifestUpdateCheck(appResp *omahaSpec.AppResponse, floors []*api.Package, target *api.Package) { + if len(floors) == 0 && target == nil { appResp.AddUpdateCheck(omahaSpec.NoUpdate) return } - // The last package in the array is the target (by convention from GetUpdatePackagesForSyncer) - targetPkg := packages[len(packages)-1] + targetAlreadyInFloors := target != nil && len(floors) > 0 && floors[len(floors)-1].ID == target.ID + + var packages []*api.Package + packages = append(packages, floors...) + if target != nil && !targetAlreadyInFloors { + packages = append(packages, target) + } + + var codeBase string + if target != nil { + codeBase = target.URL + } else { + codeBase = packages[len(packages)-1].URL + } updateCheck := appResp.AddUpdateCheck(omahaSpec.UpdateOK) - updateCheck.AddURL(targetPkg.URL) + updateCheck.AddURL(codeBase) - // Create manifest for each package with appropriate flags - for i, pkg := range packages { + for _, pkg := range packages { manifest := updateCheck.AddManifest(pkg.Version) - // Set IsFloor if package has floor metadata if pkg.IsFloor { manifest.IsFloor = true if pkg.FloorReason.Valid { @@ -374,9 +376,8 @@ func (h *Handler) prepareMultiManifestUpdateCheck(appResp *omahaSpec.AppResponse } } - // Set IsTarget if this is the last package (the target) - // Note: A package can have both IsFloor and IsTarget flags - if i == len(packages)-1 { + isTarget := target != nil && pkg.ID == target.ID + if isTarget { manifest.IsTarget = true } diff --git a/backend/pkg/omaha/omaha_floors_test.go b/backend/pkg/omaha/omaha_floors_test.go index 61f3e4843..866c66328 100644 --- a/backend/pkg/omaha/omaha_floors_test.go +++ b/backend/pkg/omaha/omaha_floors_test.go @@ -3,6 +3,7 @@ package omaha import ( "bytes" "encoding/xml" + "os" "testing" omahaSpec "github.com/flatcar/go-omaha/omaha" @@ -11,159 +12,116 @@ import ( "gopkg.in/guregu/null.v4" ) -// TestFloorUpdateScenarios tests all floor-based update scenarios func TestFloorUpdateScenarios(t *testing.T) { a := newForTest(t) defer a.Close() h := NewHandler(a) - // Helper for syncer requests - syncerRequest := func(h *Handler, version, group string, multiManifestOK bool) *omahaSpec.Response { - req := omahaSpec.NewRequest() - req.OS.Version = "3" - req.OS.Platform = "CoreOS" - req.OS.ServicePack = "linux" - req.OS.Arch = "x64" - req.Version = "CoreOSUpdateEngine-0.1.0.0" - req.InstallSource = "scheduler" - app := req.AddApp(flatcarAppID, version) - app.MachineID = "syncer-" + version - app.Track = group - app.AddUpdateCheck() - app.MultiManifestOK = multiManifestOK - - buf := bytes.NewBuffer(nil) - err := xml.NewEncoder(buf).Encode(req) - if err != nil { - t.Fatalf("Failed to encode request: %v", err) - } - respBuf := bytes.NewBuffer(nil) - err = h.Handle(buf, respBuf, "10.0.0.1") - if err != nil { - t.Fatalf("Failed to handle request: %v", err) - } - var resp omahaSpec.Response - err = xml.NewDecoder(respBuf).Decode(&resp) - if err != nil { - t.Fatalf("Failed to decode response: %v", err) - } - return &resp - } - - t.Run("RegularClientWithUpdate", func(t *testing.T) { - group, pkgs := setupOmahaFloorTest(t, a, "regular", []string{"2000.0.0", "2500.0.0"}, "3000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 3) + // RegularClient: Tests that regular Flatcar clients receive ONE package at a time + // Setup: floors [2000, 2500], target 3000 + // Regular clients progress through floors sequentially with reboots between each step + t.Run("RegularClient", func(t *testing.T) { + group, _ := setupOmahaFloorTest(t, a, "regular", []string{"2000.0.0", "2500.0.0"}, "3000.0.0") tests := []struct { instance, expected string }{ - {"1500.0.0", "2000.0.0"}, // below floors -> floor1 - {"2000.0.0", "2500.0.0"}, // at floor1 -> floor2 - {"2200.0.0", "2500.0.0"}, // between floors -> floor2 - {"2500.0.0", "3000.0.0"}, // at floor2 -> target - {"2700.0.0", "3000.0.0"}, // above floors -> target + {"1500.0.0", "2000.0.0"}, // below all floors -> gets first floor + {"2000.0.0", "2500.0.0"}, // at first floor -> gets second floor + {"2200.0.0", "2500.0.0"}, // between floors -> gets next floor above + {"2500.0.0", "3000.0.0"}, // at last floor -> gets target + {"2700.0.0", "3000.0.0"}, // above floors but below target -> gets target } - for _, tc := range tests { resp := doOmahaRequest(t, h, flatcarAppID, tc.instance, "client-"+tc.instance, group.ID, "10.0.0.1", false, true, nil) - require.NotNil(t, resp) - filename := "flatcar_" + tc.expected + ".gz" - url := "http://sample.url/" + tc.expected - checkOmahaUpdateResponse(t, resp, tc.expected, filename, url, omahaSpec.UpdateOK) + checkOmahaUpdateResponse(t, resp, tc.expected, "flatcar_"+tc.expected+".gz", "http://sample.url/"+tc.expected, omahaSpec.UpdateOK) assert.Len(t, resp.Apps[0].UpdateCheck.Manifests, 1) } }) + // RegularClientNoUpdate: Client already at target version gets NoUpdate t.Run("RegularClientNoUpdate", func(t *testing.T) { - group, pkgs := setupOmahaFloorTest(t, a, "noupdate", []string{}, "5000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 1) + group, _ := setupOmahaFloorTest(t, a, "noupdate", []string{}, "5000.0.0") resp := doOmahaRequest(t, h, flatcarAppID, "5000.0.0", "client", group.ID, "10.0.0.1", false, true, nil) checkOmahaUpdateResponse(t, resp, "", "", "", omahaSpec.NoUpdate) }) + // SyncerMultiManifest: Modern syncers (MultiManifestOK=true) receive ALL packages in one response + // Setup: floors [9000, 9500], target 10000 + // Unlike regular clients, syncers get floors + target together to sync them all at once t.Run("SyncerMultiManifest", func(t *testing.T) { - group, pkgs := setupOmahaFloorTest(t, a, "syncer", []string{"9000.0.0", "9500.0.0"}, "10000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 3) - - testCases := map[string]int{ - "8500.0.0": 3, // below floors: all manifests - "9200.0.0": 2, // between: floor2 + target - "9700.0.0": 1, // above: target only - } + group, _ := setupOmahaFloorTest(t, a, "syncer", []string{"9000.0.0", "9500.0.0"}, "10000.0.0") - for version, expectedManifests := range testCases { - resp := syncerRequest(h, version, group.ID, true) - assert.Len(t, resp.Apps[0].UpdateCheck.Manifests, expectedManifests) - // Verify last is target - last := resp.Apps[0].UpdateCheck.Manifests[expectedManifests-1] - assert.True(t, last.IsTarget) - // Verify others are floors - for i := 0; i < expectedManifests-1; i++ { + tests := []struct { + version string + expectedManifests int + }{ + {"8500.0.0", 3}, // below all -> gets 2 floors + target + {"9200.0.0", 2}, // between floors -> gets 1 floor + target + {"9700.0.0", 1}, // above floors -> gets only target + } + for _, tc := range tests { + resp := doSyncerRequest(t, h, tc.version, group.ID, true) + require.Len(t, resp.Apps[0].UpdateCheck.Manifests, tc.expectedManifests) + assert.True(t, resp.Apps[0].UpdateCheck.Manifests[tc.expectedManifests-1].IsTarget) + for i := 0; i < tc.expectedManifests-1; i++ { assert.True(t, resp.Apps[0].UpdateCheck.Manifests[i].IsFloor) } } }) - t.Run("OldSyncerBlocked", func(t *testing.T) { - group, pkgs := setupOmahaFloorTest(t, a, "oldsyncer", []string{"6000.0.0"}, "7000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 2) - resp := syncerRequest(h, "1500.0.0", group.ID, false) // multiPkgOK=false + // LegacySyncerBlocked: Old syncers without MultiManifestOK get NoUpdate when floors exist + // This prevents legacy syncers from skipping mandatory floor versions + t.Run("LegacySyncerBlocked", func(t *testing.T) { + group, _ := setupOmahaFloorTest(t, a, "oldsyncer", []string{"6000.0.0"}, "7000.0.0") + resp := doSyncerRequest(t, h, "1500.0.0", group.ID, false) // multiManifestOK=false checkOmahaUpdateResponse(t, resp, "", "", "", omahaSpec.NoUpdate) }) + // ModernSyncerNoUpdate: Modern syncer already at target version gets NoUpdate t.Run("ModernSyncerNoUpdate", func(t *testing.T) { - group, pkgs := setupOmahaFloorTest(t, a, "syncernoup", []string{}, "8000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 1) - resp := syncerRequest(h, "8000.0.0", group.ID, true) // At target version + group, _ := setupOmahaFloorTest(t, a, "syncernoup", []string{}, "8000.0.0") + resp := doSyncerRequest(t, h, "8000.0.0", group.ID, true) // at target version checkOmahaUpdateResponse(t, resp, "", "", "", omahaSpec.NoUpdate) }) + // NoFloors: When no floors are configured, both clients and syncers get direct update to target t.Run("NoFloors", func(t *testing.T) { - // Setup without floors - group, pkgs := setupOmahaFloorTest(t, a, "nofloor", []string{}, "4000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 1) + group, _ := setupOmahaFloorTest(t, a, "nofloor", []string{}, "4000.0.0") // Regular client gets direct update resp := doOmahaRequest(t, h, flatcarAppID, "1000.0.0", "client", group.ID, "10.0.0.1", false, true, nil) checkOmahaUpdateResponse(t, resp, "4000.0.0", "flatcar_4000.0.0.gz", "http://sample.url/4000.0.0", omahaSpec.UpdateOK) - // Syncer gets single manifest - resp = syncerRequest(h, "1000.0.0", group.ID, true) - assert.Len(t, resp.Apps[0].UpdateCheck.Manifests, 1) + // Syncer gets single manifest with IsTarget=true + resp = doSyncerRequest(t, h, "1000.0.0", group.ID, true) + require.Len(t, resp.Apps[0].UpdateCheck.Manifests, 1) assert.True(t, resp.Apps[0].UpdateCheck.Manifests[0].IsTarget) }) + // TargetAsFloor: Target package can also be marked as a floor + // Setup: floors [11000, 12000], target 13000, then mark 13000 as floor too + // Regular clients progress through floors including the target-floor + // Syncers receive all manifests with the last one having BOTH IsFloor=true AND IsTarget=true t.Run("TargetAsFloor", func(t *testing.T) { - // Setup where target is also a floor (critical mandatory version) group, pkgs := setupOmahaFloorTest(t, a, "targetfloor", []string{"11000.0.0", "12000.0.0"}, "13000.0.0") - require.NotNil(t, group) - require.Len(t, pkgs, 3) + // Mark target as also being a floor (critical version that must be installed) + require.NoError(t, a.AddChannelPackageFloor(group.ChannelID.String, pkgs[2].ID, null.StringFrom("Critical mandatory version"))) - // Mark the target as ALSO being a floor - err := a.AddChannelPackageFloor(group.ChannelID.String, pkgs[2].ID, - null.StringFrom("Critical mandatory version")) - require.NoError(t, err) - - // Regular client below all versions + // Regular client below all versions -> gets first floor resp := doOmahaRequest(t, h, flatcarAppID, "10000.0.0", "client-low", group.ID, "10.0.0.1", false, true, nil) checkOmahaUpdateResponse(t, resp, "11000.0.0", "flatcar_11000.0.0.gz", "http://sample.url/11000.0.0", omahaSpec.UpdateOK) - // Regular client between floors + // Regular client between floors -> gets second floor resp = doOmahaRequest(t, h, flatcarAppID, "11500.0.0", "client-mid", group.ID, "10.0.0.2", false, true, nil) checkOmahaUpdateResponse(t, resp, "12000.0.0", "flatcar_12000.0.0.gz", "http://sample.url/12000.0.0", omahaSpec.UpdateOK) - // Regular client above regular floors but below target-floor + // Regular client above regular floors but below target-floor -> gets target (which is also a floor) resp = doOmahaRequest(t, h, flatcarAppID, "12500.0.0", "client-high", group.ID, "10.0.0.3", false, true, nil) checkOmahaUpdateResponse(t, resp, "13000.0.0", "flatcar_13000.0.0.gz", "http://sample.url/13000.0.0", omahaSpec.UpdateOK) // Syncer should get all manifests with correct flags - resp = syncerRequest(h, "10000.0.0", group.ID, true) + resp = doSyncerRequest(t, h, "10000.0.0", group.ID, true) require.Len(t, resp.Apps[0].UpdateCheck.Manifests, 3) // First two are floors only @@ -171,7 +129,6 @@ func TestFloorUpdateScenarios(t *testing.T) { assert.False(t, resp.Apps[0].UpdateCheck.Manifests[0].IsTarget) assert.True(t, resp.Apps[0].UpdateCheck.Manifests[1].IsFloor) assert.False(t, resp.Apps[0].UpdateCheck.Manifests[1].IsTarget) - // Last one is BOTH floor AND target assert.True(t, resp.Apps[0].UpdateCheck.Manifests[2].IsFloor) assert.True(t, resp.Apps[0].UpdateCheck.Manifests[2].IsTarget) @@ -179,6 +136,74 @@ func TestFloorUpdateScenarios(t *testing.T) { }) } +func TestFloorLimitPagination(t *testing.T) { + oldMax := os.Getenv("NEBRASKA_MAX_FLOORS_PER_RESPONSE") + defer os.Setenv("NEBRASKA_MAX_FLOORS_PER_RESPONSE", oldMax) + os.Setenv("NEBRASKA_MAX_FLOORS_PER_RESPONSE", "2") + + a := newForTest(t) + defer a.Close() + h := NewHandler(a) + + group, _ := setupOmahaFloorTest(t, a, "floor-limit", + []string{"1000.0.0", "2000.0.0", "3000.0.0", "4000.0.0", "5000.0.0"}, "6000.0.0") + + // Test scenarios for floor limit pagination with limit=2 and floors [1000, 2000, 3000, 4000, 5000] + target 6000 + // + // Scenario 1 (round1): Syncer at 0.0.0 + // - 5 floors remain (1000-5000), exceeds limit of 2 + // - Returns floors [1000, 2000], NO target (hasTarget=false) + // - Syncer should request again with version 2000.0.0 + // + // Scenario 2 (round2): Syncer at 2000.0.0 (after processing round1) + // - 3 floors remain (3000-5000), exceeds limit of 2 + // - Returns floors [3000, 4000], NO target (hasTarget=false) + // - Syncer should request again with version 4000.0.0 + // + // Scenario 3 (round3): Syncer at 4000.0.0 (after processing round2) + // - 1 floor remains (5000), under limit + // - Returns floor [5000] + target [6000] (hasTarget=true) + // - All floors sent, syncer can update channel to target + // + // Scenario 4 (at_limit): Syncer at 3000.0.0 + // - 2 floors remain (4000, 5000), exactly at limit + // - Returns floors [4000, 5000] + target [6000] (hasTarget=true) + // - All floors sent, syncer can update channel to target + tests := []struct { + name string + version string // syncer's current version + expectedCount int // number of manifests in response + expectedFirst string // first manifest version + expectedLast string // last manifest version + hasTarget bool // whether target is included (all floors sent) + }{ + {"round1", "0.0.0", 2, "1000.0.0", "2000.0.0", false}, + {"round2", "2000.0.0", 2, "3000.0.0", "4000.0.0", false}, + {"round3", "4000.0.0", 2, "5000.0.0", "6000.0.0", true}, + {"at_limit", "3000.0.0", 3, "4000.0.0", "6000.0.0", true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp := doSyncerRequest(t, h, tc.version, group.ID, true) + + require.Len(t, resp.Apps[0].UpdateCheck.Manifests, tc.expectedCount) + assert.Equal(t, tc.expectedFirst, resp.Apps[0].UpdateCheck.Manifests[0].Version) + assert.Equal(t, tc.expectedLast, resp.Apps[0].UpdateCheck.Manifests[tc.expectedCount-1].Version) + + for i, m := range resp.Apps[0].UpdateCheck.Manifests { + isLast := i == tc.expectedCount-1 + if tc.hasTarget && isLast { + assert.True(t, m.IsTarget, "last manifest should be target") + } else { + assert.True(t, m.IsFloor, "manifest %d should be floor", i) + assert.False(t, m.IsTarget, "manifest %d should not be target when more floors remain", i) + } + } + }) + } +} + // TestLegacySyncerBlockedWithFloors tests that legacy syncers without MultiManifestOK are blocked when floors exist func TestLegacySyncerBlockedWithFloors(t *testing.T) { a := newForTest(t) diff --git a/backend/pkg/syncer/syncer.go b/backend/pkg/syncer/syncer.go index c1228360e..9b859771e 100644 --- a/backend/pkg/syncer/syncer.go +++ b/backend/pkg/syncer/syncer.go @@ -628,6 +628,9 @@ func (s *Syncer) processMultiManifestUpdate(descriptor channelDescriptor, update } // If still no target found, all manifests are floors - targetVersion remains empty + // Track highest floor version for version reporting when no target is present + var highestFloorVersion string + // Process each manifest in the response for _, manifest := range update.Manifests { version := manifest.Version @@ -650,6 +653,8 @@ func (s *Syncer) processMultiManifestUpdate(descriptor channelDescriptor, update if err := s.markPackageAsFloor(descriptor, pkg, manifest); err != nil { return fmt.Errorf("failed to mark package %s as floor: %w", version, err) } + // Track highest floor version (manifests are ordered ascending) + highestFloorVersion = version } // Track target package if this is the target @@ -662,15 +667,20 @@ func (s *Syncer) processMultiManifestUpdate(descriptor channelDescriptor, update if targetPkg == nil { // All manifests were floors with no target package identified. // This is a VALID scenario where upstream wants to establish mandatory floors - // without changing the current channel target yet (target may come later). - // We've successfully processed and marked all floors, but there's no new - // version to point the channel to, so it remains at its current version. + // without changing the current channel target yet (more floors may remain). + // We update the tracked version to the highest floor so the next sync request + // will fetch remaining floors. + if highestFloorVersion != "" { + s.versions[descriptor] = highestFloorVersion + s.bootIDs[descriptor] = "{" + uuid.New().String() + "}" + } l.Info(). Str("channel", descriptor.name). Str("arch", descriptor.arch.String()). Int("floors_processed", len(update.Manifests)). - Msg("processMultiManifestUpdate - all manifests are floors, channel remains at current version") - return nil // Success - floors processed, just no channel update + Str("highestFloor", highestFloorVersion). + Msg("processMultiManifestUpdate - all manifests are floors, tracking highest floor for next sync") + return nil // Success - floors processed, channel not updated yet } // Update channel to point to the target package diff --git a/backend/pkg/syncer/syncer_multimanifest_test.go b/backend/pkg/syncer/syncer_multimanifest_test.go index a696f1416..ed927ca43 100644 --- a/backend/pkg/syncer/syncer_multimanifest_test.go +++ b/backend/pkg/syncer/syncer_multimanifest_test.go @@ -1,6 +1,7 @@ package syncer import ( + "os" "testing" "github.com/flatcar/go-omaha/omaha" @@ -416,3 +417,155 @@ func TestSyncer_EmptyManifestError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "no manifests") } + +// TestSyncer_FloorLimitVersionTracking tests that when there are more floors than +// NEBRASKA_MAX_FLOORS_PER_RESPONSE, the syncer correctly syncs ALL floors across +// multiple sync rounds. +// +// When there are more floors remaining beyond the limit, the server +// sends ONLY floors (no target). This way: +// - Syncer processes floors and updates channel to highest floor +// - Syncer tracks highest floor version for next request +// - Next request fetches remaining floors +// - Only when all floors are sent does the server include the target +// +// Scenario with limit=2 and 5 floors: +// - Round 1: syncer at 0.0.0 -> server sends floors 1,2 (no target, more floors remain) +// - Round 2: syncer at 2000.0.0 -> server sends floors 3,4 (no target, more floors remain) +// - Round 3: syncer at 4000.0.0 -> server sends floor 5 + target (all floors sent) +func TestSyncer_FloorLimitVersionTracking(t *testing.T) { + // Set a low limit to test pagination behavior + oldMax := os.Getenv("NEBRASKA_MAX_FLOORS_PER_RESPONSE") + defer os.Setenv("NEBRASKA_MAX_FLOORS_PER_RESPONSE", oldMax) + os.Setenv("NEBRASKA_MAX_FLOORS_PER_RESPONSE", "2") + + syncer := newForTest(t, &Config{}) + a := syncer.api + t.Cleanup(func() { a.Close() }) + + tGroup := setupFlatcarAppStableGroup(t, a) + tChannel := tGroup.Channel + require.NoError(t, syncer.initialize()) + + desc := channelDescriptor{name: tChannel.Name, arch: tChannel.Arch} + + // Round 1: Server sends only floors (no target) because more floors remain + // This simulates what upstream Nebraska would send when there are 5 floors but limit is 2 + round1 := &omaha.UpdateResponse{ + Status: "ok", + URLs: []*omaha.URL{{CodeBase: "https://example.com"}}, + Manifests: []*omaha.Manifest{ + { + Version: "1000.0.0", + Packages: []*omaha.Package{{Name: "flatcar-1000.0.0.gz", SHA1: "hash1000", Size: 1000}}, + Actions: []*omaha.Action{{Event: "postinstall", SHA256: "dGVzdHNoYTI1Ng=="}}, + IsFloor: true, + FloorReason: "Floor 1", + }, + { + Version: "2000.0.0", + Packages: []*omaha.Package{{Name: "flatcar-2000.0.0.gz", SHA1: "hash2000", Size: 2000}}, + Actions: []*omaha.Action{{Event: "postinstall", SHA256: "dGVzdHNoYTI1Ng=="}}, + IsFloor: true, + FloorReason: "Floor 2", + }, + // NO TARGET - more floors remain + }, + } + + // Process round 1 + err := syncer.processMultiManifestUpdate(desc, round1) + require.NoError(t, err) + + // After round 1: syncer should track highest floor (2000.0.0) + // Channel should NOT be updated (no target in response) + trackedVersion := syncer.versions[desc] + t.Logf("After round 1, syncer tracked version: %s", trackedVersion) + + // Verify floors 1 and 2 were synced + floors, err := a.GetChannelFloorPackages(tChannel.ID) + require.NoError(t, err) + assert.Len(t, floors, 2, "Should have 2 floors after round 1") + + // Round 2: Server sends next batch of floors (still no target, more remain) + round2 := &omaha.UpdateResponse{ + Status: "ok", + URLs: []*omaha.URL{{CodeBase: "https://example.com"}}, + Manifests: []*omaha.Manifest{ + { + Version: "3000.0.0", + Packages: []*omaha.Package{{Name: "flatcar-3000.0.0.gz", SHA1: "hash3000", Size: 3000}}, + Actions: []*omaha.Action{{Event: "postinstall", SHA256: "dGVzdHNoYTI1Ng=="}}, + IsFloor: true, + FloorReason: "Floor 3", + }, + { + Version: "4000.0.0", + Packages: []*omaha.Package{{Name: "flatcar-4000.0.0.gz", SHA1: "hash4000", Size: 4000}}, + Actions: []*omaha.Action{{Event: "postinstall", SHA256: "dGVzdHNoYTI1Ng=="}}, + IsFloor: true, + FloorReason: "Floor 4", + }, + // NO TARGET - more floors remain + }, + } + + // Process round 2 + err = syncer.processMultiManifestUpdate(desc, round2) + require.NoError(t, err) + + // Verify floors 3 and 4 were synced + floors, err = a.GetChannelFloorPackages(tChannel.ID) + require.NoError(t, err) + assert.Len(t, floors, 4, "Should have 4 floors after round 2") + + // Round 3: Server sends last floor + target (all floors now sent) + round3 := &omaha.UpdateResponse{ + Status: "ok", + URLs: []*omaha.URL{{CodeBase: "https://example.com"}}, + Manifests: []*omaha.Manifest{ + { + Version: "5000.0.0", + Packages: []*omaha.Package{{Name: "flatcar-5000.0.0.gz", SHA1: "hash5000", Size: 5000}}, + Actions: []*omaha.Action{{Event: "postinstall", SHA256: "dGVzdHNoYTI1Ng=="}}, + IsFloor: true, + FloorReason: "Floor 5", + }, + { + Version: "6000.0.0", + Packages: []*omaha.Package{{Name: "flatcar-6000.0.0.gz", SHA1: "hash6000", Size: 6000}}, + Actions: []*omaha.Action{{Event: "postinstall", SHA256: "dGVzdHNoYTI1Ng=="}}, + IsTarget: true, + }, + }, + } + + // Process round 3 + err = syncer.processMultiManifestUpdate(desc, round3) + require.NoError(t, err) + + // Final verification: ALL 5 floors should be synced + floors, err = a.GetChannelFloorPackages(tChannel.ID) + require.NoError(t, err) + assert.Len(t, floors, 5, "All 5 floors should be synced after 3 rounds") + + // Verify floor versions + floorVersions := make([]string, len(floors)) + for i, f := range floors { + floorVersions[i] = f.Version + } + assert.Contains(t, floorVersions, "1000.0.0", "Floor 1 should be synced") + assert.Contains(t, floorVersions, "2000.0.0", "Floor 2 should be synced") + assert.Contains(t, floorVersions, "3000.0.0", "Floor 3 should be synced") + assert.Contains(t, floorVersions, "4000.0.0", "Floor 4 should be synced") + assert.Contains(t, floorVersions, "5000.0.0", "Floor 5 should be synced") + + // Verify channel now points to target (only after all floors sent) + updatedChannel, err := a.GetChannel(tChannel.ID) + require.NoError(t, err) + assert.Equal(t, "6000.0.0", updatedChannel.Package.Version, "Channel should point to target after all floors synced") + + // Verify syncer now tracks target version (ready for next sync cycle) + finalVersion := syncer.versions[desc] + assert.Equal(t, "6000.0.0", finalVersion, "Syncer should track target after all floors synced") +} diff --git a/docs/ARD-001-multi-step-updates.md b/docs/ARD-001-multi-step-updates.md deleted file mode 100644 index 3d0ad0062..000000000 --- a/docs/ARD-001-multi-step-updates.md +++ /dev/null @@ -1,82 +0,0 @@ -# ARD-001: Multi-Step Updates (Floor Packages) - -## Status -Implemented - -## Context -Flatcar requires mandatory intermediate versions (floors) when updating across major versions to prevent compatibility issues and system failures. - -## Decision - -### Core Architecture -- **Channel-specific floors** stored in new `channel_package_floors` junction table with indexes -- **Dynamic metadata**: `IsFloor` and `FloorReason` populated at query time, not stored in package table -- **Reuse existing tracking**: Utilize existing `last_update_version` field for already-granted state management -- **Atomic package creation**: New `AddPackageWithMetadata` API for complete metadata insertion - -### Update Behavior - -#### Regular Clients -- Receive single package (next floor or target) and progress sequentially through floors -- Already-granted instances with `last_update_version` can continue progression without re-evaluation -- NULL `last_update_version` triggers completion to force fresh grant cycle - -#### Syncers (Nebraska instances) -- **Modern syncers with `multi_manifest_ok=true`**: Receive all packages (floors + target) in one response -- **Legacy syncers without `multi_manifest_ok`**: Blocked with `NoUpdate` response when floors exist -- Syncers identified by `InstallSource="scheduler"` in Omaha request - -#### Target Detection (Syncer-specific) -For multi-manifest responses, syncers use this priority: -1. **Explicit**: Manifest with `is_target="true"` attribute -2. **Implicit**: Last manifest that is NOT a floor (backward compatibility) -3. **None**: All manifests are floors (valid - no channel update) - -### Safety Rules & Constraints - -#### Universal Constraints -1. **Floor/Blacklist Mutual Exclusion**: Packages cannot be both floor AND blacklisted for same channel -2. **Channel Target Protection**: Channel's current package cannot be blacklisted -3. **Cross-channel Independence**: Package can be floor for one channel and blacklisted for another - -#### Syncer-specific Constraints -1. **Atomic Floor Operations**: Floor marking failures abort entire update -2. **Package Verification**: Existing packages verified for hash/size match before reuse -3. **Download Cleanup**: Failed downloads cleaned up to prevent orphaned files -4. **Legacy Syncer Safety**: Syncers without multi-manifest support blocked when floors exist - -### API Endpoints - -#### Floor Management -- `POST /api/v1/apps/{app_id}/channels/{channel_id}/packages/{package_id}/floor` - Mark as floor -- `DELETE /api/v1/apps/{app_id}/channels/{channel_id}/packages/{package_id}/floor` - Unmark as floor -- `GET /api/v1/apps/{app_id}/channels/{channel_id}/packages/floors` - List floor packages - -#### Package Response Fields -- `is_floor`: Boolean indicating floor status -- `floor_reason`: Text explanation for floor requirement - -## Consequences - -### Positive -- Safe update paths preventing incompatible version jumps -- Backward compatible with existing single-step updates -- Sequential progression ensures system stability -- Channel-specific flexibility for different update strategies -- Atomic operations prevent partial states - -### Negative -- Legacy syncers blocked when floors present (requires upgrade) -- Additional database queries for floor checking -- Increased complexity in update decision logic - -## Dependencies - -### go-omaha Library -Enhanced go-omaha library with: -- Multi-manifest support (`Manifests` array replacing single `Manifest`) -- Floor attributes (`IsFloor`, `FloorReason`, `IsTarget`) -- `MultiManifestOK` capability flag for syncers - -## References -- [Flatcar Discussion #1831](https://github.com/flatcar/Flatcar/discussions/1831) \ No newline at end of file diff --git a/docs/architecture-decisions.md b/docs/architecture-decisions.md index aa4def051..f969f46d5 100644 --- a/docs/architecture-decisions.md +++ b/docs/architecture-decisions.md @@ -15,6 +15,7 @@ This document captures important architectural decisions made for the Nebraska p **Problem:** OIDC tokens were exposed in server logs via query parameters, creating security vulnerabilities. **Additional Issues:** + - Deprecated password grant authentication - localStorage token storage (XSS vulnerable) - Backend OAuth flow complexity @@ -40,6 +41,7 @@ This document captures important architectural decisions made for the Nebraska p ### Configuration Changes **Removed Flags:** + - `--oidc-client-secret` (public client, no secret needed) - `--oidc-session-secret` (stateless backend) - `--oidc-session-crypt-key` (stateless backend) @@ -64,7 +66,7 @@ For Nebraska's use case as an infrastructure admin tool: **Usage Pattern:** Administrators typically use Nebraska a few times per month for specific maintenance tasks **Session Requirements:** SSO sessions (8-12 hours) exceed typical usage duration **User Experience:** SSO provides seamless re-authentication without manual intervention -**Complexity Trade-off:** Refresh token implementation adds significant complexity for minimal benefit given the usage pattern +**Complexity Trade-off:** Refresh token implementation adds significant complexity for minimal benefit given the usage pattern The OIDC provider's SSO session cookies handle re-authentication transparently, making refresh tokens unnecessary for this low-frequency admin tool use case. Users get the same "stay logged in" experience without the additional implementation and security complexity of refresh token rotation, storage, and revocation mechanisms. @@ -78,3 +80,106 @@ The OIDC provider's SSO session cookies handle re-authentication transparently, - [RFC 7636 - PKCE](https://datatracker.ietf.org/doc/html/rfc7636) - [OAuth 2.0 Security BCP](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics) - [OAuth 2.0 for SPAs](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-browser-based-apps) + +--- + +## ADR-002: Multi-Step Updates with Floor Packages + +**Status**: Implemented +**Date**: 2025-10-27 +**PR**: [#1195 - feat: Implement multi-step updates with floor packages](https://github.com/flatcar/nebraska/pull/1195) +**Discussion**: [Flatcar #1831 - Multi-Step Update Feature Design](https://github.com/flatcar/Flatcar/discussions/1831) + +### Context + +**Problem:** Nebraska only supported single-step updates, where instances jump directly from their current version to the target. This prevents safe rollout of breaking changes that require intermediate migration steps (e.g., filesystem support changes, partition table migrations). + +### Options Considered + +1. **Package-level prerequisites**: Each package declares its required predecessor version +2. **Version range specifications**: Complex patterns like `>=3374.2.0`, `!=3450.0.0`, etc. +3. **Channel-level floor packages**: Mandatory checkpoint versions managed per channel + +### Decision + +**Solution: Channel-level floor packages** + +Floor packages are checkpoint versions that ALL clients must install when updating through a channel. They are managed separately from packages in a dedicated `channel_package_floors` junction table. + +### Why Floor Packages Over Prerequisites + +The package-level prerequisite approach was initially implemented but revealed a critical flaw: + +- If package 3602.2.0 requires 3510.2.0 as prerequisite +- Later, 3815.0.0 is released WITHOUT prerequisites if prerquisit is not inherited +- Clients can jump directly to 3815.0.0, bypassing 3510.2.0 + +This means ALL future packages would need to inherit prerequisites, even for unrelated changes. Floor-based semantics solve this by making checkpoints a channel policy, not a package property. This way we have a central management of update path instead of tracking prerequisits for all later packages individually. + +### Architecture + +**Database**: New `channel_package_floors` table with channel_id, package_id, floor_reason + +**Update Flow**: + +- Regular clients: Receive one package at a time, progress sequentially through floors +- Modern syncers (`multi_manifest_ok=true`): Receive floors in batches (up to `NEBRASKA_MAX_FLOORS_PER_RESPONSE`). When more floors remain beyond the limit, response contains only floors (no target). Syncer requests again with highest floor version until all floors are sent, then target is included. +- Legacy syncers: Blocked with `NoUpdate` when floors exist (the syncer itself must be upgraded) + +**Safety Rules**: + +- Floor/blacklist mutual exclusion (package cannot be both for same channel) +- Channel target cannot be blacklisted +- Architecture must match between floor package and channel + +### Configuration + +**Environment Variables**: + +- `NEBRASKA_MAX_FLOORS_PER_RESPONSE`: Maximum floors per syncer response (default: 5) + +**API Endpoints** (see [API spec](../backend/api/spec.yaml) for details): + +- `PUT /api/channels/{channelID}/floors/{packageID}` - Set floor (idempotent) +- `DELETE /api/channels/{channelID}/floors/{packageID}` - Remove floor +- `GET /api/channels/{channelID}/floors` - List floors for a channel +- `GET /api/apps/{appIDorProductID}/packages/{packageID}/floor-channels` - List channels where package is a floor + +### Trade-offs + +**Benefits**: + +- Consistent update paths regardless of target version +- Simpler management (channel-level, not per-package) +- No prerequisite inheritance burden on new packages +- Clear separation of policy from package identity + +**Limitations**: + +- Legacy syncers blocked when floors exist +- Requires careful timing: configure floors BEFORE channel promotion +- Must configure floors for ALL channels (stable, beta, LTS) strategically + +### Operational Considerations + +**Timing**: Configure floors BEFORE promoting channel to new target. Adding after promotion allows clients to skip floors. + +**Cross-channel**: Don't use beta/alpha packages meant as floors for stable (would switch clients to wrong channel). + +**Use cases**: Breaking compatibility changes only (e.g., filesystem support), NOT security updates. + +### What Was NOT Implemented + +From the original design discussion, the following were deferred or rejected: + +- Complex version specifications (ranges, patterns, exclusions) +- PostgreSQL semver extension +- Recovery mechanisms +- Canary deployment checkbox for bootloader changes +- Emergency bypass mechanisms + +### References + +- [Flatcar Issue #1185 - RFE: Multi-stage updates](https://github.com/flatcar/Flatcar/issues/1185) +- [#1195 - feat: Implement multi-step updates with floor packages](https://github.com/flatcar/nebraska/pull/1195) +- [Flatcar #1831 - Multi-Step Update Feature Design](https://github.com/flatcar/Flatcar/discussions/1831)