[azure-eventhub] Fix handling of connection strings with entity path #43716

zmoog · 2025-04-06T09:48:49Z

Proposed commit message

Fix entity path handling in the event hub connection string (v1 and v2 processors).

For context on this issue, see #43715.

Changes:

v1 & v2: fully parse the connection string for validation and to check the event hub name and entity path match.
v2: handle event hub name and connection string according to the new Event Hub SDK requirements to support all connection string formats.

Unrelated changes:

I had to fix an unrelated problem to metricbeat/module/windows/service/service_integration_test.go to make the linter happy again.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Closes [azure-eventhub] Processors v1 & v2 don't handle connection strings with entity path correctly #43715

mergify · 2025-04-06T09:49:27Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @zmoog? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

elasticmachine · 2025-04-07T07:02:22Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

elasticmachine · 2025-04-07T07:02:23Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

x-pack/filebeat/input/azureeventhub/config_test.go

AndersonQ · 2025-04-07T09:17:02Z

x-pack/filebeat/input/azureeventhub/connection_string.go

+		}
+
+		keyAndValue := strings.SplitN(split, "=", 2)
+		if len(keyAndValue) < 2 {


[Blocker]

Looking at the test above, won't it introduce a breaking change?
In the test the connection string was:

sb://test-ns.servicebus.windows.net/;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=SECRET

now it's

Endpoint=sb://test-ns.servicebus.windows.net/;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=SECRET

The former won't pass this check, and if there are configs using the former format, wouldn't it make this a breaking change?

The previous connection string version (starting with sb://test-ns.servicebus.windows.net...) was invalid.

The test was passing because the previous Validate() implementation only checked that the connection string wasn't empty, instead of trying to parse it.

According to the EventHubConnectionStringProperties page:

An Event Hubs connection string is a set of key-value pairs separated by semicolon. A typical example is "Endpoint=sb://foo.EventHub.windows.net/;SharedAccessKeyName=someKeyName;SharedAccessKey=someKeyValue".

A connection may have the following sections:

Endpoint, which is mandatory. The fully qualifed namespace of the Event Hubs namespace. It will look similar to "{your-namespace}.servicebus.windows.net"

SharedAccessKeyName and SharedAccessKey, optional, used to authenticate the access to the Event Hubs namespace or Event Hub instance.

EntityPath, optional, the name of the Event Hub instance.

SharedAccessSignature, optional, an alternative way to authenticate the access to an Event Hub instance.

x-pack/filebeat/input/azureeventhub/connection_string.go

AndersonQ · 2025-04-07T09:22:31Z

x-pack/filebeat/input/azureeventhub/connection_string.go

+	if csp.Emulator {
+		// check that they're only connecting to localhost
+		endpointParts := strings.SplitN(csp.Endpoint, ":", 3) // allow for a port, if it exists.
+
+		if len(endpointParts) < 2 || endpointParts[0] != "sb" || endpointParts[1] != "//localhost" {
+			// there should always be at least two parts "sb:" and "//localhost"
+			// with an optional 3rd piece that's the port "1111".
+			// (we don't need to validate it's a valid host since it's been through url.Parse() above)
+			return ConnectionStringProperties{}, fmt.Errorf("UseDevelopmentEmulator=true can only be used with sb://localhost or sb://localhost:<port number>, not %s", csp.Endpoint)
+		}
+	}
+
+	if csp.FullyQualifiedNamespace == "" {
+		return ConnectionStringProperties{}, fmt.Errorf("key %q must not be empty", endpointKey)
+	}
+
+	if csp.SharedAccessSignature == nil && csp.SharedAccessKeyName == nil {
+		return ConnectionStringProperties{}, fmt.Errorf("key %q must not be empty", sharedAccessKeyNameKey)
+	}
+
+	if csp.SharedAccessKey == nil && csp.SharedAccessSignature == nil {
+		return ConnectionStringProperties{}, fmt.Errorf("key %q or %q cannot both be empty", sharedAccessKeyKey, sharedAccessSignatureKey)
+	}


[Suggestion]
Perform all checks and accumulate the errors to return them all at once. It's rather frustrating from the user perspective to fix a validation error just to discover there is still another to be fixed.

Good point.

For context, this function comes from an internal package of the new Event Hub SDK.

As it is internal, I cannot use it directly; I planned to include it as-is — even if I did not like it very much (now even less, after your comment).

Yeah good point on accumulating errors. But could be a follow up to improve the error message to reduce scope of the PR :)

@AndersonQ, does improving the validation in a follow-up PR work for you?

This avoids re-parsing connection string multiple times.

Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>

x-pack/filebeat/input/azureeventhub/connection_string.go

Kavindu-Dodan

Overall looks fine :)

The problem is not in the connection string but in the fact that the entity in the connection string doesn't match the event hub name.

faec · 2025-04-11T18:02:26Z

x-pack/filebeat/input/azureeventhub/config.go

+	}
+	// Store the parsed connection string, so we can use it
+	// later in the input, when needed.
+	conf.ConnectionStringProperties = connectionStringProperties


This use of Validate to precache essential data makes me uncomfortable... I don't think it's a bug in this case, since your config struct is private and you can ensure that configs are only generated by explicitly unpacking, but it adds a confusing implicit sequencing dependency. For example I'd expect that a test should be able to call newEventHubInputV2 with a valid config, but the way things are written here it would break unless the config was rewritten as yaml and then manually unpacked first.

