Skip to content
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 non-public cloud environments in the Azure Storage Queue and Azure Storage Blob scalers #1863

Merged
merged 8 commits into from
Jun 8, 2021

Conversation

amirschw
Copy link
Contributor

@amirschw amirschw commented Jun 4, 2021

Introduce an optional cloud parameter to the Azure Storage Queue and Azure Storage Blob scalers to support non-public Azure clouds. cloud can be set to one of the known Azure clouds, or to Private. When set to Private, the endpointSuffix parameter must also be provided. Otherwise, the endpoint suffix is inferred from the cloud environment.

PR for updating the documentation: kedacore/keda-docs#463

Fixes (partially) #1285.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Signed-off-by: amirschw [email protected]

@tomkerkhove
Copy link
Member

.NET has an enum that allows you to define the type and get the suffix automatically, isn't there a similar feature in Go?

@amirschw
Copy link
Contributor Author

amirschw commented Jun 5, 2021

Thanks for the comment @tomkerkhove, I couldn't find something quite like that in the Azure SDK for Go, but did implement a similar method for the StorageEndpointType type. Please let me know what you think.

@amirschw amirschw changed the title Support non-public cloud environments in the Azure Storage Queue scaler Support non-public cloud environments in the Azure Storage Queue and Azure Storage Blob scalers Jun 5, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Fine by me, I've opened Azure/azure-storage-queue-go#25 to see if this can be simplified.

@amirschw
Copy link
Contributor Author

amirschw commented Jun 5, 2021

Oh, now I get that you meant the type of cloud. This could have helped to simplify things if it wasn't for some air-gapped cloud environments, where the endpoint suffix cannot be made public. For those environments, I believe that users will have to provide the endpoint suffix themselves.

@tomkerkhove
Copy link
Member

I think the sweet spot is both where cloud defaults to the public cloud and they can configure the others and we determine the endpoint suffix.

If it's stack or airgapped, then they use Private and have to configure endpointSuffix.

What do you think?

@amirschw
Copy link
Contributor Author

amirschw commented Jun 5, 2021

I like that, thanks! I'll work on the necessary changes: adding an optional cloud config, and make endpointSuffix required only when cloud is set to Private (or Other?).

@tomkerkhove
Copy link
Member

Sounds good to me! I would personally go with Private but I'm open to anything

@tomkerkhove
Copy link
Member

Is there a reason why we are changing the Azure Event Hubs scaler? Because not all parts seem to be aligned?

@amirschw
Copy link
Contributor Author

amirschw commented Jun 7, 2021

The Event Hub scaler uses ParseAzureStorageBlobConnection from azure_storage.go for getting checkpoints from blob storage. It does so using a connection string, but since I added the endpointSuffix argument to the function, I just had to pass an empty string there. Hope that answers your question.

@amirschw
Copy link
Contributor Author

amirschw commented Jun 7, 2021

While this PR partially fixes #1285, there is still work to be done to support Government and other non-public clouds in the rest of the Azure scalers (Azure Event Bub, Azure Log Analytics, Azure Monitor, Azure Service Bus). I'm not sure I'll have the capacity to add this to all of them, but will try to find time to set up another PR (at least for the Service Bus scaler which is used by our team :)).
Maybe it's a good idea to open separate issues for each of the scalers to track this. Otherwise the original issue can probably be used for tracking all of them.

@tomkerkhove
Copy link
Member

While this PR partially fixes #1285, there is still work to be done to support Government and other non-public clouds in the rest of the Azure scalers (Azure Event Bub, Azure Log Analytics, Azure Monitor, Azure Service Bus). I'm not sure I'll have the capacity to add this to all of them, but will try to find time to set up another PR (at least for the Service Bus scaler which is used by our team :)).
Maybe it's a good idea to open separate issues for each of the scalers to track this. Otherwise the original issue can probably be used for tracking all of them.

No problem at all, you're already doing a ton of work!

Feel free to open issues for the others so we can add them eventually!

@tomkerkhove
Copy link
Member

Would you mind resolving the merge conflict please? Then @zroubalik can do a review.

@amirschw
Copy link
Contributor Author

amirschw commented Jun 8, 2021

Resolved the merge conflicts and ready for review 😄

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @amirschw!

@tomkerkhove tomkerkhove merged commit fb76638 into kedacore:main Jun 8, 2021
@amirschw amirschw deleted the endpoint-suffix branch June 8, 2021 09:28
@zroubalik zroubalik added this to the v2.4.0 milestone Jul 14, 2021
nilayasiktoprak pushed a commit to nilayasiktoprak/keda that referenced this pull request Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants