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

Remove TransportClient auth/auth #1701

Merged

Conversation

jochenkressin
Copy link
Contributor

Description

With OpenSearch 2.0 the TransportClient is being removed. This means that there is no need for TransportClient auth/auth code anymore, or audit log categories that are connected with TansportLayer auth/auth. This PR removes the obsolete code and fixes some tests audit log unit tests.

Issues Resolved

#1578

Is this a backport? If so, please add backport PR # and/or commits #
No

Testing

Ran the complete test suite, adapted tests that involved TransportClient auth/auth or obsolete audit log categories

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@jochenkressin jochenkressin requested a review from a team March 23, 2022 15:47
@@ -195,7 +193,7 @@ protected boolean categoriesPresentInLog(String result, AuditCategory... categor
}

protected void logAll(AuditLog auditLog) {
//11 requests
//10 requests
Copy link
Member

@peternied peternied Mar 23, 2022

Choose a reason for hiding this comment

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

I'm curious, what does this comment mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment was in there before, and I think it just states the obvious ;-) namely, we log/generate 10 events in this method. Agreed, it's unnecessary and I can remove it if needed.

Copy link
Member

@peternied peternied Mar 23, 2022

Choose a reason for hiding this comment

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

If you have an opportunity to remove lets do it, or we will filter comments like this out over time

peternied
peternied previously approved these changes Mar 23, 2022
Signed-off-by: Jochen Kressin <jkressin@floragunn.com>
peternied
peternied previously approved these changes Mar 24, 2022
@cliu123
Copy link
Member

cliu123 commented Mar 31, 2022

@jochenkressin There're 10 test failures. Could you please take a look?

@peternied
Copy link
Member

@jochenkressin Before looking into the test failures I recommend rebasing, we've dropped support for JDK14 and speed up the runtime for the tests that should give you better results sooner

)

While we do not need any transport client auth/auth code anymore,
we still need to check the privileges of an injected user on
transport layer. This functionality was removed by mistake and
is reenabled by this commit.

Signed-off-by: Jochen Kressin <jkressin@floragunn.com>
@jochenkressin
Copy link
Contributor Author

There was indeed an issue where the privileges check for the injected user on the transport layer was not executed. The last commit fixes this. In the logs I found two other test failures that are IMHO unrelated to the changes:

DlsTest#testDlsWithMinDocCountZeroAggregations
This test seems to be flaky. When running it locally, it sometimes succeeds and sometimes fails

BasicAuditlogTest#testRestMethod
I cannot reproduce the test failure locally

@cliu123
Copy link
Member

cliu123 commented Apr 12, 2022

@jochenkressin Would you please rebase and resolved the conflicts?

@peternied
Copy link
Member

@jochenkressin Would you please rebase and resolved the conflicts?

Seeing as this change is very close to being merged, I'll take over making sure we can get it in for the 2.0.0 release. Thanks @jochenkressin

Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
Signed-off-by: Peter Nied <petern@amazon.com>
@peternied peternied self-assigned this Apr 12, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1701 (ac78bd9) into main (74618eb) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1701      +/-   ##
============================================
+ Coverage     60.42%   60.73%   +0.30%     
+ Complexity     3196     3179      -17     
============================================
  Files           253      253              
  Lines         18093    17905     -188     
  Branches       3245     3198      -47     
============================================
- Hits          10933    10874      -59     
+ Misses         5579     5453     -126     
+ Partials       1581     1578       -3     
Impacted Files Coverage Δ
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java 55.81% <ø> (ø)
...ava/org/opensearch/security/auditlog/AuditLog.java 100.00% <ø> (ø)
...org/opensearch/security/auditlog/NullAuditLog.java 0.00% <ø> (ø)
...earch/security/auditlog/impl/AbstractAuditLog.java 73.75% <ø> (-1.18%) ⬇️
...pensearch/security/auditlog/impl/AuditLogImpl.java 88.09% <ø> (-0.80%) ⬇️
.../org/opensearch/security/auth/BackendRegistry.java 62.74% <ø> (+14.30%) ⬆️
...va/org/opensearch/security/auth/RolesInjector.java 85.18% <ø> (-0.53%) ⬇️
...rg/opensearch/security/configuration/AdminDNs.java 76.36% <ø> (+2.67%) ⬆️
...arch/security/securityconf/DynamicConfigModel.java 100.00% <ø> (ø)
...ch/security/securityconf/DynamicConfigModelV6.java 0.00% <ø> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74618eb...ac78bd9. Read the comment docs.

@peternied
Copy link
Member

@opensearch-project/security Could we get a second reviewer to take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants