OCPBUGS-56913: Retry incomplete cluster registration in ABI

cmd/agentbasedinstaller/agentbasedinstaller_suite_test.go

@@ -194,6 +194,10 @@ func (m *mockTransport) Submit(op *runtime.ClientOperation) (interface{}, error)
 	case *manifests.V2CreateClusterManifestParams:
 		m.filesReceived[*v.CreateManifestParams.FileName] = *v.CreateManifestParams.Content
 		result = &manifests.V2CreateClusterManifestCreated{}
+	case *manifests.V2ListClusterManifestsParams:
+		result = &manifests.V2ListClusterManifestsOK{
+			Payload: []*models.Manifest{},
+		}
 	case *installerclient.V2ImportClusterParams:
 		m.lastImportParamsReceived = v.NewImportClusterParams
 		result = &installerclient.V2ImportClusterCreated{

cmd/agentbasedinstaller/client/main.go

@@ -35,6 +35,7 @@ import (
 
 	"github.com/kelseyhightower/envconfig"
 	"github.com/openshift/assisted-service/client"
+	"github.com/openshift/assisted-service/models"
 	"github.com/openshift/assisted-service/pkg/auth"
 	log "github.com/sirupsen/logrus"
 )
@@ -121,23 +122,37 @@ func registerCluster(ctx context.Context, log *log.Logger, bmInventory *client.A
 	if err != nil {
 		log.Fatal(err.Error())
 	}
+
+	var modelsCluster *models.Cluster
 	if existingCluster != nil {
-		log.Infof("Skipping cluster registration. Found existing cluster with id: %s", existingCluster.ID.String())
-		return existingCluster.ID.String()
-	}
+		log.Infof("Found existing cluster with id: %s", existingCluster.ID.String())
+		modelsCluster = existingCluster
+	} else {
+		pullSecret, err := agentbasedinstaller.GetPullSecret(RegisterOptions.PullSecretFile)
+		if err != nil {
+			log.Fatal("Failed to get pull secret: ", err.Error())
+		}
 
-	pullSecret, err := agentbasedinstaller.GetPullSecret(RegisterOptions.PullSecretFile)
-	if err != nil {
-		log.Fatal("Failed to get pull secret: ", err.Error())
+		modelsCluster, err = agentbasedinstaller.RegisterCluster(ctx, log, bmInventory, pullSecret,
+			RegisterOptions.ClusterDeploymentFile, RegisterOptions.AgentClusterInstallFile, RegisterOptions.ClusterImageSetFile,
+			RegisterOptions.ReleaseImageMirror, RegisterOptions.OperatorInstallFile, false)
+		if err != nil {
+			log.Fatal("Failed to register cluster with assisted-service: ", err)
+		}
 	}
 
-	modelsCluster, err := agentbasedinstaller.RegisterCluster(ctx, log, bmInventory, pullSecret,
-		RegisterOptions.ClusterDeploymentFile, RegisterOptions.AgentClusterInstallFile, RegisterOptions.ClusterImageSetFile,
-		RegisterOptions.ReleaseImageMirror, RegisterOptions.OperatorInstallFile, false)
+	// Apply installConfig overrides if present (idempotent)
+	log.Info("Applying installConfig overrides...")
+	updatedCluster, err := agentbasedinstaller.ApplyInstallConfigOverrides(ctx, log, bmInventory, modelsCluster, RegisterOptions.AgentClusterInstallFile)
 	if err != nil {
-		log.Fatal("Failed to register cluster with assisted-service: ", err)
+		log.Fatal("Failed to apply installConfig overrides: ", err)
+	}
+	if updatedCluster != nil {
+		modelsCluster = updatedCluster
 	}
 
+	// Register extra manifests (idempotent)
+	log.Info("Registering extra manifests...")
 	err = agentbasedinstaller.RegisterExtraManifests(os.DirFS(RegisterOptions.ExtraManifests), ctx, log, bmInventory.Manifests, modelsCluster)
 	if err != nil {
 		log.Fatal("Failed to register extra manifests with assisted-service: ", err)

cmd/agentbasedinstaller/register.go

@@ -1,8 +1,10 @@
 package agentbasedinstaller
 
 import (
+	"bytes"
 	"context"
 	"encoding/base64"
+	"encoding/json"
 	"fmt"
 	"io/fs"
 	"os"
@@ -120,34 +122,79 @@ func RegisterCluster(ctx context.Context, log *log.Logger, bmInventory *client.A
 
 	log.Infof("Registered cluster with id: %s", clusterResult.Payload.ID)
 
+	// Apply installConfig overrides if present
+	updatedCluster, err := ApplyInstallConfigOverrides(ctx, log, bmInventory, result, agentClusterInstallPath)
+	if err != nil {
+		return nil, err
+	}
+	if updatedCluster != nil {
+		result = updatedCluster
+	}
+
+	return result, nil
+}
+
+// ApplyInstallConfigOverrides applies installConfig overrides to an existing cluster
+// if the AgentClusterInstall manifest contains override annotations.
+// Returns the updated cluster if overrides were applied, nil if no overrides present, or error on failure.
+func ApplyInstallConfigOverrides(ctx context.Context, log *log.Logger, bmInventory *client.AssistedInstall, cluster *models.Cluster, agentClusterInstallPath string) (*models.Cluster, error) {
+	var aci hiveext.AgentClusterInstall
+	if aciErr := getFileData(agentClusterInstallPath, &aci); aciErr != nil {
+		return nil, aciErr
+	}
+
 	annotations := aci.GetAnnotations()
-	if installConfigOverrides, ok := annotations[controllers.InstallConfigOverrides]; ok {
-		var reJsonField = regexp.MustCompile(`(?i)"([^"]*(password)[^"]*)":\s*"(\\{2}|\\"|[^"])*"`)
-		updateInstallConfigParams := &installer.V2UpdateClusterInstallConfigParams{
-			ClusterID:           *clusterResult.Payload.ID,
-			InstallConfigParams: installConfigOverrides,
-		}
-		_, updateClusterErr := bmInventory.Installer.V2UpdateClusterInstallConfig(ctx, updateInstallConfigParams)
-		if updateClusterErr != nil {
-			return nil, errorutil.GetAssistedError(updateClusterErr)
-		}
+	installConfigOverrides, ok := annotations[controllers.InstallConfigOverrides]
+	if !ok {
+		// No overrides to apply
+		return nil, nil
+	}
 
-		filteredICOverrides := reJsonField.ReplaceAllString(installConfigOverrides, fmt.Sprintf(`"$1":"%s"`, "[redacted]"))
-		log.Infof("Updated cluster %s with installConfigOverrides %s", clusterResult.Payload.ID, filteredICOverrides)
+	// Check if overrides are already correctly applied by normalizing JSON before comparing
+	// This prevents unnecessary API calls when JSON is semantically identical but formatted differently
+	// If the existing overrides are invalid JSON, treat as different (will be fixed by the update)
+	existingNormalized, existingErr := normalizeJSON(cluster.InstallConfigOverrides)
+	newNormalized, newErr := normalizeJSON(installConfigOverrides)
 
-		// Need to GET cluster again so we can give a proper return value
-		getClusterResult, err := bmInventory.Installer.V2GetCluster(ctx, &installer.V2GetClusterParams{
-			ClusterID: *clusterResult.Payload.ID,
-		})
+	// If new overrides are invalid, that's an error we should propagate
+	if newErr != nil {
+		return nil, errors.Wrap(newErr, "failed to normalize new installConfig overrides")
+	}
 
-		if err != nil {
-			log.Warnf("Updated cluster %s with installConfigOverrides %s", clusterResult.Payload.ID, filteredICOverrides)
-		} else {
-			result = getClusterResult.GetPayload()
-		}
+	// If existing is invalid JSON or differs from new, proceed with update
+	// (existingErr != nil means invalid JSON in cluster, which we'll fix)
+	if existingErr == nil && existingNormalized == newNormalized {
+		log.Infof("InstallConfig overrides already correctly applied for cluster %s", *cluster.ID)
+		return nil, nil
 	}
 
-	return result, nil
+	if existingErr != nil {
+		log.Infof("Existing installConfig overrides contain invalid JSON for cluster %s, will update", *cluster.ID)
+	}
+
+	var reJsonField = regexp.MustCompile(`(?i)"([^"]*(password)[^"]*)":\s*"(\\{2}|\\"|[^"])*"`)
+	updateInstallConfigParams := &installer.V2UpdateClusterInstallConfigParams{
+		ClusterID:           *cluster.ID,
+		InstallConfigParams: installConfigOverrides,
+	}
+	_, updateClusterErr := bmInventory.Installer.V2UpdateClusterInstallConfig(ctx, updateInstallConfigParams)
+	if updateClusterErr != nil {
+		return nil, errorutil.GetAssistedError(updateClusterErr)
+	}
+
+	filteredICOverrides := reJsonField.ReplaceAllString(installConfigOverrides, fmt.Sprintf(`"$1":"%s"`, "[redacted]"))
+	log.Infof("Applied installConfig overrides to cluster %s: %s", *cluster.ID, filteredICOverrides)
+
+	// Need to GET cluster again so we can give a proper return value
+	getClusterResult, err := bmInventory.Installer.V2GetCluster(ctx, &installer.V2GetClusterParams{
+		ClusterID: *cluster.ID,
+	})
+
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get cluster after applying installConfig overrides")
+	}
+
+	return getClusterResult.GetPayload(), nil
 }
 
 func RegisterInfraEnv(ctx context.Context, log *log.Logger, bmInventory *client.AssistedInstall, pullSecret string, modelsCluster *models.Cluster,
@@ -214,13 +261,53 @@ func RegisterExtraManifests(fsys fs.FS, ctx context.Context, log *log.Logger, cl
 
 	extraManifestsFolder := "openshift"
 
+	// Get list of existing manifests to make this function idempotent
+	listParams := manifests.NewV2ListClusterManifestsParams().
+		WithClusterID(*cluster.ID)
+	existingManifests, err := client.V2ListClusterManifests(ctx, listParams)
+	if err != nil {
+		return errorutil.GetAssistedError(err)
+	}
+
+	// Build map of existing manifests for quick lookup
+	existingMap := make(map[string]string) // filename -> content
+	if existingManifests != nil && existingManifests.Payload != nil {
+		for _, manifest := range existingManifests.Payload {
+			if manifest != nil && manifest.Folder == extraManifestsFolder {
+				// Download the content to compare
+				var buf bytes.Buffer
+				downloadParams := manifests.NewV2DownloadClusterManifestParams().
+					WithClusterID(*cluster.ID).
+					WithFolder(&manifest.Folder).
+					WithFileName(manifest.FileName)
+				_, err := client.V2DownloadClusterManifest(ctx, downloadParams, &buf)
+				if err != nil {
+					return errorutil.GetAssistedError(err)
+				}
+				existingMap[manifest.FileName] = buf.String()
+			}
+		}
+	}
+
 	for _, f := range extras {
 		extraManifestFileName := f
 		bytes, err := fs.ReadFile(fsys, extraManifestFileName)
 		if err != nil {
 			return err
 		}
 
+		// Check if manifest already exists
+		if existingContent, exists := existingMap[extraManifestFileName]; exists {
+			// Manifest exists - verify content matches
+			if existingContent == string(bytes) {
+				log.Infof("Manifest %s already exists with same content, skipping", extraManifestFileName)
+				continue
+			} else {
+				return errors.Errorf("manifest %s already exists with different content", extraManifestFileName)
+			}
+		}
+
+		// Manifest doesn't exist - create it
 		extraManifestContent := base64.StdEncoding.EncodeToString(bytes)
 		params := manifests.NewV2CreateClusterManifestParams().
 			WithClusterID(*cluster.ID).
@@ -234,6 +321,7 @@ func RegisterExtraManifests(fsys fs.FS, ctx context.Context, log *log.Logger, cl
 		if err != nil {
 			return errorutil.GetAssistedError(err)
 		}
+		log.Infof("Registered manifest %s", extraManifestFileName)
 	}
 
 	return nil
@@ -362,3 +450,24 @@ func operatorsToArray(entries []models.OperatorCreateParams) []*models.OperatorC
 		return &models.OperatorCreateParams{Name: entry.Name, Properties: entry.Properties}
 	}).([]*models.OperatorCreateParams)
 }
+
+// normalizeJSON normalizes a JSON string by parsing and re-marshaling it.
+// This ensures consistent formatting and key ordering for comparison.
+// Returns empty string if input is empty to handle unset overrides.
+func normalizeJSON(jsonStr string) (string, error) {
+	if jsonStr == "" {
+		return "", nil
+	}
+
+	var data interface{}
+	if err := json.Unmarshal([]byte(jsonStr), &data); err != nil {
+		return "", err
+	}
+
+	normalized, err := json.Marshal(data)
+	if err != nil {
+		return "", err
+	}
+
+	return string(normalized), nil
+}