Add support for global connection timeout and proxy configurations #13054
Conversation
thisaltennakoon
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
📝 WalkthroughWalkthroughThis pull request introduces new endpoint security configuration options by adding fields and methods to the Changes
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/EndpointSecurity.java (1)
367-368: toString() method updated with new fieldsThe
toString()method has been properly updated to include the new fields. However, there's an inconsistency in the string formatting.There's a small formatting inconsistency in the
toString()method. The new fields have'characters after them, unlike other fields:- ", connectionTimeoutConfigType=" + connectionTimeoutConfigType + '\'' + - ", proxyConfigType=" + proxyConfigType + '\'' + + ", connectionTimeoutConfigType=" + connectionTimeoutConfigType + + ", proxyConfigType=" + proxyConfigType +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/EndpointSecurity.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/TokenEndpointConnectionConfigType.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/TemplateBuilderUtil.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointSecurityModel.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/SecurityConfigContext.java(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/org.wso2.carbon.apimgt.core.default.json(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointSecurityModel.java (1)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/EndpointSecurity.java (2)
JsonIgnoreProperties(25-371)JsonIgnoreProperties(279-344)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/TemplateBuilderUtil.java (2)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java (1)
APIConstants(24-124)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java (1)
GatewayUtils(36-241)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/SecurityConfigContext.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java (1)
GatewayUtils(36-241)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3346)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3346)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/EndpointSecurity.java (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointSecurityModel.java (1)
JsonIgnoreProperties(28-84)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (2, group2)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-carbon
- GitHub Check: build-product (3, group3)
- GitHub Check: build-product (4, group4)
- GitHub Check: run-benchmark-test
🔇 Additional comments (26)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/TokenEndpointConnectionConfigType.java (1)
1-25: Well-structured enum for token endpoint connection configuration types.The enum provides a clear set of options for configuring how token endpoints should handle connection settings: globally, endpoint-specific, or none. The naming follows Java conventions and the implementation is clean.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java (1)
172-177: Good addition of method to retrieve OAuth proxy password alias.This method follows the same pattern as the existing methods
retrieveOauthClientSecretAliasandretrieveOAuthPasswordAlias, maintaining consistency in the codebase.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointSecurityModel.java (2)
20-20: Added import for JsonIgnoreProperties annotation.Appropriate import to support the annotation added to the class.
28-28: Added JsonIgnoreProperties annotation for better JSON deserialization.This annotation allows Jackson to ignore unknown properties during deserialization, providing forward compatibility as new fields are added to the model. This is particularly important since this PR introduces new fields in the parent EndpointSecurity class.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/TemplateBuilderUtil.java (1)
1520-1536: Added support for OAuth proxy password credentials.This change correctly handles proxy credentials by extracting proxy password from endpoint security configuration and adding it to the credentials list. It follows the same pattern as existing credential handling code and utilizes the newly added
retrieveOAuthProxyPasswordAliasmethod.The implementation properly:
- Checks for the existence of proxy configuration
- Extracts the proxy password
- Creates a credential DTO with appropriate alias
- Adds it to the existing credentials list
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2 (1)
1790-1792: Well-structured timeout configuration additions for OAuth.These new timeout configuration parameters will enhance the fine-grained control over OAuth connection behavior, improving resilience against slow connections or unresponsive services.
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/org.wso2.carbon.apimgt.core.default.json (1)
91-93: Good default timeout values added for OAuth connections.The default value of 15000ms (15 seconds) for all three timeout parameters is reasonable and consistent with industry standards for HTTP connection timeouts.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/SecurityConfigContext.java (2)
171-179: Improved code organization for prefixed OAuth security configuration.The refactored code effectively consolidates the logic for setting OAuth security parameters when a prefix is provided, including the new proxy password alias configuration.
184-186: Added proxy password alias handling for non-prefixed scenarios.The code now properly sets the proxy password alias even when no prefix is provided, ensuring consistent proxy authentication configuration across different endpoint scenarios.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (5)
108-108: New import for token endpoint connection configuration types.This adds support for the new enum that defines different types of token endpoint connection configurations (global or endpoint-specific).
5590-5613: Well-structured method to populate endpoint security defaults.This new method properly parses the endpoint configuration JSON and populates default values for token endpoint connection settings for both production and sandbox endpoints. It follows good practices with null checking before processing the JSON.
5623-5650: Important logic for determining token endpoint connection configuration types.The method intelligently determines whether to use global or endpoint-specific configuration based on the values present:
- For connection timeout: Uses global config if using default timeouts, otherwise uses endpoint-specific config
- For proxy: Uses endpoint-specific config if proxy is enabled, otherwise uses global config
A well-designed approach that maintains backward compatibility while adding new functionality.
5662-5667: Efficient helper method for checking default connection timeout values.This method handles various ways a default timeout could be represented (null, string "-1", integer -1, or long -1), making the code more robust against different data formats that might exist in the configuration.
5569-5569: Proper integration of the new functionality in API retrieval.The
populateEndpointSecurityDefaultsmethod is correctly called when retrieving API details, ensuring that all API objects have proper default connection settings before being used by the application.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (5)
1853-1853: New constant added for endpoint security proxy password.This constant
ENDPOINT_SECURITY_PROXY_PASSWORDwith value "proxyPassword" will be used to store and retrieve the proxy password used in endpoint security configurations.
1854-1855: Added configuration type constants for connection timeout and proxy settings.These new constants will be used to specify the type of configuration being used for connection timeout and proxy settings, enabling different configuration approaches.
1856-1858: Added constants for various timeout duration parameters.These three constants define different aspects of connection timeout:
CONNECTION_TIMEOUT_DURATION: For the main connection establishment timeoutCONNECTION_REQUEST_TIMEOUT_DURATION: For timeout when requesting a connection from the connection managerSOCKET_TIMEOUT_DURATION: For timeout when waiting for data after the connection is establishedThis granular approach to timeout settings allows for more precise control over connection behavior.
1859-1860: Added constants for proxy configuration settings.These constants will help manage proxy-related settings:
PROXY_CONFIGS: Likely holds the full proxy configurationPROXY_ENABLED: Boolean flag to enable/disable proxy functionalityThese additions support the global proxy configuration capabilities mentioned in the PR objectives.
1861-1861: Added default timeout value constant.The
CONNECTION_TIMEOUT_DEFAULTconstant with value "-1" likely represents a disabled or infinite timeout setting, which is a common default in connection timeout scenarios.components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/EndpointSecurity.java (7)
20-25: Proper handling of unknown properties added with JsonIgnoreProperties annotationThe addition of
@JsonIgnoreProperties(ignoreUnknown = true)at the class level ensures backward compatibility by allowing the JSON deserialization process to ignore unknown properties. This is a good practice when extending APIs to prevent deserialization failures when processing data from older versions.
85-86: Copy constructor properly updated to include new fieldsThe copy constructor has been properly updated to copy the new configuration type fields. This ensures that when an EndpointSecurity object is cloned, all of its properties, including the newly added ones, are correctly transferred.
263-277: Getter and setter methods implemented for new configuration type fieldsThe implementation of getter and setter methods for the new
connectionTimeoutConfigTypeandproxyConfigTypefields follows the Java bean conventions and is consistent with the existing code style.
279-279: JsonIgnoreProperties annotation added to inner classAdding the
@JsonIgnoreProperties(ignoreUnknown = true)annotation to theProxyConfigsinner class ensures consistent handling of JSON deserialization across the model, which is good practice.
287-287: New proxyPasswordAlias field added to ProxyConfigs classThe addition of the
proxyPasswordAliasfield in theProxyConfigsclass allows for secure credential management by supporting the use of password aliases instead of plain text passwords for proxy authentication.
337-343: Getter and setter methods implemented for proxyPasswordAliasThe implementation of getter and setter methods for the new
proxyPasswordAliasfield follows Java bean conventions and is consistent with the existing code style.
64-66:✅ Verification successful
New configuration type fields added for connection timeout and proxy settings
The addition of
connectionTimeoutConfigTypeandproxyConfigTypefields of typeTokenEndpointConnectionConfigTypeseems appropriate for implementing the global connection timeout and proxy configuration support as mentioned in the PR objectives.Can you verify that the
TokenEndpointConnectionConfigTypeenum is correctly implemented? It's referenced here but not shown in the provided code.
🏁 Script executed:
#!/bin/bash # Check if TokenEndpointConnectionConfigType enum exists and examine its values fd TokenEndpointConnectionConfigType.java -exec cat {} \;Length of output: 250
Action Required: Verify TokenEndpointConnectionConfigType Implementation
The new configuration-type fields for connection timeout and proxy settings are appropriate per the PR objectives. However, we couldn’t automatically verify the contents of the
TokenEndpointConnectionConfigTypeenum due to an earlier command syntax error. Please run the above corrected script (or manually verify) to ensure that the enum is implemented correctly and includes the necessary values to support both connection timeout and proxy configuration.
🏁 Script executed:
#!/bin/bash # Correctly search for the TokenEndpointConnectionConfigType enum and display its content. # This command uses a proper regex pattern and --exec flag to print the file if found. fd "TokenEndpointConnectionConfigType\.java" --exec cat {} \;Length of output: 982
Review Comment Update: Confirmed Enum Implementation
The newly introduced fields
connectionTimeoutConfigTypeandproxyConfigTypeinEndpointSecurity.javacorrectly reference theTokenEndpointConnectionConfigTypeenum. The enum implementation is verified and contains the necessary values (GLOBAL,ENDPOINT_SPECIFIC, andNONE) to support global connection timeout and proxy configuration functionality as outlined in the PR objectives.
Fix wso2/api-manager#3590
This pull request adds support for configuring connection timeouts and proxy settings at both global and endpoint levels. Additionally, connection timeouts and proxy configurations can be disabled at the endpoint level. Changes have also been made to the API retrieval section to ensure that the existing behavior remains intact.
Furthermore, this update fixes an issue where the proxy password was not resolving correctly when the secure vault was enabled.