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

2261 Add basic authentication support to Solr health check #2263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HalHunt
Copy link

@HalHunt HalHunt commented Jul 14, 2024

What this PR does / why we need it:
Adds basic authentication support to the Solr health check.

Which issue(s) this PR fixes:
Most enterprise environments configure Solr to use SSL and some sort of authentication. This PR adds basic authentication support to the Solr health check.

Please reference the issue this PR will close: #[issue number]
#2261

Special notes for your reviewer:
Some of the existing Solr unit tests required a higher timeout from the default of 1 second in order to succeed. However I did not commit those temporary changes since it's likely it's my local testing environment. Also the new unit test I wrote requires Solr to be configured to use basic authentication.

Does this PR introduce a user-facing change?:
No. All changes are opt-in/optional.

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

@HalHunt
Copy link
Author

HalHunt commented Jul 15, 2024

@dotnet-policy-service agree company="NEOGOV"

@dario-axxes
Copy link

dario-axxes commented Nov 14, 2024

@Alirexaa Any chance you'll be able to look at this in the near future? We are currently blocked on this.

Copy link
Collaborator

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

I am supportive of the idea of making Solr health check more configurable, but I wonder if we should just ask the users for a factory method that returns SolrConnection.

Something similar to #2040, where I've enforced the Azure SDK best practices for most of the Azure health checks.

public static IHealthChecksBuilder AddDatadogPublisher(
    this IHealthChecksBuilder builder,
    string serviceCheckName,
    Func<IServiceProvider, SolrConnection>? connectionFactory = default, 
    string[]? defaultTags = default)
{
        return builder.Add(new HealthCheckRegistration(
            name ?? NAME,
            sp => new SolrHealthCheck(connectionFactory?.Invoke() ?? sp.GetRequiredService<SolrConnection>()),
            failureStatus,
            tags,
            timeout));
}

Because from what I can see it's own DI package registers it as a singleton:

https://github.com/SolrNet/SolrNet/blob/4955ddba1cce3ca3d5f7a6859dd9adc2d37ac770/Microsoft.DependencyInjection.SolrNet/ServiceCollectionExtensions.cs#L89

If we do that, the users are going to need only configure Solr in one place and the health checks is simply going to re-use that and be unaware of all the config possibilities.

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

Successfully merging this pull request may close these issues.

3 participants