-
Notifications
You must be signed in to change notification settings - Fork 273
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
Support datastreams as an AuditLog Sink #4257
Support datastreams as an AuditLog Sink #4257
Conversation
@tmanninger Thanks for the contribution, can you please since your commits with the DCO [1]? |
src/main/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSink.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Peter's comments. @tmanninger Could you please +ve/-ve test cases for this change. We would like to really ensure that explicitly marking op_type as CREATE would resolve the question.
I agree with @peternied and @DarshitChanpura . The data stream can be created via
Under different index I mean that data stream is a system index from the sec plugin point of view: According to your rollover policy, the ISM plugin will create each time a new index
Here is an example: https://github.com/opensearch-project/opensearch-java/blob/main/samples/src/main/java/org/opensearch/client/samples/DataStreamBasics.java. Furthermore, restoring from the snapshot is a trick for datastreams since you have to restore all datastreams first and only after all regular indices. |
I think the right solution would be to extend existing |
@willyborankin do you mean, the Sink should create the Template, Datastream and ISM policy? |
@tmanninger yes I think so, the main reason is that if you already has an audit index in your cluster and the cluster was migrated on the new version ... you fix will start to create a datastream ... which is not backward compatible. |
Really? |
@tmanninger to ensure backwards compatibility for your fix:
Can we add backwards compatibility tests as well? |
I will add some test cases. |
87f8f35
to
426ea31
Compare
I added a test for datastream and for an index. Thank's ok? |
src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the right solution would be to extend existing InternalOpenSearchSink or create a new Sync call it let's say internal_opensearch_datastream or something else. @peternied, @DarshitChanpura what do you thin about the idea?
@willyborankin This is a great idea, I strongly suggest we move in this direction
Add a backward combability test.
I don't think a BWC test is needed in this scenario so long as we are adding a new audit log type.
@tmanninger Let us know if you have questions, while the proposed fix could work. I'd rather there is consistency. Having to correctly configure the data stream before its used seems like a way to have a broken audit log and not be aware of it, whereas having a concrete definition ensures that the data stream is setup correctly without user intervention
I can create a new Sink named internal_opensearch_datastream
is this way ok? |
@tmanninger Please look into how the other audit log storage systems [1] work and consider what should be have sensible defaults that is configurable vs required. As far as sink names go I think As you mentioned Looks forward to those changes - thank you. |
How can i create a datastream template? When i am using this example:
I can use |
@tmanninger I'm not sure, maybe you could look at what options are available in OpenSearch's repository |
src/main/java/org/opensearch/security/auditlog/sink/InternalOpenSearchDataStreamSink.java
Outdated
Show resolved
Hide resolved
12f13e0
to
959a685
Compare
959a685
to
31cfd54
Compare
I rebased git in an incorrect way :-/ (and github was auto-closed this MR) ... |
ac82cb8
to
c1accba
Compare
Signed-off-by: tmanninger <[email protected]>
c1accba
to
1f8ce41
Compare
Signed-off-by: tmanninger <[email protected]>
Added MR for documentation: opensearch-project/documentation-website#8356 |
Signed-off-by: tmanninger <[email protected]>
I fixed the spotless errors https://github.com/opensearch-project/security/actions/runs/11010456713 . |
Signed-off-by: tmanninger <[email protected]>
Signed-off-by: tmanninger <[email protected]>
Check the doc count after rollover failed.
Sometimes, there is one more audit log in the new datastream backend, maybe the rollover is faster than writing all audit logs generated before the rollover. @willyborankin pls rerun (hope the last time). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4257 +/- ##
==========================================
+ Coverage 65.51% 65.65% +0.14%
==========================================
Files 319 321 +2
Lines 22448 22538 +90
Branches 3602 3611 +9
==========================================
+ Hits 14707 14798 +91
+ Misses 5933 5928 -5
- Partials 1808 1812 +4
|
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added MR for documentation: opensearch-project/documentation-website#8356
Thank you for creating a documentation PR for this change!
The changes look good to me, I just had one remaining question on the default value for number of shards.
Signed-off-by: tmanninger <[email protected]>
Signed-off-by: tmanninger <[email protected]> (cherry picked from commit 9c5e32a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
This PR allows writing auditlogs to datastreams.
auditlog type "internal_opensearch" currently not supporting datatstreams.
java.lang.IllegalArgumentException: only write ops with an op_type of create are allowed in data streams
This PR changes the op_type to "create" for the "internal_opensearch" auditlogs, because audit logs should never be updated.
Datastreams only supporting "create" as op_type.
Issues Resolved
Testing
Tested manually: Security auditlog with and without datastreams