corrections following comments

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

Author: pbreton

api/pkg/api/handler/allocation.go

@@ -478,15 +478,22 @@ func (cah CreateAllocationHandler) Handle(c echo.Context) error {
 		}
 		ossaDAO := cdbm.NewOperatingSystemSiteAssociationDAO(cah.dbSession)
 		for _, gos := range globalTenantOSes {
-			if _, aerr := ossaDAO.Create(ctx, tx, cdbm.OperatingSystemSiteAssociationCreateInput{
-				OperatingSystemID: gos.ID,
-				SiteID:            site.ID,
-				Status:            cdbm.OperatingSystemSiteAssociationStatusSyncing,
-				CreatedBy:         dbUser.ID,
-			}); aerr != nil {
-				logger.Error().Err(aerr).Str("osID", gos.ID.String()).Msg("Failed to auto-associate tenant global OS with new site")
+			_, aerr := ossaDAO.GetByOperatingSystemIDAndSiteID(ctx, tx, gos.ID, site.ID, nil)
+			if aerr != nil && aerr != cdb.ErrDoesNotExist {
+				logger.Error().Err(aerr).Str("osID", gos.ID.String()).Msg("Failed to check existing OS-site association")
 				return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to associate global-scoped Operating Systems with new Site", nil)
 			}
+			if aerr == cdb.ErrDoesNotExist {
+				if _, aerr = ossaDAO.Create(ctx, tx, cdbm.OperatingSystemSiteAssociationCreateInput{
+					OperatingSystemID: gos.ID,
+					SiteID:            site.ID,
+					Status:            cdbm.OperatingSystemSiteAssociationStatusSyncing,
+					CreatedBy:         dbUser.ID,
+				}); aerr != nil {
+					logger.Error().Err(aerr).Str("osID", gos.ID.String()).Msg("Failed to auto-associate tenant global OS with new site")
+					return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to associate global-scoped Operating Systems with new Site", nil)
+				}
+			}
 		}
 		if len(globalTenantOSes) > 0 {
 			logger.Info().Int("count", len(globalTenantOSes)).Msg("Auto-associated tenant global-scoped OSes with new site")

api/pkg/api/handler/allocation_test.go

@@ -652,6 +652,126 @@ func TestAllocationHandler_Create(t *testing.T) {
 	}
 }
 
+// TestAllocationHandler_Create_GlobalOSAutoAssociationIdempotent verifies that
+// creating an Allocation for a tenant that previously had (and lost) access to a
+// Site does not fail because the OperatingSystemSiteAssociation from the earlier
+// allocation already exists.
+//
+// Scenario:
+//  1. Tenant has a global-scoped IPXE OS.
+//  2. First Allocation → TenantSite is created → OS is auto-associated with Site.
+//  3. TenantSite is deleted to simulate all Allocations being removed.
+//  4. Second Allocation (same tenant + site) → TenantSite is recreated → OS
+//     auto-association must be skipped (not fail) because the row still exists.
+func TestAllocationHandler_Create_GlobalOSAutoAssociationIdempotent(t *testing.T) {
+	ctx := context.Background()
+	dbSession := testMachineInitDB(t)
+	defer dbSession.Close()
+
+	common.TestSetupSchema(t, dbSession)
+
+	ipOrg := "test-ip-org-idempotent"
+	tnOrg := "test-tn-org-idempotent"
+
+	ipu := testMachineBuildUser(t, dbSession, uuid.New().String(), []string{ipOrg}, []string{"FORGE_PROVIDER_ADMIN"})
+	tnu := testMachineBuildUser(t, dbSession, uuid.New().String(), []string{tnOrg}, []string{"FORGE_TENANT_ADMIN"})
+
+	ip := common.TestBuildInfrastructureProvider(t, dbSession, "TestIpIdempotent", ipOrg, ipu)
+	site := testIPBlockBuildSite(t, dbSession, ip, "testSiteIdempotent", cdbm.SiteStatusRegistered, true, ipu)
+	tenant := testMachineBuildTenant(t, dbSession, tnOrg, "t-idempotent")
+
+	it := common.TestBuildInstanceType(t, dbSession, "testITIdempotent", cdb.GetUUIDPtr(uuid.New()), site, map[string]string{
+		"name":        "test-instance-type-idempotent",
+		"description": "Idempotent test instance type",
+	}, ipu)
+	for i := 0; i < 5; i++ {
+		mc := testInstanceBuildMachine(t, dbSession, ip.ID, site.ID, cdb.GetBoolPtr(false), nil)
+		require.NotNil(t, mc)
+		require.NotNil(t, testInstanceBuildMachineInstanceType(t, dbSession, mc, it))
+	}
+
+	ipb := testIPBlockBuildIPBlock(t, dbSession, "testipb-idempotent", site, ip, &tenant.ID,
+		cdbm.IPBlockRoutingTypeDatacenterOnly, "10.99.0.0", 16, cdbm.IPBlockProtocolVersionV4,
+		false, cdbm.IPBlockStatusReady, ipu)
+
+	ipamStorage := ipam.NewIpamStorage(dbSession.DB, nil)
+	_, err := ipam.CreateIpamEntryForIPBlock(ctx, ipamStorage, ipb.Prefix, ipb.PrefixLength,
+		ipb.RoutingType, ipb.InfrastructureProviderID.String(), ipb.SiteID.String())
+	require.NoError(t, err)
+
+	// A tenant-owned global-scoped IPXE OS — this is what the auto-association code targets.
+	globalScope := cdbm.OperatingSystemScopeGlobal
+	globalOS := &cdbm.OperatingSystem{
+		ID:          uuid.New(),
+		Name:        "global-os-idempotent",
+		TenantID:    cdb.GetUUIDPtr(tenant.ID),
+		Type:        cdbm.OperatingSystemTypeIPXE,
+		IpxeOsScope: &globalScope,
+		IpxeScript:  cdb.GetStrPtr(common.DefaultIpxeScript),
+		IsActive:    true,
+		Status:      cdbm.OperatingSystemStatusReady,
+		CreatedBy:   tnu.ID,
+	}
+	_, err = dbSession.DB.NewInsert().Model(globalOS).Exec(ctx)
+	require.NoError(t, err)
+
+	ac := model.APIAllocationConstraintCreateRequest{
+		ResourceType:    cdbm.AllocationResourceTypeInstanceType,
+		ResourceTypeID:  it.ID.String(),
+		ConstraintType:  cdbm.AllocationConstraintTypeReserved,
+		ConstraintValue: 2,
+	}
+	body, err := json.Marshal(model.APIAllocationCreateRequest{
+		Name:                  "alloc-idempotent-1",
+		Description:           cdb.GetStrPtr(""),
+		TenantID:              tenant.ID.String(),
+		SiteID:                site.ID.String(),
+		AllocationConstraints: []model.APIAllocationConstraintCreateRequest{ac},
+	})
+	require.NoError(t, err)
+
+	// First allocation: TenantSite is created and the global OS is auto-associated.
+	a1 := testCreateAllocation(t, dbSession, ipamStorage, ipu, ipOrg, string(body))
+	require.NotNil(t, a1)
+
+	ossaDAO := cdbm.NewOperatingSystemSiteAssociationDAO(dbSession)
+	_, err = ossaDAO.GetByOperatingSystemIDAndSiteID(ctx, nil, globalOS.ID, site.ID, nil)
+	require.NoError(t, err, "OS-site association must exist after first allocation")
+
+	// Simulate all Allocations being removed: delete the TenantSite record so that
+	// the next Allocation triggers TenantSite (and OS auto-association) logic again.
+	_, err = dbSession.DB.NewDelete().TableExpr("tenant_site").
+		Where("tenant_id = ? AND site_id = ?", tenant.ID, site.ID).Exec(ctx)
+	require.NoError(t, err)
+
+	body2, err := json.Marshal(model.APIAllocationCreateRequest{
+		Name:                  "alloc-idempotent-2",
+		Description:           cdb.GetStrPtr(""),
+		TenantID:              tenant.ID.String(),
+		SiteID:                site.ID.String(),
+		AllocationConstraints: []model.APIAllocationConstraintCreateRequest{ac},
+	})
+	require.NoError(t, err)
+
+	// Second allocation on the same site: must succeed even though the
+	// OperatingSystemSiteAssociation from the first allocation still exists.
+	a2 := testCreateAllocation(t, dbSession, ipamStorage, ipu, ipOrg, string(body2))
+	require.NotNil(t, a2, "second allocation must succeed when OS-site association already exists")
+
+	// The association should still exist exactly once (not duplicated).
+	ossas, ossaCount, err := ossaDAO.GetAll(ctx, nil,
+		cdbm.OperatingSystemSiteAssociationFilterInput{
+			OperatingSystemIDs: []uuid.UUID{globalOS.ID},
+			SiteIDs:            []uuid.UUID{site.ID},
+		},
+		cdbp.PageInput{},
+		nil,
+	)
+	require.NoError(t, err)
+	assert.Equal(t, 1, ossaCount, "OS-site association must exist exactly once after both allocations")
+	_ = ossas
+}
+
 func testCreateAllocation(t *testing.T, dbSession *cdb.Session, ipamStorage cipam.Storage, user *cdbm.User, reqOrgName, reqBody string) *model.APIAllocation {
 	ctx := context.Background()
 	e := echo.New()

api/pkg/api/handler/instance.go

@@ -1910,7 +1910,7 @@ func (uih UpdateInstanceHandler) buildInstanceUpdateRequestOsConfig(c echo.Conte
 
 		// Confirm ownership between tenant and OS.
 		// Provider-owned OS (TenantID is nil) is accessible to any tenant.
-		if os.TenantID != nil && os.TenantID.String() != instance.Tenant.ID.String() {
+		if !(os.TenantID == nil || *os.TenantID == instance.TenantID) {
 			logger.Error().Msg("OperatingSystem in request is not owned by tenant")
 			return nil, nil, cutil.NewAPIError(http.StatusBadRequest, "Operating system specified in request is not owned by Tenant", nil)
 		}

api/pkg/api/handler/ipxetemplate.go

@@ -33,6 +33,7 @@ import (
 	cdb "github.com/NVIDIA/ncx-infra-controller-rest/db/pkg/db"
 	cdbm "github.com/NVIDIA/ncx-infra-controller-rest/db/pkg/db/model"
 	cdbp "github.com/NVIDIA/ncx-infra-controller-rest/db/pkg/db/paginator"
+	mapset "github.com/deckarep/golang-set/v2"
 	"github.com/google/uuid"
 	"github.com/labstack/echo/v4"
 	"github.com/rs/zerolog"
@@ -113,8 +114,8 @@ func (h GetAllIpxeTemplateHandler) Handle(c echo.Context) error {
 	// Note on tenant-path scoping: tenant access is established per-site via
 	// `TenantSite` associations (a tenant may be associated with some sites of
 	// a provider but not others).
-	providerSites := map[uuid.UUID]struct{}{}
-	tenantSites := map[uuid.UUID]struct{}{}
+	providerSites := mapset.NewSet[uuid.UUID]()
+	tenantSites := mapset.NewSet[uuid.UUID]()
 
 	if infrastructureProvider != nil {
 		siteDAO := cdbm.NewSiteDAO(h.dbSession)
@@ -128,7 +129,7 @@ func (h GetAllIpxeTemplateHandler) Handle(c echo.Context) error {
 			return cerr.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve provider sites, DB error", nil)
 		}
 		for i := range sites {
-			providerSites[sites[i].ID] = struct{}{}
+			providerSites.Add(sites[i].ID)
 		}
 	}
 
@@ -144,16 +145,12 @@ func (h GetAllIpxeTemplateHandler) Handle(c echo.Context) error {
 			return cerr.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Tenant Site associations, DB error", nil)
 		}
 		for i := range tss {
-			tenantSites[tss[i].SiteID] = struct{}{}
+			tenantSites.Add(tss[i].SiteID)
 		}
 	}
 
 	isAuthorized := func(id uuid.UUID) bool {
-		if _, ok := providerSites[id]; ok {
-			return true
-		}
-		_, ok := tenantSites[id]
-		return ok
+		return providerSites.Contains(id) || tenantSites.Contains(id)
 	}
 
 	// Determine the effective site filter:
@@ -169,20 +166,7 @@ func (h GetAllIpxeTemplateHandler) Handle(c echo.Context) error {
 		}
 		effectiveSiteIDs = requestedSiteIDs
 	} else {
-		effectiveSiteIDs = make([]uuid.UUID, 0, len(providerSites)+len(tenantSites))
-		seen := map[uuid.UUID]struct{}{}
-		for id := range providerSites {
-			if _, ok := seen[id]; !ok {
-				seen[id] = struct{}{}
-				effectiveSiteIDs = append(effectiveSiteIDs, id)
-			}
-		}
-		for id := range tenantSites {
-			if _, ok := seen[id]; !ok {
-				seen[id] = struct{}{}
-				effectiveSiteIDs = append(effectiveSiteIDs, id)
-			}
-		}
+		effectiveSiteIDs = providerSites.Union(tenantSites).ToSlice()
 	}
 
 	// No authorized sites — neither provider-owned nor reachable via a tenant account.
@@ -214,27 +198,11 @@ func (h GetAllIpxeTemplateHandler) Handle(c echo.Context) error {
 		return cerr.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve iPXE template site associations, DB error", nil)
 	}
 
-	templateIDSet := map[uuid.UUID]struct{}{}
-	templateIDs := make([]uuid.UUID, 0, len(associations))
+	templateIDSet := mapset.NewSet[uuid.UUID]()
 	for _, a := range associations {
-		if _, ok := templateIDSet[a.IpxeTemplateID]; ok {
-			continue
-		}
-		templateIDSet[a.IpxeTemplateID] = struct{}{}
-		templateIDs = append(templateIDs, a.IpxeTemplateID)
-	}
-
-	if len(templateIDs) == 0 {
-		emptyPageResponse := pagination.NewPageResponse(*pageRequest.PageNumber, *pageRequest.PageSize, 0, pageRequest.OrderByStr)
-		emptyPageHeader, err := json.Marshal(emptyPageResponse)
-		if err != nil {
-			logger.Error().Err(err).Msg("error marshaling empty pagination response")
-			return cerr.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to generate pagination response header", nil)
-		}
-		c.Response().Header().Set(pagination.ResponseHeaderName, string(emptyPageHeader))
-		logger.Info().Msg("finishing API handler")
-		return c.JSON(http.StatusOK, []*model.APIIpxeTemplate{})
+		templateIDSet.Add(a.IpxeTemplateID)
 	}
+	templateIDs := templateIDSet.ToSlice()
 
 	templateDAO := cdbm.NewIpxeTemplateDAO(h.dbSession)
 	templates, total, err := templateDAO.GetAll(

db/pkg/db/model/ipxetemplate.go

@@ -217,6 +217,10 @@ func (itd IpxeTemplateSQLDAO) GetAll(ctx context.Context, tx *db.Tx, filter Ipxe
 
 	templates := []IpxeTemplate{}
 
+	if filter.IDs != nil && len(filter.IDs) == 0 {
+		return templates, 0, nil
+	}
+
 	query := db.GetIDB(tx, itd.dbSession).NewSelect().Model(&templates)
 
 	query, err := itd.setQueryWithFilter(filter, query, span)

site-agent/pkg/components/testing.go

@@ -288,8 +288,11 @@ func TestInitElektra(t *testing.T) {
 	if testElektra != nil {
 		return
 	}
-	os.Setenv("CARBIDE_CERT_CHECK_INTERVAL", "1") // set this to check if certs were rotated every second to help with unit tests
-	defer os.Unsetenv("CARBIDE_CERT_CHECK_INTERVAL")
+	// Keep CARBIDE_CERT_CHECK_INTERVAL set for the lifetime of the test binary so that
+	// the cert-reload goroutine (started inside api.Start below) reads the 1-second
+	// interval before the ticker is created. Unsetting it via defer races against
+	// goroutine scheduling and causes the goroutine to fall back to the 15-minute default.
+	os.Setenv("CARBIDE_CERT_CHECK_INTERVAL", "1")
 
 	// Initialize test Site Agent
 	log.Info().Msg("Elektra: Initializing test Site Agent")

workflow/pkg/activity/operatingsystem/operatingsystem.go

@@ -617,6 +617,12 @@ func (mos ManageOperatingSystem) UpdateOperatingSystemsInDB(ctx context.Context,
 				slogger.Error().Err(ossaErr).Msg("Failed to create site association for new OS")
 				continue
 			}
+
+			// Newly-created OS: definition and per-site association have just been
+			// written with the reported state. Skip the existing-OS update path
+			// below (it dereferences existingOS which is nil here) and do not add
+			// to globalOrLimitedOSIDs because new records are always Local scope.
+			continue
 		}
 
 		// REST layer has already soft-deleted this OS (user-initiated)
@@ -662,32 +668,36 @@ func (mos ManageOperatingSystem) UpdateOperatingSystemsInDB(ctx context.Context,
 				slogger.Error().Err(serr).Msg("Failed to create Operating System Site Association")
 				continue
 			}
-		}
-
-		// Update existing Operating System Site Association
-		_, uerr := ossaDAO.Update(ctx, nil, cdbm.OperatingSystemSiteAssociationUpdateInput{
-			OperatingSystemSiteAssociationID: ossa.ID,
-			Status:                           &ossaStatus,
-			ControllerState:                  &controllerState,
-		})
-		if uerr != nil {
-			slogger.Error().Err(uerr).Msg("Failed to update Operating System Site Association")
-			continue
+		} else {
+			// Update existing Operating System Site Association
+			_, uerr := ossaDAO.Update(ctx, nil, cdbm.OperatingSystemSiteAssociationUpdateInput{
+				OperatingSystemSiteAssociationID: ossa.ID,
+				Status:                           &ossaStatus,
+				ControllerState:                  &controllerState,
+			})
+			if uerr != nil {
+				slogger.Error().Err(uerr).Msg("Failed to update Operating System Site Association")
+				continue
+			}
 		}
 
 		// TODO: Is this correct?
 		if !isLocalScope {
 			globalOrLimitedOSIDs[reportedOSID] = struct{}{}
 		}
 
-		// Operating System exists in both REST and Site, update the REST record if it is newer
-		// Backfill: older records may have been created with tenant_id set and no infrastructure_provider_id (before this ownership model was established).
-		needsProviderBackfill := existingOS.InfrastructureProviderID == nil
-		needsOrgBackfill := existingOS.Org == "" && site.Org != ""
-		needsIsActiveCorrection := existingOS.IsActive != reportedOS.IsActive
-		needsTenantClear := existingOS.TenantID != nil
+		// Operating System exists in both REST and Site; update the REST record only for
+		// Local-scoped OSes (Site is the source of truth for the definition).
+		// Global/Limited OSes are REST-owned: skip the definition update and rely solely on
+		// the aggregate status recomputation that runs at the end of this function.
+		// Backfill: older records may have been created with tenant_id set and no
+		// infrastructure_provider_id (before this ownership model was established).
+		needsProviderBackfill := isLocalScope && existingOS.InfrastructureProviderID == nil
+		needsOrgBackfill := isLocalScope && existingOS.Org == "" && site.Org != ""
+		needsIsActiveCorrection := isLocalScope && existingOS.IsActive != reportedOS.IsActive
+		needsTenantClear := isLocalScope && existingOS.TenantID != nil
 
-		if coreUpdated.After(existingOS.Updated) || needsProviderBackfill || needsOrgBackfill || needsIsActiveCorrection || needsTenantClear {
+		if isLocalScope && (coreUpdated.After(existingOS.Updated) || needsProviderBackfill || needsOrgBackfill || needsIsActiveCorrection || needsTenantClear) {
 			controllerState := cdbm.OperatingSystemStatusFromProtoMap[reportedOS.Status]
 			if controllerState == "" {
 				slogger.Warn().Str("Status", reportedOS.Status.String()).Msg("Received unknown status from Site, using `Syncing` as default")