-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support multiple data source prop passwordless and new prop of credential bean name #42486
base: main
Are you sure you want to change the base?
Conversation
…manager uses the correct token credential bean
…r Spring credential provider
API change check API changes are not detected in this pull request. |
*/ | ||
@Configuration(proxyBeanMethods = false) | ||
@ConditionalOnClass(AzureAuthenticationTemplate.class) | ||
public class SpringTokenCredentialProviderContextProviderAutoConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public class SpringTokenCredentialProviderContextProviderAutoConfiguration { | |
class SpringTokenCredentialProviderContextProviderAutoConfiguration { |
|
||
@Bean | ||
@ConditionalOnMissingBean | ||
static SpringTokenCredentialProviderContextProvider springTokenCredentialProviderContextProvider() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this static?
@@ -51,10 +59,11 @@ AzureRedisPasswordlessProperties redisPasswordlessProperties() { | |||
|
|||
@Bean(name = "azureRedisCredentials") | |||
@ConditionalOnMissingBean | |||
@DependsOn("springTokenCredentialProviderContextProvider") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have other better choice here, if feels like not correct to depends on this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any good suggestions?
this(null, null, username, passwordlessProperties); | ||
} | ||
|
||
public AzureRedisCredentials(GenericApplicationContext applicationContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AzureRedisCredentials
should be independent of any Spring APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should pass in a credential provider, instead of the context.
public AzureRedisCredentials(String username, | ||
TokenCredentialProvider tokenCredentialProvider, | ||
PasswordlessProperties passwordlessProperties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public AzureRedisCredentials(String username, | |
TokenCredentialProvider tokenCredentialProvider, | |
PasswordlessProperties passwordlessProperties) { | |
public AzureRedisCredentials(String username, | |
PasswordlessProperties passwordlessProperties, | |
TokenCredentialProvider tokenCredentialProvider) { |
azureAuthenticationTemplate.init(properties); | ||
this.username = resolveUsername(azureAuthenticationTemplate, username); | ||
} | ||
|
||
public AzureRedisCredentials(String username, PasswordlessProperties passwordlessProperties, TokenCredential tokenCredential) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call the newly added ctor in from this method?
factoryCustomizers.orderedStream().collect(Collectors.toList())) | ||
.createConnectionFactory(ServiceBusJmsConnectionFactory.class); | ||
} | ||
|
||
private TokenCredentialProvider getPasswordlessTokenCredentialProvider(AzureServiceBusJmsProperties serviceBusJmsProperties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we leverage the authenticate template instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should put all the same logic into the same place, instead of writing it everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will improve the logic
AzureGlobalProperties.PREFIX + ".credential.token-credential-bean-name", | ||
AzureEventHubsProperties.PREFIX + ".credential.token-credential-bean-name" | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the credential can come from all the other spring.cloud.azure.credential.xxx properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not covered all the credentials, we should add all credential types here, right?
@@ -80,8 +84,19 @@ static class ProcessorContainerConfiguration { | |||
@ConditionalOnMissingBean | |||
ServiceBusProcessorFactory defaultServiceBusNamespaceProcessorFactory( | |||
NamespaceProperties properties, | |||
ObjectProvider<PropertiesSupplier<ConsumerIdentifier, ProcessorProperties>> suppliers) { | |||
return new DefaultServiceBusNamespaceProcessorFactory(properties, suppliers.getIfAvailable()); | |||
ObjectProvider<PropertiesSupplier<ConsumerIdentifier, ProcessorProperties>> suppliers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Service Bus Integration scenario, it's used to custom the Service Bus builder factory in DefaultServiceBusNamespaceProcessorFactory
.
Description
Fixes #41977
Changelogs:
isManagedIdentityEnabled
) copy issueAll SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines