Prioritize retrieval of environment variables from IConfiguration instead of directly

[WhitWaldo]

Description

The Dapr .NET SDK takes different approaches to registering each of the various SDKs that all defer to a shared type, DaprDefaults to procure the HTTP, gRPC and API token values from the environment variables and form the various endpoints.

However, for reasons mentioned in the originating issue at #1338 I think it would be better for the SDK to prioritize pulling the value from an IConfiguration so users have the option to assign prefixes to the various default Dapr environment variable names or pull them from alternative sources. This PR seeks to do that on both the Client and Workflow SDKs. After 1.14 is released, I'll augment the Jobs PR to utilize the same approach and increasingly phase out use of the DaprDefaults.

This PR attempts to retrieve the gRPC endpoint value, the HTTP endpoint value and the API token value from the IConfiguration instance first, if any, then fails over to attempt a retrieval from the environment variable directly, then applies a default value.

This PR replaces #1339 because of a bad DCO commit in the middle.

Client SDK

Nothing particularly notable here. Where the injected IServiceProvider was previously discarded, I use it to inject an IConfiguration and, along with some helper values, build out the HTTP endpoint/port, the gRPC endpoint/port and the API token values.

Workflow SDK

As Dapr officially targets .NET 6 and up right now, there was some code in the Workflow registration that handled edge bugs noted for .NET clients older than 6. This PR simplifies and removes those targets.

Performance improvement

Further, it had a funky way of building the gRPC channel that accepted the gRPC endpoint, but then ignored and re-called it from DaprDefaults. The updated implementation uses the value previously retrieved.

Tests

I don't see that there are any unit tests for the Workflow SDK in the solution, which is surprising. A future commit for this PR will include some tests for at least my additions here and more should be added over time where relevant.

Actors SDK

The Actors SDK makes use of an Options configuration that defaults the HTTP endpoint to the value from DaprDefaults, but doesn't set the API token by default.

For this one, I simply added a check at either point during the registration to determine if the API token was empty (default) and if so, replace with the prioritization approach detailed above. A similar check is done to see if the HTTP endpoint in the options equals the default value of "http://localhost:3500" - if so, it replaces with the prioritization approach detailed above, and if not, leaves as-is (with the assumption that if either isn't default, it's because the user has opted to hard-code a new value).

Noted possible breaking changes

There are three possible (e.g. I can't find documentation on what the preferred behavior is for #1, I don't believe #2 is necessarily breaking and #3 simplifies a non-stable SDK) breaking changes that I wanted to call out.

Honors both endpoint and ports

Further, I believe there to be a bug in the DaprDefaults implementation. I noticed while writing this PR this morning that neither DAPR_HTTP_ENDPOINT nor DAPR_GRPC_ENDPOINT are specified in the environment variables documentation (added in a separate PR) so it's not clear whether this is intended, but as-is, the port will be ignored if the endpoint is itself specified (opting to use the port (implicitly or explicitly) specified with the endpoint instead). Rather, the value of the port is only used if the endpoint isn't specified (and thus can only be used if the endpoint is defaulted to "127.0.0.1".

This PR changes this behavior and instead sets the value of each endpoint per the retrieved value. Then it attempts to do the same with that protocol's port and sets it. As a result, if both are specified, both will be applied contrary to the implementation in DaprDefaults.

Defaults to localhost instead of 127.0.0.1 if endpoint isn't specified

There is a note in the Workflow SDK registration extension class that indicates that 127.0.0.1 isn't compatible with WSL2 so it opts of using DaprDefaults so it can instead specify "localhost". To ensure nothing is broken with Workflow, I default to "localhost" instead of "127.0.0.1" across both the Client and Workflow SDK registrations.

Removed registration method in Workflow registration

Today, one is supposed to call both services.AddDaprWorkflow() and services.AddDaprWorkflowClient();. To eliminate code duplication and otherwise simplify this, only the former is necessary now. Per this PR, it will inject a DaprWorkflowClientBuilderFactory that will pull the gRPC values from the IConfiguration (per the failover policy indicated above) and itself add what was previously done with a call to services.AddDaprWorkflowClient().

As of the latest commit (#c3a190f), all tests are passing on my system except the Dapr.E2E.Test and Dapr.Actor.Generators.Test (which seemingly are all failing because the path contains slashes in the wrong direction).

Approach taken in other SDKs

As this doesn't appear to be documented anywhere, I did a survey to see how each of the other language SDKs handles this and frankly, it's a mixed bag. I would suggest that this be applied more uniformly to all the SDKs and would be happy to contribute the appropriate PRs for each if there's some consensus on that.

JavaScript

The JavaScript SDK does not use the approach from the .NET SDK. Rather, it uses an approach more like what I have in this PR in that if the endpoint and the port are specified, it uses both to construct the gRPC and HTTP endpoints. Here is how it does it to build the gRPC client and here is the same function for the HTTP client.

Python

The Python SDK similarly uses the approach I take in this PR in that if the endpoint and port are specified, it uses both to construct the gRPC client endpoint. That said, I don't see that it ever uses it for building out the HTTP client, but perhaps that class is a work in progress as it looks a bit lighter.

Go

The approach taken in Go looks to be more similar to the current approach taken in the .NET client (which this PR seeks to change) in that if the endpoint is set, it creates the new client if the endpoint is specified and only checks the port if that endpoint isn't available.

Java

The approach taken in Java also appears to be more similar to the current approach taken in the .NET client (which this PR seeks to change) in that if the endpoint is set and not empty, it will use it without checking the port. It will only use the port value if the endpoint isn't specified, but from there it seems to use its own property for sourcing the local IP address. Rather than defaulting to "127.0.0.1" as most of the other SDKs do (or "localhost" as the .NET Workflow SDK does), it instead defers to the DEFAULT_SIDECAR_IP property value which appears to be intended to be populated as an environment variable, but which isn't documented in the environment variables reference documentation, so that's a little bit of a different twist.

Rust

Rust follows the approach I'm proposing in this PR in that it uses both the endpoint and the port (concatenating both together) to specify the intended endpoint value to connect to.

C++

I tried to do a string search on GitHub for any use of "DAPR_GRPC_ENDPOINT" or "DAPR_HTTP_ENDPOINT", but it was unable to find any reference except on the example application and even then, it was just the port values from environment values with a hard-coded "127.0.0.1", so perhaps this SDK is still under construction.

PHP

I was similarly unable to find any use of "DAPR_GRPC_" in this project, so perhaps it's still under construction.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1336 and #1032
This PR also supersedes #1040

Checklist

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

• Code compiles correctly
• Created/updated tests
• Extended the documentation

[philliphoff]

My understanding was that the newer DAPR_GRPC_ENDPOINT and DAPR_HTTP_ENDPOINT variables were intended to completely supercede their corresponding DAPR_GRPC_PORT and DAPR_HTTP_PORT variables (see dapr/dapr#6035). That is, any explicit port should be part of the endpoint value.

[WhitWaldo]

My understanding was that the newer DAPR_GRPC_ENDPOINT and DAPR_HTTP_ENDPOINT variables were intended to completely supercede their corresponding DAPR_GRPC_PORT and DAPR_HTTP_PORT variables (see dapr/dapr#6035). That is, any explicit port should be part of the endpoint value.

Good catch. I've updated the offending code so it reflects the proposal:

• If the endpoint env var is provided, it will use that value in its totality (inferring port from the protocol provided if not explicitly provided in the endpoint)
• If the port is provided and the endpoint env var not, it will default to a "127.0.0.1" equivalent - notably, it will use "localhost" instead because of the issue documented at Use "localhost" instead of "127.0.0.1" for the sidecar host address #1032
• If neither is provided, it will default to http://localhost:{defaultPort} as pulled from DaprDefaults for either HTTP or gRPC.

This PR supersedes the one at #1040

[philliphoff]

I think it should be possible to collapse the multiple implementations into just a single set, and a question about endpoint vs. port.

[WhitWaldo]

@philliphoff My bad - hold off on this PR for now. It occurs to me that it'd be mildly overhauled following part of the Jobs PR (shifting from the floating shared classes including DaprDefaults.cs to an explicit shared project).

[philliphoff]

Just a couple of final nits, but overall looks good.

[philliphoff]