feat: add observe status access-key for pre-check and logging only (#5216) #5236
Conversation
…polloconfig#5216) - ALTER TABLE `AccessKey` ADD COLUMN `Mode`, 0: filter，1: observer - portal: CRUD for observe status access-key - configservice: pre-check and logging via ClientAuthenticationFilter
dosubot
bot
added
the
size:L
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
apollo-portal/src/main/resources/static/scripts/controller/AccessKeyController.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/AccessKeyController.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/AccessKeyController.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/AccessKeyController.js
Show resolved
Hide resolved
WalkthroughThe changes enhance the access key management functionality in the Apollo framework by introducing mode handling during enable and disable operations. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AccessKeyService.java (1)
52-53: Add unit tests for the updated method signature.The method signature has been updated to include an additional
modeparameter. Ensure that the unit tests for this method have been updated to cover the new parameter.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloconfigdb-v230-v240.sql (1)
29-30: LGTM!The SQL statement for adding the
Modecolumn to theAccessKeytable is well-structured and follows best practices:
- The column is defined with an appropriate data type (
tinyint(2) unsigned) and default value (0).- The comment provides clarity on the purpose and possible values of the column.
- The
AFTERclause ensures proper positioning of the new column within the table structure.Consider adding a check constraint to enforce the allowed values for the
Modecolumn:ALTER TABLE `AccessKey` ADD COLUMN `Mode` tinyint(2) unsigned NOT NULL DEFAULT '0' COMMENT '密钥模式，0: filter，1: observer' AFTER `Secret`, ADD CONSTRAINT `CK_AccessKey_Mode` CHECK (`Mode` IN (0, 1));This will ensure that only valid values (
0for "filter" and1for "observer") can be inserted into theModecolumn.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java (1)
82-82: Avoid logging sensitive information directlyIncluding
timestampin logs is generally acceptable, but ensure that logging practices comply with security policies regarding sensitive information.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (26)
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AccessKeyController.java (4 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/entity/AccessKey.java (3 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/AccessKeyService.java (1 hunks)
- apollo-biz/src/test/resources/sql/accesskey-test.sql (1 hunks)
- apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/AccessKeyMode.java (1 hunks)
- apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/AccessKeyDTO.java (2 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java (3 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/AccessKeyServiceWithCache.java (3 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/util/AccessKeyUtil.java (2 hunks)
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (2 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/api/AdminServiceAPI.java (1 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java (1 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AccessKeyService.java (1 hunks)
- apollo-portal/src/main/resources/static/app/access_key.html (2 hunks)
- apollo-portal/src/main/resources/static/i18n/en.json (2 hunks)
- apollo-portal/src/main/resources/static/i18n/zh-CN.json (2 hunks)
- apollo-portal/src/main/resources/static/scripts/controller/AccessKeyController.js (1 hunks)
- apollo-portal/src/main/resources/static/scripts/services/AccessKeyService.js (2 hunks)
- scripts/sql/profiles/h2-default/apolloconfigdb.sql (1 hunks)
- scripts/sql/profiles/h2-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
- scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1 hunks)
- scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
- scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1 hunks)
- scripts/sql/profiles/mysql-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
- scripts/sql/src/apolloconfigdb.sql (1 hunks)
- scripts/sql/src/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
Additional comments not posted (45)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/AccessKeyMode.java (1)
19-25: LGTM!The
AccessKeyModeinterface is well-defined and serves its purpose as a container for the access key mode constants. The constant names are clear and self-explanatory, and their values are unique and follow a sequential pattern. The interface is placed in an appropriate package for common constants.scripts/sql/src/delta/v230-v240/apolloconfigdb-v230-v240.sql (1)
22-23: LGTM!The SQL script for adding the
Modecolumn to theAccessKeytable looks good. The data type, constraints, and default value are appropriate for the intended purpose, and the comment provides clear documentation on the meaning of the different mode values.A few additional suggestions for consideration:
Consider adding a check constraint to ensure that the
Modecolumn only accepts valid values (0 or 1). This can help prevent invalid data from being inserted into the table.If there are any existing queries or application code that relies on the current structure of the
AccessKeytable, make sure to update them to handle the newModecolumn appropriately.Verify that the upgrade process handles the addition of the new column gracefully, especially if there is a large amount of existing data in the
AccessKeytable.Overall, the changes in this SQL script are well-structured and align with the objectives of introducing the new access key management feature.
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/AccessKeyDTO.java (2)
27-27: LGTM!The new
modefield aligns with the PR objective and follows best practices.
55-57: LGTM!The new getter and setter methods for the
modefield follow the JavaBean convention and maintain encapsulation.Also applies to: 59-61
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AccessKeyService.java (1)
52-53: Verify the method signature change in the codebase.The method signature has been updated to include an additional
modeparameter. Ensure that all method calls toenablehave been updated to pass themodeparameter.Run the following script to verify the method usage:
Also, consider adding validation for the
modeparameter to handle invalid values gracefully and prevent unexpected behavior.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/entity/AccessKey.java (3)
39-40: LGTM!The new
modefield is correctly defined with appropriate access modifier, annotation, and data type.
61-67: LGTM!The getter and setter methods for the
modefield are implemented correctly, following the JavaBean convention and providing proper encapsulation.
80-80: LGTM!The
toString()method is correctly updated to include themodefield, enhancing the debugging and logging capabilities of theAccessKeyclass.scripts/sql/profiles/mysql-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (1)
31-32: LGTM! Verify the application code and dependent queries/views.The SQL statement for adding the
Modecolumn to theAccessKeytable looks good. The column type, constraints, and comment are appropriate.Please ensure that:
- The application code has been updated to handle the new
Modecolumn appropriately, both for reading and writing data.- Any queries or views that reference the
AccessKeytable have been updated to include the new column if necessary.You can use the following script to search for potential references to the
AccessKeytable in the codebase:scripts/sql/profiles/h2-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (2)
30-30: LGTM!The
UNIX_TIMESTAMPfunction alias is created correctly using theCREATE ALIASstatement and maps to a fully qualified Java method for timestamp conversion.
34-34: LGTM!The
Modecolumn is added correctly to theAccessKeytable using theALTER TABLEstatement with theADD COLUMNclause. The column definition is valid, specifying the data type, size, nullability, and default value. The column is added at an appropriate position and has a clear comment explaining its purpose and possible values.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/util/AccessKeyUtil.java (2)
49-51: LGTM!The new
findObservableSecretsmethod is well-implemented and serves a clear purpose. It retrieves observable secrets for a given application ID by delegating to theaccessKeyServiceWithCacheinstance. The method signature and logic are straightforward and easy to understand.
79-81: Clarify the purpose and intended usage of thepreCheckInvalidmethod.The
preCheckInvalidmethod is currently empty and only contains a comment indicating it's for test mocking. It's unclear how this method fits into the overall functionality of theAccessKeyUtilclass and the access key pre-check feature.Please provide more context or documentation to explain the following:
- What is the intended purpose of this method?
- How is it meant to be used in the context of access key pre-checking?
- Are there any plans to implement the method's logic in the future, or will it remain a placeholder for testing?
Adding this information will help developers better understand the role of this method and how it relates to the broader access key management functionality.
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AccessKeyController.java (4)
21-21: LGTM!The import statement for the
AccessKeyModeclass is correctly added.
31-31: LGTM!The import statement for the
RequestParamannotation is correctly added.
66-70: LGTM!The changes to the
enablemethod signature and the usage of themodeparameter are implemented correctly:
- The
modeparameter is marked as optional with a default value of0, ensuring backward compatibility.- The
modevalue is correctly set in theAccessKeyentity before updating it.
81-81: LGTM!Setting the mode to
AccessKeyMode.FILTERwhen disabling an access key is a valid approach to ensure the access key operates in the filter mode.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/AccessKeyService.java (1)
77-77: LGTM!The change to set the mode of the
accessKeyobject based on the incomingentityparameter is a valid enhancement to theupdatemethod. It allows for a more comprehensive update of theAccessKeyentity by including the mode attribute along with the enabled status and last modified information.The change is consistent with the method's purpose and does not introduce any apparent issues or side effects. It aligns well with the overall access key management feature being introduced in this PR.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java (1)
80-83: LGTM! Verify theaccessKeyService.enablemethod implementation.The addition of the optional
modeparameter to theenablemethod provides more flexibility in enabling access keys by allowing the mode to be specified. The default value of0ensures backward compatibility for existing callers.Please verify that the
accessKeyService.enablemethod has been updated to handle the newmodeparameter correctly.Run the following script to verify the
accessKeyService.enablemethod signature:apollo-portal/src/main/resources/static/scripts/services/AccessKeyService.js (1)
34-34: LGTM!The addition of the
modequery parameter to the URL aligns with the PR objective of introducing an observe status access-key feature. This change allows for more granular control over the enabling process.apollo-portal/src/main/resources/static/scripts/controller/AccessKeyController.js (1)
139-153: LGTM!The changes to the
enablefunction look good:
- The new
modeparameter is handled correctly, defaulting to 0 if not explicitly set to 1.- The
tipsPrefixis constructed based on themodevalue, allowing for different user feedback messages.- The confirmation message and success/error notifications are updated to use the
tipsPrefix, providing contextually relevant messages.- The call to
AccessKeyService.enable_access_keyis updated to pass themodeparameter along with the existing parameters.The changes enhance the functionality of the
enablemethod by allowing it to handle different operational modes and providing more specific user feedback based on the selected mode. The changes are also backward compatible as themodeparameter defaults to 0, maintaining the existing behavior if not explicitly set.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/AccessKeyServiceWithCache.java (4)
22-22: LGTM!The import statement is necessary for the new methods being added.
41-41: LGTM!The import statement is necessary for the new
getSecretsmethod being added.
90-95: LGTM!The new public methods
getAvailableSecretsandgetObservableSecretsprovide a way to retrieve access keys based on their mode and enabled status. The methods use the new privategetSecretsmethod with a predicate filter, which is a good way to reuse code and avoid duplication.
97-104: LGTM!The new private method
getSecretsprovides a way to retrieve access keys based on a predicate filter. The method uses theaccessKeyCacheto retrieve access keys, which is a good way to avoid hitting the database for every request. The method uses thestreamAPI to filter and map the access keys, which is a good way to write concise and readable code.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (2)
150-174: LGTM!The
testPreCheckInvalidtest method is well-structured and comprehensive. It effectively verifies the behavior of theclientAuthenticationFilterwhen an invalid signature is provided.The mocking setup covers the necessary scenarios, and the assertions ensure that no error responses are sent, the filter chain is executed, and the
preCheckInvalidmethod is called the expected number of times.
176-199: LGTM!The
testPreCheckSuccessfullytest method is well-structured and effectively verifies the behavior of theclientAuthenticationFilterwhen a valid signature is provided.The mocking setup is appropriate for the valid signature case, and the assertions ensure that no error responses are sent, the filter chain is executed, and the
preCheckInvalidmethod is not called.scripts/sql/src/apolloconfigdb.sql (1)
400-400: LGTM!The addition of the
Modecolumn to theAccessKeytable is a valid change that introduces the capability to specify access key modes. The chosen data type, constraints, and default value align with the intended functionality.scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
396-396: LGTM!The addition of the
Modecolumn to theAccessKeytable looks good:
- The column is defined with the appropriate data type
tinyint(2) unsignedand default value0.- The comment clearly explains the purpose and allowed values for the column.
- This is a non-breaking change as existing code will continue to work due to the default value.
Note: Application code changes will be needed to set and interpret the
Modevalues in order to utilize this new functionality.scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1)
407-407: Approve the addition of theModecolumn to theAccessKeytable.The new
Modecolumn introduces a way to categorize access keys into "filter" and "observer" modes. The chosen data type and default value are appropriate.Consider the following impacts:
- Update the application code to handle the new
Modecolumn correctly.- Verify that assigning the "filter" mode by default to existing access keys aligns with the intended behavior.
- Ensure that the access control logic is updated to differentiate between the "filter" and "observer" modes as needed.
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
412-412: LGTM!The addition of the
Modecolumn to theAccessKeytable is a good enhancement that allows differentiating between "filter" and "observer" modes for access keys. The chosen data type and default value are appropriate.Consider the following:
- Update any existing code that interacts with the
AccessKeytable to handle the newModecolumn appropriately.- Ensure that the application logic correctly utilizes the
Modevalue when processing access keys.- Update the relevant documentation to reflect the new access key modes and their behavior.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/api/AdminServiceAPI.java (2)
320-322: LGTM!The changes to the
enablemethod signature and the corresponding update to the URL template look good.
320-322: Verify the method invocation changes in the codebase.Please ensure that all invocations of the
enablemethod have been updated to provide themodeargument.Run the following script to verify the method usage:
apollo-portal/src/main/resources/static/i18n/zh-CN.json (4)
519-524: Looks good!The added localization strings provide clear guidance about access key usage and mention the new "Observe" status. The key names also follow existing naming conventions.
533-536: Looks good!The added strings for the new "Observe" access key operation look appropriate and consistent with existing operation label conventions.
541-546: Looks good!The added success and error message strings for the new observe access key operation are consistent with existing message conventions and provide suitable user-facing text.
550-550: Looks good!The confirmation message for the observe access key operation is consistent with other confirmation prompts and provides a clear text to confirm the action.
apollo-portal/src/main/resources/static/i18n/en.json (4)
520-524: LGTM!The added localization strings for access key tips are clear and informative. The JSON syntax is valid.
533-536: Looks good!The new localization strings for the "Observe" access key operation are consistent with the existing entries. The JSON syntax is valid.
541-546: Approved.The success and error message strings for the "Observe" access key operation are clear and consistent with the existing entries. The JSON syntax is valid.
550-551: Good to go!The confirmation prompt string for the "Observe" access key operation is clear and consistent with the existing entries. The JSON syntax is valid.
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java (3)
20-20: Imports added for client IP extraction and tracingThe addition of
WebUtilsandTracerimports is appropriate for retrieving client IP addresses and logging events, supporting the new logging functionality.Also applies to: 24-24
78-78: Retrieve and utilize client IP for enhanced loggingFetching the client IP address using
WebUtils.tryToGetClientIp(request)enhances logging details and aids in monitoring and troubleshooting client requests.
159-164: Review the use ofaccessKeyUtil.preCheckInvalid()in production codeThe
preCheckInvalidLoggingmethod includes a call toaccessKeyUtil.preCheckInvalid();, with a comment indicating it's "only for test mock". If this method is intended solely for testing purposes, it should not be invoked in production code to avoid unintended side effects.Confirm whether
accessKeyUtil.preCheckInvalid()should be called in the production environment. If not, consider removing it:private void preCheckInvalidLogging(String message) { logger.warn(message); Tracer.logEvent("Apollo.AccessKey.PreCheck", message); - // only for test mock - accessKeyUtil.preCheckInvalid(); }apollo-portal/src/main/resources/static/app/access_key.html (1)
63-67: Ensure Translations for New Tips Are IncludedThe new tips
AccessKey.Tips.4and its sub-items (AccessKey.Tips.4.1,AccessKey.Tips.4.2,AccessKey.Tips.4.3) have been added. Please verify that corresponding translations exist in all localization files to ensure they are displayed correctly to users.Verification successful
Translations Verified Successfully
All new translation keys (
AccessKey.Tips.4,AccessKey.Tips.4.1,AccessKey.Tips.4.2,AccessKey.Tips.4.3) are present in the localization files (en.json,zh-CN.json).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if new translation keys are present in localization files. # List of new translation keys keys=( 'AccessKey.Tips.4' 'AccessKey.Tips.4.1' 'AccessKey.Tips.4.2' 'AccessKey.Tips.4.3' ) # Loop through each key and check if it's present in the localization files for key in "${keys[@]}"; do echo "Checking for $key..." if ! rg --files-with-matches "$key" ./src/main/resources/i18n/; then echo "Missing translation for $key" fi doneLength of output: 1436
Script:
#!/bin/bash # Description: Locate all i18n directories in the repository. fd --type d i18nLength of output: 64
Script:
#!/bin/bash # Description: Check if new translation keys are present in localization files. # List of new translation keys keys=( 'AccessKey.Tips.4' 'AccessKey.Tips.4.1' 'AccessKey.Tips.4.2' 'AccessKey.Tips.4.3' ) # Loop through each key and check if it's present in the localization files for key in "${keys[@]}"; do echo "Checking for $key..." if ! rg --files-with-matches "$key" ./apollo-portal/src/main/resources/static/i18n/; then echo "Missing translation for $key" fi doneLength of output: 1328
apollo-portal/src/main/resources/static/scripts/services/AccessKeyService.js
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...ce/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AccessKeyController.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (4)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/util/AccessKeyUtil.java (1)
49-51: LGTM! Consider adding a brief Javadoc comment.The new
findObservableSecretsmethod is well-implemented and aligns with the PR objectives. It follows a consistent pattern with the existingfindAvailableSecretmethod.Consider adding a brief Javadoc comment to explain the purpose of this method and its relationship to the pre-check functionality. For example:
/** * Retrieves a list of observable secrets for the given appId. * This method is used for pre-checking and logging purposes without blocking requests. * * @param appId the application ID * @return a list of observable secrets */ public List<String> findObservableSecrets(String appId) { return accessKeyServiceWithCache.getObservableSecrets(appId); }apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AccessKeyController.java (2)
67-68: LGTM: Method signature updated correctly.The addition of the
modeparameter with@RequestParamallows for optional specification of the mode when enabling an access key, which aligns with the PR objectives. The default value ensures backward compatibility.Consider applying the suggestion from the previous review for improved readability:
public void enable(@PathVariable String appId, @PathVariable long id, @RequestParam(required = false, defaultValue = ""+AccessKeyMode.FILTER) int mode, String operator) {This change makes it clearer that
FILTERis part of theAccessKeyModeenum.
82-82: LGTM: Mode is explicitly set when disabling.The addition of
entity.setMode(FILTER);ensures that the AccessKey entity's mode is consistently set toFILTERwhen disabling it. This aligns with the current requirements and PR objectives.Consider if there might be future scenarios where different modes for disabled keys could be beneficial. If so, you might want to make this configurable or parameterized similar to the
enablemethod for increased flexibility.apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java (1)
93-93: Typographical correction in Javadoc commentIn the method documentation for
doCheck, the verb should agree with the plural noun "authentication checks".Apply this diff to correct the grammar:
- * @return true if authentication checks is successful, false otherwise + * @return true if authentication checks are successful, false otherwise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AccessKeyController.java (4 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java (3 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/util/AccessKeyUtil.java (1 hunks)
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (4 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/util/AccessKeyUtil.java (1)
49-51: Verify usage of the new method in the codebase.The new
findObservableSecretsmethod looks good and doesn't introduce any breaking changes. To ensure it's being used as intended for the pre-check functionality, we should verify its usage in other parts of the codebase.Run the following script to check for usage of the new method:
✅ Verification successful
findObservableSecretsusage is properly integrated and no issues found.
- Used in
ClientAuthenticationFilter.java- Used in
ClientAuthenticationFilterTest.java🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of findObservableSecrets method # Search for method invocations echo "Searching for findObservableSecrets usage:" rg --type java "findObservableSecrets\(" -g "!AccessKeyUtil.java" # Search for potential places where it should be used (e.g., alongside findAvailableSecret) echo "\nSearching for potential usage locations:" rg --type java "findAvailableSecret\(" -g "!AccessKeyUtil.java"Length of output: 2459
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AccessKeyController.java (2)
19-20: LGTM: Import statements are correctly updated.The new imports for
FILTERandRequestParamare necessary and correctly added to support the changes in the class methods.Also applies to: 32-32
71-71: LGTM: Mode is correctly set in the enable method.The addition of
entity.setMode(mode);ensures that the AccessKey entity is updated with the specified (or default) mode before enabling it. This change correctly implements the new functionality introduced by the method signature update.apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (3)
153-201: Ensure timestamp values align with test scenariosIn
testPreCheckInvalid(), theoneMinAgoTimestampis set to a value 61 seconds in the past:String oneMinAgoTimestamp = Long.toString(System.currentTimeMillis() - 61 * 1000);Given that the time difference tolerance is set to 60 seconds, this timestamp exceeds the allowed skew. However, the test verifies that no error is sent for "RequestTimeTooSkewed". Confirm that this is the intended behavior in pre-check mode, where requests should not be blocked due to time skew but should log the invalid attempt.
64-64: Good use of Mockitospyfor method verificationReplacing the instantiation with a spy in the
setUp()method allows for verification of interactions withclientAuthenticationFilter. This enhances the test's ability to assert internal method calls.
178-201: TesttestPreCheckSuccessfully()accurately validates successful pre-checkThe test ensures that when there are no available secrets but observable secrets exist, and the authorization is correct, the request proceeds without triggering
preCheckInvalidLogging(). This effectively verifies the intended behavior of the pre-check mechanism when clients provide valid credentials.
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java
Show resolved
Hide resolved
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java
Show resolved
Hide resolved
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilter.java
Show resolved
Hide resolved
...est/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java
Outdated
Show resolved
Hide resolved
...est/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java
Outdated
Show resolved
Hide resolved
👍🏻 All the comments have been resolved, Please take a moment to review again. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (2)
150-170: LGTM: New test method for pre-check invalid scenario.The
testPreCheckInvalidmethod is well-structured and tests the pre-check functionality when the authentication is invalid. The test setup correctly mocks the necessary dependencies and method calls.Regarding the previous review comment about clarifying the expected number of pre-check logging invocations:
The test verifies thatpreCheckInvalidLoggingis called twice. This could be due to multiple checks in the filter's logic. However, it would be beneficial to add a comment explaining why two invocations are expected, to improve the test's clarity and maintainability.Consider adding a comment explaining why
preCheckInvalidLoggingis expected to be called twice in this scenario.
Line range hint
194-199: LGTM: New helper method improves test code structure.The
verifySuccessAndDoFiltermethod is a good addition that consolidates common verification logic used in multiple test methods. It improves code readability and reduces duplication.Regarding the previous review comment about consolidating duplicate test setup code:
While this helper method addresses part of the duplication concern by consolidating verification logic, there's still an opportunity to further reduce duplication in the test setup code. Consider extracting common setup logic (e.g., mocking dependencies) into a separate helper method or using parameterized tests for scenarios with similar setups but different inputs/expectations.Consider further reducing duplication by extracting common setup logic into a separate helper method or using parameterized tests for similar scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/filter/ClientAuthenticationFilterTest.java (4)
20-22: LGTM: New imports added correctly.The new imports for
anyStringandspyfrom Mockito are necessary for the new test methods and the modifiedsetUpmethod. They are correctly placed and follow the existing import style.
64-64: LGTM: Spy used for better test control.The change to use
spyfor instantiatingclientAuthenticationFilteris a good improvement. It allows for partial mocking, which is useful for verifying method calls on the filter while still executing the real methods. This change is consistent with the new test methods that verify specific method calls.
172-192: LGTM: New test method for pre-check success scenario.The
testPreCheckSuccessfullymethod is well-structured and effectively tests the pre-check functionality for a successful authentication scenario. The test setup correctly mocks the necessary dependencies and method calls. The verification thatpreCheckInvalidLoggingis never called is appropriate for a successful pre-check scenario.
Line range hint
1-199: Overall, the changes improve test coverage and align with PR objectives.The additions to
ClientAuthenticationFilterTest.javaeffectively support the PR's objective of introducing an observe status access-key feature for pre-checking and logging. The new test methods (testPreCheckInvalidandtestPreCheckSuccessfully) provide coverage for both successful and invalid pre-check scenarios, which is crucial for validating the new feature's behavior.The introduction of the
verifySuccessAndDoFilterhelper method is a step towards reducing code duplication and improving test readability. However, there's still an opportunity to further refactor common setup code across test methods.To further enhance the test suite:
- Consider using parameterized tests for scenarios with similar setups but different inputs/expectations.
- Add comments to clarify the expected behavior, especially for cases like the double invocation of
preCheckInvalidLoggingintestPreCheckInvalid.- Ensure that edge cases and potential error scenarios are also covered in the test suite.
These changes provide a solid foundation for testing the new pre-check functionality while maintaining the overall quality and structure of the test class.
Co-authored-by: Jason Song <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
scripts/sql/src/apolloconfigdb.sql (1)
400-400: Approve the addition of theModecolumn with minor suggestions.The addition of the
Modecolumn to theAccessKeytable is well-implemented and aligns with the PR objectives. However, consider the following suggestions:
- Remove the display width from the
tinyintdata type, as it's deprecated in MySQL 8.0:- `Mode` tinyint unsigned NOT NULL DEFAULT '0' COMMENT '密钥模式，0: filter，1: observer', + `Mode` tinyint unsigned NOT NULL DEFAULT 0 COMMENT '密钥模式，0: filter，1: observer',
- Consider adding an index on the
Modecolumn if you anticipate frequent queries filtering on this column:KEY `IX_Mode` (`Mode`)
Done. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
CHANGES.md (1)
14-14: LGTM! Consider a minor wording adjustment.The addition of the new feature to the changelog is appropriate and consistent with the PR objectives. The format matches other entries, and the link to the pull request is correctly included.
For slightly improved clarity, consider adjusting the wording as follows:
-* [Feature support the observe status access-key for pre-check and logging only](https://github.com/apolloconfig/apollo/pull/5236) +* [Feature: Support observe status for access-key pre-check and logging without blocking](https://github.com/apolloconfig/apollo/pull/5236)This adjustment emphasizes that the feature is for observation and doesn't block requests, which aligns more closely with the PR objectives.
scripts/sql/profiles/mysql-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (1)
31-32: LGTM! Consider adding a CHECK constraint for data integrity.The ALTER TABLE statement correctly adds the new 'Mode' column to the 'AccessKey' table, aligning with the PR objectives. The column type, default value, and comment are all appropriate.
To further enhance data integrity, consider adding a CHECK constraint to ensure only valid values (0 or 1) can be inserted:
ALTER TABLE `AccessKey` ADD COLUMN `Mode` tinyint unsigned NOT NULL DEFAULT '0' COMMENT '密钥模式，0: filter，1: observer' AFTER `Secret`, ADD CONSTRAINT `chk_mode` CHECK (`Mode` IN (0, 1));This ensures that only the specified modes (filter or observer) can be set for the access keys.
scripts/sql/profiles/h2-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (2)
28-30: LGTM: UNIX_TIMESTAMP alias creation looks good.The alias creation for UNIX_TIMESTAMP is correct and follows good practices. Using
IF NOT EXISTSprevents errors if the alias already exists, which is a good defensive programming technique.Consider adding a brief comment explaining the purpose of this alias, e.g.:
-- Create UNIX_TIMESTAMP alias for H2 database to provide Unix timestamp functionality CREATE ALIAS IF NOT EXISTS UNIX_TIMESTAMP FOR "com.ctrip.framework.apollo.common.jpa.H2Function.unixTimestamp";
34-34: LGTM: AccessKey table alteration is correct and well-documented.The addition of the 'Mode' column to the AccessKey table is implemented correctly. The use of a tinyint with a default value ensures backward compatibility and space efficiency.
Consider the following suggestions:
- For consistency, consider using English for the comment:
COMMENT 'Key mode, 0: filter, 1: observer'- To improve readability, you might want to split the ALTER TABLE statement across multiple lines:
ALTER TABLE `AccessKey` ADD COLUMN `Mode` tinyint unsigned NOT NULL DEFAULT '0' COMMENT 'Key mode, 0: filter, 1: observer' AFTER `Secret`;scripts/sql/profiles/h2-default/apolloconfigdb.sql (1)
396-396: Summary of changes and recommendationsThe SQL script modification is minimal and focused:
- A new
Modecolumn has been added to theAccessKeytable.- The change is isolated and doesn't directly affect other tables.
Recommendations:
- Update the application's documentation to reflect the new
Modecolumn and its purpose.- Ensure that database migration scripts (if used) include this change for existing deployments.
- Consider adding a database index on the
Modecolumn if it will be frequently used in queries.To help with documentation and potential performance optimization, consider running the following script:
#!/bin/bash # Check for existing documentation and suggest index creation echo "Searching for AccessKey documentation:" rg --type md "AccessKey" echo "Analyzing potential need for an index on the Mode column:" cat << EOF Consider adding an index on the Mode column if you anticipate frequent queries filtering on this column. You can add an index with the following SQL command: CREATE INDEX IX_AccessKey_Mode ON AccessKey (Mode); Analyze your query patterns to determine if this index would be beneficial. EOFPlease review the script output and consider implementing the suggestions if appropriate for your use case.
scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (2)
Line range hint
405-419: LGTM! Consider adding an index for theModecolumn.The addition of the
Modecolumn to theAccessKeytable is well-implemented. The column is properly defined with an appropriate data type (tinyint unsigned), a default value (0), and a clear comment explaining its purpose and possible values.Consider adding an index on the
Modecolumn if you anticipate frequent queries filtering or sorting by this column. This could improve query performance in such scenarios. Here's a suggested ALTER TABLE statement:ALTER TABLE `AccessKey` ADD INDEX `IX_Mode` (`Mode`);Add this statement after the table creation if you decide to include the index.
Line range hint
524-529: Consider adding a ServerConfig entry for the new Mode feature.While the addition of the
Modecolumn to theAccessKeytable is well-implemented, it might be beneficial to add a corresponding entry in theServerConfigtable. This would allow for easy configuration of the default behavior of the new Mode feature.Consider adding an INSERT statement for a new configuration entry in the
ServerConfigtable. Here's a suggested statement:INSERT INTO `ServerConfig` (`Key`, `Cluster`, `Value`, `Comment`) VALUES ('accesskey.mode.default', 'default', '0', 'Default mode for access keys. 0: filter, 1: observer');Add this statement after the existing INSERT statements for the
ServerConfigtable.scripts/sql/profiles/mysql-default/apolloconfigdb.sql (2)
412-412: LGTM! The newModecolumn enhances theAccessKeytable functionality.The addition of the
Modecolumn to theAccessKeytable is well-implemented and aligns with the PR objectives. It allows for different operational modes (filter or observer) for access keys, which is crucial for the new pre-check and logging feature.The column definition is appropriate:
tinyint unsignedis suitable for the binary mode options.NOT NULLensures data integrity.DEFAULT '0'maintains backwards compatibility by defaulting to the 'filter' mode.Consider adding an index on the
Modecolumn if you expect to frequently query or filter based on this column:ALTER TABLE `AccessKey` ADD INDEX `IX_Mode` (`Mode`);This can improve query performance when selecting or filtering access keys based on their mode.
Line range hint
1-524: Overall structure is good, but consider some enhancements for robustness and performance.The SQL script is well-structured and follows good practices. However, I have a few suggestions to enhance its robustness and performance:
- Consider wrapping the entire script in a transaction for atomic execution:
START TRANSACTION; -- (existing script content) COMMIT;
- Explicitly set the character set and collation when creating the database:
CREATE DATABASE IF NOT EXISTS ApolloConfigDB DEFAULT CHARACTER SET = utf8mb4 COLLATE = utf8mb4_unicode_ci;
- For large tables that may grow significantly over time (e.g.,
Audit,ReleaseHistory), consider implementing partitioning to improve query performance and manageability. For example:CREATE TABLE `Audit` ( -- (existing columns) ) PARTITION BY RANGE (UNIX_TIMESTAMP(DataChange_CreatedTime)) ( PARTITION p0 VALUES LESS THAN (UNIX_TIMESTAMP('2023-01-01 00:00:00')), PARTITION p1 VALUES LESS THAN (UNIX_TIMESTAMP('2024-01-01 00:00:00')), PARTITION p2 VALUES LESS THAN (UNIX_TIMESTAMP('2025-01-01 00:00:00')) );These enhancements can improve the script's reliability, consistency, and performance as the database grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- CHANGES.md (1 hunks)
- scripts/sql/profiles/h2-default/apolloconfigdb.sql (1 hunks)
- scripts/sql/profiles/h2-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
- scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1 hunks)
- scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
- scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1 hunks)
- scripts/sql/profiles/mysql-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
- scripts/sql/src/delta/v230-v240/apolloconfigdb-v230-v240.sql (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
scripts/sql/src/delta/v230-v240/apolloconfigdb-v230-v240.sql (3)
1-15: LGTM: Appropriate license header included.The file correctly includes the Apache 2.0 license header, which is essential for open-source projects and aligns with best practices for code licensing.
22-23: LGTM: Well-structured ALTER TABLE statement.The ALTER TABLE statement is correctly formed and adds the new 'Mode' column with appropriate attributes:
- Type: tinyint unsigned (suitable for small integer values 0-255)
- NOT NULL constraint with DEFAULT '0' (ensures data integrity)
- Clear COMMENT explaining the column's purpose
- Logical placement AFTER the 'Secret' column
This change aligns well with the PR objectives, supporting the new observe status access-key feature.
1-25: LGTM: Clear and purposeful database upgrade script.This SQL script effectively upgrades the Apollo configuration database from v2.3.0 to v2.4.0. It includes:
- Appropriate license header
- Clear comments indicating the purpose of the script
- Placeholders for auto-generated declarations and H2 functions
- A well-structured ALTER TABLE statement to add the new 'Mode' column
The script is concise, focused, and achieves its intended purpose of introducing the new 'Mode' column to support the observe status access-key feature.
scripts/sql/profiles/mysql-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (2)
1-41: Overall structure and content of the script look good.The script follows best practices for SQL migrations:
- It includes appropriate copyright and license information.
- It's clearly marked as generated and not to be edited manually.
- It specifies the correct database (ApolloConfigDB) to be used.
- The migration is focused and concise, addressing only the necessary changes.
1-41: Summary: SQL changes align well with PR objectives.This SQL script successfully implements the database changes required to support the new observe status access-key feature described in the PR objectives. The addition of the 'Mode' column to the 'AccessKey' table provides the necessary infrastructure to distinguish between 'filter' and 'observer' modes for access keys.
Key points:
- The change supports the pre-check capability without blocking requests.
- It allows for the implementation of a configuration switch (although this is likely handled in application code).
- The new column can be used to determine when to log client access attempts without required keys.
scripts/sql/profiles/h2-default/delta/v230-v240/apolloconfigdb-v230-v240.sql (2)
1-26: LGTM: Copyright and license information is correct and complete.The copyright notice is up to date, and the Apache License 2.0 is correctly referenced. The generated file warning is also present, which is good practice.
1-43: Summary: SQL changes align well with PR objectives.This SQL script successfully implements the database changes required for the new observe status access-key feature. The addition of the 'Mode' column to the AccessKey table directly supports the PR's objective of introducing a pre-check capability for access keys.
Key points:
- The UNIX_TIMESTAMP alias addition may be used for timestamp-related operations in the new feature.
- The new 'Mode' column in the AccessKey table allows for distinguishing between 'filter' and 'observer' modes, which is crucial for the pre-check functionality.
These changes provide the necessary database structure to support the new feature without breaking existing functionality, aligning well with the PR objectives.
scripts/sql/profiles/h2-default/apolloconfigdb.sql (2)
396-396: Ensure code compatibility with newModecolumnWhile the schema change is straightforward, it's crucial to update any code that interacts with the
AccessKeytable to handle the newModecolumn. This may include:
- Data access objects (DAOs) or repositories
- Service layer code that creates or updates access keys
- API endpoints that return access key information
To help identify areas that might need updates, run the following script:
#!/bin/bash # Search for files that might need updates due to the new Mode column echo "Searching for files that interact with AccessKey:" rg --type java --type kotlin "AccessKey" echo "Checking for any AccessKey-related tests that might need updating:" rg --type java --type kotlin "test.*AccessKey"Please review the output of this script and update the relevant files to accommodate the new
Modecolumn.
396-396: New columnModeadded toAccessKeytableThe
Modecolumn has been added to theAccessKeytable with the following properties:
- Data type:
tinyint unsigned- Default value:
0- Not nullable
- Comment: '密钥模式，0: filter，1: observer'
This addition allows for different modes of access keys, which aligns with the PR objective of introducing an observe status access-key for pre-check and logging.
To ensure this change is consistent with the rest of the codebase, please run the following script:
✅ Verification successful
No references to
AccessKey.ModefoundThe newly added
Modecolumn in theAccessKeytable is not referenced elsewhere in the codebase, and there are no TODO comments related toAccessKeyorMode. This indicates that the change is isolated and does not affect other parts of the system.
- No usage of
AccessKey.Modein Java or Kotlin files- No pending TODOs related to
AccessKeyorMode🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the Mode column is referenced in other parts of the codebase echo "Searching for references to AccessKey.Mode in the codebase:" rg --type java --type kotlin "AccessKey.*Mode" echo "Checking for any TODO comments related to AccessKey or Mode:" rg --type java --type kotlin "TODO.*AccessKey|TODO.*Mode"Length of output: 30205
Script:
#!/bin/bash # Check if the Mode column is referenced in other parts of the codebase echo 'Searching for references to AccessKey.Mode in the codebase:' rg --type java --type kotlin 'AccessKey\.Mode' echo 'Checking for any TODO comments related to AccessKey or Mode:' rg --type java --type kotlin 'TODO.*AccessKey|TODO.*Mode'Length of output: 368
scripts/sql/profiles/mysql-database-not-specified/apolloconfigdb.sql (1)
Line range hint
1-529: Summary: Well-implemented change with minimal impact.The addition of the
Modecolumn to theAccessKeytable is the primary change in this script. This modification is well-implemented and isolated, minimizing the risk of unintended side effects on other parts of the database schema.Key points:
- The new
Modecolumn is properly defined with appropriate data type, default value, and clear comment.- No changes to existing constraints or indexes were required.
- No data migration is necessary due to the use of a default value for the new column.
The suggestions provided earlier (adding an index on the
Modecolumn and a configuration entry in theServerConfigtable) can potentially improve query performance and system configurability, respectively.Overall, this change appears to be a solid enhancement to the access key functionality, allowing for more granular control over access permissions.
scripts/sql/profiles/mysql-default/apolloconfigdb.sql (1)
Line range hint
1-524: Summary: The SQL script changes are appropriate, with room for overall improvements.The addition of the
Modecolumn to theAccessKeytable is well-implemented and aligns with the PR objectives. The overall script is structurally sound, but could benefit from the following enhancements:
- Wrapping the script in a transaction for atomic execution.
- Explicitly setting character set and collation for the database.
- Considering partitioning for large tables.
- Adding an index on the new
Modecolumn in theAccessKeytable.These changes would further improve the script's reliability, consistency, and performance. Overall, the current implementation is acceptable, and these suggestions are for future optimization.
scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloconfigdb-v230-v240.sql
Show resolved
Hide resolved
What's the purpose of this PR
add observe status access-key for pre-check and logging only
Which issue(s) this PR fixes:
Fixes #5216
Brief changelog
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Release Notes
New Features
modeparameter for enabling and disabling access keys, allowing users to specify operational modes (filter or observer).Bug Fixes
Database Changes
Modecolumn to theAccessKeytable, specifying key modes for enhanced access key management.These changes enhance user control and clarity in managing access keys within the application.