OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.37k stars 2.37k forks source link

Azure AI Search settings throws, failing to resolve service #14984

Closed bleroy closed 8 months ago

bleroy commented 8 months ago

Describe the bug

Settings for the new Azure AI Search are not accessible after setting up a simple site from scratch.

cc @MikeAlhayek

To Reproduce

Create a new Agency site with everything default Activate Azure AI Search feature as well as Indexing and Search Select Search / Settings from the admin menu

Expected behavior

Search screen to appear

Actual

Getting an exception:

Exception has occurred: CLR/System.InvalidOperationException
An exception of type 'System.InvalidOperationException' occurred in Microsoft.Extensions.DependencyInjection.dll but was not handled in user code: 'Unable to resolve service for type 'OrchardCore.Search.AzureAI.Services.AzureAISearchIndexSettingsService' while attempting to activate 'OrchardCore.Search.AzureAI.Drivers.AzureAISearchSettingsDisplayDriver'.'
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateArgumentCallSites(ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain, ParameterInfo[] parameters, Boolean throwIfCallSiteNotFound)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(ResultCache lifetime, ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceDescriptor descriptor, ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain, Int32 slot)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateEnumerable(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateArgumentCallSites(ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain, ParameterInfo[] parameters, Boolean throwIfCallSiteNotFound)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(ResultCache lifetime, ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateOpenGeneric(ServiceDescriptor descriptor, ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain, Int32 slot, Boolean throwOnConstraintViolation)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(ServiceIdentifier serviceIdentifier)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
   at OrchardCore.Environment.Shell.Scope.ShellScopeServices.GetService(Type serviceType) in C:\Projects\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScopeServices.cs:line 16
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerFactoryProvider.<>c__DisplayClass6_0.<CreateControllerFactory>g__CreateController|0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()

Screenshots

If applicable, add screenshots to help explain your problem.

MikeAlhayek commented 8 months ago

@bleroy check your logs please. I am guessing you did not provide your settings.

bleroy commented 8 months ago

What settings should I have provided? I'm very confused, I was trying to get to the settings screen to configure the feature, and now you're telling me I need to provide settings?

MikeAlhayek commented 8 months ago

Is search services, we follow similar logic where we don't register service if the configuration does not exists. Maybe this is not the best idea.

I'll check. Maybe I'll drop the return statement if register all service anyway

https://github.com/OrchardCMS/OrchardCore/blob/42837c9ba0ad0bc86728c358926d4cde72620eff/src/OrchardCore/OrchardCore.Search.AzureAI.Core/Extensions/ServiceCollectionExtensions.cs#L29

bleroy commented 8 months ago

I'm expecting to be able to configure my search AI service from the settings screen, not from some text file (which I'm assuming couldn't even work at all if I'm on a hosted Orchard instance).

bleroy commented 8 months ago

An exception stack trace and logs also seem like a poor discovery mechanism.

MikeAlhayek commented 8 months ago

@bleroy sorry I was not clear earlier. You'll need to update your appsettings.json file

{
  "OrchardCore":{
    "OrchardCore_AzureAISearch":{
      "Endpoint":"https://[search service name].search.windows.net",
      "IndexesPrefix":"",
      "Credential":{
        "Key":"the server key goes here"
      }
    }
  }
}

https://docs.orchardcore.net/en/latest/docs/reference/modules/AzureAISearch/

Does this solve the problem?

sebastienros commented 8 months ago

Using configuration here allows you to setup the secrets in ENV, have different values per environment (App Service), and even support service connectors auth (does AI Search support it?)

bleroy commented 8 months ago

Yes, it does support managed identities, in which case there may be no secret at all to specify anywhere in Orchard.

My other point about discoverability stands.

sebastienros commented 8 months ago

managed identities != service connectors (was new to me, but I find it awesome)

MikeAlhayek commented 8 months ago

When using Managed Service or app connector, don't we have to provide some sort of secret in OC to connect with?

Maybe what we need is a new module in OC for authenticating in Azure via (Managed Services, app connectors or others)

This way, any Azure module we have like Azure AI Search can specify which type of connection to use "appsettings (default)", managed service, connector app or anything else.

Meanwhile, I'll change the Azure AI Search implementation to not throw exception if the configuration does not exist. Instead, we'll show warning on the UI

MikeAlhayek commented 8 months ago

@bleroy I am looking for example on how to connect the to Azure AI Search instance using Managed Identity or App Connector. But, I am not finding much. Either way, I am guessing you'll need to provide some sort of settings to initialize the client. For example,

Do you agree that this info need to be in the appsettings.json or the tenant settings in order to connect to the server?

Are you able to point me to the correct docs (example if available), on how to configure the Azure Client using connection string?

bleroy commented 8 months ago

Sure, this should give you a good view of the ways to authenticate to Search: https://learn.microsoft.com/en-us/azure/search/search-security-overview

Actually, no, I don't agree that info needs to be in appsettings. I don't see any reason why it couldn't be stored in the Orchard database, but I'm not up-to-date on Orchard Core settings, how it manages secrets, if it supports Key Vault, etc. @sebastienros must have some opinions and knowledge about that that I lack currently.

MikeAlhayek commented 8 months ago

@bleroy There are two different consumption scenario for this (and probably the other search) modules in OC which are as follow:

  1. A SaaS owner want to give their tenants access to Azure Search AI service. In this case the tenant don't need to worry about connecting their tenant to a dedicated Azure AI Search instance. In this scenario, the SaaS owners specify the connection string in the appsettings.json file for all tenants to relay on (Currently only key-based authentication is supported). With this setup, none of the tenants have to worry about infrastructure or dependent service. This is the current default use case in OC.
  2. Each tenant must provide their own instances of Azure Search AI service. In this case, the tenant would will using UI and configure the connection to the Azure Search AI. This isn't something we currently support. But should be easy to add giving the current code structure. We already have ways to protect secrets using IDataProtectionProvider to ensure that connection strings are secure in the database. If we choose to support this option, we would probably have to make it as the new default. However, give the SaaS owner a way to disable this behavior from the appsettings.json file if needed so they can archive scenario 1 as well if needed.

Since scenario 1 is the only option we have today. You'll need to edit the appsettings.json file to provide Endpoint and Key. This is why @sebastienros and I mentioned the need to edit appsettings.json OR provide environment variables like so

I can defiantly add support for scenario 2 (which seems to be what you are after). Better yet, we should have a way to allow case 1, case 2 or both cases.

In either way, I think we need to first provide support for other types of authentication into Azure Search AI.

Currently, we configure the Azure Clients here https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore.Search.AzureAI.Core/Extensions/ServiceCollectionExtensions.cs#L42-L45

For the above to work, the configuration section will need to look like this

{
      "Endpoint":"https://[search service name].search.windows.net",
      "IndexesPrefix":"",
      "Credential":{
        "Key":"the server key goes here"
      }
}

Question If we were to connect to the Azure service using managed-identity or Microsoft Entra, how would the connection need to look like so that the client is configured using managed-identity instead of the Key?

Knowing this will then allow me to add support for multi authentication types. Then add support for both consumption scenarios and allow the admin to specify which behavior they want to enable.

bleroy commented 8 months ago

Thanks for the additional details.

1 would imply the SaaS owner also owns the search service, gets billed for it, and transfers the cost to their customer, the end user. Billing for a search service can be quite hairy. As a SaaS business owner, I would see this as quite complex to setup, including in terms of accounting. I'd rather give the user the option to bring their own service, so they're the ones dealing with the cost. 2 seems like the more likely scenario to me.

Also thanks for the description of the settings that are currently necessary. It would be great to have that info as part of the error message or in docs.

Here's some more docs that have info about principals and auth when accessing the service through the API: https://learn.microsoft.com/en-us/azure/search/search-manage-rest

MikeAlhayek commented 8 months ago

@bleroy

I'll update the docs along with the environment variable once I am done implementing scenario 2.

The docs link you sent me does not explain should the appsettings.json file look like. I am guessing it would need to be something like this

{
      "ConnectionString":"......",
      "IndexesPrefix":""
}

instead of

{
      "Endpoint":"https://[search service name].search.windows.net",
      "IndexesPrefix":"",
      "Credential":{
        "Key":"the server key goes here"
      }
}

I just want to know how to configure the Azure client using C# with using manage identity.

bleroy commented 8 months ago

Well, the stuff you put into appsettings is up to you, it's just one way to store configuration for the client library. It's your code that's going to read those values and use them to configure the client. You're going to create a client with an instance of AzureCredentials that you're going to build from settings that may come from appsettings, environment variables, KeyVault, Orchard or wherever. No? Maybe this doc will be more relevant to you: https://learn.microsoft.com/en-us/dotnet/api/overview/azure/search.documents-readme?view=azure-dotnet Maybe also this: https://github.com/Azure/azure-sdk-for-net/blob/Azure.ResourceManager.Search_1.2.1/doc/dev/mgmt_quickstart.md

bleroy commented 8 months ago

Do I need to do anything special to cause the index to get populated? I setup everything in Search / Settings after I created my index. I verified in the portal that the index was created and seems to have all the right fields, but no matter what I do (including reset and rebuild from Orchard), the index is still 0 documents and 0 bytes from the portal, and no query returns any document. Yet, from the Orchard admin, it says there's one item in the index.

My logs contain the following suspicious stack trace:

2024-01-05 17:07:34.8060|Default|00-cb0d202005155a2ff08500c4f213387e-4bbe812f9724f7d3-00||OrchardCore.Search.AzureAI.Services.AzureAIIndexDocumentManager|ERROR|Unable to delete documents from Azure AI Search Settings System.ArgumentException: An item with the same key has already been added. Key: Content.ContentItem.FullText
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey](List`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
   at OrchardCore.Search.AzureAI.Models.AzureAISearchIndexSettings.GetMaps() in C:\Projects\OrchardCore\src\OrchardCore\OrchardCore.Search.AzureAI.Core\Models\AzureAISearchIndexSettings.cs:line 37
   at OrchardCore.Search.AzureAI.Services.AzureAIIndexDocumentManager.MergeOrUploadDocumentsAsync(String indexName, IList`1 indexDocuments, AzureAISearchIndexSettings indexSettings) in C:\Projects\OrchardCore\src\OrchardCore\OrchardCore.Search.AzureAI.Core\Services\AzureAISearchIndexDocumentManager.cs:line 129    at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey](List`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
   at OrchardCore.Search.AzureAI.Models.AzureAISearchIndexSettings.GetMaps() in C:\Projects\OrchardCore\src\OrchardCore\OrchardCore.Search.AzureAI.Core\Models\AzureAISearchIndexSettings.cs:line 37
   at OrchardCore.Search.AzureAI.Services.AzureAIIndexDocumentManager.MergeOrUploadDocumentsAsync(String indexName, IList`1 indexDocuments, AzureAISearchIndexSettings indexSettings) in C:\Projects\OrchardCore\src\OrchardCore\OrchardCore.Search.AzureAI.Core\Services\AzureAISearchIndexDocumentManager.cs:line 129
MikeAlhayek commented 8 months ago

@bleroy You should not need to do anything. After creating the index, we run a process in the back ground which populates the index. You only need to reset the index if you want to reindex all the docs all over again. And the re-indexing happens in the background too.

That error in interesting, maybe multiple parts are trying to compose the same field. I did not encounter that error on my side. I'll try to reproduce it.

You may need to check your content definition to ensure that the parts that you want to index are enabled. If you edit the content part, you should have a settings in there to include the part in the Azure AI search index.

bleroy commented 8 months ago

OK, so I got the latest version, reset all my data and started over. I'm able to create an index, and I see it's been populated from the Azure portal where I can query it and get results. When I try to do that through a search widget however, I can't seem to get any results.

I'm still getting a null ref on the settings page if I fail to provide the azure search service connection information in appsettings though, so that part still doesn't work for me.

MikeAlhayek commented 8 months ago

@bleroy are you using preview nuget packages, using main branch or the ma/azure-search-cleanup branch?

bleroy commented 8 months ago

Locally compiled tip of main. I thought this was merged into main. I'll try that branch.

MikeAlhayek commented 8 months ago

I did not merge the recent changes. Yes please try out ma/azure-search-cleanup branch. I was able to perform all the functions with no issues on my end. The only thing I could not test was the default credentials. Also, pay attention to the readme file as it has updated instruction on configuring via UI and/or appsettings