I'd prefer that the cached connection string data wasn't in the user config struct at all, but if the field is there, can we at least initialize it in newEventHubInput*? Validation can be done with an uncached local copy (precalculating in this case is for code simplicity not performance, so there's no harm in parsing the string twice).

faec · 2025-04-11T18:06:37Z

x-pack/filebeat/input/azureeventhub/connection_string.go

+
+		// if a key value pair has `=` in the value, recombine them
+		key := keyAndValue[0]
+		value := strings.Join(keyAndValue[1:], "=")


Isn't the Join here superfluous since the SplitN parameter above ensures only the first = produces a split?

faec · 2025-04-11T18:10:45Z

x-pack/filebeat/input/azureeventhub/connection_string.go

+		return ConnectionStringProperties{}, fmt.Errorf("key %q must not be empty", endpointKey)
+	}
+
+	if csp.SharedAccessSignature == nil && csp.SharedAccessKeyName == nil {


Couldn't these be non-nil but still empty strings? (and below)

zmoog · 2025-04-14T10:49:36Z

Hey @AndersonQ, @faec, thank you so much for reviewing this PR! 🙇

I tried to cut some corners to meet a FF, but it's not worth it since we have a workaround for this issue.

I will rework (1) ConnectionStringProperties cache and (2) our copy of the internal code I copied from the Microsoft libraries to parse the connection string.

Moving the status back to draft. And I'll request a new review when ready.

zmoog added bugfix input:azure-eventhub labels Apr 6, 2025

zmoog self-assigned this Apr 6, 2025

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 6, 2025

zmoog added the backport-active-all Automated backport with mergify to all the active branches label Apr 6, 2025

zmoog force-pushed the zmoog/fix/azure-eventhub/entity-path branch from 665e226 to 573a462 Compare April 6, 2025 13:29

zmoog added Team:obs-ds-hosted-services Label for the Observability Hosted Services team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Apr 7, 2025

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 7, 2025

zmoog marked this pull request as ready for review April 7, 2025 07:02

zmoog requested review from a team as code owners April 7, 2025 07:02

zmoog requested review from AndersonQ and faec April 7, 2025 07:02

zmoog changed the title ~~[azure-eventhub] Processor v2: handle connection string with entity path~~ [azure-eventhub] Fix handling of connection strings with entity path Apr 7, 2025

AndersonQ reviewed Apr 7, 2025

View reviewed changes

zmoog and others added 11 commits April 7, 2025 18:13

Handle entity path in event hub connection string

41a78c5

Add event hub connection string validation tests

5834348

Store parsed connection string instead of a flag

1a71bf6

Clarify why we're changing this

8b75149

Cleanup

89cd22b

Make golangci-lint happy applying De Morgan’s law

080254e

Make Linter Happy Again

acf0613

Parse connection string for v1 and v2

b3eedf6

Reuse ConnectionStringProperties from config

d0f515f

This avoids re-parsing connection string multiple times.

Cleanup & clarify

145014d

Apply suggestions from code review

b7c2d97

Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>

Switch to require for concise/expressive-ness

f13635c

zmoog force-pushed the zmoog/fix/azure-eventhub/entity-path branch from 93cb87f to f13635c Compare April 7, 2025 16:13

Kavindu-Dodan reviewed Apr 7, 2025

View reviewed changes

x-pack/filebeat/input/azureeventhub/connection_string.go Show resolved Hide resolved

Kavindu-Dodan approved these changes Apr 7, 2025

View reviewed changes

zmoog added 2 commits April 7, 2025 22:16

Add connection string parser tests

f567e2d

Make Linter Happy Again

d745c24

zmoog requested a review from AndersonQ April 8, 2025 11:50

Clarify error message

190308b

The problem is not in the connection string but in the fact that the entity in the connection string doesn't match the event hub name.

faec reviewed Apr 11, 2025

View reviewed changes

zmoog marked this pull request as draft April 14, 2025 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[azure-eventhub] Fix handling of connection strings with entity path #43716

[azure-eventhub] Fix handling of connection strings with entity path #43716

zmoog commented Apr 6, 2025 •

edited

Loading

mergify bot commented Apr 6, 2025

elasticmachine commented Apr 7, 2025

elasticmachine commented Apr 7, 2025

AndersonQ Apr 7, 2025

zmoog Apr 7, 2025

AndersonQ Apr 7, 2025

zmoog Apr 7, 2025

Kavindu-Dodan Apr 7, 2025

zmoog Apr 10, 2025 •

edited

Loading

Kavindu-Dodan left a comment

faec Apr 11, 2025

faec Apr 11, 2025

faec Apr 11, 2025

zmoog commented Apr 14, 2025 •

edited

Loading

[azure-eventhub] Fix handling of connection strings with entity path #43716

Are you sure you want to change the base?

[azure-eventhub] Fix handling of connection strings with entity path #43716

Conversation

zmoog commented Apr 6, 2025 • edited Loading

Proposed commit message

Checklist

Related issues

mergify bot commented Apr 6, 2025

elasticmachine commented Apr 7, 2025

elasticmachine commented Apr 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmoog Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Kavindu-Dodan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmoog commented Apr 14, 2025 • edited Loading

zmoog commented Apr 6, 2025 •

edited

Loading

zmoog Apr 10, 2025 •

edited

Loading

zmoog commented Apr 14, 2025 •

edited

Loading