From 6982436d3321f6e5aca6c871861b27b0b554aff8 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 27 Feb 2026 13:11:39 -0500 Subject: [PATCH 01/13] ARO-17482 admin-api nodepool VM serial console logs --- admin/server/go.mod | 1 + admin/server/go.sum | 6 + admin/server/handlers/hcp/serialconsole.go | 155 +++++++++++++++++++++ admin/server/server/admin.go | 4 + 4 files changed, 166 insertions(+) create mode 100644 admin/server/handlers/hcp/serialconsole.go diff --git a/admin/server/go.mod b/admin/server/go.mod index 6b35dc56bd..b80ae2e5eb 100644 --- a/admin/server/go.mod +++ b/admin/server/go.mod @@ -7,6 +7,7 @@ require ( github.com/Azure/ARO-HCP/sessiongate v0.0.0-00010101000000-000000000000 github.com/Azure/azure-kusto-go v0.16.1 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6 v6.2.0 github.com/go-logr/logr v1.4.3 github.com/microsoft/go-otel-audit v0.2.2 diff --git a/admin/server/go.sum b/admin/server/go.sum index aadfe1aee3..9b56a29adb 100644 --- a/admin/server/go.sum +++ b/admin/server/go.sum @@ -14,12 +14,18 @@ github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos v1.4.1 h1:ToPLhnXvatKVN4Zkcx github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos v1.4.1/go.mod h1:Krtog/7tz27z75TwM5cIS8bxEH4dcBUezcq+kGVeZEo= github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2 h1:9iefClla7iYpfYWdzPCRDozdmndjTm8DXdpCzPajMgA= github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2/go.mod h1:XtLgD3ZD34DAaVIIAyG3objl5DynM3CQ/vMcbBNJZGI= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 h1:LkHbJbgF3YyvC53aqYGR+wWQDn2Rdp9AQdGndf9QvY4= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0/go.mod h1:QyiQdW4f4/BIfB8ZutZ2s+28RAgfa/pT+zS++ZHyM1I= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFGRSlMKCQelWwxUyYVEUqseBJVemLyqWJjvMyt0do= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.1 h1:1kpY4qe+BGAH2ykv4baVSqyx+AY5VjXeJ15SldlU6hs= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.1/go.mod h1:nT6cWpWdUt+g81yuKmjeYPUtI73Ak3yQIT4PVVsCEEQ= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6 v6.2.0 h1:HYGD75g0bQ3VO/Omedm54v4LrD3B1cGImuRF3AJ5wLo= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6 v6.2.0/go.mod h1:ulHyBFJOI0ONiRL4vcJTmS7rx18jQQlEPmAgo80cRdM= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeployments v0.2.0 h1:bYq3jfB2x36hslKMHyge3+esWzROtJNk/4dCjsKlrl4= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeployments v0.2.0/go.mod h1:fewgRjNVE84QVVh798sIMFb7gPXPp7NmnekGnboSnXk= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 h1:Dd+RhdJn0OTtVGaeDLZpcumkIVCtA/3/Fo42+eoYvVM= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0/go.mod h1:5kakwfW5CjC9KK+Q4wjXAg+ShuIm2mBMua0ZFj2C8PE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources/v3 v3.0.1 h1:guyQA4b8XB2sbJZXzUnOF9mn0WDBv/ZT7me9wTipKtE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources/v3 v3.0.1/go.mod h1:8h8yhzh9o+0HeSIhUxYny+rEQajScrfIpNktvgYG3Q8= github.com/Azure/retry v0.0.0-20250221010952-92c9290cea0f h1:XjKfallhRhddiRmBG0u2gs+Rd75QjvAlPVDy3ZWLjPg= diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go new file mode 100644 index 0000000000..51a343d8f7 --- /dev/null +++ b/admin/server/handlers/hcp/serialconsole.go @@ -0,0 +1,155 @@ +// 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 ( + "fmt" + "io" + "net/http" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" + + "github.com/Azure/ARO-HCP/internal/api/arm" + "github.com/Azure/ARO-HCP/internal/database" + "github.com/Azure/ARO-HCP/internal/fpa" + "github.com/Azure/ARO-HCP/internal/ocm" + "github.com/Azure/ARO-HCP/internal/utils" +) + +// HCPSerialConsoleHandler handles requests to retrieve VM serial console logs +// for debugging purposes. This endpoint allows SREs to access boot diagnostics +// and console output from VMs in the HCP cluster's managed resource group. +type HCPSerialConsoleHandler struct { + dbClient database.DBClient + csClient ocm.ClusterServiceClientSpec + fpaCredentialRetriever fpa.FirstPartyApplicationTokenCredentialRetriever +} + +// NewHCPSerialConsoleHandler creates a new serial console handler with the required dependencies +func NewHCPSerialConsoleHandler( + dbClient database.DBClient, + csClient ocm.ClusterServiceClientSpec, + fpaCredentialRetriever fpa.FirstPartyApplicationTokenCredentialRetriever, +) *HCPSerialConsoleHandler { + return &HCPSerialConsoleHandler{ + dbClient: dbClient, + csClient: csClient, + fpaCredentialRetriever: fpaCredentialRetriever, + } +} + +// ServeHTTP handles GET requests to retrieve serial console output for a specified VM. +// Query parameters: +// - vmName (required): The name of the VM to retrieve console logs +func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request *http.Request) error { + // get the azure resource ID for this HCP + resourceID, err := utils.ResourceIDFromContext(request.Context()) + if err != nil { + return arm.NewCloudError( + http.StatusBadRequest, + arm.CloudErrorCodeInvalidRequestContent, + "", + "invalid resource identifier in request", + ) + } + + //Extract and validate vmName query parameter + vmName := request.URL.Query().Get("vmName") + if vmName == "" { + return arm.NewCloudError( + http.StatusBadRequest, + arm.CloudErrorCodeInvalidRequestContent, + "", + "vmName query parameter is required", + ) + } + + // get HCP details + hcp, err := h.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(request.Context(), resourceID.Name) + if err != nil { + return fmt.Errorf("failed to get HCP from database: %w", err) + } + + // 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) + } + + // 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) + } + + // 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) + } + + // Retrieve boot diagnostics data containing serial console blob URI + managedResourceGroup := hcp.CustomerProperties.Platform.ManagedResourceGroup + result, err := computeClient.RetrieveBootDiagnosticsData( + request.Context(), + managedResourceGroup, + vmName, + nil, + ) + if err != nil { + return fmt.Errorf("failed to retrieve boot diagnostics data: %w", err) + } + + // verify serial console log blob URI is available + if result.SerialConsoleLogBlobURI == nil || *result.SerialConsoleLogBlobURI == "" { + return arm.NewCloudError( + http.StatusNotFound, + arm.CloudErrorCodeNotFound, + "", + "serial console not available for VM %s", + vmName, + ) + } + + // fetch blob content via HTTP GET + // The blob URI contains a SAS token for authentication, so we can use a simple HTTP GET + blobReq, err := http.NewRequestWithContext(request.Context(), http.MethodGet, *result.SerialConsoleLogBlobURI, nil) + if err != nil { + return fmt.Errorf("failed to create blob request: %w", err) + } + + // download blob content + httpClient := &http.Client{} + blobResp, err := httpClient.Do(blobReq) + if err != nil { + return fmt.Errorf("failed to download serial console log: %w", err) + } + defer blobResp.Body.Close() + + if blobResp.StatusCode != http.StatusOK { + return fmt.Errorf("failed to download serial console log: unexpected status %d", blobResp.StatusCode) + } + + // stream response as text/plain + writer.Header().Set("Content-Type", "text/plain; charset=utf-8") + writer.WriteHeader(http.StatusOK) + _, err = io.Copy(writer, blobResp.Body) + if err != nil { + // If we fail during streaming, log it + return fmt.Errorf("failed to stream serial console log: %w", err) + } + + return nil +} diff --git a/admin/server/server/admin.go b/admin/server/server/admin.go index f6e8f35be2..85889eb9ce 100644 --- a/admin/server/server/admin.go +++ b/admin/server/server/admin.go @@ -109,6 +109,10 @@ func NewAdminAPI( middleware.V1HCPResourcePattern("GET", "/cosmosdump"), hcpMiddleware.HandlerFunc(errorutils.ReportError(cosmosdump.NewCosmosDumpHandler(dbClient).ServeHTTP)), ) + middlewareMux.Handle( + middleware.V1HCPResourcePattern("GET", "/serialconsole"), + hcpMiddleware.HandlerFunc(errorutils.ReportError(hcp.NewHCPSerialConsoleHandler(dbClient, clustersServiceClient, fpaCredentialRetriever).ServeHTTP)), + ) // Non-HCP admin routes middlewareMux.Handle("GET /admin/helloworld", handlers.HelloWorldHandler()) From 303e699edcf1250eec45733f20907f9bd0bf8b5a Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 27 Feb 2026 13:27:33 -0500 Subject: [PATCH 02/13] ARO-17482 admin-api serial console unit-test --- .../server/handlers/hcp/serialconsole_test.go | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 admin/server/handlers/hcp/serialconsole_test.go diff --git a/admin/server/handlers/hcp/serialconsole_test.go b/admin/server/handlers/hcp/serialconsole_test.go new file mode 100644 index 0000000000..aefa60adaf --- /dev/null +++ b/admin/server/handlers/hcp/serialconsole_test.go @@ -0,0 +1,252 @@ +// 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 ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/go-logr/logr/testr" + sdk "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" + "go.uber.org/mock/gomock" + + "github.com/Azure/ARO-HCP/internal/api" + "github.com/Azure/ARO-HCP/internal/database" + "github.com/Azure/ARO-HCP/internal/ocm" + "github.com/Azure/ARO-HCP/internal/utils" +) + +// mockFPACredentialRetriever implements fpa.FirstPartyApplicationTokenCredentialRetriever for testing +type mockFPACredentialRetriever struct { + credential azcore.TokenCredential + err error +} + +func (m *mockFPACredentialRetriever) RetrieveCredential(tenantId string, additionallyAllowedTenants ...string) (azcore.TokenCredential, error) { + return m.credential, m.err +} + +func TestSerialConsoleHandler(t *testing.T) { + tests := []struct { + name string + resourceID string + vmName string + setupMocks func(*gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) + expectedStatusCode int + expectedError string + }{ + { + name: "missing vmName parameter", + resourceID: api.TestClusterResourceID, + vmName: "", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + return database.NewMockDBClient(ctrl), ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + }, + expectedStatusCode: http.StatusBadRequest, + expectedError: "vmName query parameter is required", + }, + { + name: "HCP cluster not found in database", + resourceID: api.TestClusterResourceID, + vmName: "test-vm", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + mockDB := database.NewMockDBClient(ctrl) + mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + + resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) + mockDB.EXPECT(). + HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). + Return(mockCRUD) + mockCRUD.EXPECT(). + Get(gomock.Any(), resourceID.Name). + Return(nil, fmt.Errorf("cluster not found")) + + return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + }, + expectedStatusCode: http.StatusInternalServerError, + expectedError: "cluster not found", + }, + { + name: "CS cluster retrieval fails", + resourceID: api.TestClusterResourceID, + vmName: "test-vm", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + mockDB := database.NewMockDBClient(ctrl) + mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + + resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) + clusterServiceID, _ := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cs-id") + + hcp := &api.HCPOpenShiftCluster{ + ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ + ClusterServiceID: clusterServiceID, + }, + } + + mockDB.EXPECT(). + HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). + Return(mockCRUD) + mockCRUD.EXPECT(). + Get(gomock.Any(), resourceID.Name). + Return(hcp, nil) + + mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) + mockCS.EXPECT(). + GetCluster(gomock.Any(), clusterServiceID). + Return(nil, fmt.Errorf("CS cluster not found")) + + return mockDB, mockCS, &mockFPACredentialRetriever{} + }, + expectedStatusCode: http.StatusInternalServerError, + expectedError: "CS cluster not found", + }, + { + name: "FPA credential retrieval fails", + resourceID: api.TestClusterResourceID, + vmName: "test-vm", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + mockDB := database.NewMockDBClient(ctrl) + mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + + resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) + clusterServiceID, _ := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cs-id") + + hcp := &api.HCPOpenShiftCluster{ + ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ + ClusterServiceID: clusterServiceID, + }, + } + + mockDB.EXPECT(). + HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). + Return(mockCRUD) + mockCRUD.EXPECT(). + Get(gomock.Any(), resourceID.Name). + Return(hcp, nil) + + // Mock CS cluster with Azure tenant ID + csCluster, _ := sdk.NewCluster(). + Azure(sdk.NewAzure().TenantID("test-tenant-id")). + Build() + + mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) + mockCS.EXPECT(). + GetCluster(gomock.Any(), clusterServiceID). + Return(csCluster, nil) + + // Mock FPA failure + mockFPA := &mockFPACredentialRetriever{ + err: fmt.Errorf("failed to get FPA credentials"), + } + + return mockDB, mockCS, mockFPA + }, + expectedStatusCode: http.StatusInternalServerError, + expectedError: "failed to get FPA credentials", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // Setup mocks + mockDB, mockCS, mockFPA := tt.setupMocks(ctrl) + + // Create handler + handler := NewHCPSerialConsoleHandler(mockDB, mockCS, mockFPA) + + // Parse resource ID and add to context + resourceID, err := azcorearm.ParseResourceID(tt.resourceID) + if err != nil { + t.Fatalf("Failed to parse resource ID: %v", err) + } + ctx = utils.ContextWithResourceID(ctx, resourceID) + + // Create request + url := "/serialconsole" + if tt.vmName != "" { + url += "?vmName=" + tt.vmName + } + req := httptest.NewRequest(http.MethodGet, url, nil) + req = req.WithContext(ctx) + + // Execute request + recorder := httptest.NewRecorder() + err = handler.ServeHTTP(recorder, req) + + // Validate response + if tt.expectedStatusCode >= 400 { + if err == nil { + t.Errorf("Expected error but got none") + } else if tt.expectedError != "" { + if !containsError(err, tt.expectedError) { + t.Errorf("Expected error containing %q but got %q", tt.expectedError, err.Error()) + } + } + } else { + if err != nil { + t.Errorf("Expected no error but got %q", err.Error()) + } + } + }) + } +} + +func TestSerialConsoleHandler_InvalidResourceID(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockDB := database.NewMockDBClient(ctrl) + mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) + mockFPA := &mockFPACredentialRetriever{} + + handler := NewHCPSerialConsoleHandler(mockDB, mockCS, mockFPA) + + // Create request WITHOUT adding resource ID to context + req := httptest.NewRequest(http.MethodGet, "/serialconsole?vmName=test-vm", nil) + req = req.WithContext(ctx) + + recorder := httptest.NewRecorder() + err := handler.ServeHTTP(recorder, req) + + if err == nil { + t.Error("Expected error for missing resource ID but got none") + } else if !containsError(err, "invalid resource identifier") { + t.Errorf("Expected error containing 'invalid resource identifier' but got %q", err.Error()) + } +} + +// containsError checks if an error message contains the expected substring +func containsError(err error, expected string) bool { + if err == nil { + return expected == "" + } + errMsg := err.Error() + for i := 0; i <= len(errMsg)-len(expected); i++ { + if errMsg[i:i+len(expected)] == expected { + return true + } + } + return false +} From 91a50e61ced1b016ff2b5d695499a8c556fc600e Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 27 Feb 2026 14:53:28 -0500 Subject: [PATCH 03/13] ARO-17482 admin-api serial console GA e2e --- test-integration/go.mod | 1 + test-integration/go.sum | 4 + test/e2e/admin_api.go | 91 +++++++++++++++++++ ...cd_check_paralleldev_cd_check_parallel.txt | 1 + ...rp_api_compat_all_parallel_development.txt | 1 + test/util/framework/admin_api_helpers.go | 87 ++++++++++++++++++ 6 files changed, 185 insertions(+) diff --git a/test-integration/go.mod b/test-integration/go.mod index fc7d746ba7..208a65e5e1 100644 --- a/test-integration/go.mod +++ b/test-integration/go.mod @@ -27,6 +27,7 @@ require ( require ( cloud.google.com/go/compute/metadata v0.9.0 // indirect github.com/Azure/azure-kusto-go v0.16.1 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6 v6.2.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeployments v0.2.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/tracing/azotel v0.4.0 // indirect diff --git a/test-integration/go.sum b/test-integration/go.sum index 83af9d3330..a1dc476739 100644 --- a/test-integration/go.sum +++ b/test-integration/go.sum @@ -16,6 +16,10 @@ github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos v1.4.1 h1:ToPLhnXvatKVN4Zkcx github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos v1.4.1/go.mod h1:Krtog/7tz27z75TwM5cIS8bxEH4dcBUezcq+kGVeZEo= github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2 h1:9iefClla7iYpfYWdzPCRDozdmndjTm8DXdpCzPajMgA= github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2/go.mod h1:XtLgD3ZD34DAaVIIAyG3objl5DynM3CQ/vMcbBNJZGI= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 h1:LkHbJbgF3YyvC53aqYGR+wWQDn2Rdp9AQdGndf9QvY4= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0/go.mod h1:QyiQdW4f4/BIfB8ZutZ2s+28RAgfa/pT+zS++ZHyM1I= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFGRSlMKCQelWwxUyYVEUqseBJVemLyqWJjvMyt0do= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.1 h1:1kpY4qe+BGAH2ykv4baVSqyx+AY5VjXeJ15SldlU6hs= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.1/go.mod h1:nT6cWpWdUt+g81yuKmjeYPUtI73Ak3yQIT4PVVsCEEQ= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6 v6.2.0 h1:HYGD75g0bQ3VO/Omedm54v4LrD3B1cGImuRF3AJ5wLo= diff --git a/test/e2e/admin_api.go b/test/e2e/admin_api.go index e620613698..438f5c399c 100644 --- a/test/e2e/admin_api.go +++ b/test/e2e/admin_api.go @@ -258,6 +258,97 @@ var _ = Describe("SRE", func() { By("and expecting cluster access to be denied") Expect(verifiers.VerifyWhoAmI("aro-sre").Verify(ctx, otherUserRestConfig)).To(HaveOccurred()) }) + + It("should be able to retrieve serial console logs for a VM", + labels.RequireNothing, + labels.Medium, + labels.Positive, + labels.CoreInfraService, + labels.DevelopmentOnly, + labels.AroRpApiCompatible, + func(ctx context.Context) { + const ( + engineeringNetworkSecurityGroupName = "sre-nsg-name" + engineeringVnetName = "sre-vnet-name" + engineeringVnetSubnetName = "sre-vnet-subnet1" + engineeringClusterName = "sre-hcp-cluster-sc" + ) + tc := framework.NewTestContext() + + if tc.UsePooledIdentities() { + err := tc.AssignIdentityContainers(ctx, 1, 60*time.Second) + Expect(err).NotTo(HaveOccurred()) + } + + By("creating a resource group") + resourceGroup, err := tc.NewResourceGroup(ctx, "admin-api-serialconsole", tc.Location()) + Expect(err).NotTo(HaveOccurred()) + + By("creating cluster parameters") + clusterParams := framework.NewDefaultClusterParams() + clusterParams.ClusterName = engineeringClusterName + managedResourceGroupName := framework.SuffixName(*resourceGroup.Name, "-managed", 64) + clusterParams.ManagedResourceGroupName = managedResourceGroupName + + By("creating customer resources") + clusterParams, err = tc.CreateClusterCustomerResources(ctx, + resourceGroup, + clusterParams, + map[string]interface{}{ + "customerNsgName": engineeringNetworkSecurityGroupName, + "customerVnetName": engineeringVnetName, + "customerVnetSubnetName": engineeringVnetSubnetName, + }, + TestArtifactsFS, + framework.RBACScopeResourceGroup, + ) + Expect(err).NotTo(HaveOccurred()) + + By("creating the HCP cluster") + err = tc.CreateHCPClusterFromParam( + ctx, + GinkgoLogr, + *resourceGroup.Name, + clusterParams, + 45*time.Minute, + ) + Expect(err).NotTo(HaveOccurred()) + + By("creating a nodepool to provision worker VMs") + nodePoolParams := framework.NewDefaultNodePoolParams() + nodePoolParams.ClusterName = engineeringClusterName + nodePoolParams.NodePoolName = "worker" + nodePoolParams.Replicas = int32(1) + + err = tc.CreateNodePoolFromParam(ctx, + *resourceGroup.Name, + engineeringClusterName, + nodePoolParams, + 45*time.Minute, + ) + Expect(err).NotTo(HaveOccurred()) + + hcpResourceID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenshift/hcpOpenShiftClusters/%s", api.Must(tc.SubscriptionID(ctx)), *resourceGroup.Name, engineeringClusterName) + + By("resolving current Azure identity") + currentIdentity, err := tc.GetCurrentAzureIdentityDetails(ctx) + Expect(err).NotTo(HaveOccurred()) + + By("getting VM name from managed resource group") + vmName, err := tc.GetFirstVMFromManagedResourceGroup(ctx, managedResourceGroupName) + Expect(err).NotTo(HaveOccurred()) + Expect(vmName).NotTo(BeEmpty()) + + By(fmt.Sprintf("retrieving serial console logs for VM %s", vmName)) + logs, err := tc.GetSerialConsoleLogs(ctx, hcpResourceID, vmName, currentIdentity) + Expect(err).NotTo(HaveOccurred()) + Expect(logs).NotTo(BeEmpty()) + + By("verifying serial console logs contain boot information") + // Serial console logs typically contain boot messages, kernel output, or systemd logs + // We just verify that we got some content back + Expect(len(logs)).To(BeNumerically(">", 0)) + }) }) // waitForSessionExpiration sleeps until the session's expiration time has passed. diff --git a/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_dev_cd_check_paralleldev_cd_check_parallel.txt b/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_dev_cd_check_paralleldev_cd_check_parallel.txt index 62623410e8..a79443d813 100644 --- a/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_dev_cd_check_paralleldev_cd_check_parallel.txt +++ b/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_dev_cd_check_paralleldev_cd_check_parallel.txt @@ -1,5 +1,6 @@ Customer should be able to list available HCP OpenShift versions and validate response content SRE should be able to log into a cluster via a breakglass session +SRE should be able to retrieve serial console logs for a VM Customer should be able to test admin credentials before cluster ready, then full admin credential lifecycle Customer should be able to create node pools with ARM64-based VMs Authorized CIDRs Connectivity should allow API access only from authorized VM IP diff --git a/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt b/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt index 7afecfd716..d568c30d0a 100644 --- a/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt +++ b/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt @@ -1,5 +1,6 @@ Customer should be able to list available HCP OpenShift versions and validate response content SRE should be able to log into a cluster via a breakglass session +SRE should be able to retrieve serial console logs for a VM Customer should be able to test admin credentials before cluster ready, then full admin credential lifecycle Customer should be able to create node pools with ARM64-based VMs Authorized CIDRs Connectivity should allow API access only from authorized VM IP diff --git a/test/util/framework/admin_api_helpers.go b/test/util/framework/admin_api_helpers.go index 01c9acc5a2..9925c24ef9 100644 --- a/test/util/framework/admin_api_helpers.go +++ b/test/util/framework/admin_api_helpers.go @@ -34,6 +34,7 @@ import ( clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" ) const ( @@ -311,3 +312,89 @@ func (tc *perItOrDescribeTestContext) CreateSREBreakglassCredentials(ctx context } return restConfig, expiresAt, nil } + +// GetFirstVMFromManagedResourceGroup retrieves the name of the first VM found in the managed resource group. +// Returns an error if no VMs are found or if the Azure API calls fail. +func (tc *perItOrDescribeTestContext) GetFirstVMFromManagedResourceGroup(ctx context.Context, managedResourceGroupName string) (string, error) { + cred, err := tc.perBinaryInvocationTestContext.getAzureCredentials() + if err != nil { + return "", fmt.Errorf("failed to get Azure credentials: %w", err) + } + + subscriptionID, err := tc.SubscriptionID(ctx) + if err != nil { + return "", fmt.Errorf("failed to get subscription ID: %w", err) + } + + computeClient, err := armcompute.NewVirtualMachinesClient(subscriptionID, cred, nil) + if err != nil { + return "", fmt.Errorf("failed to create compute client: %w", err) + } + + pager := computeClient.NewListPager(managedResourceGroupName, nil) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return "", fmt.Errorf("failed to list VMs: %w", err) + } + if len(page.Value) > 0 { + if page.Value[0].Name != nil { + return *page.Value[0].Name, nil + } + } + } + + return "", fmt.Errorf("no VMs found in managed resource group %s", managedResourceGroupName) +} + +// GetSerialConsoleLogs retrieves serial console logs for a VM in an HCP cluster's managed resource group +func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, resourceID string, vmName string, identityDetails *AzureIdentityDetails) (string, error) { + tlsConfig := &tls.Config{} + if IsDevelopmentEnvironment() { + tlsConfig.InsecureSkipVerify = true + } + httpClient := &http.Client{ + Transport: &clientPrincipalTransport{ + base: &http.Transport{ + TLSClientConfig: tlsConfig, + }, + identityDetails: identityDetails, + }, + Timeout: adminAPIRequestTimeout, + } + + adminAPIEndpoint := tc.perBinaryInvocationTestContext.adminAPIAddress + + serialConsoleEndpoint := fmt.Sprintf("%s/admin/v1/hcp%s/serialconsole?vmName=%s", + adminAPIEndpoint, + resourceID, + vmName, + ) + + By(fmt.Sprintf("reaching out to the admin API to retrieve serial console logs for VM %s: %s", vmName, serialConsoleEndpoint)) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, serialConsoleEndpoint, nil) + if err != nil { + return "", fmt.Errorf("failed to create request: %w", err) + } + + resp, err := httpClient.Do(req) + if err != nil { + return "", fmt.Errorf("failed to send request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return "", fmt.Errorf("expected status 200 OK, got %d (failed to read body: %w)", resp.StatusCode, readErr) + } + return "", fmt.Errorf("expected status 200 OK, got %d: %s", resp.StatusCode, string(body)) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("failed to read response body: %w", err) + } + + return string(body), nil +} From f890a44a85c491e39c039d7c68280e53cce4aa45 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 27 Feb 2026 15:22:27 -0500 Subject: [PATCH 04/13] ARO-17482 admin-api serial console README update --- admin/README.md | 1 + admin/server/go.mod | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/admin/README.md b/admin/README.md index 59fa1cd1c1..e865359a9e 100644 --- a/admin/README.md +++ b/admin/README.md @@ -18,6 +18,7 @@ This prefix is abbreviated as `{resourceId}` below. |--------|------|-------------| | `PUT` | `/admin/v1/hcp{resourceId}/breakglass?group=...&ttl=...` | Create a breakglass session ([details](breakglass.md)) | | `GET` | `/admin/v1/hcp{resourceId}/breakglass/{sessionName}/kubeconfig` | Get kubeconfig for a breakglass session ([details](breakglass.md)) | +| `GET` | `/admin/v1/hcp{resourceId}/serialconsole?vmName=...` | Retrieve serial console logs for a VM ([details](serialconsole.md)) | | `GET` | `/admin/v1/hcp{resourceId}/cosmosdump` | Cosmos DB dump for a cluster | | `GET` | `/admin/v1/hcp{resourceId}/helloworld` | HCP hello world (dev/test) | | `GET` | `/admin/helloworld` | Hello world (dev/test) | diff --git a/admin/server/go.mod b/admin/server/go.mod index b80ae2e5eb..b574938980 100644 --- a/admin/server/go.mod +++ b/admin/server/go.mod @@ -15,6 +15,7 @@ require ( github.com/prometheus/client_golang v1.23.2 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.11.1 + go.uber.org/mock v0.6.0 k8s.io/apimachinery v0.34.3 k8s.io/client-go v0.34.3 k8s.io/utils v0.0.0-20260108192941-914a6e750570 @@ -109,7 +110,6 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.40.0 // indirect go.opentelemetry.io/otel/trace v1.40.0 // indirect go.opentelemetry.io/proto/otlp v1.9.0 // indirect - go.uber.org/mock v0.6.0 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.49.0 // indirect From 3021a071e25dbf2a60a1db937e7565bd55bea371 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 27 Feb 2026 15:48:05 -0500 Subject: [PATCH 05/13] ARO-17482 lint fix --- admin/server/handlers/hcp/serialconsole_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/admin/server/handlers/hcp/serialconsole_test.go b/admin/server/handlers/hcp/serialconsole_test.go index aefa60adaf..14a41b293a 100644 --- a/admin/server/handlers/hcp/serialconsole_test.go +++ b/admin/server/handlers/hcp/serialconsole_test.go @@ -21,11 +21,13 @@ import ( "net/http/httptest" "testing" + "github.com/go-logr/logr/testr" + "go.uber.org/mock/gomock" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" - "github.com/go-logr/logr/testr" + sdk "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" - "go.uber.org/mock/gomock" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/database" From fb55ec2d388357986804afef5c410d5b533215ba Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 6 Mar 2026 08:01:44 -0500 Subject: [PATCH 06/13] ARO-17482 resolve PR comments --- admin/server/go.mod | 1 + admin/server/go.sum | 2 + .../server/handlers/hcp/breakglass/create.go | 21 +-- admin/server/handlers/hcp/helpers.go | 38 ++++ admin/server/handlers/hcp/helpers_test.go | 156 +++++++++++++++++ admin/server/handlers/hcp/serialconsole.go | 36 +++- .../server/handlers/hcp/serialconsole_test.go | 117 ++++++++++++- internal/validation/validate_vmname_test.go | 165 ++++++++++++++++++ internal/validation/validators.go | 7 + test/e2e/admin_api.go | 108 ++++++++++++ test/util/framework/admin_api_helpers.go | 74 ++++++-- 11 files changed, 680 insertions(+), 45 deletions(-) create mode 100644 admin/server/handlers/hcp/helpers.go create mode 100644 admin/server/handlers/hcp/helpers_test.go create mode 100644 internal/validation/validate_vmname_test.go diff --git a/admin/server/go.mod b/admin/server/go.mod index b574938980..59374fc45c 100644 --- a/admin/server/go.mod +++ b/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 diff --git a/admin/server/go.sum b/admin/server/go.sum index 9b56a29adb..daa9244e28 100644 --- a/admin/server/go.sum +++ b/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= diff --git a/admin/server/handlers/hcp/breakglass/create.go b/admin/server/handlers/hcp/breakglass/create.go index dbf0e90df4..80362bdf39 100644 --- a/admin/server/handlers/hcp/breakglass/create.go +++ b/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) -} diff --git a/admin/server/handlers/hcp/helpers.go b/admin/server/handlers/hcp/helpers.go new file mode 100644 index 0000000000..3b234fe2a1 --- /dev/null +++ b/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) +} diff --git a/admin/server/handlers/hcp/helpers_test.go b/admin/server/handlers/hcp/helpers_test.go new file mode 100644 index 0000000000..b9651afb37 --- /dev/null +++ b/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 +} diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go index 51a343d8f7..7c0f6ce2c7 100644 --- a/admin/server/handlers/hcp/serialconsole.go +++ b/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 diff --git a/admin/server/handlers/hcp/serialconsole_test.go b/admin/server/handlers/hcp/serialconsole_test.go index 14a41b293a..4324fae95d 100644 --- a/admin/server/handlers/hcp/serialconsole_test.go +++ b/admin/server/handlers/hcp/serialconsole_test.go @@ -16,6 +16,7 @@ package hcp import ( "context" + "errors" "fmt" "net/http" "net/http/httptest" @@ -28,8 +29,10 @@ import ( azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" sdk "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" + ocmerrors "github.com/openshift-online/ocm-sdk-go/errors" "github.com/Azure/ARO-HCP/internal/api" + "github.com/Azure/ARO-HCP/internal/api/arm" "github.com/Azure/ARO-HCP/internal/database" "github.com/Azure/ARO-HCP/internal/ocm" "github.com/Azure/ARO-HCP/internal/utils" @@ -65,7 +68,38 @@ func TestSerialConsoleHandler(t *testing.T) { expectedError: "vmName query parameter is required", }, { - name: "HCP cluster not found in database", + name: "invalid vmName format", + resourceID: api.TestClusterResourceID, + vmName: "-invalid-vm-name", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + return database.NewMockDBClient(ctrl), ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + }, + expectedStatusCode: http.StatusBadRequest, + expectedError: "vmName contains invalid characters or format", + }, + { + name: "HCP cluster not found in database (generic error)", + resourceID: api.TestClusterResourceID, + vmName: "test-vm", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + mockDB := database.NewMockDBClient(ctrl) + mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + + resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) + mockDB.EXPECT(). + HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). + Return(mockCRUD) + mockCRUD.EXPECT(). + Get(gomock.Any(), resourceID.Name). + Return(nil, fmt.Errorf("failed to get HCP from database: cluster not found")) + + return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + }, + expectedStatusCode: http.StatusInternalServerError, + expectedError: "failed to get HCP from database", + }, + { + name: "HCP cluster not found in database (404 ResponseError)", resourceID: api.TestClusterResourceID, vmName: "test-vm", setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { @@ -78,15 +112,52 @@ func TestSerialConsoleHandler(t *testing.T) { Return(mockCRUD) mockCRUD.EXPECT(). Get(gomock.Any(), resourceID.Name). - Return(nil, fmt.Errorf("cluster not found")) + Return(nil, &azcore.ResponseError{StatusCode: http.StatusNotFound}) return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} }, expectedStatusCode: http.StatusInternalServerError, - expectedError: "cluster not found", + expectedError: "failed to get HCP from database", // Wrapped error, ReportError converts to 404 ResourceNotFoundError + }, + { + name: "CS cluster not found (OCM 404)", + resourceID: api.TestClusterResourceID, + vmName: "test-vm", + setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { + mockDB := database.NewMockDBClient(ctrl) + mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + + resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) + clusterServiceID, _ := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cs-id") + + hcp := &api.HCPOpenShiftCluster{ + ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ + ClusterServiceID: clusterServiceID, + }, + } + + mockDB.EXPECT(). + HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). + Return(mockCRUD) + mockCRUD.EXPECT(). + Get(gomock.Any(), resourceID.Name). + Return(hcp, nil) + + // Create OCM 404 error + ocmErr, _ := ocmerrors.NewError().Status(http.StatusNotFound).Build() + + mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) + mockCS.EXPECT(). + GetCluster(gomock.Any(), clusterServiceID). + Return(nil, ocmErr) + + return mockDB, mockCS, &mockFPACredentialRetriever{} + }, + expectedStatusCode: http.StatusNotFound, + expectedError: "cluster data not found in cluster service", }, { - name: "CS cluster retrieval fails", + name: "CS cluster retrieval fails (generic error)", resourceID: api.TestClusterResourceID, vmName: "test-vm", setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { @@ -117,7 +188,7 @@ func TestSerialConsoleHandler(t *testing.T) { return mockDB, mockCS, &mockFPACredentialRetriever{} }, expectedStatusCode: http.StatusInternalServerError, - expectedError: "CS cluster not found", + expectedError: "failed to get cluster data from cluster service", }, { name: "FPA credential retrieval fails", @@ -161,7 +232,7 @@ func TestSerialConsoleHandler(t *testing.T) { return mockDB, mockCS, mockFPA }, expectedStatusCode: http.StatusInternalServerError, - expectedError: "failed to get FPA credentials", + expectedError: "failed to retrieve Azure credentials", }, } @@ -200,7 +271,23 @@ func TestSerialConsoleHandler(t *testing.T) { if tt.expectedStatusCode >= 400 { if err == nil { t.Errorf("Expected error but got none") - } else if tt.expectedError != "" { + return + } + + // Check if it's a CloudError when status is 400 or 404 + if tt.expectedStatusCode == http.StatusBadRequest || tt.expectedStatusCode == http.StatusNotFound { + var cloudErr *arm.CloudError + if !errors.As(err, &cloudErr) { + t.Errorf("Expected CloudError but got %T: %v", err, err) + return + } + if cloudErr.StatusCode != tt.expectedStatusCode { + t.Errorf("Expected status code %d but got %d", tt.expectedStatusCode, cloudErr.StatusCode) + } + } + + // Check error message + if tt.expectedError != "" { if !containsError(err, tt.expectedError) { t.Errorf("Expected error containing %q but got %q", tt.expectedError, err.Error()) } @@ -234,7 +321,21 @@ func TestSerialConsoleHandler_InvalidResourceID(t *testing.T) { if err == nil { t.Error("Expected error for missing resource ID but got none") - } else if !containsError(err, "invalid resource identifier") { + return + } + + // Should be a CloudError with 400 status + var cloudErr *arm.CloudError + if !errors.As(err, &cloudErr) { + t.Errorf("Expected CloudError but got %T: %v", err, err) + return + } + + if cloudErr.StatusCode != http.StatusBadRequest { + t.Errorf("Expected status code %d but got %d", http.StatusBadRequest, cloudErr.StatusCode) + } + + if !containsError(err, "invalid resource identifier") { t.Errorf("Expected error containing 'invalid resource identifier' but got %q", err.Error()) } } diff --git a/internal/validation/validate_vmname_test.go b/internal/validation/validate_vmname_test.go new file mode 100644 index 0000000000..ead07d0e59 --- /dev/null +++ b/internal/validation/validate_vmname_test.go @@ -0,0 +1,165 @@ +// 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 validation + +import ( + "testing" +) + +func TestIsValidAzureVMName(t *testing.T) { + tests := []struct { + name string + vmName string + expected bool + }{ + // Valid cases + { + name: "single alphanumeric character", + vmName: "a", + expected: true, + }, + { + name: "single digit", + vmName: "1", + expected: true, + }, + { + name: "simple alphanumeric name", + vmName: "testvm", + expected: true, + }, + { + name: "name with hyphen", + vmName: "test-vm", + expected: true, + }, + { + name: "name with underscore", + vmName: "test_vm", + expected: true, + }, + { + name: "name with period", + vmName: "test.vm", + expected: true, + }, + { + name: "name with all allowed characters", + vmName: "test-vm_01.master", + expected: true, + }, + { + name: "name ending with underscore", + vmName: "test_vm_", + expected: true, + }, + { + name: "name ending with digit", + vmName: "testvm01", + expected: true, + }, + { + name: "64 character name", + vmName: "a234567890123456789012345678901234567890123456789012345678901234", + expected: true, + }, + { + name: "uppercase alphanumeric", + vmName: "TestVM", + expected: true, + }, + { + name: "mixed case with special chars", + vmName: "Test-VM_01", + expected: true, + }, + + // Invalid cases + { + name: "empty string", + vmName: "", + expected: false, + }, + { + name: "starts with hyphen", + vmName: "-testvm", + expected: false, + }, + { + name: "starts with underscore", + vmName: "_testvm", + expected: false, + }, + { + name: "starts with period", + vmName: ".testvm", + expected: false, + }, + { + name: "ends with hyphen", + vmName: "testvm-", + expected: false, + }, + { + name: "ends with period", + vmName: "testvm.", + expected: false, + }, + { + name: "contains space", + vmName: "test vm", + expected: false, + }, + { + name: "contains special character @", + vmName: "test@vm", + expected: false, + }, + { + name: "contains special character #", + vmName: "test#vm", + expected: false, + }, + { + name: "too long (65 characters)", + vmName: "a2345678901234567890123456789012345678901234567890123456789012345", + expected: false, + }, + { + name: "only hyphen", + vmName: "-", + expected: false, + }, + { + name: "only underscore", + vmName: "_", + expected: false, + }, + { + name: "only period", + vmName: ".", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsValidAzureVMName(tt.vmName) + if result != tt.expected { + t.Errorf("IsValidAzureVMName(%q) = %v, expected %v", tt.vmName, result, tt.expected) + } + }) + } +} diff --git a/internal/validation/validators.go b/internal/validation/validators.go index e44ebfb8e2..c370d61f9f 100644 --- a/internal/validation/validators.go +++ b/internal/validation/validators.go @@ -248,6 +248,9 @@ var ( imageDigestMirroredRegistry = `^((?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?)(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$` imageDigestMirroredRegistryRegex = regexp.MustCompile(imageDigestMirroredRegistry) imageDigestMirroredRegistryErrorString = `(must be a valid mirrored image registry)` + + azureVMName = `^[a-zA-Z0-9]([a-zA-Z0-9._-]{0,62}[a-zA-Z0-9_])?$` + azureVMNameRegex = regexp.MustCompile(azureVMName) ) func MatchesRegex(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *string, regex *regexp.Regexp, errorString string) field.ErrorList { @@ -276,6 +279,10 @@ func ValidateUUID(_ context.Context, _ operation.Operation, fldPath *field.Path, return nil } +func IsValidAzureVMName(name string) bool { + return azureVMNameRegex.MatchString(name) +} + func CIDRv4(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *string) field.ErrorList { if value == nil { return nil diff --git a/test/e2e/admin_api.go b/test/e2e/admin_api.go index 438f5c399c..19827719db 100644 --- a/test/e2e/admin_api.go +++ b/test/e2e/admin_api.go @@ -348,6 +348,114 @@ var _ = Describe("SRE", func() { // Serial console logs typically contain boot messages, kernel output, or systemd logs // We just verify that we got some content back Expect(len(logs)).To(BeNumerically(">", 0)) + + By("testing error case: non-existent VM name") + _, err = tc.GetSerialConsoleLogs(ctx, hcpResourceID, "non-existent-vm-12345", currentIdentity) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("404")) + + By("testing error case: invalid VM name format") + _, err = tc.GetSerialConsoleLogs(ctx, hcpResourceID, "-invalid-vm-name", currentIdentity) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("400")) + + By("testing error case: empty VM name") + _, err = tc.GetSerialConsoleLogs(ctx, hcpResourceID, "", currentIdentity) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("400")) + }) + + It("should return 409 when boot diagnostics is disabled on a VM", + labels.RequireNothing, + labels.Medium, + labels.Negative, + labels.CoreInfraService, + labels.DevelopmentOnly, + labels.IntegrationOnly, + labels.AroRpApiCompatible, + func(ctx context.Context) { + const ( + engineeringNetworkSecurityGroupName = "sre-nsg-name" + engineeringVnetName = "sre-vnet-name" + engineeringVnetSubnetName = "sre-vnet-subnet1" + engineeringClusterName = "sre-hcp-cluster-bootdiag" + ) + tc := framework.NewTestContext() + + if tc.UsePooledIdentities() { + err := tc.AssignIdentityContainers(ctx, 1, 60*time.Second) + Expect(err).NotTo(HaveOccurred()) + } + + By("creating a resource group") + resourceGroup, err := tc.NewResourceGroup(ctx, "admin-api-serialconsole-bootdiag", tc.Location()) + Expect(err).NotTo(HaveOccurred()) + + By("creating cluster parameters") + clusterParams := framework.NewDefaultClusterParams() + clusterParams.ClusterName = engineeringClusterName + managedResourceGroupName := framework.SuffixName(*resourceGroup.Name, "-managed", 64) + clusterParams.ManagedResourceGroupName = managedResourceGroupName + + By("creating customer resources") + clusterParams, err = tc.CreateClusterCustomerResources(ctx, + resourceGroup, + clusterParams, + map[string]interface{}{ + "customerNsgName": engineeringNetworkSecurityGroupName, + "customerVnetName": engineeringVnetName, + "customerVnetSubnetName": engineeringVnetSubnetName, + }, + TestArtifactsFS, + framework.RBACScopeResourceGroup, + ) + Expect(err).NotTo(HaveOccurred()) + + By("creating the HCP cluster") + err = tc.CreateHCPClusterFromParam( + ctx, + GinkgoLogr, + *resourceGroup.Name, + clusterParams, + 45*time.Minute, + ) + Expect(err).NotTo(HaveOccurred()) + + By("creating a nodepool to provision worker VMs") + nodePoolParams := framework.NewDefaultNodePoolParams() + nodePoolParams.ClusterName = engineeringClusterName + nodePoolParams.NodePoolName = "worker" + nodePoolParams.Replicas = int32(1) + + err = tc.CreateNodePoolFromParam(ctx, + *resourceGroup.Name, + engineeringClusterName, + nodePoolParams, + 45*time.Minute, + ) + Expect(err).NotTo(HaveOccurred()) + + hcpResourceID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenshift/hcpOpenShiftClusters/%s", api.Must(tc.SubscriptionID(ctx)), *resourceGroup.Name, engineeringClusterName) + + By("resolving current Azure identity") + currentIdentity, err := tc.GetCurrentAzureIdentityDetails(ctx) + Expect(err).NotTo(HaveOccurred()) + + By("getting VM name from managed resource group") + vmName, err := tc.GetFirstVMFromManagedResourceGroup(ctx, managedResourceGroupName) + Expect(err).NotTo(HaveOccurred()) + Expect(vmName).NotTo(BeEmpty()) + + By("testing error case: boot diagnostics disabled") + err = tc.DisableVMBootDiagnostics(ctx, managedResourceGroupName, vmName) + Expect(err).NotTo(HaveOccurred()) + + _, err = tc.GetSerialConsoleLogs(ctx, hcpResourceID, vmName, currentIdentity) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("409")) + Expect(err.Error()).To(ContainSubstring("Conflict")) + Expect(err.Error()).To(ContainSubstring("Diagnostics might be disabled")) + Expect(err.Error()).To(ContainSubstring(vmName)) }) }) diff --git a/test/util/framework/admin_api_helpers.go b/test/util/framework/admin_api_helpers.go index 9925c24ef9..9f1a70af1b 100644 --- a/test/util/framework/admin_api_helpers.go +++ b/test/util/framework/admin_api_helpers.go @@ -34,6 +34,7 @@ import ( clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" ) @@ -265,12 +266,14 @@ func (tc *perItOrDescribeTestContext) GetCurrentAzureIdentityDetails(ctx context return tc.perBinaryInvocationTestContext.GetCurrentAzureIdentityDetails(ctx) } -func (tc *perItOrDescribeTestContext) CreateSREBreakglassCredentials(ctx context.Context, resourceID string, ttl time.Duration, accessLevel string, identityDetails *AzureIdentityDetails) (*rest.Config, time.Time, error) { +// createAdminAPIHTTPClient creates an HTTP client configured for Admin API calls with +// client principal authentication and TLS settings appropriate for the environment. +func createAdminAPIHTTPClient(identityDetails *AzureIdentityDetails) *http.Client { tlsConfig := &tls.Config{} if IsDevelopmentEnvironment() { tlsConfig.InsecureSkipVerify = true } - httpClient := &http.Client{ + return &http.Client{ Transport: &clientPrincipalTransport{ base: &http.Transport{ TLSClientConfig: tlsConfig, @@ -279,6 +282,10 @@ func (tc *perItOrDescribeTestContext) CreateSREBreakglassCredentials(ctx context }, Timeout: adminAPIRequestTimeout, } +} + +func (tc *perItOrDescribeTestContext) CreateSREBreakglassCredentials(ctx context.Context, resourceID string, ttl time.Duration, accessLevel string, identityDetails *AzureIdentityDetails) (*rest.Config, time.Time, error) { + httpClient := createAdminAPIHTTPClient(identityDetails) adminAPIEndpoint := tc.perBinaryInvocationTestContext.adminAPIAddress @@ -347,22 +354,59 @@ func (tc *perItOrDescribeTestContext) GetFirstVMFromManagedResourceGroup(ctx con return "", fmt.Errorf("no VMs found in managed resource group %s", managedResourceGroupName) } -// GetSerialConsoleLogs retrieves serial console logs for a VM in an HCP cluster's managed resource group -func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, resourceID string, vmName string, identityDetails *AzureIdentityDetails) (string, error) { - tlsConfig := &tls.Config{} - if IsDevelopmentEnvironment() { - tlsConfig.InsecureSkipVerify = true +func (tc *perItOrDescribeTestContext) DisableVMBootDiagnostics(ctx context.Context, managedResourceGroupName string, vmName string) error { + cred, err := tc.perBinaryInvocationTestContext.getAzureCredentials() + if err != nil { + return fmt.Errorf("failed to get Azure credentials: %w", err) } - httpClient := &http.Client{ - Transport: &clientPrincipalTransport{ - base: &http.Transport{ - TLSClientConfig: tlsConfig, - }, - identityDetails: identityDetails, - }, - Timeout: adminAPIRequestTimeout, + + subscriptionID, err := tc.SubscriptionID(ctx) + if err != nil { + return fmt.Errorf("failed to get subscription ID: %w", err) + } + + computeClient, err := armcompute.NewVirtualMachinesClient(subscriptionID, cred, nil) + if err != nil { + return fmt.Errorf("failed to create compute client: %w", err) + } + + By(fmt.Sprintf("disabling boot diagnostics for VM %s", vmName)) + + // Get current VM + vm, err := computeClient.Get(ctx, managedResourceGroupName, vmName, nil) + if err != nil { + return fmt.Errorf("failed to get VM: %w", err) + } + + // Disable boot diagnostics + if vm.Properties == nil { + vm.Properties = &armcompute.VirtualMachineProperties{} + } + if vm.Properties.DiagnosticsProfile == nil { + vm.Properties.DiagnosticsProfile = &armcompute.DiagnosticsProfile{} + } + vm.Properties.DiagnosticsProfile.BootDiagnostics = &armcompute.BootDiagnostics{ + Enabled: to.Ptr(false), } + // Update VM + poller, err := computeClient.BeginCreateOrUpdate(ctx, managedResourceGroupName, vmName, vm.VirtualMachine, nil) + if err != nil { + return fmt.Errorf("failed to begin VM update: %w", err) + } + + _, err = poller.PollUntilDone(ctx, nil) + if err != nil { + return fmt.Errorf("failed to disable boot diagnostics: %w", err) + } + + return nil +} + +// GetSerialConsoleLogs retrieves serial console logs for a VM in an HCP cluster's managed resource group +func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, resourceID string, vmName string, identityDetails *AzureIdentityDetails) (string, error) { + httpClient := createAdminAPIHTTPClient(identityDetails) + adminAPIEndpoint := tc.perBinaryInvocationTestContext.adminAPIAddress serialConsoleEndpoint := fmt.Sprintf("%s/admin/v1/hcp%s/serialconsole?vmName=%s", From a642b3ee0fbc85dc5f2b921854f643aff36d8e73 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 6 Mar 2026 08:49:16 -0500 Subject: [PATCH 07/13] ARO-17482 resolve copilot comments --- admin/README.md | 2 +- admin/server/go.mod | 1 - admin/server/go.sum | 2 -- admin/server/handlers/hcp/serialconsole.go | 19 ++++++++++++++----- test/util/framework/admin_api_helpers.go | 10 +++++++--- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/admin/README.md b/admin/README.md index e865359a9e..b8aa27d3d4 100644 --- a/admin/README.md +++ b/admin/README.md @@ -18,7 +18,7 @@ This prefix is abbreviated as `{resourceId}` below. |--------|------|-------------| | `PUT` | `/admin/v1/hcp{resourceId}/breakglass?group=...&ttl=...` | Create a breakglass session ([details](breakglass.md)) | | `GET` | `/admin/v1/hcp{resourceId}/breakglass/{sessionName}/kubeconfig` | Get kubeconfig for a breakglass session ([details](breakglass.md)) | -| `GET` | `/admin/v1/hcp{resourceId}/serialconsole?vmName=...` | Retrieve serial console logs for a VM ([details](serialconsole.md)) | +| `GET` | `/admin/v1/hcp{resourceId}/serialconsole?vmName=...` | Retrieve serial console logs for a VM | | `GET` | `/admin/v1/hcp{resourceId}/cosmosdump` | Cosmos DB dump for a cluster | | `GET` | `/admin/v1/hcp{resourceId}/helloworld` | HCP hello world (dev/test) | | `GET` | `/admin/helloworld` | Hello world (dev/test) | diff --git a/admin/server/go.mod b/admin/server/go.mod index 59374fc45c..b574938980 100644 --- a/admin/server/go.mod +++ b/admin/server/go.mod @@ -58,7 +58,6 @@ 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 diff --git a/admin/server/go.sum b/admin/server/go.sum index daa9244e28..9b56a29adb 100644 --- a/admin/server/go.sum +++ b/admin/server/go.sum @@ -106,8 +106,6 @@ 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= diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go index 7c0f6ce2c7..4bcfbae5ab 100644 --- a/admin/server/handlers/hcp/serialconsole.go +++ b/admin/server/handlers/hcp/serialconsole.go @@ -18,6 +18,7 @@ import ( "fmt" "io" "net/http" + "time" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" @@ -158,8 +159,10 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request return fmt.Errorf("failed to create blob request: %w", err) } - // download blob content - httpClient := &http.Client{} + // download blob content with timeout to avoid stuck handlers on slow blob endpoints + httpClient := &http.Client{ + Timeout: 30 * time.Second, + } blobResp, err := httpClient.Do(blobReq) if err != nil { return fmt.Errorf("failed to download serial console log: %w", err) @@ -170,13 +173,19 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request return fmt.Errorf("failed to download serial console log: unexpected status %d", blobResp.StatusCode) } - // stream response as text/plain + // stream response as text/plain and prevent caching of potentially sensitive console output writer.Header().Set("Content-Type", "text/plain; charset=utf-8") + writer.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate") + writer.Header().Set("Pragma", "no-cache") + writer.Header().Set("Expires", "0") writer.WriteHeader(http.StatusOK) _, err = io.Copy(writer, blobResp.Body) if err != nil { - // If we fail during streaming, log it - return fmt.Errorf("failed to stream serial console log: %w", err) + // After headers are sent, we cannot return an error response + // Log the error and return nil to avoid panic + logger := utils.LoggerFromContext(request.Context()) + logger.Error(err, "failed to stream serial console log", "vmName", vmName) + return nil } return nil diff --git a/test/util/framework/admin_api_helpers.go b/test/util/framework/admin_api_helpers.go index 9f1a70af1b..7dfed3477b 100644 --- a/test/util/framework/admin_api_helpers.go +++ b/test/util/framework/admin_api_helpers.go @@ -409,18 +409,22 @@ func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, adminAPIEndpoint := tc.perBinaryInvocationTestContext.adminAPIAddress - serialConsoleEndpoint := fmt.Sprintf("%s/admin/v1/hcp%s/serialconsole?vmName=%s", + serialConsoleEndpoint := fmt.Sprintf("%s/admin/v1/hcp%s/serialconsole", adminAPIEndpoint, resourceID, - vmName, ) - By(fmt.Sprintf("reaching out to the admin API to retrieve serial console logs for VM %s: %s", vmName, serialConsoleEndpoint)) + By(fmt.Sprintf("reaching out to the admin API to retrieve serial console logs for VM %s", vmName)) req, err := http.NewRequestWithContext(ctx, http.MethodGet, serialConsoleEndpoint, nil) if err != nil { return "", fmt.Errorf("failed to create request: %w", err) } + // Add query parameter with proper encoding + q := req.URL.Query() + q.Add("vmName", vmName) + req.URL.RawQuery = q.Encode() + resp, err := httpClient.Do(req) if err != nil { return "", fmt.Errorf("failed to send request: %w", err) From cb2e1a722214512b9407802ce94ecf4bda135f83 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 6 Mar 2026 09:25:22 -0500 Subject: [PATCH 08/13] ARO-17482 update e2e fixtures --- ...pat_all_parallel_01rp_api_compat_all_parallel_development.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt b/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt index d568c30d0a..588eefa606 100644 --- a/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt +++ b/test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt @@ -1,6 +1,7 @@ Customer should be able to list available HCP OpenShift versions and validate response content SRE should be able to log into a cluster via a breakglass session SRE should be able to retrieve serial console logs for a VM +SRE should return 409 when boot diagnostics is disabled on a VM Customer should be able to test admin credentials before cluster ready, then full admin credential lifecycle Customer should be able to create node pools with ARM64-based VMs Authorized CIDRs Connectivity should allow API access only from authorized VM IP From 007c1bfa9c60556c2dbfb6437182e91c689b7304 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 18 Mar 2026 09:58:30 -0400 Subject: [PATCH 09/13] ARO-17482 limit blob reader and other fix --- admin/server/handlers/hcp/helpers_test.go | 13 +---- admin/server/handlers/hcp/serialconsole.go | 57 +++++++++++-------- .../server/handlers/hcp/serialconsole_test.go | 19 +------ 3 files changed, 39 insertions(+), 50 deletions(-) diff --git a/admin/server/handlers/hcp/helpers_test.go b/admin/server/handlers/hcp/helpers_test.go index b9651afb37..84a9617ee5 100644 --- a/admin/server/handlers/hcp/helpers_test.go +++ b/admin/server/handlers/hcp/helpers_test.go @@ -17,6 +17,7 @@ package hcp import ( "errors" "net/http" + "strings" "testing" ocmerrors "github.com/openshift-online/ocm-sdk-go/errors" @@ -132,7 +133,7 @@ func TestClusterServiceError(t *testing.T) { } // Verify error message contains expected text - if !contains(result.Error(), tt.expectedMessage) { + if !strings.Contains(result.Error(), tt.expectedMessage) { t.Errorf("Expected error to contain %q, got %q", tt.expectedMessage, result.Error()) } @@ -144,13 +145,3 @@ func TestClusterServiceError(t *testing.T) { }) } } - -// 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 -} diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go index 4bcfbae5ab..9795781525 100644 --- a/admin/server/handlers/hcp/serialconsole.go +++ b/admin/server/handlers/hcp/serialconsole.go @@ -15,11 +15,12 @@ package hcp import ( + "errors" "fmt" "io" "net/http" - "time" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" "github.com/Azure/ARO-HCP/internal/api/arm" @@ -30,6 +31,10 @@ import ( "github.com/Azure/ARO-HCP/internal/validation" ) +const ( + maxBytes = 100 * 1024 * 1024 // 100MB +) + // HCPSerialConsoleHandler handles requests to retrieve VM serial console logs // for debugging purposes. This endpoint allows SREs to access boot diagnostics // and console output from VMs in the HCP cluster's managed resource group. @@ -113,30 +118,37 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request // Retrieve boot diagnostics data containing serial console blob URI managedResourceGroup := hcp.CustomerProperties.Platform.ManagedResourceGroup + sasTokenExpirationMinutes := int32(5) + options := &armcompute.VirtualMachinesClientRetrieveBootDiagnosticsDataOptions{ + SasURIExpirationTimeInMinutes: &sasTokenExpirationMinutes, + } result, err := computeClient.RetrieveBootDiagnosticsData( request.Context(), managedResourceGroup, vmName, - nil, + options, ) if err != nil { - // 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, - ) + var azErr *azcore.ResponseError + if ok := errors.As(err, &azErr); ok && azErr != nil { + // Azure returns 404 when VM or resource group doesn't exist + if azErr.StatusCode == 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 azErr.StatusCode == 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) } @@ -160,9 +172,7 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request } // download blob content with timeout to avoid stuck handlers on slow blob endpoints - httpClient := &http.Client{ - Timeout: 30 * time.Second, - } + httpClient := &http.Client{} blobResp, err := httpClient.Do(blobReq) if err != nil { return fmt.Errorf("failed to download serial console log: %w", err) @@ -179,7 +189,8 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request writer.Header().Set("Pragma", "no-cache") writer.Header().Set("Expires", "0") writer.WriteHeader(http.StatusOK) - _, err = io.Copy(writer, blobResp.Body) + limitedReader := io.LimitReader(blobResp.Body, maxBytes) + _, err = io.Copy(writer, limitedReader) if err != nil { // After headers are sent, we cannot return an error response // Log the error and return nil to avoid panic diff --git a/admin/server/handlers/hcp/serialconsole_test.go b/admin/server/handlers/hcp/serialconsole_test.go index 4324fae95d..e2f130f2c3 100644 --- a/admin/server/handlers/hcp/serialconsole_test.go +++ b/admin/server/handlers/hcp/serialconsole_test.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/go-logr/logr/testr" @@ -288,7 +289,7 @@ func TestSerialConsoleHandler(t *testing.T) { // Check error message if tt.expectedError != "" { - if !containsError(err, tt.expectedError) { + if !strings.Contains(err.Error(), tt.expectedError) { t.Errorf("Expected error containing %q but got %q", tt.expectedError, err.Error()) } } @@ -335,21 +336,7 @@ func TestSerialConsoleHandler_InvalidResourceID(t *testing.T) { t.Errorf("Expected status code %d but got %d", http.StatusBadRequest, cloudErr.StatusCode) } - if !containsError(err, "invalid resource identifier") { + if !strings.Contains(err.Error(), "invalid resource identifier") { t.Errorf("Expected error containing 'invalid resource identifier' but got %q", err.Error()) } } - -// containsError checks if an error message contains the expected substring -func containsError(err error, expected string) bool { - if err == nil { - return expected == "" - } - errMsg := err.Error() - for i := 0; i <= len(errMsg)-len(expected); i++ { - if errMsg[i:i+len(expected)] == expected { - return true - } - } - return false -} From 5491e73bf49bb40f03fe42dfb01b6a5d79de02fa Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 20 Mar 2026 11:48:59 -0400 Subject: [PATCH 10/13] ARO-17482 get tenant id from subscription doc --- admin/server/handlers/hcp/serialconsole.go | 29 ++++--- .../server/handlers/hcp/serialconsole_test.go | 87 +++++++++---------- 2 files changed, 58 insertions(+), 58 deletions(-) diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go index 9795781525..3c3ef5ed06 100644 --- a/admin/server/handlers/hcp/serialconsole.go +++ b/admin/server/handlers/hcp/serialconsole.go @@ -95,25 +95,32 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request // get HCP details hcp, err := h.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(request.Context(), resourceID.Name) if err != nil { - return fmt.Errorf("failed to get HCP from database: %w", err) + return utils.TrackError(fmt.Errorf("failed to get HCP from database: %w", err)) } - // get CS cluster data to retrieve tenant ID - csCluster, err := h.csClient.GetCluster(request.Context(), hcp.ServiceProviderProperties.ClusterServiceID) + subscription, err := h.dbClient.Subscriptions().Get(request.Context(), resourceID.SubscriptionID) + if database.IsResponseError(err, http.StatusNotFound) { + return arm.NewCloudError( + http.StatusNotFound, + arm.CloudErrorCodeNotFound, + "", + "subscription %s not found", resourceID.SubscriptionID, + ) + } if err != nil { - return ClusterServiceError(err, "cluster data") + return utils.TrackError(err) } // get FPA credentials for customer tenant - tokenCredential, err := h.fpaCredentialRetriever.RetrieveCredential(csCluster.Azure().TenantID()) + tokenCredential, err := h.fpaCredentialRetriever.RetrieveCredential(*subscription.Properties.TenantId) if err != nil { - return fmt.Errorf("failed to retrieve Azure credentials: %w", err) + return utils.TrackError(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 Azure compute client: %w", err) + return utils.TrackError(fmt.Errorf("failed to create Azure compute client: %w", err)) } // Retrieve boot diagnostics data containing serial console blob URI @@ -150,7 +157,7 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request ) } } - return fmt.Errorf("failed to retrieve boot diagnostics data for VM %s: %w", vmName, err) + return utils.TrackError(fmt.Errorf("failed to retrieve boot diagnostics data for VM %s: %w", vmName, err)) } // verify serial console log blob URI is available @@ -168,19 +175,19 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request // The blob URI contains a SAS token for authentication, so we can use a simple HTTP GET blobReq, err := http.NewRequestWithContext(request.Context(), http.MethodGet, *result.SerialConsoleLogBlobURI, nil) if err != nil { - return fmt.Errorf("failed to create blob request: %w", err) + return utils.TrackError(fmt.Errorf("failed to create blob request: %w", err)) } // download blob content with timeout to avoid stuck handlers on slow blob endpoints httpClient := &http.Client{} blobResp, err := httpClient.Do(blobReq) if err != nil { - return fmt.Errorf("failed to download serial console log: %w", err) + return utils.TrackError(fmt.Errorf("failed to download serial console log: %w", err)) } defer blobResp.Body.Close() if blobResp.StatusCode != http.StatusOK { - return fmt.Errorf("failed to download serial console log: unexpected status %d", blobResp.StatusCode) + return utils.TrackError(fmt.Errorf("failed to download serial console log: unexpected status %d", blobResp.StatusCode)) } // stream response as text/plain and prevent caching of potentially sensitive console output diff --git a/admin/server/handlers/hcp/serialconsole_test.go b/admin/server/handlers/hcp/serialconsole_test.go index e2f130f2c3..12e8ee1e95 100644 --- a/admin/server/handlers/hcp/serialconsole_test.go +++ b/admin/server/handlers/hcp/serialconsole_test.go @@ -29,9 +29,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" - sdk "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" - ocmerrors "github.com/openshift-online/ocm-sdk-go/errors" - "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" "github.com/Azure/ARO-HCP/internal/database" @@ -121,21 +118,17 @@ func TestSerialConsoleHandler(t *testing.T) { expectedError: "failed to get HCP from database", // Wrapped error, ReportError converts to 404 ResourceNotFoundError }, { - name: "CS cluster not found (OCM 404)", + name: "subscription not found", resourceID: api.TestClusterResourceID, vmName: "test-vm", setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { mockDB := database.NewMockDBClient(ctrl) mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + mockSubscriptionCRUD := database.NewMockSubscriptionCRUD(ctrl) resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - clusterServiceID, _ := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cs-id") - hcp := &api.HCPOpenShiftCluster{ - ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ - ClusterServiceID: clusterServiceID, - }, - } + hcp := &api.HCPOpenShiftCluster{} mockDB.EXPECT(). HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). @@ -144,35 +137,31 @@ func TestSerialConsoleHandler(t *testing.T) { Get(gomock.Any(), resourceID.Name). Return(hcp, nil) - // Create OCM 404 error - ocmErr, _ := ocmerrors.NewError().Status(http.StatusNotFound).Build() - - mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) - mockCS.EXPECT(). - GetCluster(gomock.Any(), clusterServiceID). - Return(nil, ocmErr) + // Mock subscription not found + mockDB.EXPECT(). + Subscriptions(). + Return(mockSubscriptionCRUD) + mockSubscriptionCRUD.EXPECT(). + Get(gomock.Any(), resourceID.SubscriptionID). + Return(nil, &azcore.ResponseError{StatusCode: http.StatusNotFound}) - return mockDB, mockCS, &mockFPACredentialRetriever{} + return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} }, expectedStatusCode: http.StatusNotFound, - expectedError: "cluster data not found in cluster service", + expectedError: "not found", }, { - name: "CS cluster retrieval fails (generic error)", + name: "subscription retrieval fails (generic error)", resourceID: api.TestClusterResourceID, vmName: "test-vm", setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { mockDB := database.NewMockDBClient(ctrl) mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + mockSubscriptionCRUD := database.NewMockSubscriptionCRUD(ctrl) resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - clusterServiceID, _ := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cs-id") - hcp := &api.HCPOpenShiftCluster{ - ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ - ClusterServiceID: clusterServiceID, - }, - } + hcp := &api.HCPOpenShiftCluster{} mockDB.EXPECT(). HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). @@ -181,15 +170,18 @@ func TestSerialConsoleHandler(t *testing.T) { Get(gomock.Any(), resourceID.Name). Return(hcp, nil) - mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) - mockCS.EXPECT(). - GetCluster(gomock.Any(), clusterServiceID). - Return(nil, fmt.Errorf("CS cluster not found")) + // Mock subscription retrieval error + mockDB.EXPECT(). + Subscriptions(). + Return(mockSubscriptionCRUD) + mockSubscriptionCRUD.EXPECT(). + Get(gomock.Any(), resourceID.SubscriptionID). + Return(nil, fmt.Errorf("database error")) - return mockDB, mockCS, &mockFPACredentialRetriever{} + return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} }, expectedStatusCode: http.StatusInternalServerError, - expectedError: "failed to get cluster data from cluster service", + expectedError: "database error", }, { name: "FPA credential retrieval fails", @@ -198,15 +190,11 @@ func TestSerialConsoleHandler(t *testing.T) { setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { mockDB := database.NewMockDBClient(ctrl) mockCRUD := database.NewMockHCPClusterCRUD(ctrl) + mockSubscriptionCRUD := database.NewMockSubscriptionCRUD(ctrl) resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - clusterServiceID, _ := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cs-id") - hcp := &api.HCPOpenShiftCluster{ - ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ - ClusterServiceID: clusterServiceID, - }, - } + hcp := &api.HCPOpenShiftCluster{} mockDB.EXPECT(). HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). @@ -215,22 +203,27 @@ func TestSerialConsoleHandler(t *testing.T) { Get(gomock.Any(), resourceID.Name). Return(hcp, nil) - // Mock CS cluster with Azure tenant ID - csCluster, _ := sdk.NewCluster(). - Azure(sdk.NewAzure().TenantID("test-tenant-id")). - Build() + // Mock subscription with tenant ID + tenantID := "test-tenant-id" + subscription := &arm.Subscription{ + Properties: &arm.SubscriptionProperties{ + TenantId: &tenantID, + }, + } - mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) - mockCS.EXPECT(). - GetCluster(gomock.Any(), clusterServiceID). - Return(csCluster, nil) + mockDB.EXPECT(). + Subscriptions(). + Return(mockSubscriptionCRUD) + mockSubscriptionCRUD.EXPECT(). + Get(gomock.Any(), resourceID.SubscriptionID). + Return(subscription, nil) // Mock FPA failure mockFPA := &mockFPACredentialRetriever{ err: fmt.Errorf("failed to get FPA credentials"), } - return mockDB, mockCS, mockFPA + return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), mockFPA }, expectedStatusCode: http.StatusInternalServerError, expectedError: "failed to retrieve Azure credentials", From 0471720f974d4f7ffd81ac1cbaa9402f957f7649 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Fri, 20 Mar 2026 12:30:16 -0400 Subject: [PATCH 11/13] ARO-17482 copilot fixes --- test/util/framework/admin_api_helpers.go | 34 +++++++++++------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/test/util/framework/admin_api_helpers.go b/test/util/framework/admin_api_helpers.go index 7dfed3477b..16912defd1 100644 --- a/test/util/framework/admin_api_helpers.go +++ b/test/util/framework/admin_api_helpers.go @@ -372,25 +372,19 @@ func (tc *perItOrDescribeTestContext) DisableVMBootDiagnostics(ctx context.Conte By(fmt.Sprintf("disabling boot diagnostics for VM %s", vmName)) - // Get current VM - vm, err := computeClient.Get(ctx, managedResourceGroupName, vmName, nil) - if err != nil { - return fmt.Errorf("failed to get VM: %w", err) - } - - // Disable boot diagnostics - if vm.Properties == nil { - vm.Properties = &armcompute.VirtualMachineProperties{} - } - if vm.Properties.DiagnosticsProfile == nil { - vm.Properties.DiagnosticsProfile = &armcompute.DiagnosticsProfile{} - } - vm.Properties.DiagnosticsProfile.BootDiagnostics = &armcompute.BootDiagnostics{ - Enabled: to.Ptr(false), + // Create update payload with only the diagnostics profile + vmUpdate := armcompute.VirtualMachineUpdate{ + Properties: &armcompute.VirtualMachineProperties{ + DiagnosticsProfile: &armcompute.DiagnosticsProfile{ + BootDiagnostics: &armcompute.BootDiagnostics{ + Enabled: to.Ptr(false), + }, + }, + }, } // Update VM - poller, err := computeClient.BeginCreateOrUpdate(ctx, managedResourceGroupName, vmName, vm.VirtualMachine, nil) + poller, err := computeClient.BeginUpdate(ctx, managedResourceGroupName, vmName, vmUpdate, nil) if err != nil { return fmt.Errorf("failed to begin VM update: %w", err) } @@ -432,14 +426,18 @@ func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - body, readErr := io.ReadAll(resp.Body) + // Limit error response body to 1MB for test error messages + limitedReader := io.LimitReader(resp.Body, 1*1024*1024) + body, readErr := io.ReadAll(limitedReader) if readErr != nil { return "", fmt.Errorf("expected status 200 OK, got %d (failed to read body: %w)", resp.StatusCode, readErr) } return "", fmt.Errorf("expected status 200 OK, got %d: %s", resp.StatusCode, string(body)) } - body, err := io.ReadAll(resp.Body) + // Limit response body to 10MB for serial console logs in tests + limitedReader := io.LimitReader(resp.Body, 10*1024*1024) + body, err := io.ReadAll(limitedReader) if err != nil { return "", fmt.Errorf("failed to read response body: %w", err) } From f3f21e44f66ffe3611dcb8b584ba72fafaa86a8c Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Tue, 24 Mar 2026 11:06:29 -0400 Subject: [PATCH 12/13] ARO-17482 resolve mockDBClient conflicts --- admin/server/go.mod | 2 +- admin/server/handlers/hcp/serialconsole.go | 4 - .../server/handlers/hcp/serialconsole_test.go | 210 ++++++------------ admin/server/server/admin.go | 2 +- 4 files changed, 66 insertions(+), 152 deletions(-) diff --git a/admin/server/go.mod b/admin/server/go.mod index b574938980..b80ae2e5eb 100644 --- a/admin/server/go.mod +++ b/admin/server/go.mod @@ -15,7 +15,6 @@ require ( github.com/prometheus/client_golang v1.23.2 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.11.1 - go.uber.org/mock v0.6.0 k8s.io/apimachinery v0.34.3 k8s.io/client-go v0.34.3 k8s.io/utils v0.0.0-20260108192941-914a6e750570 @@ -110,6 +109,7 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.40.0 // indirect go.opentelemetry.io/otel/trace v1.40.0 // indirect go.opentelemetry.io/proto/otlp v1.9.0 // indirect + go.uber.org/mock v0.6.0 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.49.0 // indirect diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go index 3c3ef5ed06..4124f8ba55 100644 --- a/admin/server/handlers/hcp/serialconsole.go +++ b/admin/server/handlers/hcp/serialconsole.go @@ -26,7 +26,6 @@ import ( "github.com/Azure/ARO-HCP/internal/api/arm" "github.com/Azure/ARO-HCP/internal/database" "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" ) @@ -40,19 +39,16 @@ const ( // and console output from VMs in the HCP cluster's managed resource group. type HCPSerialConsoleHandler struct { dbClient database.DBClient - csClient ocm.ClusterServiceClientSpec fpaCredentialRetriever fpa.FirstPartyApplicationTokenCredentialRetriever } // NewHCPSerialConsoleHandler creates a new serial console handler with the required dependencies func NewHCPSerialConsoleHandler( dbClient database.DBClient, - csClient ocm.ClusterServiceClientSpec, fpaCredentialRetriever fpa.FirstPartyApplicationTokenCredentialRetriever, ) *HCPSerialConsoleHandler { return &HCPSerialConsoleHandler{ dbClient: dbClient, - csClient: csClient, fpaCredentialRetriever: fpaCredentialRetriever, } } diff --git a/admin/server/handlers/hcp/serialconsole_test.go b/admin/server/handlers/hcp/serialconsole_test.go index 12e8ee1e95..298e86f43a 100644 --- a/admin/server/handlers/hcp/serialconsole_test.go +++ b/admin/server/handlers/hcp/serialconsole_test.go @@ -24,15 +24,14 @@ import ( "testing" "github.com/go-logr/logr/testr" - "go.uber.org/mock/gomock" + "github.com/stretchr/testify/require" "github.com/Azure/azure-sdk-for-go/sdk/azcore" azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" - "github.com/Azure/ARO-HCP/internal/database" - "github.com/Azure/ARO-HCP/internal/ocm" + "github.com/Azure/ARO-HCP/internal/databasetesting" "github.com/Azure/ARO-HCP/internal/utils" ) @@ -51,7 +50,8 @@ func TestSerialConsoleHandler(t *testing.T) { name string resourceID string vmName string - setupMocks func(*gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) + setupData func(context.Context, *testing.T, *databasetesting.MockDBClient, *azcorearm.ResourceID) + mockFPA *mockFPACredentialRetriever expectedStatusCode int expectedError string }{ @@ -59,9 +59,9 @@ func TestSerialConsoleHandler(t *testing.T) { name: "missing vmName parameter", resourceID: api.TestClusterResourceID, vmName: "", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - return database.NewMockDBClient(ctrl), ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + setupData: func(ctx context.Context, t *testing.T, mockDB *databasetesting.MockDBClient, resourceID *azcorearm.ResourceID) { }, + mockFPA: &mockFPACredentialRetriever{}, expectedStatusCode: http.StatusBadRequest, expectedError: "vmName query parameter is required", }, @@ -69,161 +69,82 @@ func TestSerialConsoleHandler(t *testing.T) { name: "invalid vmName format", resourceID: api.TestClusterResourceID, vmName: "-invalid-vm-name", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - return database.NewMockDBClient(ctrl), ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + setupData: func(ctx context.Context, t *testing.T, mockDB *databasetesting.MockDBClient, resourceID *azcorearm.ResourceID) { }, + mockFPA: &mockFPACredentialRetriever{}, expectedStatusCode: http.StatusBadRequest, expectedError: "vmName contains invalid characters or format", }, { - name: "HCP cluster not found in database (generic error)", + name: "HCP cluster not found in database", resourceID: api.TestClusterResourceID, vmName: "test-vm", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - mockDB := database.NewMockDBClient(ctrl) - mockCRUD := database.NewMockHCPClusterCRUD(ctrl) - - resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - mockDB.EXPECT(). - HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). - Return(mockCRUD) - mockCRUD.EXPECT(). - Get(gomock.Any(), resourceID.Name). - Return(nil, fmt.Errorf("failed to get HCP from database: cluster not found")) - - return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + setupData: func(ctx context.Context, t *testing.T, mockDB *databasetesting.MockDBClient, resourceID *azcorearm.ResourceID) { }, + mockFPA: &mockFPACredentialRetriever{}, expectedStatusCode: http.StatusInternalServerError, expectedError: "failed to get HCP from database", }, - { - name: "HCP cluster not found in database (404 ResponseError)", - resourceID: api.TestClusterResourceID, - vmName: "test-vm", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - mockDB := database.NewMockDBClient(ctrl) - mockCRUD := database.NewMockHCPClusterCRUD(ctrl) - - resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - mockDB.EXPECT(). - HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). - Return(mockCRUD) - mockCRUD.EXPECT(). - Get(gomock.Any(), resourceID.Name). - Return(nil, &azcore.ResponseError{StatusCode: http.StatusNotFound}) - - return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} - }, - expectedStatusCode: http.StatusInternalServerError, - expectedError: "failed to get HCP from database", // Wrapped error, ReportError converts to 404 ResourceNotFoundError - }, { name: "subscription not found", resourceID: api.TestClusterResourceID, vmName: "test-vm", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - mockDB := database.NewMockDBClient(ctrl) - mockCRUD := database.NewMockHCPClusterCRUD(ctrl) - mockSubscriptionCRUD := database.NewMockSubscriptionCRUD(ctrl) - - resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - - hcp := &api.HCPOpenShiftCluster{} - - mockDB.EXPECT(). - HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). - Return(mockCRUD) - mockCRUD.EXPECT(). - Get(gomock.Any(), resourceID.Name). - Return(hcp, nil) - - // Mock subscription not found - mockDB.EXPECT(). - Subscriptions(). - Return(mockSubscriptionCRUD) - mockSubscriptionCRUD.EXPECT(). - Get(gomock.Any(), resourceID.SubscriptionID). - Return(nil, &azcore.ResponseError{StatusCode: http.StatusNotFound}) - - return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} + setupData: func(ctx context.Context, t *testing.T, mockDB *databasetesting.MockDBClient, resourceID *azcorearm.ResourceID) { + // Create HCP cluster with InternalID + internalID, err := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cluster-id") + require.NoError(t, err) + hcp := &api.HCPOpenShiftCluster{ + TrackedResource: arm.TrackedResource{ + Resource: arm.Resource{ID: resourceID}, + }, + ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ + ClusterServiceID: internalID, + }, + } + _, err = mockDB.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Create(ctx, hcp, nil) + require.NoError(t, err) }, + mockFPA: &mockFPACredentialRetriever{}, expectedStatusCode: http.StatusNotFound, expectedError: "not found", }, - { - name: "subscription retrieval fails (generic error)", - resourceID: api.TestClusterResourceID, - vmName: "test-vm", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - mockDB := database.NewMockDBClient(ctrl) - mockCRUD := database.NewMockHCPClusterCRUD(ctrl) - mockSubscriptionCRUD := database.NewMockSubscriptionCRUD(ctrl) - - resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - - hcp := &api.HCPOpenShiftCluster{} - - mockDB.EXPECT(). - HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). - Return(mockCRUD) - mockCRUD.EXPECT(). - Get(gomock.Any(), resourceID.Name). - Return(hcp, nil) - - // Mock subscription retrieval error - mockDB.EXPECT(). - Subscriptions(). - Return(mockSubscriptionCRUD) - mockSubscriptionCRUD.EXPECT(). - Get(gomock.Any(), resourceID.SubscriptionID). - Return(nil, fmt.Errorf("database error")) - - return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), &mockFPACredentialRetriever{} - }, - expectedStatusCode: http.StatusInternalServerError, - expectedError: "database error", - }, { name: "FPA credential retrieval fails", resourceID: api.TestClusterResourceID, vmName: "test-vm", - setupMocks: func(ctrl *gomock.Controller) (database.DBClient, ocm.ClusterServiceClientSpec, *mockFPACredentialRetriever) { - mockDB := database.NewMockDBClient(ctrl) - mockCRUD := database.NewMockHCPClusterCRUD(ctrl) - mockSubscriptionCRUD := database.NewMockSubscriptionCRUD(ctrl) - - resourceID, _ := azcorearm.ParseResourceID(api.TestClusterResourceID) - - hcp := &api.HCPOpenShiftCluster{} - - mockDB.EXPECT(). - HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName). - Return(mockCRUD) - mockCRUD.EXPECT(). - Get(gomock.Any(), resourceID.Name). - Return(hcp, nil) + setupData: func(ctx context.Context, t *testing.T, mockDB *databasetesting.MockDBClient, resourceID *azcorearm.ResourceID) { + // Create HCP cluster with InternalID + internalID, err := api.NewInternalID("/api/clusters_mgmt/v1/clusters/test-cluster-id") + require.NoError(t, err) + hcp := &api.HCPOpenShiftCluster{ + TrackedResource: arm.TrackedResource{ + Resource: arm.Resource{ID: resourceID}, + }, + ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ + ClusterServiceID: internalID, + }, + } + _, err = mockDB.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Create(ctx, hcp, nil) + require.NoError(t, err) - // Mock subscription with tenant ID + // Create subscription with tenant ID tenantID := "test-tenant-id" + subscriptionResourceID := api.Must(azcorearm.ParseResourceID("/subscriptions/" + resourceID.SubscriptionID)) subscription := &arm.Subscription{ + CosmosMetadata: arm.CosmosMetadata{ + ResourceID: subscriptionResourceID, + }, + ResourceID: subscriptionResourceID, + State: arm.SubscriptionStateRegistered, Properties: &arm.SubscriptionProperties{ TenantId: &tenantID, }, } - - mockDB.EXPECT(). - Subscriptions(). - Return(mockSubscriptionCRUD) - mockSubscriptionCRUD.EXPECT(). - Get(gomock.Any(), resourceID.SubscriptionID). - Return(subscription, nil) - - // Mock FPA failure - mockFPA := &mockFPACredentialRetriever{ - err: fmt.Errorf("failed to get FPA credentials"), - } - - return mockDB, ocm.NewMockClusterServiceClientSpec(ctrl), mockFPA + _, err = mockDB.Subscriptions().Create(ctx, subscription, nil) + require.NoError(t, err) + }, + mockFPA: &mockFPACredentialRetriever{ + err: fmt.Errorf("failed to get FPA credentials"), }, expectedStatusCode: http.StatusInternalServerError, expectedError: "failed to retrieve Azure credentials", @@ -233,20 +154,20 @@ func TestSerialConsoleHandler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - // Setup mocks - mockDB, mockCS, mockFPA := tt.setupMocks(ctrl) - // Create handler - handler := NewHCPSerialConsoleHandler(mockDB, mockCS, mockFPA) + // Setup database and test data + mockDB := databasetesting.NewMockDBClient() // Parse resource ID and add to context resourceID, err := azcorearm.ParseResourceID(tt.resourceID) - if err != nil { - t.Fatalf("Failed to parse resource ID: %v", err) - } + require.NoError(t, err) + + // Setup test data + tt.setupData(ctx, t, mockDB, resourceID) + + // Create handler + handler := NewHCPSerialConsoleHandler(mockDB, tt.mockFPA) + ctx = utils.ContextWithResourceID(ctx, resourceID) // Create request @@ -297,14 +218,11 @@ func TestSerialConsoleHandler(t *testing.T) { func TestSerialConsoleHandler_InvalidResourceID(t *testing.T) { ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockDB := database.NewMockDBClient(ctrl) - mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) + mockDB := databasetesting.NewMockDBClient() mockFPA := &mockFPACredentialRetriever{} - handler := NewHCPSerialConsoleHandler(mockDB, mockCS, mockFPA) + handler := NewHCPSerialConsoleHandler(mockDB, mockFPA) // Create request WITHOUT adding resource ID to context req := httptest.NewRequest(http.MethodGet, "/serialconsole?vmName=test-vm", nil) diff --git a/admin/server/server/admin.go b/admin/server/server/admin.go index 85889eb9ce..fe2ffd0587 100644 --- a/admin/server/server/admin.go +++ b/admin/server/server/admin.go @@ -111,7 +111,7 @@ func NewAdminAPI( ) middlewareMux.Handle( middleware.V1HCPResourcePattern("GET", "/serialconsole"), - hcpMiddleware.HandlerFunc(errorutils.ReportError(hcp.NewHCPSerialConsoleHandler(dbClient, clustersServiceClient, fpaCredentialRetriever).ServeHTTP)), + hcpMiddleware.HandlerFunc(errorutils.ReportError(hcp.NewHCPSerialConsoleHandler(dbClient, fpaCredentialRetriever).ServeHTTP)), ) // Non-HCP admin routes From c715df007d6566c067bb224ba4ec2c866b285803 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 25 Mar 2026 08:15:55 -0400 Subject: [PATCH 13/13] ARO-17482 remove the obvious comments --- admin/server/handlers/hcp/serialconsole.go | 16 ---------------- test/util/framework/admin_api_helpers.go | 8 -------- 2 files changed, 24 deletions(-) diff --git a/admin/server/handlers/hcp/serialconsole.go b/admin/server/handlers/hcp/serialconsole.go index 4124f8ba55..78fe2d5bc0 100644 --- a/admin/server/handlers/hcp/serialconsole.go +++ b/admin/server/handlers/hcp/serialconsole.go @@ -54,10 +54,7 @@ func NewHCPSerialConsoleHandler( } // ServeHTTP handles GET requests to retrieve serial console output for a specified VM. -// Query parameters: -// - vmName (required): The name of the VM to retrieve console logs func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request *http.Request) error { - // get the azure resource ID for this HCP resourceID, err := utils.ResourceIDFromContext(request.Context()) if err != nil { return arm.NewCloudError( @@ -68,7 +65,6 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request ) } - //Extract and validate vmName query parameter vmName := request.URL.Query().Get("vmName") if vmName == "" { return arm.NewCloudError( @@ -88,7 +84,6 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request ) } - // get HCP details hcp, err := h.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(request.Context(), resourceID.Name) if err != nil { return utils.TrackError(fmt.Errorf("failed to get HCP from database: %w", err)) @@ -107,19 +102,16 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request return utils.TrackError(err) } - // get FPA credentials for customer tenant tokenCredential, err := h.fpaCredentialRetriever.RetrieveCredential(*subscription.Properties.TenantId) if err != nil { return utils.TrackError(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 utils.TrackError(fmt.Errorf("failed to create Azure compute client: %w", err)) } - // Retrieve boot diagnostics data containing serial console blob URI managedResourceGroup := hcp.CustomerProperties.Platform.ManagedResourceGroup sasTokenExpirationMinutes := int32(5) options := &armcompute.VirtualMachinesClientRetrieveBootDiagnosticsDataOptions{ @@ -134,7 +126,6 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request if err != nil { var azErr *azcore.ResponseError if ok := errors.As(err, &azErr); ok && azErr != nil { - // Azure returns 404 when VM or resource group doesn't exist if azErr.StatusCode == http.StatusNotFound { return arm.NewCloudError( http.StatusNotFound, @@ -143,7 +134,6 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request "VM %s not found in resource group %s", vmName, managedResourceGroup, ) } - // Azure returns 409 when boot diagnostics is disabled if azErr.StatusCode == http.StatusConflict { return arm.NewCloudError( http.StatusConflict, @@ -156,7 +146,6 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request return utils.TrackError(fmt.Errorf("failed to retrieve boot diagnostics data for VM %s: %w", vmName, err)) } - // verify serial console log blob URI is available if result.SerialConsoleLogBlobURI == nil || *result.SerialConsoleLogBlobURI == "" { return arm.NewCloudError( http.StatusNotFound, @@ -167,14 +156,11 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request ) } - // fetch blob content via HTTP GET - // The blob URI contains a SAS token for authentication, so we can use a simple HTTP GET blobReq, err := http.NewRequestWithContext(request.Context(), http.MethodGet, *result.SerialConsoleLogBlobURI, nil) if err != nil { return utils.TrackError(fmt.Errorf("failed to create blob request: %w", err)) } - // download blob content with timeout to avoid stuck handlers on slow blob endpoints httpClient := &http.Client{} blobResp, err := httpClient.Do(blobReq) if err != nil { @@ -195,8 +181,6 @@ func (h *HCPSerialConsoleHandler) ServeHTTP(writer http.ResponseWriter, request limitedReader := io.LimitReader(blobResp.Body, maxBytes) _, err = io.Copy(writer, limitedReader) if err != nil { - // After headers are sent, we cannot return an error response - // Log the error and return nil to avoid panic logger := utils.LoggerFromContext(request.Context()) logger.Error(err, "failed to stream serial console log", "vmName", vmName) return nil diff --git a/test/util/framework/admin_api_helpers.go b/test/util/framework/admin_api_helpers.go index 16912defd1..e98554d026 100644 --- a/test/util/framework/admin_api_helpers.go +++ b/test/util/framework/admin_api_helpers.go @@ -266,8 +266,6 @@ func (tc *perItOrDescribeTestContext) GetCurrentAzureIdentityDetails(ctx context return tc.perBinaryInvocationTestContext.GetCurrentAzureIdentityDetails(ctx) } -// createAdminAPIHTTPClient creates an HTTP client configured for Admin API calls with -// client principal authentication and TLS settings appropriate for the environment. func createAdminAPIHTTPClient(identityDetails *AzureIdentityDetails) *http.Client { tlsConfig := &tls.Config{} if IsDevelopmentEnvironment() { @@ -320,8 +318,6 @@ func (tc *perItOrDescribeTestContext) CreateSREBreakglassCredentials(ctx context return restConfig, expiresAt, nil } -// GetFirstVMFromManagedResourceGroup retrieves the name of the first VM found in the managed resource group. -// Returns an error if no VMs are found or if the Azure API calls fail. func (tc *perItOrDescribeTestContext) GetFirstVMFromManagedResourceGroup(ctx context.Context, managedResourceGroupName string) (string, error) { cred, err := tc.perBinaryInvocationTestContext.getAzureCredentials() if err != nil { @@ -372,7 +368,6 @@ func (tc *perItOrDescribeTestContext) DisableVMBootDiagnostics(ctx context.Conte By(fmt.Sprintf("disabling boot diagnostics for VM %s", vmName)) - // Create update payload with only the diagnostics profile vmUpdate := armcompute.VirtualMachineUpdate{ Properties: &armcompute.VirtualMachineProperties{ DiagnosticsProfile: &armcompute.DiagnosticsProfile{ @@ -383,7 +378,6 @@ func (tc *perItOrDescribeTestContext) DisableVMBootDiagnostics(ctx context.Conte }, } - // Update VM poller, err := computeClient.BeginUpdate(ctx, managedResourceGroupName, vmName, vmUpdate, nil) if err != nil { return fmt.Errorf("failed to begin VM update: %w", err) @@ -397,7 +391,6 @@ func (tc *perItOrDescribeTestContext) DisableVMBootDiagnostics(ctx context.Conte return nil } -// GetSerialConsoleLogs retrieves serial console logs for a VM in an HCP cluster's managed resource group func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, resourceID string, vmName string, identityDetails *AzureIdentityDetails) (string, error) { httpClient := createAdminAPIHTTPClient(identityDetails) @@ -414,7 +407,6 @@ func (tc *perItOrDescribeTestContext) GetSerialConsoleLogs(ctx context.Context, return "", fmt.Errorf("failed to create request: %w", err) } - // Add query parameter with proper encoding q := req.URL.Query() q.Add("vmName", vmName) req.URL.RawQuery = q.Encode()