From c90957d06d9409598c070b154a4ea580fc728bd9 Mon Sep 17 00:00:00 2001 From: ryanfkeepers Date: Tue, 24 Oct 2023 13:53:16 -0600 Subject: [PATCH] setting aside --- src/internal/m365/graph/errors.go | 14 ++++++++++ src/internal/m365/graph/errors_test.go | 34 +++++++++++++++++++++++++ src/internal/m365/graph/http_wrapper.go | 1 + src/internal/m365/graph/middleware.go | 32 +++++++++++++++++++++++ src/internal/m365/graph/service.go | 1 + src/internal/stats/stats.go | 7 ++--- src/pkg/backup/backup.go | 15 ++++++++--- src/pkg/backup/backup_test.go | 23 +++++++++++++---- src/pkg/fault/skipped.go | 6 +++++ 9 files changed, 121 insertions(+), 12 deletions(-) diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go index 915f72fd4..2273b6d74 100644 --- a/src/internal/m365/graph/errors.go +++ b/src/internal/m365/graph/errors.go @@ -120,6 +120,16 @@ var ( // replies, no error should get returned. ErrMultipleResultsMatchIdentifier = clues.New("multiple results match the identifier") + // ErrNoRespServerFailure is a generic name for a specific condition: when the request + // fails out after all attempted retries with the conditions: + // 1. response status code 503 + // 2. response content length <= 0 + // This can indicate a persistent inability to access the requested resource. It's + // difficult to determine the underlying cause, since the server provides no response + // body. In many cases this is a non-transient issue and must be skipped to ensure + // the operation succeeds. + ErrNoRespServerFailure = clues.New("server failed to respond to request") + // ErrResourceLocked occurs when a resource has had its access locked. // Example case: https://learn.microsoft.com/en-us/sharepoint/manage-lock-status // This makes the resource inaccessible for any Corso operations. @@ -285,6 +295,10 @@ func IsErrResourceLocked(err error) bool { hasErrorCode(err, NotAllowed) } +func IsErrNoRespServerFailure(err error) bool { + return errors.Is(err, ErrNoRespServerFailure) +} + // --------------------------------------------------------------------------- // error parsers // --------------------------------------------------------------------------- diff --git a/src/internal/m365/graph/errors_test.go b/src/internal/m365/graph/errors_test.go index e46955035..0640818d3 100644 --- a/src/internal/m365/graph/errors_test.go +++ b/src/internal/m365/graph/errors_test.go @@ -162,6 +162,40 @@ func (suite *GraphErrorsUnitSuite) TestIsErrAuthenticationError() { } } +func (suite *GraphErrorsUnitSuite) TestIsErrNoRespServerFailure() { + table := []struct { + name string + err error + expect assert.BoolAssertionFunc + }{ + { + name: "nil", + err: nil, + expect: assert.False, + }, + { + name: "non-matching", + err: assert.AnError, + expect: assert.False, + }, + { + name: "non-matching oDataErr", + err: odErr(ErrNoRespServerFailure.Error()), + expect: assert.False, + }, + { + name: "matching error", + err: ErrNoRespServerFailure, + expect: assert.True, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + test.expect(suite.T(), IsErrNoRespServerFailure(test.err)) + }) + } +} + func (suite *GraphErrorsUnitSuite) TestIsErrDeletedInFlight() { table := []struct { name string diff --git a/src/internal/m365/graph/http_wrapper.go b/src/internal/m365/graph/http_wrapper.go index 7f5c840b2..6582909ec 100644 --- a/src/internal/m365/graph/http_wrapper.go +++ b/src/internal/m365/graph/http_wrapper.go @@ -180,6 +180,7 @@ func defaultTransport() http.RoundTripper { func internalMiddleware(cc *clientConfig) []khttp.Middleware { mw := []khttp.Middleware{ + &ErrorIdentifierMiddleware{}, &RetryMiddleware{ MaxRetries: cc.maxRetries, Delay: cc.minDelay, diff --git a/src/internal/m365/graph/middleware.go b/src/internal/m365/graph/middleware.go index d5935a5b0..74b76026e 100644 --- a/src/internal/m365/graph/middleware.go +++ b/src/internal/m365/graph/middleware.go @@ -369,3 +369,35 @@ func (mw *MetricsMiddleware) Intercept( return resp, err } + +// --------------------------------------------------------------------------- +// Error Edge Case Identifier +// --------------------------------------------------------------------------- + +// ErrorIdentifierMiddleware ensures known edge cases result in well-represented errors. +type ErrorIdentifierMiddleware struct{} + +func (mw *ErrorIdentifierMiddleware) Intercept( + pipeline khttp.Pipeline, + middlewareIndex int, + req *http.Request, +) (*http.Response, error) { + ctx := req.Context() + resp, err := pipeline.Next(req, middlewareIndex) + + if resp == nil || err != nil { + return resp, err + } + + if resp.StatusCode == http.StatusServiceUnavailable && resp.ContentLength <= 0 { + // log the response body dump just for security. Sometimes a "0 content length" + // is actually due to the client's inability to parse the response, and not that + // the response content is actually missing. + dump := getRespDump(ctx, resp, true) + logger.Ctx(ctx).Infow("graph api resp - 503 with no content", "response", dump) + + return nil, clues.Stack(ErrNoRespServerFailure) + } + + return resp, err +} diff --git a/src/internal/m365/graph/service.go b/src/internal/m365/graph/service.go index 0b54b5589..06a754eda 100644 --- a/src/internal/m365/graph/service.go +++ b/src/internal/m365/graph/service.go @@ -274,6 +274,7 @@ func kiotaMiddlewares( ) []khttp.Middleware { mw := []khttp.Middleware{ msgraphgocore.NewGraphTelemetryHandler(options), + &ErrorIdentifierMiddleware{}, &RetryMiddleware{ MaxRetries: cc.maxRetries, Delay: cc.minDelay, diff --git a/src/internal/stats/stats.go b/src/internal/stats/stats.go index 4ecc5a811..72fbc7d3c 100644 --- a/src/internal/stats/stats.go +++ b/src/internal/stats/stats.go @@ -39,7 +39,8 @@ func (bc *ByteCounter) Count(i int64) { } type SkippedCounts struct { - TotalSkippedItems int `json:"totalSkippedItems"` - SkippedMalware int `json:"skippedMalware"` - SkippedInvalidOneNoteFile int `json:"skippedInvalidOneNoteFile"` + TotalSkippedItems int `json:"totalSkippedItems"` + SkippedMalware int `json:"skippedMalware"` + SkippedInvalidOneNoteFile int `json:"skippedInvalidOneNoteFile"` + SkippedPermanentServiceFailure int `json:"skippedPermanentServiceFailure"` } diff --git a/src/pkg/backup/backup.go b/src/pkg/backup/backup.go index 6a9a27a7b..8e26d93d3 100644 --- a/src/pkg/backup/backup.go +++ b/src/pkg/backup/backup.go @@ -90,7 +90,7 @@ func New( skipCount = len(fe.Skipped) failMsg string - malware, invalidONFile, otherSkips int + malware, invalidONFile, permanentServiceFailure, otherSkips int ) if fe.Failure != nil { @@ -104,6 +104,8 @@ func New( malware++ case s.HasCause(fault.SkipOneNote): invalidONFile++ + case s.HasCause(fault.SkipPermanentServiceFailure): + permanentServiceFailure++ default: otherSkips++ } @@ -134,9 +136,10 @@ func New( ReadWrites: rw, StartAndEndTime: se, SkippedCounts: stats.SkippedCounts{ - TotalSkippedItems: skipCount, - SkippedMalware: malware, - SkippedInvalidOneNoteFile: invalidONFile, + TotalSkippedItems: skipCount, + SkippedMalware: malware, + SkippedInvalidOneNoteFile: invalidONFile, + SkippedPermanentServiceFailure: permanentServiceFailure, }, } } @@ -245,6 +248,10 @@ func (b Backup) Values() []string { skipped = append(skipped, fmt.Sprintf("%d invalid OneNote file", b.SkippedInvalidOneNoteFile)) } + if b.SkippedPermanentServiceFailure > 0 { + skipped = append(skipped, fmt.Sprintf("%d permanent service failures", b.SkippedPermanentServiceFailure)) + } + status += strings.Join(skipped, ", ") if errCount+b.TotalSkippedItems > 0 { diff --git a/src/pkg/backup/backup_test.go b/src/pkg/backup/backup_test.go index c1ce6f1cf..ae6dd345f 100644 --- a/src/pkg/backup/backup_test.go +++ b/src/pkg/backup/backup_test.go @@ -202,17 +202,30 @@ func (suite *BackupUnitSuite) TestBackup_Values_statusVariations() { expect: "test (42 errors, 1 skipped: 1 invalid OneNote file)", }, { - name: "errors, malware, notFound, invalid OneNote", + name: "errors and permanent service failures", bup: backup.Backup{ Status: "test", ErrorCount: 42, SkippedCounts: stats.SkippedCounts{ - TotalSkippedItems: 1, - SkippedMalware: 1, - SkippedInvalidOneNoteFile: 1, + TotalSkippedItems: 1, + SkippedPermanentServiceFailure: 1, }, }, - expect: "test (42 errors, 1 skipped: 1 malware, 1 invalid OneNote file)", + expect: "test (42 errors, 1 skipped: 1 permanent service failures)", + }, + { + name: "errors, malware, notFound, invalid OneNote, permanent service failures", + bup: backup.Backup{ + Status: "test", + ErrorCount: 42, + SkippedCounts: stats.SkippedCounts{ + TotalSkippedItems: 1, + SkippedMalware: 1, + SkippedInvalidOneNoteFile: 1, + SkippedPermanentServiceFailure: 1, + }, + }, + expect: "test (42 errors, 1 skipped: 1 malware, 1 invalid OneNote file, 1 permanent service failures)", }, } for _, test := range table { diff --git a/src/pkg/fault/skipped.go b/src/pkg/fault/skipped.go index 6aead57b1..f676772ce 100644 --- a/src/pkg/fault/skipped.go +++ b/src/pkg/fault/skipped.go @@ -32,6 +32,12 @@ const ( //nolint:lll // https://support.microsoft.com/en-us/office/restrictions-and-limitations-in-onedrive-and-sharepoint-64883a5d-228e-48f5-b3d2-eb39e07630fa#onenotenotebooks SkipOneNote skipCause = "inaccessible_one_note_file" + + // SkipPermanentServiceFailure identifies that a file was skipped + // because a request failed out with a 503 status code and a response + // with no content. We assume this case to represent non-transient + // conditions. + SkipPermanentServiceFailure skipCause = "permanent_service_failure" ) var _ print.Printable = &Skipped{}