CNTRLPLANE-3324: Restore non-obvious comments after gocyclo refactor

cmd/cluster/core/create.go

@@ -355,6 +355,7 @@ func prototypeResources(ctx context.Context, opts *CreateOptions) (*resources, e
 }
 
 func resolveReleaseImage(ctx context.Context, opts *CreateOptions) error {
+	// allow client side defaulting when release image is empty but release stream is set.
 	if len(opts.ReleaseImage) != 0 || len(opts.ReleaseStream) == 0 {
 		return nil
 	}
@@ -386,6 +387,7 @@ func parseKeyValuePairs(items []string, kind string) (map[string]string, error)
 }
 
 func resolvePullSecret(opts *CreateOptions) ([]byte, error) {
+	// overrides if pullSecretFile is set
 	if len(opts.PullSecretFile) > 0 {
 		data, err := os.ReadFile(opts.PullSecretFile)
 		if err != nil {
@@ -430,6 +432,7 @@ func applyEtcdConfig(cluster *hyperv1.HostedCluster, opts *CreateOptions) error
 func applySSHKey(prototype *resources, opts *CreateOptions) error {
 	sshKey, sshPrivateKey := opts.PublicKey, opts.PrivateKey
 	var err error
+	// overrides secret if SSHKeyFile is set
 	if len(opts.SSHKeyFile) > 0 {
 		if opts.GenerateSSH {
 			return fmt.Errorf("--generate-ssh and --ssh-key cannot be specified together")
@@ -468,6 +471,8 @@ func applySSHKey(prototype *resources, opts *CreateOptions) error {
 }
 
 func applyPausedUntil(cluster *hyperv1.HostedCluster, opts *CreateOptions) error {
+	// validate pausedUntil value
+	// valid values are either "true" or RFC3339 format date
 	if len(opts.PausedUntil) == 0 || opts.PausedUntil == "true" {
 		return nil
 	}
@@ -725,14 +730,18 @@ func (opts *RawCreateOptions) Validate(ctx context.Context) (*ValidatedCreateOpt
 		return nil, err
 	}
 
+	// Validate HostedCluster name follows RFC1123 standard
+	// https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names
 	errs := validation.IsDNS1123Label(opts.Name)
 	if len(errs) > 0 {
 		return nil, fmt.Errorf("HostedCluster name failed RFC1123 validation: %s", strings.Join(errs[:], " "))
 	}
 
+	// Validate HostedCluster with this name doesn't exist in the namespace
 	if err := opts.validateClusterExistence(ctx); err != nil {
 		return nil, err
 	}
+	// Validate multi-arch aspects
 	if err := opts.validateArchAndFeatureSet(); err != nil {
 		return nil, err
 	}
@@ -804,6 +813,7 @@ func (opts *RawCreateOptions) validateArchAndFeatureSet() error {
 		return fmt.Errorf("specified arch %q is not supported", opts.Arch)
 	}
 
+	// Validate feature set is "", TechPreviewNoUpgrade, or DevPreviewNoUpgrade
 	switch opts.FeatureSet {
 	case string(configv1.Default), string(configv1.TechPreviewNoUpgrade), string(configv1.DevPreviewNoUpgrade):
 	case string(configv1.CustomNoUpgrade):

cmd/install/assets/hypershift_operator.go

@@ -993,6 +993,9 @@ func (o HyperShiftOperatorDeployment) addAzurePlatformResources(envVars *[]corev
 	if o.AzurePLSResourceGroup != "" {
 		*envVars = append(*envVars, corev1.EnvVar{Name: "AZURE_RESOURCE_GROUP", Value: o.AzurePLSResourceGroup})
 	}
+	// Workload identity mode: the SA annotation triggers Azure AD Workload Identity
+	// webhook to inject federated tokens. Set the client ID as an env var so the
+	// HO platform controller can construct credentials.
 	if o.AzurePLSManagedIdentityClientID != "" {
 		*envVars = append(*envVars, corev1.EnvVar{Name: "AZURE_PLS_CLIENT_ID", Value: o.AzurePLSManagedIdentityClientID})
 		if o.AzurePLSSubscriptionID != "" {

cmd/install/install.go

@@ -185,6 +185,8 @@ func (o *Options) validatePlatformConfig() []error {
 			errs = append(errs, fmt.Errorf("--aws-private-region and --aws-private-creds or --aws-private-secret are required with --private-platform=%s", hyperv1.AWSPlatform))
 		}
 	case hyperv1.GCPPlatform:
+		// GCP uses Workload Identity Federation, no credentials required.
+		// However, --gcp-project and --gcp-region must be set together.
 		if (o.GCPProject == "") != (o.GCPRegion == "") {
 			errs = append(errs, fmt.Errorf("--gcp-project and --gcp-region must be set together when --private-platform=%s", hyperv1.GCPPlatform))
 		}
@@ -242,6 +244,7 @@ func (o *Options) validateExternalDNSConfig() []error {
 		return nil
 	}
 	var errs []error
+	// Credentials are optional for GCP when using Workload Identity
 	credentialsRequired := o.ExternalDNSProvider != "google"
 	if credentialsRequired && len(o.ExternalDNSCredentials) == 0 && len(o.ExternalDNSCredentialsSecret) == 0 {
 		errs = append(errs, fmt.Errorf("--external-dns-credentials or --external-dns-credentials-secret are required with --external-dns-provider"))
@@ -282,20 +285,24 @@ func (o *Options) validateImageConfig() []error {
 	return errs
 }
 
+// Validate scale-from-zero credentials
 func (o *Options) validateScaleFromZeroConfig() []error {
 	if len(o.ScaleFromZeroCreds) == 0 && len(o.ScaleFromZeroCredentialsSecret) == 0 {
 		return nil
 	}
 	var errs []error
 	supportedProviders := set.New("aws")
+	// Check mutual exclusivity - only one of file or secret should be provided
 	if len(o.ScaleFromZeroCreds) != 0 && len(o.ScaleFromZeroCredentialsSecret) != 0 {
 		errs = append(errs, fmt.Errorf("only one of --scale-from-zero-creds or --scale-from-zero-secret is supported"))
 	}
+	// Provider is required when using scale-from-zero credentials
 	if len(o.ScaleFromZeroProvider) == 0 {
 		errs = append(errs, fmt.Errorf("--scale-from-zero-provider is required when using scale-from-zero credentials"))
 	} else if !supportedProviders.Has(o.ScaleFromZeroProvider) {
 		errs = append(errs, fmt.Errorf("invalid --scale-from-zero-provider: %s (must be one of: %v)", o.ScaleFromZeroProvider, supportedProviders.UnsortedList()))
 	}
+	// Validate credentials file exists and is accessible if provided
 	if len(o.ScaleFromZeroCreds) > 0 {
 		if _, err := os.Stat(o.ScaleFromZeroCreds); err != nil {
 			if os.IsNotExist(err) {
@@ -324,6 +331,7 @@ func (o *Options) validateMiscConfig() []error {
 	if len(o.ManagedService) > 0 && o.ManagedService != hyperv1.AroHCP {
 		errs = append(errs, fmt.Errorf("not a valid managed service type: %s", o.ManagedService))
 	}
+	// Validate all the platforms in the list are valid
 	for _, platform := range o.PlatformsToInstall {
 		platformToCheck := strings.ToLower(platform)
 		if !ValidPlatforms.Has(platformToCheck) {

control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go

@@ -677,8 +677,10 @@ func deleteEndpointIfWrongService(ctx context.Context, ec2Client awsapi.EC2API,
 }
 
 func modifyEndpointIfNeeded(ctx context.Context, ec2Client awsapi.EC2API, awsEndpointService *hyperv1.AWSEndpointService, endpoint ec2types.VpcEndpoint, endpointID string, log logr.Logger) error {
+	// Ensure endpoint has the right subnets.
 	addedSubnet, removedSubnet := diffIDs(awsEndpointService.Spec.SubnetIDs, endpoint.SubnetIds)
 
+	// Ensure endpoint has the right SG.
 	existingSG := make([]string, 0)
 	for _, group := range endpoint.Groups {
 		existingSG = append(existingSG, aws.ToString(group.GroupId))
@@ -816,6 +818,8 @@ func (r *AWSEndpointServiceReconciler) reconcileExternalNameServices(ctx context
 	isPublic := netutil.IsPublicHCP(hcp)
 	externalNames := hcpExternalNames(hcp)
 
+	// only if not public and external names are configured, create services of type ExternalName so external-dns
+	// can create records for them
 	if !isPublic && len(externalNames) > 0 {
 		var errs []error
 		for svcType, externalName := range externalNames {
@@ -839,6 +843,7 @@ func (r *AWSEndpointServiceReconciler) reconcileExternalNameServices(ctx context
 		return nil
 	}
 
+	// if the cluster is public, ensure that any ExternalName services are removed
 	privateExternalServices := &corev1.ServiceList{}
 	if err := r.List(ctx, privateExternalServices, client.InNamespace(hcp.Namespace), client.HasLabels{externalPrivateServiceLabel}); err != nil {
 		return fmt.Errorf("cannot list private external services: %w", err)
@@ -1106,6 +1111,7 @@ func (r *AWSEndpointServiceReconciler) delete(ctx context.Context, awsEndpointSe
 		}
 
 		if output != nil && len(output.VpcEndpoints) != 0 {
+			// Once the VPC Endpoint is deleted, we need to return an error to reexecute the reconciliation
 			return false, fmt.Errorf("resource requested for deletion but still present")
 		}
 

control-plane-operator/controllers/azureprivatelinkservice/controller.go

@@ -860,6 +860,9 @@ func (r *AzurePrivateLinkServiceReconciler) handleAzureError(ctx context.Context
 // CPO finalizer has completed PE cleanup and removed its finalizer from the CR.
 func (r *AzurePrivateLinkServiceReconciler) reconcileDelete(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, log logr.Logger) error {
 	resourceGroup := azPLS.Spec.ResourceGroupName
+	// Delete DNS resources using the zone name persisted in status.
+	// This avoids a dependency on the HostedControlPlane during deletion, which may
+	// already be torn down or unavailable when the finalizer runs.
 	dnsZoneName := azPLS.Status.DNSZoneName
 
 	if dnsZoneName != "" {
@@ -961,6 +964,9 @@ func (r *AzurePrivateLinkServiceReconciler) deleteBaseDomainResources(ctx contex
 		return err
 	}
 
+	// Only delete the base domain DNS zone if no other CRs share it.
+	// When multiple CRs (e.g., private-router and oauth-openshift) use the same
+	// base domain zone, the zone must not be deleted until the last CR is removed.
 	hasSiblings, err := r.hasSiblingCR(ctx, azPLS)
 	if err != nil {
 		return fmt.Errorf("failed to check for sibling CRs during base domain zone cleanup: %w", err)
@@ -988,6 +994,9 @@ func (r *AzurePrivateLinkServiceReconciler) deleteBaseDomainARecords(ctx context
 	var baseDomainRecords []string
 	if azPLS.Name == privateRouterCRName {
 		baseDomainRecords = append(baseDomainRecords, kasBaseDomainRecordPrefix+clusterName)
+		// Only delete the oauth record if there is no sibling OAuth CR that
+		// owns it. This prevents the private-router deletion from removing
+		// an oauth record that now belongs to the dedicated OAuth CR.
 		hasSiblings, err := r.hasSiblingCR(ctx, azPLS)
 		if err != nil {
 			return fmt.Errorf("failed to check for sibling CRs during base domain cleanup: %w", err)
@@ -1013,6 +1022,10 @@ func (r *AzurePrivateLinkServiceReconciler) deleteBaseDomainARecords(ctx context
 	return nil
 }
 
+// deletePrivateEndpoint always attempts deletion by deterministic name, even
+// when PrivateEndpointID is empty. If the status was never populated (e.g.,
+// status update failed after PE creation), relying solely on PrivateEndpointID
+// would orphan the PE in the customer's subscription.
 func (r *AzurePrivateLinkServiceReconciler) deletePrivateEndpoint(ctx context.Context, resourceGroup, crName, statusPEID string, log logr.Logger) error {
 	peName := privateEndpointName(crName)
 	log.Info("Deleting Private Endpoint", "name", peName, "hasStatusID", statusPEID != "")

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -502,6 +502,7 @@ func (r *HostedControlPlaneReconciler) reconcileKASStatus(ctx context.Context, h
 			return fmt.Errorf("failed to fetch Kube APIServer deployment %s/%s: %w", deployment.Namespace, deployment.Name, err)
 		}
 	} else {
+		// Assume the deployment is unavailable until proven otherwise.
 		newCondition = metav1.Condition{
 			Type:   string(hyperv1.KubeAPIServerAvailable),
 			Status: metav1.ConditionFalse,
@@ -670,6 +671,8 @@ func (r *HostedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return reconcile.Result{}, fmt.Errorf("failed to look up release image metadata: %w", err)
 	}
 
+	// This runs after LookupReleaseImage so we can use the version and resolved
+	// digest from the release image metadata.
 	if err := r.reconcileControlPlaneVersionStatus(ctx, hostedControlPlane, originalHostedControlPlane, releaseImage); err != nil {
 		return reconcile.Result{}, err
 	}
@@ -847,6 +850,8 @@ func (r *HostedControlPlaneReconciler) reconcileControlPlaneVersionStatus(ctx co
 	if err := r.Client.Get(ctx, client.ObjectKeyFromObject(pullSecret), pullSecret); err != nil {
 		return fmt.Errorf("failed to get pull secret for version reconciliation: %w", err)
 	}
+	// Resolve the release image to its digest so controlPlaneVersion records
+	// the immutable image reference, consistent with how CVO records images.
 	_, resolvedRef, err := r.ImageMetadataProvider.GetDigest(ctx, util.HCPControlPlaneReleaseImage(hostedControlPlane), pullSecret.Data[corev1.DockerConfigJsonKey])
 	if err != nil {
 		return fmt.Errorf("failed to resolve control plane release image digest: %w", err)
@@ -855,6 +860,9 @@ func (r *HostedControlPlaneReconciler) reconcileControlPlaneVersionStatus(ctx co
 
 	componentsList := &hyperv1.ControlPlaneComponentList{}
 	if listErr := r.Client.List(ctx, componentsList, client.InNamespace(hostedControlPlane.Namespace)); listErr != nil {
+		// On list failure, ensure a Partial entry exists so consumers
+		// know an upgrade was attempted. Preserve observedGeneration.
+		// Persist the Partial entry before returning the error.
 		hostedControlPlane.Status.ControlPlaneVersion = ensureControlPlaneVersionPartial(hostedControlPlane, clk, releaseImage.Version(), resolvedImage)
 		if patchErr := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); patchErr != nil {
 			return fmt.Errorf("failed to patch status after component list failure: %w (list error: %v)", patchErr, listErr)
@@ -1590,6 +1598,10 @@ func (r *HostedControlPlaneReconciler) reconcileOLMAndMiscCerts(ctx context.Cont
 }
 
 func (r *HostedControlPlaneReconciler) reconcileNetworkServingCerts(ctx context.Context, hcp *hyperv1.HostedControlPlane, p *pki.PKIParams, createOrUpdate upsert.CreateOrUpdateFN, rootCASecret *corev1.Secret) error {
+	// For the Multus Admission Controller, Network Node Identity, and OVN Control Plane Metrics Serving Certs:
+	//   We want to remove the secret if there was an existing one created by service-ca; otherwise, it will cause
+	//   issues in cases where you are upgrading an older CPO prior to us adding the feature to reconcile the serving
+	//   cert secret ourselves.
 	if !netutil.IsDisableMultiNetwork(hcp) {
 		multusAdmissionControllerService := manifests.MultusAdmissionControllerService(hcp.Namespace)
 		if err := r.Get(ctx, client.ObjectKeyFromObject(multusAdmissionControllerService), multusAdmissionControllerService); err != nil {
@@ -1598,6 +1610,8 @@ func (r *HostedControlPlaneReconciler) reconcileNetworkServingCerts(ctx context.
 			}
 		}
 
+		// If the service doesn't have the service ca annotation, delete any previous secret with the annotation and
+		// reconcile the secret with our own rootCA; otherwise, skip reconciling the secret with our own rootCA.
 		if hasServiceCAAnnotation := doesServiceHaveServiceCAAnnotation(multusAdmissionControllerService); !hasServiceCAAnnotation {
 			multusAdmissionControllerServingCertSecret := manifests.MultusAdmissionControllerServingCert(hcp.Namespace)
 
@@ -1620,6 +1634,8 @@ func (r *HostedControlPlaneReconciler) reconcileNetworkServingCerts(ctx context.
 		}
 	}
 
+	// If the service doesn't have the service ca annotation, delete any previous secret with the annotation and
+	// reconcile the secret with our own rootCA; otherwise, skip reconciling the secret with our own rootCA.
 	if hasServiceCAAnnotation := doesServiceHaveServiceCAAnnotation(networkNodeIdentityService); !hasServiceCAAnnotation {
 		networkNodeIdentityServingCertSecret := manifests.NetworkNodeIdentityControllerServingCert(hcp.Namespace)
 
@@ -1641,6 +1657,8 @@ func (r *HostedControlPlaneReconciler) reconcileNetworkServingCerts(ctx context.
 		}
 	}
 
+	// If the service doesn't have the service ca annotation, delete any previous secret with the annotation and
+	// reconcile the secret with our own rootCA; otherwise, skip reconciling the secret with our own rootCA.
 	if hasServiceCAAnnotation := doesServiceHaveServiceCAAnnotation(ovnControlPlaneService); !hasServiceCAAnnotation {
 		ovnControlPlaneMetricsServingCertSecret := manifests.OVNControlPlaneMetricsServingCert(hcp.Namespace)
 

control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.go

@@ -191,7 +191,7 @@ func convertGitLabIDP(providerConfig *configv1.IdentityProviderConfig, i int, id
 		ClientSecret: configv1.StringSource{StringSourceSpec: configv1.StringSourceSpec{
 			File: idpVolumeMounts.SecretPath(i, gitlabConfig.ClientSecret.Name, "client-secret", configv1.ClientSecretKey),
 		}},
-		Legacy: new(bool),
+		Legacy: new(bool), // we require OIDC for GitLab now
 	}
 	if gitlabConfig.CA.Name != "" {
 		provider.CA = idpVolumeMounts.ConfigMapPath(i, gitlabConfig.CA.Name, "ca", corev1.ServiceAccountRootCAKey)
@@ -235,7 +235,7 @@ func convertKeystoneIDP(providerConfig *configv1.IdentityProviderConfig, i int,
 		TypeMeta:             metav1.TypeMeta{Kind: "KeystonePasswordIdentityProvider", APIVersion: osinv1.GroupVersion.String()},
 		RemoteConnectionInfo: configv1.RemoteConnectionInfo{URL: keystoneConfig.URL},
 		DomainName:           keystoneConfig.DomainName,
-		UseKeystoneIdentity:  true,
+		UseKeystoneIdentity:  true, // force use of keystone ID
 	}
 	if keystoneConfig.CA.Name != "" {
 		provider.RemoteConnectionInfo.CA = idpVolumeMounts.ConfigMapPath(i, keystoneConfig.CA.Name, "ca", corev1.ServiceAccountRootCAKey)
@@ -291,6 +291,7 @@ func convertOpenIDIDP(ctx context.Context, providerConfig *configv1.IdentityProv
 		ExtraScopes:              openIDConfig.ExtraScopes,
 		ExtraAuthorizeParameters: openIDConfig.ExtraAuthorizeParameters,
 	}
+	// Handle special case for IBM Cloud's OIDC provider (need to override some fields not available in public api)
 	if configOverride != nil {
 		openIDProvider.URLs = configOverride.URLs
 		openIDProvider.Claims = configOverride.Claims
@@ -308,6 +309,7 @@ func convertOpenIDIDP(ctx context.Context, providerConfig *configv1.IdentityProv
 			}
 		}
 		openIDProvider.Claims = osinv1.OpenIDClaims{
+			// There is no longer a user-facing setting for ID as it is considered unsafe
 			ID:                []string{configv1.UserIDClaim},
 			PreferredUsername: openIDConfig.Claims.PreferredUsername,
 			Name:              openIDConfig.Claims.Name,
@@ -322,6 +324,9 @@ func convertOpenIDIDP(ctx context.Context, providerConfig *configv1.IdentityProv
 	if configOverride != nil && configOverride.Challenge != nil {
 		data.challenge = *configOverride.Challenge
 	} else {
+		// openshift CR validating in kube-apiserver does not allow
+		// challenge-redirecting IdPs to be configured with OIDC so it is safe
+		// to allow challenge-issuing flow if it's available on the OIDC side
 		challengeFlowsAllowed, err := checkOIDCPasswordGrantFlow(ctx, kclient, openIDProvider.URLs.Token, openIDConfig.ClientID, namespace, openIDConfig.CA, openIDConfig.ClientSecret, skipKonnectivityDialer)
 		if err != nil {
 			return nil, fmt.Errorf("error attempting password grant flow: %v", err)

control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.go

@@ -381,7 +381,7 @@ func convertOpenIDIDP(
 		ExtraScopes:              openIDConfig.ExtraScopes,
 		ExtraAuthorizeParameters: openIDConfig.ExtraAuthorizeParameters,
 	}
-	//Handle special case for IBM Cloud's OIDC provider (need to override some fields not available in public api)
+	// Handle special case for IBM Cloud's OIDC provider (need to override some fields not available in public api)
 	if configOverride != nil {
 		openIDProvider.URLs = configOverride.URLs
 		openIDProvider.Claims = configOverride.Claims