ARO-17482 resolve PR comments

Author: rajdeepc2792

admin/server/go.mod

@@ -58,6 +58,7 @@ require (
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/gorilla/css v1.0.1 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3 // indirect
+	github.com/hashicorp/go-version v1.7.0 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/jedib0t/go-pretty/v6 v6.6.7 // indirect
 	github.com/josharian/intern v1.0.0 // indirect

admin/server/go.sum

@@ -106,6 +106,8 @@ github.com/gorilla/css v1.0.1 h1:ntNaBIghp6JmvWnxbZKANoLyuXTPZ4cAMlo6RyhlbO8=
 github.com/gorilla/css v1.0.1/go.mod h1:BvnYkspnSzMmwRK+b8/xgNPLiIuNZr6vbZBTPQ2A3b0=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3 h1:NmZ1PKzSTQbuGHw9DGPFomqkkLWMC+vZCkfs+FHv1Vg=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3/go.mod h1:zQrxl1YP88HQlA6i9c63DSVPFklWpGX4OWAc9bFuaH4=
+github.com/hashicorp/go-version v1.7.0 h1:5tqGy27NaOTB8yJKUZELlFAS/LTKJkrmONwQKeRZfjY=
+github.com/hashicorp/go-version v1.7.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
 github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
 github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
 github.com/itchyny/gojq v0.12.7 h1:hYPTpeWfrJ1OT+2j6cvBScbhl0TkdwGM4bc66onUSOQ=

admin/server/handlers/hcp/breakglass/create.go

@@ -16,7 +16,6 @@ package breakglass
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"net/http"
 	"time"
@@ -25,8 +24,7 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/utils/set"
 
-	ocmerrors "github.com/openshift-online/ocm-sdk-go/errors"
-
+	hcphelpers "github.com/Azure/ARO-HCP/admin/server/handlers/hcp"
 	"github.com/Azure/ARO-HCP/admin/server/middleware"
 	"github.com/Azure/ARO-HCP/internal/api/arm"
 	"github.com/Azure/ARO-HCP/internal/database"
@@ -73,12 +71,12 @@ func (h *HCPBreakglassSessionCreationHandler) ServeHTTP(writer http.ResponseWrit
 
 	clusterHypershiftDetails, err := h.csClient.GetClusterHypershiftDetails(request.Context(), hcp.ServiceProviderProperties.ClusterServiceID)
 	if err != nil {
-		return clusterServiceError(err, "hypershift details")
+		return hcphelpers.ClusterServiceError(err, "hypershift details")
 	}
 
 	provisionShard, err := h.csClient.GetClusterProvisionShard(request.Context(), hcp.ServiceProviderProperties.ClusterServiceID)
 	if err != nil {
-		return clusterServiceError(err, "provision shard")
+		return hcphelpers.ClusterServiceError(err, "provision shard")
 	}
 
 	group, ttl, err := h.validateSessionParameters(request)
@@ -185,16 +183,3 @@ func (h *HCPBreakglassSessionCreationHandler) validateSessionParameters(request
 
 	return body.Group, ttl, utilerrors.NewAggregate(errs)
 }
-
-// clusterServiceError checks if err is an OCM not-found error and returns a
-// specific CloudError. This prevents ReportError from misinterpreting it as
-// "HCP resource not found" (the HCP was already found in the database).
-// Non-OCM errors are wrapped for ReportError to handle as internal errors.
-func clusterServiceError(err error, what string) error {
-	var ocmErr *ocmerrors.Error
-	if errors.As(err, &ocmErr) && ocmErr.Status() == http.StatusNotFound {
-		return arm.NewCloudError(http.StatusNotFound, arm.CloudErrorCodeNotFound, "",
-			"%s not found in cluster service", what)
-	}
-	return fmt.Errorf("failed to get %s from cluster service: %w", what, err)
-}

admin/server/handlers/hcp/helpers.go

@@ -0,0 +1,38 @@
+// Copyright 2025 Microsoft Corporation
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package hcp
+
+import (
+	"errors"
+	"fmt"
+	"net/http"
+
+	ocmerrors "github.com/openshift-online/ocm-sdk-go/errors"
+
+	"github.com/Azure/ARO-HCP/internal/api/arm"
+)
+
+// ClusterServiceError checks if err is an OCM not-found error and returns a
+// specific CloudError. This prevents ReportError from misinterpreting it as
+// "HCP resource not found" (the HCP was already found in the database).
+// Non-OCM errors are wrapped for ReportError to handle as internal errors.
+func ClusterServiceError(err error, what string) error {
+	var ocmErr *ocmerrors.Error
+	if errors.As(err, &ocmErr) && ocmErr.Status() == http.StatusNotFound {
+		return arm.NewCloudError(http.StatusNotFound, arm.CloudErrorCodeNotFound, "",
+			"%s not found in cluster service", what)
+	}
+	return fmt.Errorf("failed to get %s from cluster service: %w", what, err)
+}

admin/server/handlers/hcp/helpers_test.go

@@ -0,0 +1,156 @@
+// Copyright 2025 Microsoft Corporation
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package hcp
+
+import (
+	"errors"
+	"net/http"
+	"testing"
+
+	ocmerrors "github.com/openshift-online/ocm-sdk-go/errors"
+
+	"github.com/Azure/ARO-HCP/internal/api/arm"
+)
+
+func TestClusterServiceError(t *testing.T) {
+	// Helper to create OCM errors
+	createOCMError := func(status int) error {
+		ocmErr, err := ocmerrors.NewError().Status(status).Build()
+		if err != nil {
+			t.Fatalf("Failed to create OCM error: %v", err)
+		}
+		return ocmErr
+	}
+
+	tests := []struct {
+		name               string
+		err                error
+		what               string
+		expectedStatusCode int
+		expectedErrorCode  string
+		expectedMessage    string
+		isCloudError       bool
+	}{
+		{
+			name:               "OCM not-found error returns 404 CloudError",
+			err:                createOCMError(http.StatusNotFound),
+			what:               "cluster data",
+			expectedStatusCode: http.StatusNotFound,
+			expectedErrorCode:  arm.CloudErrorCodeNotFound,
+			expectedMessage:    "cluster data not found in cluster service",
+			isCloudError:       true,
+		},
+		{
+			name:               "OCM not-found error for hypershift details",
+			err:                createOCMError(http.StatusNotFound),
+			what:               "hypershift details",
+			expectedStatusCode: http.StatusNotFound,
+			expectedErrorCode:  arm.CloudErrorCodeNotFound,
+			expectedMessage:    "hypershift details not found in cluster service",
+			isCloudError:       true,
+		},
+		{
+			name:            "OCM internal server error wraps error",
+			err:             createOCMError(http.StatusInternalServerError),
+			what:            "cluster data",
+			expectedMessage: "failed to get cluster data from cluster service",
+			isCloudError:    false,
+		},
+		{
+			name:            "OCM bad request error wraps error",
+			err:             createOCMError(http.StatusBadRequest),
+			what:            "provision shard",
+			expectedMessage: "failed to get provision shard from cluster service",
+			isCloudError:    false,
+		},
+		{
+			name:            "OCM unauthorized error wraps error",
+			err:             createOCMError(http.StatusUnauthorized),
+			what:            "cluster data",
+			expectedMessage: "failed to get cluster data from cluster service",
+			isCloudError:    false,
+		},
+		{
+			name:            "non-OCM error wraps error",
+			err:             errors.New("network timeout"),
+			what:            "cluster data",
+			expectedMessage: "failed to get cluster data from cluster service",
+			isCloudError:    false,
+		},
+		{
+			name:            "generic error wraps error",
+			err:             errors.New("connection refused"),
+			what:            "hypershift details",
+			expectedMessage: "failed to get hypershift details from cluster service",
+			isCloudError:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := ClusterServiceError(tt.err, tt.what)
+
+			if result == nil {
+				t.Fatal("Expected error but got nil")
+			}
+
+			if tt.isCloudError {
+				// Check if it's a CloudError
+				var cloudErr *arm.CloudError
+				if !errors.As(result, &cloudErr) {
+					t.Fatalf("Expected CloudError but got %T: %v", result, result)
+				}
+
+				if cloudErr.StatusCode != tt.expectedStatusCode {
+					t.Errorf("Expected status code %d, got %d", tt.expectedStatusCode, cloudErr.StatusCode)
+				}
+
+				if cloudErr.Code != tt.expectedErrorCode {
+					t.Errorf("Expected error code %q, got %q", tt.expectedErrorCode, cloudErr.Code)
+				}
+
+				if cloudErr.Message != tt.expectedMessage {
+					t.Errorf("Expected message %q, got %q", tt.expectedMessage, cloudErr.Message)
+				}
+			} else {
+				// Check if it's a wrapped error (not CloudError)
+				var cloudErr *arm.CloudError
+				if errors.As(result, &cloudErr) {
+					t.Fatalf("Expected wrapped error but got CloudError: %v", result)
+				}
+
+				// Verify error message contains expected text
+				if !contains(result.Error(), tt.expectedMessage) {
+					t.Errorf("Expected error to contain %q, got %q", tt.expectedMessage, result.Error())
+				}
+
+				// Verify original error is wrapped
+				if !errors.Is(result, tt.err) {
+					t.Errorf("Expected error to wrap original error")
+				}
+			}
+		})
+	}
+}
+
+// contains checks if a string contains a substring
+func contains(s, substr string) bool {
+	for i := 0; i <= len(s)-len(substr); i++ {
+		if s[i:i+len(substr)] == substr {
+			return true
+		}
+	}
+	return false
+}

admin/server/handlers/hcp/serialconsole.go

@@ -26,6 +26,7 @@ import (
 	"github.com/Azure/ARO-HCP/internal/fpa"
 	"github.com/Azure/ARO-HCP/internal/ocm"
 	"github.com/Azure/ARO-HCP/internal/utils"
+	"github.com/Azure/ARO-HCP/internal/validation"
 )
 
 // HCPSerialConsoleHandler handles requests to retrieve VM serial console logs
@@ -76,6 +77,15 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request
 		)
 	}
 
