Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add apollo audit log common solution backend #4985

Merged
merged 54 commits into from
Oct 28, 2023

Conversation

BlackBear2003
Copy link
Contributor

@BlackBear2003 BlackBear2003 commented Sep 25, 2023

What's the purpose of this PR

add audit log module, PR for code review and further discussion

Which issue(s) this PR fixes:

Fixes #

#3505

Brief changelog

Add Apollo-Audit module which contains 4 modules of annotation, api, impl, springbootstarter

Add basic front-end pages

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

BlackBear2003 and others added 27 commits July 4, 2023 00:42
add ApolloAuditLogApi for manually logging needs.
add ApolloAuditLogDataInfluenceTableId (maybe multi-primary-key in the future).
add ApolloAuditController by @bean registered
- add @DomainEvents to BaseEntity.java
- change the way that data influences append
- add annotations and codes to make app audited
- change some classes' name
- make api to query and record api
- add event and listener to catch data influences
- try to reduce the affection of audit module on other modules
# Conflicts:
#	apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #4985 (c670758) into master (a632cc1) will increase coverage by 0.41%.
The diff coverage is 56.00%.

❗ Current head c670758 differs from pull request most recent head f4de3b3. Consider uploading reports for the commit f4de3b3 to get more accurate results

@@             Coverage Diff              @@
##             master    #4985      +/-   ##
============================================
+ Coverage     48.93%   49.34%   +0.41%     
- Complexity     1782     1894     +112     
============================================
  Files           348      372      +24     
  Lines         10991    11538     +547     
  Branches       1095     1123      +28     
============================================
+ Hits           5378     5693     +315     
- Misses         5284     5507     +223     
- Partials        329      338       +9     
Files Coverage Δ
...lo/audit/component/ApolloAuditHttpInterceptor.java 100.00% <100.00%> (ø)
...ctrip/framework/apollo/biz/service/AppService.java 48.71% <ø> (ø)
.../com/ctrip/framework/apollo/common/entity/App.java 0.00% <ø> (ø)
...p/framework/apollo/portal/api/AdminServiceAPI.java 13.19% <ø> (ø)
...k/apollo/portal/component/RestTemplateFactory.java 100.00% <100.00%> (ø)
...mework/apollo/portal/controller/AppController.java 29.33% <ø> (ø)
...ork/apollo/portal/service/AppNamespaceService.java 66.66% <ø> (ø)
...ip/framework/apollo/portal/service/AppService.java 49.46% <100.00%> (+9.90%) ⬆️
.../apollo/audit/context/ApolloAuditTraceContext.java 93.75% <93.75%> (ø)
...p/framework/apollo/common/entity/AppNamespace.java 0.00% <0.00%> (ø)
... and 23 more

... and 1 file with indirect coverage changes

@@ -65,6 +65,7 @@
<li><a href="{{ '/delete_app_cluster_namespace.html' | prefixPath }}" target="_blank">{{'Common.Nav.DeleteApp-Cluster-Namespace' | translate }}</a></li>
<li><a href="{{ '/system_info.html' | prefixPath }}" target="_blank">{{'Common.Nav.SystemInfo' | translate }}</a></li>
<li><a href="{{ '/config_export.html' | prefixPath }}" target="_blank">{{'Common.Nav.ConfigExport' | translate }}</a></li>
<li><a href="{{ '/audit_log_menu.html' | prefixPath }}" target="_blank">{{'ApolloAuditLog.Title' | translate }}</a></li>

Check warning

Code scanning / CodeQL

Potentially unsafe external link Medium

External links without noopener/noreferrer are a potential security risk.
@Anilople
Copy link
Contributor

When user disable the audit log, could we prompt user?

Audit Log disable already, you can xxxx to enable it, more detail please see the document

And hide the page too.

image

@BlackBear2003
Copy link
Contributor Author

After writing a series of unit tests about the context package, I summarized my trace-context model.

It is implemented based on the RequestContext provided by spring boot. In the same request, a Tracer instance is maintained in the context, which can be shared by all threads in this request.
This is in line with my original design intention. A request is regarded as a trace (or part of the trace). Tracer maintains only one active Scope and corresponding Span at the same time. This is also true in a trace. Should be the only one.
Tracer is request-independent. Different requests correspond to different Tracers, which represent different links.
Since the concurrency of Apollo is not high, after using JMeter to conduct a 50-concurrency test, the current audit logic has no impact. Therefore, I am not sure whether I need to maintain a critical section to ensure that only one thread can enter the critical section to operate Tracer at the same time.
Next, do I need to continue to complete the implementation of this feature in this PR (use the code of my audit module in other application modules)? Or do anything else? I'm looking forward to your guidance!

Could you try more concurrency in JMeter, for example 100, 200? And append the result in PR

The result is very charming!!! How could I post my jmeter-output-html on here?

@BlackBear2003
Copy link
Contributor Author

Audit Log disable already, you can xxxx to enable it, more detail please see the document

Added, but it may not be very beautiful

image

- add API for get isEnabled of audit log feature

- fix sql problem and add it to delta

- remove 2 classes
@BlackBear2003
Copy link
Contributor Author

BlackBear2003 commented Oct 19, 2023

About JMeter 200 concurrency test

image

image

image

Besides, the data increased in database are correct in counts and logic. It could be queried well in my audit-log-menu page.

It is my first time using JMeter, am I providing the necessary info?

@BlackBear2003
Copy link
Contributor Author

BlackBear2003 commented Oct 19, 2023

run https://www.apolloconfig.com/#/zh/development/apollo-release-guide?id=_422-%e6%89%93%e5%8c%85 to build project,

try to deploy it.

  1. watch audit log work or not
  2. change the .properties file, apollo.audit.log.enabled = false, watch the audit log close or not

finished deploy!

at http://luke.host:8070/

test account is (username: apollo/ password: admin)

@BlackBear2003
Copy link
Contributor Author

when I add "apollo.audit.log.enabled=false" to application-github.properties file and restart the portal, the feature is successfully turned off!

image


@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface ApolloAuditLog {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need javadoc in all annotation and api


@GetMapping("/enabled")
public String isEnabled() {
return "{\"enabled\": "+ properties.isEnabled() +"}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return ApolloAuditProperties directly?

Suggested change
return "{\"enabled\": "+ properties.isEnabled() +"}";
return properties;

List<ApolloAuditLog> findByOpName(String opName, Pageable page);

@Query("SELECT log FROM ApolloAuditLog log WHERE log.opName = :opName AND (log.dataChangeCreatedTime >= :startDate) AND (log.dataChangeCreatedTime <= :endDate)")
List<ApolloAuditLog> findByOpNameAndTime(@Param("opName") String opName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use jpa's method name instead of write sql by self.

reference https://stackoverflow.com/questions/39784344/check-date-between-two-other-dates-spring-data-jpa

try use findByOpNameAndCreatedTimeGreaterThanEqualAndCreatedTimeGreaterLessThanEqual or another method name.

@nobodyiam
Copy link
Member

Fantastic work, you've really put in the effort! Kudos to you! 🌟
A suggestion to make it even more awesome: consider enriching the README.md in the apollo-audit module with some instructions or sample code. This would be a huge help for other developers eager to make the most out of this audit function. Keep shining! ✨

- add javadoc
- Update ApolloAuditController to return ApolloAuditProperties directly
@@ -16,7 +16,7 @@
#
SERVICE_NAME=apollo-adminservice
## Adjust log dir if necessary
LOG_DIR=/opt/logs
LOG_DIR=/home/luke/apollo/logs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot change here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry! I pushed this accidentally, I will change it back quickly.


/**
* This annotation is mainly used for operations(mainly reflected through methods) that need to be
* audited.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark which method should be audit, add to controller or service's method

* Example usage:
* <pre>
* {@code
* public App batchDeleteByAppId(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add ApolloAuditLog on the method too

import java.lang.annotation.Target;

/**
* It is mainly used to mark the method's parameters that need to be audited.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conbine with {@link ApolloAuditLog}, mark which method's parameter is audit log's data change.

- add README.md
@BlackBear2003
Copy link
Contributor Author

BlackBear2003 commented Oct 25, 2023

Fantastic work, you've really put in the effort! Kudos to you! 🌟 A suggestion to make it even more awesome: consider enriching the README.md in the apollo-audit module with some instructions or sample code. This would be a huge help for other developers eager to make the most out of this audit function. Keep shining! ✨

Thanks! I have added the README.md ~

If it weren't for my mentor who took great care of me, I wouldn't be able to complete this task. 🥹

@@ -0,0 +1,114 @@
# Features: Apollo-Audit-Log

This module mainly provides audit log functions for other Apollo modules.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This module mainly provides audit log functions for other Apollo modules.
This module provides audit log functions for other Apollo modules.
Only apolloconfig's developer need to read it,
apolloconfig's user doesn't need.


This module mainly provides audit log functions for other Apollo modules.

## How to switch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## How to switch
## How to enable/disable

apollo.audit.log.enabled = true
```

## How to use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## How to use
## How to generate audit log


```java
public App create() {
Autocloseable auditScope = api.appendAuditLog(type, name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try with resources?


```java
public App create() {
Autocloseable auditScope = api.appendAuditLog(type, name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try with resources?


### Append an AuditLog

We can do this by using annotations:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may use 5W1H to describe it.


### Append DataInfluence

This function can also be implemented automatically and manually.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes there is not DataInfluence, we should tell developer about it.


This function can also be implemented automatically and manually.

#### Automatically
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#### Automatically
#### Mark which data change

* Append DataInfluences by a list of entities needs to be audited, and their
* audit-bean-definition.
*/
ApolloAuditLogApi.appendDataInfluences(List<Object> entities, Class<?> beanDefinition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recomand use annotation instead of api, so we can left the api to the bottom of README.md?


And this will call appendDataInfluences automatically by the listener.

#### Manually
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should describe how to verify the audit log work too...

For example, which table's data change, or how to check it by UI

Copy link
Contributor

@Anilople Anilople left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Anilople Anilople merged commit bc55ba6 into apolloconfig:master Oct 28, 2023
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2023
@nobodyiam nobodyiam added this to the 2.2.0 milestone Nov 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants