-
Notifications
You must be signed in to change notification settings - Fork 524
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
Allow specifying an Event Hub to use as the default EntityPath for namespace connection string #7105
base: main
Are you sure you want to change the base?
Conversation
If we can agree on tihs, I'll send a PR for the same thing in Azure Service Bus for queue/topic in EntityPath, or I can put it in this one |
Co-authored-by: David Fowler <[email protected]>
Co-authored-by: David Fowler <[email protected]>
Does this also work when deployed? |
It's funny you should ask -- I had just deployed the playground project to test, and I found a bug. I'll report back when fixed. |
Well, sh*t. It turns out that there's no way to officially pass the event hub name when using the FQNS + TokenCredential for a real Event Hub. It's fine for the emulator, because the non-tokencredential connection string allows you to pass an EntityPath. I had mistakely believed you could include the hub name as part of the Uri in the non-emulator case. One way to do it would be to add Aspire-specific handling of a FQNS that includes the hub name as a query string, e.g. The other way might be to emit an environment variable, but then this needs even more thought for scoping/namespacing it. |
We can use our own connection string format that the client understands. This is what openai does |
What about setting the ENV that will be bound to the Another comment on this PR, late one sorry, I personally prefer calling a helper like |
Hi @sebastienros - I had thought of that in an earlier point in time, but given that there are five different clients for eventhub, we'd have to inject five env vars to bind to all client settings. Also, they would take precedence over client appsettings which might be at best surprising and at worst possibly breaking, right? If we use a custom connection string then it's just a hint and the regular configuration should take precedence, if provided. As for the WithEntityPath idea, I guess so? What does everyone else think? |
I prefer a single connection string… This brings back our connection info discussion 🙂 |
Alrighty -- so I'll add a query path hint in the FQNS. And are we good with having WithEntityPath(string) ? It would just set IsDefaultEntity on the model, and I'd drop the visibility to internal on the field. I take it we don't want to start down the "two ways to do everything" path. |
before making the change, can we compare both API samples? |
Avar eventHub = builder.AddAzureEventHubs("eventhubns")
.RunAsEmulator()
.WithHub("hub1")
.WithHub("hub2", h => h.IsDefaultEntity = true); vs Bvar eventHub = builder.AddAzureEventHubs("eventhubns")
.RunAsEmulator()
.WithHub("hub1")
.WithHub("hub2")
.WithDefaultEntity("hub2"); // WithEntityPath ? At first you might think why not |
I am fine with both approaches, my preference being an extension method (whatever name makes more sense), or an argument on the Does my suggestion to generate a separate ENV to flow to the settings make sense? It would be prefixed with the resource name to bind to the correct settings. |
Yes, I thought I had mentioned this above. It does make sense from a mechanical perspective, but like I had said, there are five different clients that ship in the integration and you'd have to inject an ENV var for all of them since you cannot know in the hosting side which clients will be used. Personally, I prefer to inline a hint in the connectionstring. It keeps the scope narrower. Although injecting environment variables means we could probably get away without modifying the clients, it might cause unexpected behaviour when someone tries to specify/override the event hub in appsetting.json -- the env var will always win for configuration. |
Isn't it of higher priority than ENVs? And then the configure callback has the most priority.
I checked the code as I am not familiar with it, I see the issue is each component has its own settings class, but the Would something like this work: protected override void BindSettingsToConfiguration(AzureMessagingEventHubsBufferedProducerSettings settings,
IConfiguration configuration)
{
configuration.Bind((AzureMessagingEventHubsSettings)settings);
configuration.Bind(settings);
} If not set in component specific section, it would take the one from the apphost config. |
Nope. AppSettings are loaded first, then environment. Last one wins. Priority is effectively: https://learn.microsoft.com/en-us/dotnet/core/extensions/configuration |
…client logic to differentiate between Uri and FQNS (prior bug!)
Marking this as draft while I update the integration bits and add some more tests. Note that I also found an existing bug in the current connection string validation, I'll add more details after. |
What happens when you use this feature with azure functions, none of this applies right? |
It only improves the scenario for Functions that use the SASKey style connection string that officially supports EntityPath. Existing Function bindings that are explicit about the Event Hub name will override this, so I don't forsee any breaking behaviour there. For those bound via an FQNS + managed identity, I don't touch it since we don't have control over the client to parse out the hint. |
Ooops, I accidentally checked in overrides in the playground to use AzureCliCredential for my local testing. I'll remove them in the next commit. |
update note for README about WithDefaultEntity
Ready for review @davidfowl I've updated the PR description with more details to help with the review. |
src/Components/Aspire.Azure.Messaging.EventHubs/EventHubsComponent.cs
Outdated
Show resolved
Hide resolved
….com/oising/aspire into add-isdefaultentity-eventhub-hosting
Description
Only one EventHub can be specified as the default entity path. The entity path can still be overridden by settings in integrations. This method can be called multiple times with the same name.
An exception is thrown early if WithDefaultEntity sees that a hub has already been flagged.
Manual testing (playground project):
azd up
full deployment to azureNotes
I cleaned up the validation logic and tried to simplify it in the base client shared component
EventHubsComponent.cs
in the methodEnsureConnectionStringOrNamespaceProvided
. I also fixed an edge case bug that already existed for parsing. I added#define
clauses to the playground for disabling the emulator to make it easier to test. There are also some#define
conditionals in the projects to allow using AzureCli credential source, but functionally the playground project as it stands is unchanged.As the FQNS & token credential method doesn't work with docker and the emulator, I couldn't add local unit testing. That said, all scenarios were fully tested, and the code that extracts the event hub name hint for the FQNS method is shared among all five client types, so I'm confident it's solid.
For those not super familiar with the quirks of event hub connection strings, this PR ensures that the SAS-key style connection string has the
EntityPath
keyword set ifWithDefaultEntity(string hubName)
is called:i.e.
Endpoint=sb://localhost:57195;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=SAS_KEY_VALUE;UseDevelopmentEmulator=true;EntityPath=hub
For the FQNS (Uri) & Token Credential connection, this PR stashes a hint in the FQNS (fully qualified namespace) Uri via a QueryString parameter:
https://foobar.servicebus.azure.net:443/?EntityPath=hub
The client integration packages will extract this hint from the Url and assign it to the client's
settings.EventHubName
if it is found to be unset, thus using the prior tested code path of the settings callback.Fixes #7093
Checklist
<remarks />
and<code />
elements on your triple slash comments?breaking-change
template):doc-idea
template):