CORS-4537: GCP: gracefully handle 503 in disk type check - #10732
Conversation
Add 500-type errors, such as 503 Service Unavailable to the graceful handling of the disk type validation. The disk type validation is supposed to prevent known failures, but if for some reason the API call fails we do not want that in and of itself to be fatal. That was the original design of the validation, this commit just adds 503 to that handling.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4537 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughChangesDisk type availability validation
Estimated code review effort: 2 (Simple) | ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/asset/installconfig/gcp/validation_test.go (1)
1452-1462: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the exact HTTP 500 boundary.
The implementation branches on
gerr.Code < 500, but the new tests cover only 503 and the existing 403 case. Addhttp.StatusInternalServerErrorto ensure status 500 also degrades gracefully and prevent a future boundary regression.As per path instructions, validation tests should cover edge cases, especially boundary behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/installconfig/gcp/validation_test.go` around lines 1452 - 1462, The disk-type validation tests should cover the exact HTTP 500 boundary used by the error-handling branch. Extend the relevant test cases around the existing 403 and 503 scenarios with a googleapi.Error using http.StatusInternalServerError, and assert it produces the same expectedWarn graceful-degradation message.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/installconfig/gcp/validation_test.go`:
- Around line 1483-1486: In the warning-validation block of the test, replace
the non-fatal hook.Entries check with a fatal assertion before calling
hook.LastEntry().Message. Keep the existing expectedWarn condition and regexp
validation unchanged, ensuring the test stops safely when no warning entries are
emitted.
In `@pkg/asset/installconfig/gcp/validation.go`:
- Around line 203-207: Update the disk-type verification error handling near the
gerr status check to avoid passing raw err into logrus.Warnf. Log only sanitized
information derived from the API error, such as its status code and reason,
while preserving the existing field.Invalid behavior for codes below 500 and
skip-check behavior otherwise.
---
Nitpick comments:
In `@pkg/asset/installconfig/gcp/validation_test.go`:
- Around line 1452-1462: The disk-type validation tests should cover the exact
HTTP 500 boundary used by the error-handling branch. Extend the relevant test
cases around the existing 403 and 503 scenarios with a googleapi.Error using
http.StatusInternalServerError, and assert it produces the same expectedWarn
graceful-degradation message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 39b66530-c831-49f7-ac6f-2d51f951303e
📒 Files selected for processing (2)
pkg/asset/installconfig/gcp/validation.gopkg/asset/installconfig/gcp/validation_test.go
| if test.expectedWarn != "" { | ||
| assert.NotEmpty(t, hook.Entries) | ||
| assert.Regexp(t, test.expectedWarn, hook.LastEntry().Message) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='pkg/asset/installconfig/gcp/validation_test.go'
# Locate the relevant area and surrounding context.
wc -l "$file"
sed -n '1450,1505p' "$file"
# Find how hook is created/used in this test file.
rg -n 'hook|LastEntry\(|expectedWarn|assert\.NotEmpty|require\.NotEmpty' "$file"Repository: openshift/installer
Length of output: 2998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='pkg/asset/installconfig/gcp/validation_test.go'
wc -l "$file"
sed -n '1450,1505p' "$file"
rg -n 'hook|LastEntry\(|expectedWarn|assert\.NotEmpty|require\.NotEmpty' "$file"Repository: openshift/installer
Length of output: 2998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "hello"Repository: openshift/installer
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1470,1495p' pkg/asset/installconfig/gcp/validation_test.goRepository: openshift/installer
Length of output: 982
Guard the warning assertion.
assert.NotEmpty is non-fatal, so hook.LastEntry().Message can still panic when no warning is emitted. Use require.NotEmpty or wrap the regexp check in the assertion result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/asset/installconfig/gcp/validation_test.go` around lines 1483 - 1486, In
the warning-validation block of the test, replace the non-fatal hook.Entries
check with a fatal assertion before calling hook.LastEntry().Message. Keep the
existing expectedWarn condition and regexp validation unchanged, ensuring the
test stops safely when no warning entries are emitted.
Source: MCP tools
| if errors.As(err, &gerr) && gerr.Code < 500 { | ||
| return append(allErrs, field.Invalid(fieldPath.Child("diskType"), diskType, err.Error())) | ||
| } | ||
| return append(allErrs, field.InternalError(fieldPath.Child("diskType"), err)) | ||
| logrus.Warnf("could not verify disk type %s availability in %s, skipping API check: %v", diskType, region, err) | ||
| return allErrs |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/asset/installconfig/gcp/validation.go"
# Show the relevant section with line numbers
sed -n '150,240p' "$file" | cat -n
# Find the function and related call sites
rg -n "GetDiskTypeWithZones|could not verify disk type|Warnf\\(" pkg/asset/installconfig/gcp -n
# Inspect the helper implementation and any error wrapping nearby
rg -n "type .*Client|func .*GetDiskTypeWithZones|return .*error|Errorf\\(" pkg/asset/installconfig/gcp pkg/asset/installconfig -nRepository: openshift/installer
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the GCP disk-type check is implemented in the repository
rg -n "GetDiskTypeWithZones" .Repository: openshift/installer
Length of output: 2224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the installer GCP validation file and the API client used by the check.
sed -n '1,260p' pkg/asset/installconfig/gcp/validation.go | cat -n
echo '---'
rg -n "GetDiskTypeWithZones|disk type|zone" pkg/asset/installconfig/gcp pkg/asset/installconfigRepository: openshift/installer
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the GetDiskTypeWithZones implementation and nearby error wrapping.
sed -n '227,270p' pkg/asset/installconfig/gcp/client.go | cat -n
echo '---'
sed -n '1380,1485p' pkg/asset/installconfig/gcp/validation_test.go | cat -nRepository: openshift/installer
Length of output: 6145
🌐 Web query:
googleapi.Error Error() includes request URL response body gcp client Go
💡 Result:
In the Google APIs Client Library for Go, googleapi.Error is the standard error type returned by client library methods when an API call fails [1][2]. The googleapi.Error struct is defined in the google.golang.org/api/googleapi package and contains the following relevant fields [3][4]: Code: The HTTP response status code (int) [3][4]. Message: The server error message, populated if provided by the JSON response [3][4]. Body: The raw response body returned by the server (string) [3][4]. Header: The HTTP response headers (http.Header) [3][4]. Note that the googleapi.Error struct does not explicitly include a Request URL field [3][4]. To obtain information about the request, you should manage the request URL within your own application logic before or during the execution of the API call. To handle these errors, you should use a type assertion to check if a returned error is of type *googleapi.Error [5][1]: if err!= nil { if gErr, ok:= err.(*googleapi.Error); ok { fmt.Printf("HTTP Status: %d\n", gErr.Code) fmt.Printf("Response Body: %s\n", gErr.Body) } } Additionally, modern Google Cloud Go client libraries often wrap errors in apierror.APIError (from the github.com/googleapis/gax-go/v2/apierror package), which provides more structured access to API error details [5][6]. You can use errors.As to inspect these errors [5][6]: var aErr *apierror.APIError if errors.As(err, &aErr) { fmt.Printf("Reason: %s\n", aErr.Reason) }
Citations:
- 1: https://github.com/googleapis/google-api-go-client/blob/main/GettingStarted.md
- 2: https://code.googlesource.com/google-api-go-client/+/master/GettingStarted.md
- 3: https://github.com/googleapis/google-api-go-client/blob/main/googleapi/googleapi.go
- 4: https://github.com/googleapis/google-api-go-client/blob/master/googleapi/googleapi.go
- 5: https://github.com/googleapis/google-api-go-client/blob/main/doc.go
- 6: https://github.com/googleapis/gax-go/blob/v2.21.0/v2/apierror/apierror.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("pkg/asset/installconfig/gcp/client.go")
lines = p.read_text().splitlines()
for start, end in [(227, 270), (230, 268)]:
print(f"--- {p}:{start}-{end} ---")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d} {lines[i]}")
PYRepository: openshift/installer
Length of output: 2834
🌐 Web query:
googleapi.Error Error() method body message source google.golang.org/api/googleapi
💡 Result:
In the Google APIs Client Library for Go, the Error type in the google.golang.org/api/googleapi package implements the error interface through its Error method [1][2]. The implementation dynamically constructs a human-readable string based on the available error fields (Code, Message, Details, and Errors) [1][3]. The Error method body performs the following steps: 1. Basic Check: If both the Errors slice and the Message field are empty, it returns a simple string containing the HTTP status code and raw body [1][4]. 2. Initialization: It initializes a bytes.Buffer and writes the prefix "googleapi: Error : " [1][4]. 3. Message: If the Message field is populated, it appends it to the buffer [1][4]. 4. Details: If the Details slice is not empty, it encodes the details as indented JSON and appends them to the buffer [1][4]. 5. Termination/Extended Errors: - If the Errors slice is empty, it returns the trimmed string [1][4]. - If there is exactly one error in the slice and its message matches the primary Message field, it appends the reason and returns the result [1][4]. - Otherwise, it appends "More details:" followed by a list of all errors in the slice, including their individual reasons and messages [1][4]. The full source code for this method is maintained in the google-api-go-client repository [1][4]. top_results: [1][2][4]
Citations:
- 1: https://github.com/googleapis/google-api-go-client/blob/main/googleapi/googleapi.go
- 2: https://pkg.go.dev/google.golang.org/api/googleapi
- 3: https://mugli.github.io/tmp-literate-golds/src/google.golang.org/api/googleapi/googleapi.go.html
- 4: https://github.com/googleapis/google-api-go-client/blob/master/googleapi/googleapi.go
🌐 Web query:
site:github.com/googleapis/google-api-go-client googleapi.Error Error() Body
💡 Result:
In the Google API Go client library, googleapi.Error is a struct used to represent error responses returned by Google Cloud APIs [1][2]. The Body field within this struct contains the raw response body returned by the server, which is stored as a string [1][2]. Key details regarding this field and the Error struct: - Purpose: The Body field provides access to the unparsed response content [1][2]. While many Google APIs return error details in a structured JSON format that the library attempts to parse into other fields (like Message or Errors), the Body field captures the complete raw data, which is useful for debugging or handling non-JSON responses [1][2]. - Population: When an API request fails (i.e., returns a non-2xx HTTP status code), the library's internal response checkers (such as CheckResponse or CheckResponseWithBody) populate the Error struct [1][2]. If the response body could not be parsed as structured JSON, or even if it was, the raw response is captured in the Body field [1][2]. - Usage: Developers often cast errors returned by the client's Do method to *googleapi.Error to inspect the HTTP status code (via the Code field) or the raw error body (via the Body field) [3][4]. Example of inspecting the error: if _, err:= svc.Resource.Method.Do; err!= nil { if gErr, ok:= err.(*googleapi.Error); ok { fmt.Printf("Status Code: %d\n", gErr.Code) fmt.Printf("Raw Body: %s\n", gErr.Body) } } Note that in modern versions of the library, errors may also be wrapped as *apierror.APIError, which provides more advanced, structured access to error details [3]. Using errors.As is the recommended practice for inspecting these wrapped errors [3].
Citations:
- 1: https://github.com/googleapis/google-api-go-client/blob/master/googleapi/googleapi.go
- 2: https://github.com/googleapis/google-api-go-client/blob/main/googleapi/googleapi.go
- 3: https://github.com/googleapis/google-api-go-client/blob/main/doc.go
- 4: https://github.com/googleapis/google-api-go-client/blob/main/GettingStarted.md
🌐 Web query:
"func (e *Error) Error()" "googleapi" Body
💡 Result:
The function signature func (e *Error) Error() googleapi Body is not valid Go syntax [1][2]. In the google.golang.org/api/googleapi package, the Error struct implements the standard Go error interface with the following method signature [3][1]: func (e *Error) Error string This method returns a string representation of the error, which includes the HTTP status code and, if available, the error message and details provided by the API server [1][2]. While the Error struct does contain a Body field (which holds the raw response body from the server) [1][2], it is a string field, not a type or a return type for the Error() method [1][2]. If you are trying to access the raw response body, you should access the Body field directly after type-asserting the error to *googleapi.Error [4][5]: if gapiErr, ok:= err.(*googleapi.Error); ok { // Access the raw body here fmt.Println(gapiErr.Body) }
Citations:
- 1: https://github.com/googleapis/google-api-go-client/blob/master/googleapi/googleapi.go
- 2: https://github.com/googleapis/google-api-go-client/blob/main/googleapi/googleapi.go
- 3: https://pkg.go.dev/google.golang.org/api/googleapi
- 4: https://golang.org/pkg/cloud.google.com/go/storage/
- 5: https://stackoverflow.com/questions/53660549/how-to-get-gmail-api-error-code-by-struct-field-name
Avoid logging the raw API error here. googleapi.Error.Error() can include the server response body/details, so err may leak more than a status code. Log a sanitized status/reason instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/asset/installconfig/gcp/validation.go` around lines 203 - 207, Update the
disk-type verification error handling near the gerr status check to avoid
passing raw err into logrus.Warnf. Log only sanitized information derived from
the API error, such as its status code and reason, while preserving the existing
field.Invalid behavior for codes below 500 and skip-check behavior otherwise.
Source: Coding guidelines
|
/test e2e-gcp-ovn |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/approve |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/override-sticky ci/prow/e2e-azure-nat-gateway-single-zone No need to run other platform tests 😅 |
|
@tthvo: Overrode contexts on behalf of tthvo: ci/prow/e2e-aws-ovn, ci/prow/e2e-azure-nat-gateway-single-zone These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5ac9b82
into
openshift:main
4 participants
Follow-up #10687
Handles failures like: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cloud-provider-gcp/131/pull-ci-openshift-cloud-provider-gcp-main-e2e-gcp-ovn/2082552071290097664
Add 500-type errors, such as 503 Service Unavailable to the graceful handling of the disk type validation. The disk type validation is supposed to prevent known failures, but if for some reason the API call fails we do not want that in and of itself to be fatal.
That was the original design of the validation, this commit just adds 503 to that handling.
Summary by CodeRabbit