+	if !validation.IsValidAzureVMName(vmName) {
+		return arm.NewCloudError(
+			http.StatusBadRequest,
+			arm.CloudErrorCodeInvalidRequestContent,
+			"",
+			"vmName contains invalid characters or format",
+		)
+	}
+
 	// get HCP details
 	hcp, err := h.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(request.Context(), resourceID.Name)
 	if err != nil {
@@ -85,19 +95,19 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request
 	// get CS cluster data to retrieve tenant ID
 	csCluster, err := h.csClient.GetCluster(request.Context(), hcp.ServiceProviderProperties.ClusterServiceID)
 	if err != nil {
-		return fmt.Errorf("failed to get CS cluster data: %w", err)
+		return ClusterServiceError(err, "cluster data")
 	}
 
 	// get FPA credentials for customer tenant
 	tokenCredential, err := h.fpaCredentialRetriever.RetrieveCredential(csCluster.Azure().TenantID())
 	if err != nil {
-		return fmt.Errorf("failed to get FPA token credentials: %w", err)
+		return fmt.Errorf("failed to retrieve Azure credentials: %w", err)
 	}
 
 	// Create Azure Compute client for customer subscription
 	computeClient, err := armcompute.NewVirtualMachinesClient(hcp.ID.SubscriptionID, tokenCredential, nil)
 	if err != nil {
-		return fmt.Errorf("failed to create compute client: %w", err)
+		return fmt.Errorf("failed to create Azure compute client: %w", err)
 	}
 
 	// Retrieve boot diagnostics data containing serial console blob URI
@@ -109,7 +119,25 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request
 		nil,
 	)
 	if err != nil {
-		return fmt.Errorf("failed to retrieve boot diagnostics data: %w", err)
+		// Azure returns 404 when VM or resource group doesn't exist
+		if database.IsResponseError(err, http.StatusNotFound) {
+			return arm.NewCloudError(
+				http.StatusNotFound,
+				arm.CloudErrorCodeNotFound,
+				"",
+				"VM %s not found in resource group %s", vmName, managedResourceGroup,
+			)
+		}
+		// Azure returns 409 when boot diagnostics is disabled
+		if database.IsResponseError(err, http.StatusConflict) {
+			return arm.NewCloudError(
+				http.StatusConflict,
+				arm.CloudErrorCodeConflict,
+				"",
+				"Diagnostics might be disabled for VM %s", vmName,
+			)
+		}
+		return fmt.Errorf("failed to retrieve boot diagnostics data for VM %s: %w", vmName, err)
 	}
 
 	// verify serial console log blob URI is available