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

Fix tests after Apache HttpClient5 / HttpCore5 update since those use deprecated APIs #5035

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

reta
Copy link
Collaborator

@reta reta commented Jan 16, 2025

Description

See please opensearch-project/OpenSearch#16757

Issues Resolved

See please https://github.com/opensearch-project/security/actions/runs/12813953884/job/35729309491?pr=5034

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

N/A

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

willyborankin
willyborankin previously approved these changes Jan 16, 2025
cwperks
cwperks previously approved these changes Jan 16, 2025
@cwperks
Copy link
Member

cwperks commented Jan 16, 2025

Not sure if backport is necessary on this if its specific to apache httpcore5/httpclient5

@reta
Copy link
Collaborator Author

reta commented Jan 16, 2025

Not sure if backport is necessary on this if its specific to apache httpcore5/httpclient5

No, it is only 3.x line

@cwperks cwperks removed the backport 2.x backport to 2.x branch label Jan 16, 2025
willyborankin
willyborankin previously approved these changes Jan 16, 2025
derek-ho
derek-ho previously approved these changes Jan 16, 2025
@cwperks
Copy link
Member

cwperks commented Jan 16, 2025

Unfortunately there are still failing tests in citest: https://github.com/opensearch-project/security/actions/runs/12816725705/job/35739621739?pr=5035

@reta
Copy link
Collaborator Author

reta commented Jan 16, 2025

Unfortunately there are still failing tests in citest: https://github.com/opensearch-project/security/actions/runs/12816725705/job/35739621739?pr=5035

Sadly may need more work, too many clients in too many places :(

@cwperks
Copy link
Member

cwperks commented Jan 16, 2025

Unfortunately there are still failing tests in citest: https://github.com/opensearch-project/security/actions/runs/12816725705/job/35739621739?pr=5035

Sadly may need more work, too many clients in too many places :(

I would offer to help, but I'm not exactly sure what needs to change.

I'm currently looking at the failing tests in WebhookAuditLogTest and I'm not sure if the issue is in setting up the server, or if its the client configuration in WebhookSink.

I've got some homework to do to read up more on the Apache HttpCore/HttpClient5 docs. The deprecation messages in that library don't indicate what the replacements are.

@reta
Copy link
Collaborator Author

reta commented Jan 16, 2025

I would offer to help, but I'm not exactly sure what needs to change.

Thanks a lot @cwperks , for these failures I actually don't know the cause yet, I would need to do a debug session sadly ...

@reta
Copy link
Collaborator Author

reta commented Jan 17, 2025

I would offer to help, but I'm not exactly sure what needs to change.

Ah, @cwperks we do have to accommodate ServerBootstrap changes - we don't use that in core, hence I've not seen such failures, working on that.

@cwperks
Copy link
Member

cwperks commented Jan 17, 2025

The SAMLHTTPMetadataResolver is still using httpcomponents v4 (via OpenSAML 4.3.2). Would this be a blocker from fully upgrading to Apache httpcomponents v5? #2932

@reta
Copy link
Collaborator Author

reta commented Jan 17, 2025

The SAMLHTTPMetadataResolver is still using httpcomponents v4 (via OpenSAML 4.3.2). Would this be a blocker from fully upgrading to Apache httpcomponents v5? #2932

No, v4 and v5 are independent and could be used at the same time.

@reta reta force-pushed the fix.test branch 5 times, most recently from fce7be0 to bfe2520 Compare January 17, 2025 18:25
@willyborankin
Copy link
Collaborator

The SAMLHTTPMetadataResolver is still using httpcomponents v4 (via OpenSAML 4.3.2). Would this be a blocker from fully upgrading to Apache httpcomponents v5? #2932

No, v4 and v5 are independent and could be used at the same time.

@reta OpenSAML upgrade #5038

… deprecated APIs

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.36%. Comparing base (1924e41) to head (fa2b456).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5035      +/-   ##
==========================================
- Coverage   71.50%   71.36%   -0.15%     
==========================================
  Files         336      336              
  Lines       22625    22617       -8     
  Branches     3598     3599       +1     
==========================================
- Hits        16178    16140      -38     
- Misses       4648     4685      +37     
+ Partials     1799     1792       -7     
Files with missing lines Coverage Δ
.../dlic/auth/http/jwt/keybyoidc/KeySetRetriever.java 79.04% <100.00%> (ø)
...opensearch/security/auditlog/sink/WebhookSink.java 76.30% <100.00%> (-0.28%) ⬇️
...org/opensearch/security/httpclient/HttpClient.java 84.04% <100.00%> (-1.11%) ⬇️

... and 18 files with indirect coverage changes

@reta
Copy link
Collaborator Author

reta commented Jan 17, 2025

@cwperks @willyborankin @DarshitChanpura we should be set for now, I will continue to chase #5035 (comment) to understand why it is needed, thanks folks!

@derek-ho derek-ho merged commit 00016e0 into opensearch-project:main Jan 17, 2025
41 of 42 checks passed
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