bleroy commented 8 months ago

OK, so yes, I was able to put the settings in the admin, great. However, still no search results. Nothing in logs.

MikeAlhayek commented 8 months ago

@bleroy Did you rebuild your indexes after changing branch? If you view the result in Azure portal when you search, which field are the results are found in?

Are you able to export your content type definitions, and your content items so I can try to replicate your issue? To confirm, you can set the default search provider as Azure AI Search? Search >> Settings >> Content tab. This will tell the Search module that allows site search to use Azure AI Search provider.

image

Did you configure which index to search with in by default? Search >> Settings >> Content tab. This tells the search client to search with in this index by default as you can have many indexes for different reasons.

image

Here us my index image

Here is the content type definition of my Article

image

When I search for test on the from end /search this works for me image

MikeAlhayek commented 8 months ago

@bleroy also we have our weekly steering meeting in 10 mins. If you are able to, you can join and we can troubleshoot your issue 1 on 1 and ensure to get to the bottom of it. If you can join, orchardcore.net/meet

bleroy commented 8 months ago

I did rebuild everything after the branch change, and also deleted the previous index from Azure. I started from scratch using the blog recipe and indexed the blog type.

Here's the result of a * search in the Azure portal:

{
  "@odata.context": "https://beleroy-cognitive-search.search.windows.net/indexes('default-orchardindex')/$metadata#docs(*)",
  "value": [
    {
      "@search.score": 1,
      "ContentItemId": "4emy98p5yd9zt0g5xarvg1j1n7",
      "ContentItemVersionId": "4xsdrntt76mzjvhpzk1gpcn551",
      "Content__ContentItem__Owner": "4hxtc7qrq7bdy1pv10q8eb6kxa",
      "Content__ContentItem__DisplayText__Analyzed": [
        "Man must explore, and this is exploration at its greatest"
      ],
      "Content__ContentItem__FullText": [
        "Man must explore, and this is exploration at its greatest <p>Never in all their history have men been able truly to conceive of the world as one: a single sphere, a globe, having the qualities of a globe, a round earth in which all the directions eventually meet, in which there is no center because every point, or none, is center — an equal earth which all men occupy as equals. The airman's earth, if free men make it, will be truly round: a globe in practice, not in theory.</p>\n<p>Science cuts two ways, of course; its products can be used for both good and evil. But there's no turning back from science. The early warnings about technological dangers also come from science.</p>\n<p>What was most significant about the lunar voyage was not that man set foot on the Moon but that they set eye on the earth.</p>\n<p>A Chinese tale tells of some men sent to harm a young girl who, upon seeing her beauty, become her protectors rather than her violators. That's how I felt seeing the Earth for the first time. I could not help but love and cherish her.</p>\n<p>For those who have seen the Earth from space, and for the hundreds and perhaps thousands more who will, the experience most certainly changes your perspective. The things that we share in our world are far more valuable than those which divide us.</p>\n "
      ],
      "Content__BodyAspect__Body": [
        "<p>Never in all their history have men been able truly to conceive of the world as one: a single sphere, a globe, having the qualities of a globe, a round earth in which all the directions eventually meet, in which there is no center because every point, or none, is center — an equal earth which all men occupy as equals. The airman's earth, if free men make it, will be truly round: a globe in practice, not in theory.</p>\n<p>Science cuts two ways, of course; its products can be used for both good and evil. But there's no turning back from science. The early warnings about technological dangers also come from science.</p>\n<p>What was most significant about the lunar voyage was not that man set foot on the Moon but that they set eye on the earth.</p>\n<p>A Chinese tale tells of some men sent to harm a young girl who, upon seeing her beauty, become her protectors rather than her violators. That's how I felt seeing the Earth for the first time. I could not help but love and cherish her.</p>\n<p>For those who have seen the Earth from space, and for the hundreds and perhaps thousands more who will, the experience most certainly changes your perspective. The things that we share in our world are far more valuable than those which divide us.</p>\n"
      ],
      "Content__ContentItem__DisplayText__keyword": "Man must explore, and this is exploration at its greatest",
      "Content__ContentItem__DisplayText__Normalized": "man must explore, and this is exploration at its greatest",
      "Content__ContentItem__ContentType": "BlogPost",
      "Content__ContentItem__CreatedUtc": "2024-01-09T19:19:21.021Z",
      "Content__ContentItem__Latest": true,
      "Content__ContentItem__Author": "admin",
      "Content__ContentItem__ModifiedUtc": "2024-01-09T19:19:21.024Z",
      "Content__ContentItem__Published": true,
      "Content__ContentItem__PublishedUtc": "2024-01-09T19:19:21.045Z"
    }
  ]
}

