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

Added integration test for telemetry Plugin #4003

Closed
wants to merge 4 commits into from

Conversation

atharvasharma61
Copy link

@atharvasharma61 atharvasharma61 commented Jan 30, 2024

Description

[Describe what this change achieves]
Added an integration test to check if the cluster comes up healthy with both the telemetry and security plugin installed.

Issues Resolved

#3961

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

Testing

Both plugins were loaded into the local cluster and test succeeded

2024-01-30 17:17:25 SUITE-TelemetryPluginIntegrationTests-seed#[E0B6B58EF43BC0AC] INFO  PluginsService:233 - loaded plugin [org.opensearch.security.OpenSearchSecurityPlugin]
2024-01-30 17:17:25 SUITE-TelemetryPluginIntegrationTests-seed#[E0B6B58EF43BC0AC] INFO  PluginsService:233 - loaded plugin [org.opensearch.telemetry.OTelTelemetryPlugin]

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.

…he Telemetry plugin and Security plugin

Signed-off-by: Atharva Sharma <[email protected]>
Signed-off-by: Atharva Sharma <[email protected]>
build.gradle Outdated Show resolved Hide resolved
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Is this PR just adding a sanity check for successful installation of Telemetry plugin with a secure cluster?

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

@atharvasharma61 Thanks for this contribution. There are several failing checks - please get those to green.

We need to resolve the comment thread around how the telemetry plugin is referenced and used in these test cases. Please follow up on https://github.com/opensearch-project/security/pull/4003/files#r1471334295

@stephen-crawford
Copy link
Contributor

Hi @atharvasharma61, are there still plans to try to move forward with this change or is the Telemetry plugin heading in a different direction for testing with security?

@atharvasharma61
Copy link
Author

@Gaganjuneja to take the call.

@Gaganjuneja
Copy link
Contributor

One thing needs to be clarified here is that there is no dependency between telemetry and security plugins. Telemetry plugin is an implementation of telemetry lib in the core. So once the telemetry feature is enabled through plugin we are instrumenting the network layer which creates some wrapper objects. So the ask here is to bring up the OpenSearch cluster with security plugin installed and telemetry feature enabled.

If we have a mechanism to enable the security plugin in the OpenSearch integ testcases then it can easily be tested there.

@DarshitChanpura
Copy link
Member

Thanks for following up @Gaganjuneja

  1. Could you please run ./gradlew spotlessApply to fix hygiene check failures.
  2. Could you add your comment on this thread: Added integration test for telemetry Plugin #4003 (comment)

@Gaganjuneja
Copy link
Contributor

Thanks for following up @Gaganjuneja

  1. Could you please run ./gradlew spotlessApply to fix hygiene check failures.
  2. Could you add your comment on this thread: Added integration test for telemetry Plugin #4003 (comment)

Thanks, I will update on this.

@Gaganjuneja
Copy link
Contributor

Thanks for following up @Gaganjuneja

  1. Could you please run ./gradlew spotlessApply to fix hygiene check failures.
  2. Could you add your comment on this thread: Added integration test for telemetry Plugin #4003 (comment)

Thanks, I will update on this.

If we are ok having it as a testImplementation dependency. We can cleanup this PR and bring it to the ready state for merge.

@Gaganjuneja
Copy link
Contributor

@reta, What do you suggest, should we hold on this till #4010 or we can go ahead with adding the plugin dependency testImplementation?

@reta
Copy link
Collaborator

reta commented Mar 19, 2024

@Gaganjuneja we've discussion regarding that during OpenSearch Security Triage meeting: the conclusions are mutlifold, with these highlights:

  • we know there is no direct dependencies between telemetry plugin and any other plugins (including security)
  • we know that some plugins heavily rely on OpenSearch core implementation details that telemetry instrumentation changes (fe security, performance-analyzer)

The idea is to have smoke run during the distribution builds with most of the plugins installed. It might be much easier than hunting plugin after plugin. I will try to talk to build team today to understand if that is feasible.

@peternied
Copy link
Member

@reta Did you have an update from conversations with the build team?

@reta
Copy link
Collaborator

reta commented Mar 20, 2024

@reta Did you have an update from conversations with the build team?

Yes, I did actually! The good news - we could leverage the existing integTest scripts to run post distribution smoke tests, I am learning the way it works now to open up the detailed issue against opensearch-build repo, just need a bit more time, thanks @peternied

@peternied
Copy link
Member

This repository is not the best place to include these test cases, please work with the opensearch-build's test workflow [1] to create and onboard a telemetry plugin test suite. Closing out this pull request.

@peternied peternied closed this Mar 28, 2024
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.

6 participants