[Event Hubs Client] Shared Key Credential + Connection String Parser#16044
[Event Hubs Client] Shared Key Credential + Connection String Parser#16044jsquire merged 1 commit intoAzure:masterfrom
Conversation
jsquire
added
Event Hubs
Client
jsquire
requested review from
HarshaNalluru,
KieranBrantnerMagee,
christothes,
conniey,
hemanttanwar,
ramya-rao-a,
richardpark-msft and
yunhaoling
jsquire
requested review from
KrzysztofCwalina,
serkantkaraca and
tg-msft
as code owners
jsquire
force-pushed
the
eventhubs/sharedkeycredential
branch
from
8ae1e06 to
b07faee
Compare
jsquire
force-pushed
the
eventhubs/sharedkeycredential
branch
from
b07faee to
e927f73
Compare
The focus of these changes is to expose the shared key credential and connection string parser as part of the API surface. Adjustments to the internal representation were made to support SAS rotation and refining the API surface.
jsquire
force-pushed
the
eventhubs/sharedkeycredential
branch
from
e927f73 to
b77628a
Compare
christothes
left a comment
christothes
left a comment
There was a problem hiding this comment.
LGTM - just some minor comments / questions
| string fullyQualifiedNamespace, | ||
| string eventHubName, | ||
| EventHubsSharedAccessKeyCredential credential, | ||
| EventProcessorClientOptions clientOptions = default) : base((clientOptions ?? DefaultClientOptions).CacheEventCount, consumerGroup, fullyQualifiedNamespace, eventHubName, credential, CreateOptions(clientOptions)) |
There was a problem hiding this comment.
Is it ok that the first arg to the base ctor conditionally passes DefaultClientOptions but the last arg doesn't?
There was a problem hiding this comment.
Scared me there. The first argument sets the batch size using either the specified option or the default size. The latter translates the derived options into the base type and passes it as the options set.
The CacheEventCount used for the batch size in the first argument does not exist as part of the base options, due to it being batch-driven natively and expecting that callers will need to specify the size of batches that they want to process.
| SharedAccessKeyName = null; | ||
| SharedAccessKey = null; |
There was a problem hiding this comment.
Just curious - why is setting the key name and key to null a side effect of this method?
There was a problem hiding this comment.
The intent expressed is to update the credential with the sharedAccessSignature passed to the method, which invalidates and replaces any key/value that may have been there. This is an effect of the credential supporting both key/value and SAS.
This is all very likely to change after next week's arch board meeting; there's a push to split the concept and standardize the credential types in Core.
| var hasSharedKey = ((!string.IsNullOrEmpty(SharedAccessKeyName)) && (!string.IsNullOrEmpty(SharedAccessKey))); | ||
| var hasSharedSignature = (!string.IsNullOrEmpty(SharedAccessSignature)); |
There was a problem hiding this comment.
nit: if this block moves above the previous one, the previous validation can be simplified to
if(!hasSharedKey && !hasSharedSignature)
There was a problem hiding this comment.
Not.... quite. The above says: "if I have a signature and I have either a key or value, then my intent was not clear because I specified all or parts of both." Using hasSharedKey would change the semantics to "If I have a signature and I have BOTH a key and value..."
The reason that I used this form was to ensure that we don't assume that having a SAS alone is a clear statement of intent. If I give you a SAS and some part of a key/value pair, then I'm giving you two forms of authorization, though one is not in a valid form.
|
|
||
| if (string.IsNullOrEmpty(Endpoint?.Host) | ||
| || ((string.IsNullOrEmpty(explicitEventHubName)) && (string.IsNullOrEmpty(EventHubName))) | ||
| || ((!hasSharedKey) && (!hasSharedSignature))) |
There was a problem hiding this comment.
Is this additional validation needed? It looks like we'd have thrown OnlyOneSharedAccessAuthorizationMayBeSpecified above if this were true.
There was a problem hiding this comment.
The block above that I think you're referring to validates that "only one of these is present" but doesn't cover the case where both are missing. Line 222 validates that at least one of them was present.
|
.... and I just inadvertently merged before pushing changes. I'll address the feedback in a follow-up PR. |
The focus of these changes is to incorporate feedback that was inadvertently missed before merging Azure#16044.
The focus of these changes is to incorporate feedback that was inadvertently missed before merging #16044.
…zure#16044) The focus of these changes is to expose the shared key credential and connection string parser as part of the API surface. Adjustments to the internal representation were made to support SAS rotation and refining the API surface.
The focus of these changes is to incorporate feedback that was inadvertently missed before merging Azure#16044.
2 participants
Summary
The focus of these changes is to expose the shared key credential and connection string parser as part of the API surface. Adjustments to the
internal representation were made to support SAS rotation and refining the API surface.
Note: These changes have been reviewed and approved by the architecture board and the .NET language architect. Marking with the corresponding tag for tracking purposes.
Last Upstream Rebase
Monday, October 19, 2:29pm (EDT)
References