[receiver/azureventhub] Add support for azure auth #44934
Conversation
|
@dyl10s As a codeowner, can you take a look at this? For some reason, I can't request your review |
dyl10s
left a comment
dyl10s
left a comment
There was a problem hiding this comment.
Just a quick code skim through. Have not had time to test this out yet.
| azureeventhub/auth_missing_feature_gate: | ||
| auth: azureauth |
There was a problem hiding this comment.
Can you make this a valid configuration, since the issue with it is supposed to be that we have the feature gate disabled?
There was a problem hiding this comment.
Sorry, I don't understand. Why would this be valid? auth is only available when the feature gate is enabled
There was a problem hiding this comment.
| azureeventhub/auth_missing_feature_gate: | |
| auth: azureauth | |
| azureeventhub/auth_missing_feature_gate: | |
| auth: azureauth | |
| event_hub: | |
| namespace: ns | |
| name: hub |
Apologies for the confusion. I was trying to say that because this test is specifically testing what happens when the feature gate is missing, there shouldn't be other issues with it. It is currently missing the namespace and the name parameters that would be required with the azureauth.
If the code were to get refactored down the line and the validation order changed, this test would fail. This update is just to make the test slightly more resilient.
There was a problem hiding this comment.
I am fine with this refactor, but the old tests tested the flag enabled and disabled state for most of the cases. Can you make sure the new test suite can run on both versions if the old version did that?
There was a problem hiding this comment.
The tests are doing that already:
There was a problem hiding this comment.
Took a closer look this morning, the feature gate scenarios that are missing were not providing any additional coverage. So I am all good with this 👍
| // EventHubConfig defines the configuration for an Azure Event Hub when | ||
| // using authentication. | ||
| type EventHubConfig struct { | ||
| // Name is the name of the Event Hub. | ||
| Name string `mapstructure:"name"` | ||
| // Namespace is the fully qualified namespace of the Event Hub. | ||
| Namespace string `mapstructure:"namespace"` | ||
| } |
There was a problem hiding this comment.
Do you have an opinion about parsing this from a connection string so that it doesn't require two different configuration settings?
There was a problem hiding this comment.
Yes,we want to get rid of the connection string because this is a security issue. You can read it in this page, Important section
3 participants
Description
Add support for azure auth when feature gate
receiver.azureeventhubreceiver.UseAzeventhubsis enabled.Link to tracking issue
Fixes #40711.
Testing
Unit tests added.
Documentation
README updated.