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

Invalid Uri error with version 1.8.2 #15160

Open
vmahant opened this issue Jan 24, 2024 · 6 comments · Fixed by #15178
Open

Invalid Uri error with version 1.8.2 #15160

vmahant opened this issue Jan 24, 2024 · 6 comments · Fixed by #15178
Labels
Milestone

Comments

@vmahant
Copy link

vmahant commented Jan 24, 2024

Describe the bug

To Reproduce

Steps to reproduce the behaviour:

  1. upgrade to orchard core 1.7.2 to 1.8.2 as per the documentation

  2. connection method is azure key vault refer https://docs.orchardcore.net/en/latest/docs/reference/core/KeyVault.Azure/#configuration

  3. See error

Unhandled exception. System.UriFormatException: Invalid URI: The hostname could not be parsed. at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind, UriCreationOptions& creationOptions) at System.Uri..ctor(String uriString) at OrchardCore.Configuration.KeyVault.Extensions.AzureKeyVaultConfigurationExtension.AddOrchardCoreAzureKeyVault(IConfigurationBuilder builder, IConfiguration configuration, TokenCredential tokenCredential) at OrchardCore.Configuration.KeyVault.Extensions.AzureKeyVaultConfigurationExtension.<>c__DisplayClass0_0.<AddOrchardCoreAzureKeyVault>b__0(HostBuilderContext context, IConfigurationBuilder builder) at [Microsoft.Extensions.Hosting](http://microsoft.extensions.hosting/).HostBuilder.InitializeAppConfiguration()

Expected behaviour

Orchard core 1.8.2 should run successfully with azure key vault connection

Screenshots

image
@MikeAlhayek
Copy link
Member

MikeAlhayek commented Jan 26, 2024

@vmahant the only way you would encounter this exception if you did not provide a value in KeyVaultName in your configuration.

Can you please check you settings. I'll submit a PR to provide a detailed exception. But you need to provide KeyVaultName for this to work.

@buzzjames
Copy link

I have also encountered this issue when upgrading to 1.8.2. I had the KeyVaultName setting already configured and working prior to the update.

It looks like an issue may have been introduced by this change #14550 for applications that are using the older dotnet core 3.x/5 builder to bootstrap the application. Which is shown in the documentation

using OrchardCore.Configuration.KeyVault.Extensions;

public class Program
{
    public static Task Main(string[] args)
        => BuildHost(args).RunAsync();

    public static IHost BuildHost(string[] args) =>
        Host.CreateDefaultBuilder(args)
            .AddOrchardCoreAzureKeyVault()
            .ConfigureWebHostDefaults(webBuilder => webBuilder.UseStartup<Startup>())
            .Build();
}

https://docs.orchardcore.net/en/main/docs/reference/core/KeyVault.Azure/#configuration

I have since refactored my Program.cs to use the newer WebApplicationBuilder which has resolved the issue for me.

var builder = WebApplication.CreateBuilder(args);

builder.Host.AddOrchardCoreAzureKeyVault();

// Omitted for simplicity

var app = builder.Build();

// Omitted for simplicity

await app.RunAsync();

I am happy with the refactor of the Program.cs as a fix for me but I believe the older generic hosting model is still supported and is provided as the example in the documentation so an investigation into a fix or updated documentation may be appropriate?

@MikeAlhayek Unless I am missing something obvious would it be possible to reopen this issue?

From what I have been able to see when using the older generic hosting model the builder context doesn't include the appsettings configurations (or they aren't available yet) which causes an issue when creating the URI for KeyVault as the OrchardCore:OrchardCore_KeyVault_Azure:KeyVaultName is not in the IConfiguration used by AddOrchardCoreAzureKeyVault so results in a null KeyVaultName

@MikeAlhayek MikeAlhayek reopened this Apr 4, 2024
@MikeAlhayek
Copy link
Member

@buzzjames I reopen the issue. would you be interested in submitting a PR with the fix?

@buzzjames
Copy link

@MikeAlhayek Happy to help where I can but if i am honest I don't know what the fix should be.

From what I have been able to see it might not be be possible to use the current AddOrchardCoreAzureKeyVault extension with the callback style generic host builder without relying on building a temporary configuration which it was doing before #14550.

I think it might be possible to load the appsetting manually so that they are available for AddOrchardCoreAzureKeyVault but that feels like a more complicated way building a temporary instance of the configuration

Perhaps the best option is to include a fail safe i.e. attempt to retrieve the 'KeyVaultName' from the context if that is null build a temporary version of the configuration and retrieve the value from that if not found after that it can fail.

@jtkech Having had previous experience with this do you have any ideas / suggestions on the best route forward?

@MikeAlhayek
Copy link
Member

@buzzjames I don't have time at the moment to look into this. Maybe someone else can look @hishamco if you have time.

A sad note about @jtkech can be found in #14954

@buzzjames
Copy link

I am very sorry to hear of Jean-Thierry passing. I haven't been a active member of Orchard Core long but reading the comments in the thread I can see that he was highly regarded member and one who will be missed. Very sad news.

No worries, we have a work around that suits us for now. I am more than happy to contribute and submit a PR but I think it would be good to get a new perspective on it if someone is available. If I get time to dive into it some more / have any new ideas will update here.

@sebastienros sebastienros added this to the 1.x milestone Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants