[TT-7560] skip loading API when custom middleware bundle fetch fails by jeffy-mathew · Pull Request #6211 · TykTechnologies/tyk

jeffy-mathew · 2024-04-08T15:00:55Z

User description

Description

Skip loading an API when the custom middleware bundle fetch fails.

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

Type

bug_fix, enhancement

Description

Modified MakeSpec in api_definition.go to return an error along with *APISpec, enhancing error handling during API spec creation.
Implemented error handling in various functions and tests that call MakeSpec, ensuring that API loading can be skipped if a custom middleware bundle fetch fails.
Added and updated tests to cover new scenarios related to custom middleware bundle fetching and API loading behavior.

Changes walkthrough

Relevant files

Enhancement

api_definition.go `Handle Errors in API Spec Creation and Skip on Bundle Fetch Failures` gateway/api_definition.go Changed `MakeSpec` to return an error along with `*APISpec`. Updated `prepareSpecs` and `loadDefFromFilePath` to handle errors from `MakeSpec`. Modified API loading to skip APIs when bundle fetch fails.	+11/-8
tracing.go `Improve Error Handling in Trace Handler` gateway/tracing.go Updated trace handler to handle errors from `MakeSpec` and return an appropriate response.	+6/-1

Tests

coprocess_bundle_test.go `Test Custom Middleware Bundle Fetch Scenarios` gateway/coprocess_bundle_test.go Added tests to verify API loading behavior with custom middleware bundle fetch scenarios.	+24/-0
coprocess_id_extractor_test.go `Update Spec Creation in ID Extractor Tests` gateway/coprocess_id_extractor_test.go Updated `createSpecTestFrom` to handle errors from `MakeSpec`.	+1/-1
gateway_test.go `Test API Loading Failure on Bundle Fetch Failure` gateway/gateway_test.go Added a test case to verify the behavior when API loading fails due to bundle fetch failure.	+13/-0
testutil.go `Handle Spec Creation Errors in Test Utilities` gateway/testutil.go Updated `CreateDefinitionFromString` to handle errors from `MakeSpec`.	+1/-1

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-04-08T15:01:58Z

API Changes

--- prev.txt	2024-04-08 20:19:20.804391951 +0000
+++ current.txt	2024-04-08 20:19:18.012350648 +0000
@@ -7085,7 +7085,7 @@
 
 func (a APIDefinitionLoader) GetOASFilepath(path string) string
 
-func (a APIDefinitionLoader) MakeSpec(def *nestedApiDefinition, logger *logrus.Entry) *APISpec
+func (a APIDefinitionLoader) MakeSpec(def *nestedApiDefinition, logger *logrus.Entry) (*APISpec, error)
     MakeSpec will generate a flattened URLSpec from and APIDefinitions'
     VersionInfo data. paths are keyed to the Api version name, which is
     determined during routing to speed up lookups

github-actions · 2024-04-08T15:01:59Z

PR Description updated to latest commit (112c705)

github-actions · 2024-04-08T15:02:39Z

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant changes to the core functionality of API loading and error handling. It requires a deep understanding of the existing architecture and the implications of the changes on API loading and middleware bundle fetching.
🧪 Relevant tests	No
🔍 Possible issues	Error Handling Consistency: The PR introduces a change where `MakeSpec` now returns an error which is handled differently across various calls. In some places, the error leads to skipping the loading of an API, but it's not clear if this behavior is consistent and expected in all scenarios.
🔍 Possible issues	Bundle Fetching Logic: The logic to skip loading an API when the custom middleware bundle fetch fails might introduce issues with API availability, especially in scenarios where the failure is temporary or expected to be resolved without manual intervention.
🔒 Security concerns	No

Code feedback:

relevant file	gateway/api_definition.go
suggestion	Consider adding more detailed logging for the error scenarios where API loading is skipped due to bundle fetch failures. This will help in debugging and understanding the frequency and reasons for these skips. [important]
relevant line	return nil, err

relevant file	gateway/api_definition.go
suggestion	It might be beneficial to implement a retry mechanism or a fallback strategy for bundle fetching to handle temporary network issues or misconfigurations, enhancing the robustness of the API loading process. [medium]
relevant line	return nil, err

relevant file	gateway/coprocess_bundle_test.go
suggestion	Add tests to verify the behavior of the system when the bundle fetch fails after a certain number of retries or when the bundle becomes available after being initially unavailable. This will ensure the system's resilience and reliability. [medium]
relevant line	assert.Empty(t, ts.Gw.apiSpecs)

relevant file	gateway/gateway_test.go
suggestion	Consider adding a test case to verify the behavior of tracing functionality when the API spec loading fails due to bundle fetch issues. This will ensure that tracing and debugging capabilities are not adversely affected by these changes. [medium]
relevant line	{Method: "POST", Path: "/tyk/debug", Data: traceRequest{Spec: apiDefWithBundle, Request: &traceHttpRequest{Method: "GET", Path: "/", Headers: authHeaders}}, AdminAuth: true, Code: http.StatusBadRequest, BodyMatch: `Couldn't load bundle`},

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

github-actions · 2024-04-08T15:03:33Z

PR Code Suggestions

Category	Suggestions
Possible issue	Add nil check for logger before logging errors. Consider handling the case where `logger` is `nil` before attempting to log errors. This can prevent potential nil pointer dereferences if `logger` is not provided. gateway/api_definition.go [309] -logger.WithError(err).WithField("name", def.Name).Error("Failed to JSON marshal API definition") +if logger != nil { + logger.WithError(err).WithField("name", def.Name).Error("Failed to JSON marshal API definition") +}
Enhancement	Return partially constructed spec on error for better debugging. *When `MakeSpec` encounters an error, it's returning `nil` for the `APISpec`. It might be more informative for debugging if it returned the partially constructed `spec` along with the error. This way, the caller can inspect the state of `spec` at the time of the error. gateway/api_definition.go [310] -return nil, err +return spec, err
	Log errors in prepareSpecs when MakeSpec fails. In the `prepareSpecs` function, when an error occurs in `MakeSpec`, it continues to the next iteration without logging the error. It would be beneficial to log these errors for troubleshooting and visibility into why certain API definitions failed to load. gateway/api_definition.go [721] +if logger != nil { + logger.WithError(err).Error("Failed to prepare spec") +} continue
	Correct test case description to match the simulated scenario. The test case description "skip when fetch is successful" seems incorrect based on the test setup, which simulates a fetch failure by setting an invalid bundle base URL. Consider updating the description to accurately reflect the test scenario, such as "skip loading API when bundle fetch fails". gateway/coprocess_bundle_test.go [243] -t.Run("skip when fetch is successful", func(t testing.T) { +t.Run("skip loading API when bundle fetch fails", func(t testing.T) {
	Include specific error messages when MakeSpec fails in tracing handler. The error message returned when `MakeSpec` fails is generic and does not provide specific details about the failure. Consider including the error message from `MakeSpec` in the response to give more context about why the operation failed. gateway/tracing.go [122] -doJSONWrite(w, http.StatusBadRequest, traceResponse{Message: "error", Logs: logStorage.String()}) +doJSONWrite(w, http.StatusBadRequest, traceResponse{Message: fmt.Sprintf("error: %v", err), Logs: logStorage.String()})

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

github-actions · 2024-04-08T15:08:12Z

💥 CI tests failed 🙈