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

Provide support for using SAS token in Service Bus scaler #2920

Closed
tomkerkhove opened this issue Apr 20, 2022 · 22 comments · Fixed by #3673 or #3607
Closed

Provide support for using SAS token in Service Bus scaler #2920

tomkerkhove opened this issue Apr 20, 2022 · 22 comments · Fixed by #3673 or #3607
Assignees
Labels
feature All issues for new features that have been committed to needs-discussion

Comments

@tomkerkhove
Copy link
Member

Proposal

Provide support for using SAS token in Service Bus scaler.

Use-Case

Different scenario for authentication, similar to #2705

Anything else?

No response

@tomkerkhove tomkerkhove added needs-discussion feature-request All issues for new features that have not been committed to feature All issues for new features that have been committed to and removed feature-request All issues for new features that have not been committed to labels Apr 20, 2022
@sushmithavangala
Copy link
Contributor

Working on this.

@sushmithavangala
Copy link
Contributor

@tomkerkhove quick doubt - SAS authentication is currently handled if it's provided through the connection string. Are we looking at a provision to use SAS token other than through connection string?

@tomkerkhove
Copy link
Member Author

If that's the case then we can close it, I thought we didn't support that yet.

Thanks for checking!

@zzhakhin
Copy link

zzhakhin commented Apr 27, 2022

@tomkerkhove @SushmithaVReddy I try to setup scaledObject with trigger "azure-servicebus" that uses SAS token(trigger authentication ---> k8s secret). At the current moment KEDA 2.6 doesn't support this feature.

When I deploy the scaledObject with SAS token, I get ' "Endpoint" must not be empty' error message.

https://github.com/Azure/azure-amqp-common-go/blob/6b7304f631077f049a7249a2f36fe87e5c27b674/conn/conn.go#L100

Could you try to use new lib for Azure Service Bus where you can send SAS token as connection parameter?

https://github.com/Azure/azure-sdk-for-go/blob/241bdb849ce431e1a5e398a5649cde93149ee374/sdk/messaging/azservicebus/client.go#L92

@tomkerkhove
Copy link
Member Author

@SushmithaVReddy Can you verify this please?

@tomkerkhove tomkerkhove reopened this Apr 27, 2022
@sushmithavangala
Copy link
Contributor

Hi @zzhakhin , are you using a key value pair like "connectionString" : " Endpoint=sb://.servicebus.windows.net;SharedAccessSignature=SharedAccessSignature sr=.servicebus.windows.net&sig=&se=&skn=" in your trigger auth CRD referred in scaled object?

@zzhakhin
Copy link

Hi @SushmithaVReddy. These are my setup

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: worker-scaledobject
  namespace: somenamespace
spec:
  scaleTargetRef:
    name: nameofpod
  pollingInterval: 30
  cooldownPeriod: 300
  minReplicaCount: 1
  maxReplicaCount: 3
  advanced:
    restoreToOriginalReplicaCount: true
  triggers:
  - type: azure-servicebus
    metadata:
      queueName: queue-name
      messageCount: "5"
    authenticationRef:
      name: worker-trigger-authentication
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: worker-trigger-authentication
  namespace: somenamespace
spec:
  secretTargetRef:
  - parameter: connection
    name: worker-queue-secret
    key: queue-connection-string

And I create secret with key:value

queue-connection-string='SharedAccessSignature sr=https://servicebussand.servicebus.windows.net/queue-name&sig=<Signature>&se=<Unixtime>&skn=root'

@sushmithavangala
Copy link
Contributor

@zzhakhin thanks for sharing the yaml content. You are getting the Endpoint can't be null error because the connection string you are providing in your secret doesn't have endpoint information. For eg: Connection string can be defined in two ways

  1. Endpoint=sb://.servicebus.windows.net/;SharedAccessKeyName=;SharedAccessKey=
  2. Endpoint=sb://.servicebus.windows.net;SharedAccessSignature=SharedAccessSignature sr=.servicebus.windows.net&sig=&se=&skn=

I understand you want to use the 2nd way of pointing connection string. If that's the case, yes, keda doesn't support it yet.(tag: @tomkerkhove )

@v-shenoy
Copy link
Contributor

We use the older Service Bus sdk .

It only supports connection strings of the first type -

Endpoint=sb://<sb>.servicebus.windows.net/;SharedAccessKeyName=<key name>;SharedAccessKey=<key value>

and not

Endpoint=sb://<sb>.servicebus.windows.net;SharedAccessSignature=SharedAccessSignature sr=<sb>.servicebus.windows.net&sig=<base64-sig>&se=<expiry>&skn=<keyname>

In fact, if I recall correctly, service bus is not the only place we use the older Azure SDKs. We should ideally be migrating to the newer versions for all of the Azure scalers.

@sushmithavangala
Copy link
Contributor

@tomkerkhove should we work on migrating to the sdk that seems to be maintained - https://github.com/Azure/azure-sdk-for-go

@tomkerkhove
Copy link
Member Author

Endpoint=sb://<sb>.servicebus.windows.net;SharedAccessSignature=SharedAccessSignature sr=<sb>.servicebus.windows.net&sig=<base64-sig>&se=<expiry>&skn=<keyname>

The goal of the issue was actually to support this scenario which I thought we were supporting but I've misread. Can you work on this please @SushmithaVReddy?

@sushmithavangala
Copy link
Contributor

@tomkerkhove quick pointer - https://github.com/Azure/azure-sdk-for-go/blob/main/README.md requires go1.18+ and keda supports go1.17+ , so we'll have to modify this

keda/go.mod

Line 3 in ac86c94

go 1.17
to 1.18 . Let me know if this understanding is right and any repercussions on customers using <= 1.17.

@tomkerkhove
Copy link
Member Author

Let's do a seperate PR to bump to go 1.18 unless @kedacore/keda-maintainers don't agree?

But we might want to hold this off until KEDA 2.7 is shipped on thursday.

@JorTurFer
Copy link
Member

I don't have any strong opinion about that, but I'm really interested on @zroubalik opinion here

@sushmithavangala
Copy link
Contributor

@tomkerkhove if we have to do this separately we'll have to 1st migrate KEDA to 1.18 and only then push fix for #2920.

@tomkerkhove
Copy link
Member Author

@zroubalik Any reason why we didn't go to 1.18 in #3019?

@zroubalik
Copy link
Member

zroubalik commented May 9, 2022

@tomkerkhove 1.18 is relatively new. I want to wait a couple of micro release for it. And also majority of libs that we depend on (k8s) aren't build on top of 1.18. It might bring problems with code generators etc.

@tomkerkhove
Copy link
Member Author

Let's put this on hold then @SushmithaVReddy!

@v-shenoy
Copy link
Contributor

v-shenoy commented Jun 7, 2022

Even though the docs mention go1.18 this package is already being used in the repo.

@tomkerkhove
Copy link
Member Author

Then we should revise this, unless the limitation is just for Azure Service Bus.

@v-shenoy
Copy link
Contributor

v-shenoy commented Jun 7, 2022

They migrated to 1.18 in March as per this PR. But I don't think they're using any of the new 1.18 features.

@v-shenoy
Copy link
Contributor

v-shenoy commented Jun 7, 2022

Honestly, it's confusing as to how this repo works to me. It's a monorepo with all of SDKs for multiple Azure offerings. Some of them have individual go.mod files, some of them don't. There are also multiple versions of the same package in the repo so that users can import older ones if required.

It's a bit of a headache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature All issues for new features that have been committed to needs-discussion
Projects
Archived in project
6 participants