I have a conflict with the Tuesday meetings, but that meeting ended early, so I just joined.

MikeAlhayek commented 8 months ago

@bleroy I fixed the issue. Can you please pull the latest and try again? Make sure you set the Search settings.

bleroy commented 8 months ago

No luck. Still no search results. I took the latest and recompiled. I didn't recreate the site or the index. Do I need to?

MikeAlhayek commented 8 months ago

@bleroy I can't reproduce the issue at all. I did made some changes and wonder if you can pull the latest and test it one more time?

If your issue that you do NOT see a search form?

If you do see the search form, can you please show me a screenshot of the search settings? Search >> Settings >> Content tab

and Search >> Settings >> Azure Search AI tab?

MikeAlhayek commented 8 months ago

@sebastienros @bleroy I added Managed Identity authentication settings with an optional way of providing identity client id.

image

MikeAlhayek commented 8 months ago

@bleroy

I was able to create a new site using the blog recipe and everything worked for me. Below, you'll see a screencast of the exact steps I took and everything worked. Maybe for your demo today, you should start with a new tenant using the blog theme.

However, there is a bug in the TheBlogTheme theme that I have no explanation for as of now. If you enable Azure Search AI and Search modules only, the search form does NOT show up. Very strange issue. There are two workarounds for this bug

  1. Enable either Lucene or Elasticsearch along with Azure Search AI feature.
  2. Or, you may use TheDefault theme instead of the BlogTheme and everything works fine without having to enable Lucene or Elasticsearch.

You'll notices in the screencast below, I enabled Lucene feature to work around the bug in TheBlogTheme theme There are too many changes in this PR so I am going to be merging it so others can test it out as well. I'll address new issues in new PR.

azure-cognitive-search-3

bleroy commented 8 months ago

OK, so the search results were still not there, until I enabled Lucene like you did. I guess Lucene should be a dependency of the feature, at least for now...

MikeAlhayek commented 8 months ago

OK, so the search results were still not there, until I enabled Lucene like you did. I guess Lucene should be a dependency of the feature, at least for now...

@bleroy I rather not add this kind of dependency. If you switch your theme using TheDefault theme it works. So the issue must be in TheBlogTheme. I created an issue #15033 so we can fix it

So did everything worked for you now to 100% demo?

bleroy commented 8 months ago

So did everything worked for you now to 100% demo?

Technically, on the Orchard side, yes, the demo works 100%. Thanks a lot for all your efforts. Teams kept crashing every couple of minutes though so we had to postpone. Sad.

MikeAlhayek commented 8 months ago

@bleroy PR #15035 will fix the issue you and I experienced with TheBlogTheme. If you pull the latest from the main branch and run the code, you should be able to completely disable Lucene and Elasticsearch and everything should work as expected.