From 1e9a5af4ba517e4cd31776d8c461f9955bd70c82 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 5 Jul 2024 16:58:38 +1000 Subject: [PATCH] figured it out :) --- .../failurediagnostics/virtualmachines.go | 9 ++- .../virtualmachines_test.go | 55 +++---------------- .../admin_openshiftcluster_serialconsole.go | 23 +++++--- pkg/frontend/adminactions/azureactions.go | 2 +- pkg/frontend/adminactions/vmserialconsole.go | 9 +-- .../adminactions/vmserialconsole_test.go | 12 +++- .../mgmt/compute/virtualmachines_addons.go | 20 +++---- pkg/util/mocks/adminactions/adminactions.go | 13 ++--- .../mocks/azureclient/mgmt/compute/compute.go | 13 ++--- test/e2e/adminapi_serialconsole.go | 23 +------- 10 files changed, 62 insertions(+), 117 deletions(-) diff --git a/pkg/cluster/failurediagnostics/virtualmachines.go b/pkg/cluster/failurediagnostics/virtualmachines.go index e9e18dd44c6..9c740563e47 100644 --- a/pkg/cluster/failurediagnostics/virtualmachines.go +++ b/pkg/cluster/failurediagnostics/virtualmachines.go @@ -5,8 +5,8 @@ package failurediagnostics import ( "bufio" + "bytes" "context" - "encoding/base64" "fmt" "github.com/Azure/ARO-RP/pkg/util/stringutils" @@ -49,16 +49,15 @@ func (m *manager) LogAzureInformation(ctx context.Context) (interface{}, error) // Fetch boot diagnostics URIs for the VMs for _, vmName := range vmNames { - serialConsoleBlob, err := m.virtualMachines.GetSerialConsoleForVM(ctx, resourceGroupName, vmName) + blob := &bytes.Buffer{} + err := m.virtualMachines.GetSerialConsoleForVM(ctx, resourceGroupName, vmName, blob) if err != nil { items = append(items, fmt.Sprintf("vm boot diagnostics retrieval error for %s: %s", vmName, err)) continue } logForVM := m.log.WithField("failedRoleInstance", vmName) - - b64Reader := base64.NewDecoder(base64.StdEncoding, serialConsoleBlob) - scanner := bufio.NewScanner(b64Reader) + scanner := bufio.NewScanner(blob) for scanner.Scan() { logForVM.Info(scanner.Text()) } diff --git a/pkg/cluster/failurediagnostics/virtualmachines_test.go b/pkg/cluster/failurediagnostics/virtualmachines_test.go index 1f650fb8e07..83753d0835e 100644 --- a/pkg/cluster/failurediagnostics/virtualmachines_test.go +++ b/pkg/cluster/failurediagnostics/virtualmachines_test.go @@ -87,8 +87,8 @@ func TestVirtualMachines(t *testing.T) { }, nil) vmClient.EXPECT().GetSerialConsoleForVM( - gomock.Any(), "resourceGroupCluster", "somename", - ).Times(1).Return(nil, errors.New("explod")) + gomock.Any(), "resourceGroupCluster", "somename", gomock.Any(), + ).Times(1).Return(errors.New("explod")) }, expectedLogs: []map[string]types.GomegaMatcher{}, expectedOutput: []interface{}{ @@ -96,46 +96,6 @@ func TestVirtualMachines(t *testing.T) { "vm boot diagnostics retrieval error for somename: explod", }, }, - { - name: "failed blob decoding", - mock: func(vmClient *mock_compute.MockVirtualMachinesClient) { - vmClient.EXPECT().List(gomock.Any(), "resourceGroupCluster").Return([]mgmtcompute.VirtualMachine{ - { - Name: to.StringPtr("somename"), - Location: to.StringPtr("eastus"), - VirtualMachineProperties: &mgmtcompute.VirtualMachineProperties{ - InstanceView: &mgmtcompute.VirtualMachineInstanceView{ - BootDiagnostics: &mgmtcompute.BootDiagnosticsInstanceView{ - SerialConsoleLogBlobURI: to.StringPtr("bogusurl/boguscontainer/bogusblob"), - }, - }, - }, - }, - }, nil) - - out := io.NopCloser(bytes.NewBufferString("aGVsbG8KdGhlcmUgOikKZ")) - - vmClient.EXPECT().GetSerialConsoleForVM( - gomock.Any(), "resourceGroupCluster", "somename", - ).Times(1).Return(out, nil) - }, - expectedLogs: []map[string]types.GomegaMatcher{ - { - "level": gomega.Equal(logrus.InfoLevel), - "msg": gomega.Equal(`hello`), - "failedRoleInstance": gomega.Equal("somename"), - }, - { - "level": gomega.Equal(logrus.InfoLevel), - "msg": gomega.Equal(`there :)`), - "failedRoleInstance": gomega.Equal("somename"), - }, - }, - expectedOutput: []interface{}{ - `vm somename: {"location":"eastus","properties":{}}`, - `blob storage scan on somename: unexpected EOF`, - }, - }, { name: "success", mock: func(vmClient *mock_compute.MockVirtualMachinesClient) { @@ -147,11 +107,14 @@ func TestVirtualMachines(t *testing.T) { }, }, nil) - out := io.NopCloser(bytes.NewBufferString("aGVsbG8KdGhlcmUgOikK")) - + iothing := bytes.NewBufferString("hello\nthere :)") vmClient.EXPECT().GetSerialConsoleForVM( - gomock.Any(), "resourceGroupCluster", "somename", - ).Times(1).Return(out, nil) + gomock.Any(), "resourceGroupCluster", "somename", gomock.Any(), + ).Times(1).DoAndReturn(func(ctx context.Context, + rg string, vmName string, target io.Writer) error { + _, err := io.Copy(target, iothing) + return err + }) }, expectedLogs: []map[string]types.GomegaMatcher{ { diff --git a/pkg/frontend/admin_openshiftcluster_serialconsole.go b/pkg/frontend/admin_openshiftcluster_serialconsole.go index 28d8d8660d0..2046dff71ed 100644 --- a/pkg/frontend/admin_openshiftcluster_serialconsole.go +++ b/pkg/frontend/admin_openshiftcluster_serialconsole.go @@ -4,7 +4,9 @@ package frontend // Licensed under the Apache License 2.0. import ( + "bytes" "context" + "io" "net/http" "path/filepath" "strings" @@ -22,22 +24,25 @@ func (f *frontend) getAdminOpenShiftClusterSerialConsole(w http.ResponseWriter, log := ctx.Value(middleware.ContextKeyLog).(*logrus.Entry) r.URL.Path = filepath.Dir(r.URL.Path) - b, err := f._getAdminOpenShiftClusterSerialConsole(ctx, r, log) + buf := &bytes.Buffer{} + + err := f._getAdminOpenShiftClusterSerialConsole(ctx, r, log, buf) if err == nil { w.Header().Set("Content-Type", "text/plain") + _, err = io.Copy(w, buf) } - adminReply(log, w, nil, b, err) + adminReply(log, w, nil, nil, err) } -func (f *frontend) _getAdminOpenShiftClusterSerialConsole(ctx context.Context, r *http.Request, log *logrus.Entry) ([]byte, error) { +func (f *frontend) _getAdminOpenShiftClusterSerialConsole(ctx context.Context, r *http.Request, log *logrus.Entry, w io.Writer) error { resType, resName, resGroupName := chi.URLParam(r, "resourceType"), chi.URLParam(r, "resourceName"), chi.URLParam(r, "resourceGroupName") vmName := r.URL.Query().Get("vmName") err := validateAdminVMName(vmName) if err != nil { - return nil, err + return err } resourceID := strings.TrimPrefix(r.URL.Path, "/admin") @@ -45,20 +50,20 @@ func (f *frontend) _getAdminOpenShiftClusterSerialConsole(ctx context.Context, r doc, err := f.dbOpenShiftClusters.Get(ctx, resourceID) switch { case cosmosdb.IsErrorStatusCode(err, http.StatusNotFound): - return nil, api.NewCloudError(http.StatusNotFound, api.CloudErrorCodeResourceNotFound, "", "The Resource '%s/%s' under resource group '%s' was not found.", resType, resName, resGroupName) + return api.NewCloudError(http.StatusNotFound, api.CloudErrorCodeResourceNotFound, "", "The Resource '%s/%s' under resource group '%s' was not found.", resType, resName, resGroupName) case err != nil: - return nil, err + return err } subscriptionDoc, err := f.getSubscriptionDocument(ctx, doc.Key) if err != nil { - return nil, err + return err } a, err := f.azureActionsFactory(log, f.env, doc.OpenShiftCluster, subscriptionDoc) if err != nil { - return nil, err + return err } - return a.VMSerialConsole(ctx, log, vmName) + return a.VMSerialConsole(ctx, log, vmName, w) } diff --git a/pkg/frontend/adminactions/azureactions.go b/pkg/frontend/adminactions/azureactions.go index e9d8456b29d..bf807db2582 100644 --- a/pkg/frontend/adminactions/azureactions.go +++ b/pkg/frontend/adminactions/azureactions.go @@ -34,7 +34,7 @@ type AzureActions interface { VMSizeList(ctx context.Context) ([]mgmtcompute.ResourceSku, error) VMResize(ctx context.Context, vmName string, vmSize string) error ResourceGroupHasVM(ctx context.Context, vmName string) (bool, error) - VMSerialConsole(ctx context.Context, log *logrus.Entry, vmName string) ([]byte, error) + VMSerialConsole(ctx context.Context, log *logrus.Entry, vmName string, target io.Writer) error ResourceDeleteAndWait(ctx context.Context, resourceID string) error } diff --git a/pkg/frontend/adminactions/vmserialconsole.go b/pkg/frontend/adminactions/vmserialconsole.go index 4d1b190c704..8ce8f51700b 100644 --- a/pkg/frontend/adminactions/vmserialconsole.go +++ b/pkg/frontend/adminactions/vmserialconsole.go @@ -13,13 +13,8 @@ import ( ) func (a *azureActions) VMSerialConsole(ctx context.Context, - log *logrus.Entry, vmName string) ([]byte, error) { + log *logrus.Entry, vmName string, target io.Writer) error { clusterRGName := stringutils.LastTokenByte(a.oc.Properties.ClusterProfile.ResourceGroupID, '/') - blob, err := a.virtualMachines.GetSerialConsoleForVM(ctx, clusterRGName, vmName) - if err != nil { - return nil, err - } - - return io.ReadAll(blob) + return a.virtualMachines.GetSerialConsoleForVM(ctx, clusterRGName, vmName, target) } diff --git a/pkg/frontend/adminactions/vmserialconsole_test.go b/pkg/frontend/adminactions/vmserialconsole_test.go index b4f4c2ac1b2..d6bbd923574 100644 --- a/pkg/frontend/adminactions/vmserialconsole_test.go +++ b/pkg/frontend/adminactions/vmserialconsole_test.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "fmt" + "io" "testing" "github.com/go-test/deep" @@ -33,7 +34,11 @@ func TestVMSerialConsole(t *testing.T) { mocks: func(vmc *mock_compute.MockVirtualMachinesClient) { iothing := bytes.NewBufferString("outputhere") - vmc.EXPECT().GetSerialConsoleForVM(gomock.Any(), clusterRG, "vm1").Return(iothing, nil) + vmc.EXPECT().GetSerialConsoleForVM(gomock.Any(), clusterRG, "vm1", gomock.Any()).DoAndReturn(func(ctx context.Context, + rg string, vmName string, target io.Writer) error { + _, err := io.Copy(target, iothing) + return err + }) }, wantResponse: []byte(`outputhere`), }, @@ -64,11 +69,12 @@ func TestVMSerialConsole(t *testing.T) { ctx := context.Background() - resp, err := a.VMSerialConsole(ctx, log, "vm1") + target := &bytes.Buffer{} + err := a.VMSerialConsole(ctx, log, "vm1", target) utilerror.AssertErrorMessage(t, err, tt.wantError) - for _, errs := range deep.Equal(resp, tt.wantResponse) { + for _, errs := range deep.Equal(target.Bytes(), tt.wantResponse) { t.Error(errs) } }) diff --git a/pkg/util/azureclient/mgmt/compute/virtualmachines_addons.go b/pkg/util/azureclient/mgmt/compute/virtualmachines_addons.go index 4640a1952b5..57ec05ca02a 100644 --- a/pkg/util/azureclient/mgmt/compute/virtualmachines_addons.go +++ b/pkg/util/azureclient/mgmt/compute/virtualmachines_addons.go @@ -4,7 +4,6 @@ package compute // Licensed under the Apache License 2.0. import ( - "bytes" "context" "fmt" "io" @@ -22,7 +21,7 @@ type VirtualMachinesClientAddons interface { StartAndWait(ctx context.Context, resourceGroupName string, VMName string) error StopAndWait(ctx context.Context, resourceGroupName string, VMName string, deallocateVM bool) error List(ctx context.Context, resourceGroupName string) (result []mgmtcompute.VirtualMachine, err error) - GetSerialConsoleForVM(ctx context.Context, resourceGroupName string, VMName string) (blob io.Reader, err error) + GetSerialConsoleForVM(ctx context.Context, resourceGroupName string, VMName string, target io.Writer) error } func (c *virtualMachinesClient) CreateOrUpdateAndWait(ctx context.Context, resourceGroupName string, VMName string, parameters mgmtcompute.VirtualMachine) error { @@ -118,32 +117,31 @@ func (c *virtualMachinesClient) retrieveBootDiagnosticsData(ctx context.Context, // GetSerialConsoleForVM will return the serial console log blob as an // io.ReadCloser, or an error if it cannot be retrieved. -func (c *virtualMachinesClient) GetSerialConsoleForVM(ctx context.Context, resourceGroupName string, vmName string) (io.Reader, error) { +func (c *virtualMachinesClient) GetSerialConsoleForVM(ctx context.Context, resourceGroupName string, vmName string, target io.Writer) error { serialConsoleLogBlobURI, err := c.retrieveBootDiagnosticsData(ctx, resourceGroupName, vmName) if err != nil { - return nil, fmt.Errorf("failure getting boot diagnostics URI Azure: %w", err) + return fmt.Errorf("failure getting boot diagnostics URI Azure: %w", err) } req, err := http.NewRequestWithContext(ctx, http.MethodGet, serialConsoleLogBlobURI, nil) if err != nil { - return nil, err + return err } resp, err := http.DefaultClient.Do(req) if err != nil { - return nil, fmt.Errorf("failure downloading blob URI: %w", err) + return fmt.Errorf("failure downloading blob URI: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("got %d instead of 200 downloading blob URI", resp.StatusCode) + return fmt.Errorf("got %d instead of 200 downloading blob URI", resp.StatusCode) } - buf := &bytes.Buffer{} - _, err = io.Copy(buf, resp.Body) + _, err = io.Copy(target, resp.Body) if err != nil { - return nil, fmt.Errorf("failure downloading blob URI body: %w", err) + return fmt.Errorf("failure copying blob URI body: %w", err) } - return buf, nil + return nil } diff --git a/pkg/util/mocks/adminactions/adminactions.go b/pkg/util/mocks/adminactions/adminactions.go index ef2d23c3684..0fb390effa4 100644 --- a/pkg/util/mocks/adminactions/adminactions.go +++ b/pkg/util/mocks/adminactions/adminactions.go @@ -325,18 +325,17 @@ func (mr *MockAzureActionsMockRecorder) VMResize(arg0, arg1, arg2 interface{}) * } // VMSerialConsole mocks base method. -func (m *MockAzureActions) VMSerialConsole(arg0 context.Context, arg1 *logrus.Entry, arg2 string) ([]byte, error) { +func (m *MockAzureActions) VMSerialConsole(arg0 context.Context, arg1 *logrus.Entry, arg2 string, arg3 io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "VMSerialConsole", arg0, arg1, arg2) - ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "VMSerialConsole", arg0, arg1, arg2, arg3) + ret0, _ := ret[0].(error) + return ret0 } // VMSerialConsole indicates an expected call of VMSerialConsole. -func (mr *MockAzureActionsMockRecorder) VMSerialConsole(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockAzureActionsMockRecorder) VMSerialConsole(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "VMSerialConsole", reflect.TypeOf((*MockAzureActions)(nil).VMSerialConsole), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "VMSerialConsole", reflect.TypeOf((*MockAzureActions)(nil).VMSerialConsole), arg0, arg1, arg2, arg3) } // VMSizeList mocks base method. diff --git a/pkg/util/mocks/azureclient/mgmt/compute/compute.go b/pkg/util/mocks/azureclient/mgmt/compute/compute.go index 2532aff8241..0f05283b764 100644 --- a/pkg/util/mocks/azureclient/mgmt/compute/compute.go +++ b/pkg/util/mocks/azureclient/mgmt/compute/compute.go @@ -170,18 +170,17 @@ func (mr *MockVirtualMachinesClientMockRecorder) Get(arg0, arg1, arg2, arg3 inte } // GetSerialConsoleForVM mocks base method. -func (m *MockVirtualMachinesClient) GetSerialConsoleForVM(arg0 context.Context, arg1, arg2 string) (io.Reader, error) { +func (m *MockVirtualMachinesClient) GetSerialConsoleForVM(arg0 context.Context, arg1, arg2 string, arg3 io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetSerialConsoleForVM", arg0, arg1, arg2) - ret0, _ := ret[0].(io.Reader) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "GetSerialConsoleForVM", arg0, arg1, arg2, arg3) + ret0, _ := ret[0].(error) + return ret0 } // GetSerialConsoleForVM indicates an expected call of GetSerialConsoleForVM. -func (mr *MockVirtualMachinesClientMockRecorder) GetSerialConsoleForVM(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockVirtualMachinesClientMockRecorder) GetSerialConsoleForVM(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSerialConsoleForVM", reflect.TypeOf((*MockVirtualMachinesClient)(nil).GetSerialConsoleForVM), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSerialConsoleForVM", reflect.TypeOf((*MockVirtualMachinesClient)(nil).GetSerialConsoleForVM), arg0, arg1, arg2, arg3) } // List mocks base method. diff --git a/test/e2e/adminapi_serialconsole.go b/test/e2e/adminapi_serialconsole.go index f9bf8a5f959..70d8b060172 100644 --- a/test/e2e/adminapi_serialconsole.go +++ b/test/e2e/adminapi_serialconsole.go @@ -4,11 +4,7 @@ package e2e // Licensed under the Apache License 2.0. import ( - "bufio" - "bytes" "context" - "encoding/base64" - "fmt" "net/http" "net/url" "strings" @@ -49,22 +45,7 @@ var _ = Describe("[Admin API] VM serial console action", func() { Expect(err).NotTo(HaveOccurred()) Expect(resp.StatusCode).To(Equal(http.StatusOK)) - By("decoding the logs, we can see Linux serial console") - log.Infof("got logs: %s", logs) - foundLogs := false - b64Reader := base64.NewDecoder(base64.StdEncoding, bytes.NewBufferString(logs)) - scanner := bufio.NewScanner(b64Reader) - output := "" - for scanner.Scan() { - output = output + scanner.Text() - } - Expect(scanner.Err()).NotTo(HaveOccurred()) - - if strings.Contains(output, "Red Hat Enterprise Linux CoreOS") { - foundLogs = true - } - - Expect(foundLogs).To(BeTrue(), fmt.Sprintf("expected to find serial console logs in b64: %s", logs)) - + By("we can see Linux serial console") + Expect(logs).To(ContainSubstring("Red Hat Enterprise Linux CoreOS")) }) })