[v2] Add SSL support to RabbitMQ Scaler #1073

ansonauhk · 2020-09-03T14:34:12Z

Signed-off-by: Anson Au [email protected]

Provide a description of what has been changed

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Tests have been added
A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
Changelog has been updated

Fixes #967

Signed-off-by: Anson Au <[email protected]>

zroubalik

Looking good, could you please write a few tests? For the auth properties parsing. Feel free to look at the PR for an inspiration (Kafka) #1074

zroubalik · 2020-09-03T15:21:14Z

Could you please update docs: kedacore/keda-docs#231

And add an entry to the Changelog please: https://github.com/kedacore/keda/blob/v2/CHANGELOG.md#v200

ahmelsayed · 2020-09-03T18:54:55Z

pkg/scalers/rabbitmq_scaler.go

+		if val, ok := authParams["ca"]; ok {
+			meta.ca = val
+		} else {
+			return nil, fmt.Errorf("no ca given")
+		}
+		if val, ok := authParams["cert"]; ok {
+			meta.cert = val
+		} else {
+			return nil, fmt.Errorf("no cert given")
+		}
+		if val, ok := authParams["key"]; ok {
+			meta.key = val
+		} else {
+			return nil, fmt.Errorf("no key given")


from the usage below, I'm assuming those are expected to have file paths, not actual certificate values, right? I think you would want to use metadata map instead of the authParams map, as the latter contains values resolved from a TriggerAuthentication CRD if present for a particular trigger.

also a bit of nitpicking, but perhaps rabbitmq host is using amqps://, but no key/cert/ca provided would be a bit more descriptive error message.

also a bit of nitpicking, but perhaps rabbitmq host is using amqps://, but no key/cert/ca provided would be a bit more descriptive error message.

Done

ansonauhk · 2020-09-04T01:48:43Z

No,not from a file. One has to upload the certificates content to the K8S secrets. I wonder how a file path works , as scaledObject is living in a keda namespace. In a enterprise environment, it's not flexible to have file contains secret stored in a shared namespace.

…

On Fri, Sep 4, 2020 at 2:55 AM Ahmed ElSayed ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/scalers/rabbitmq_scaler.go <#1073 (comment)>: > + if val, ok := authParams["ca"]; ok { + meta.ca = val + } else { + return nil, fmt.Errorf("no ca given") + } + if val, ok := authParams["cert"]; ok { + meta.cert = val + } else { + return nil, fmt.Errorf("no cert given") + } + if val, ok := authParams["key"]; ok { + meta.key = val + } else { + return nil, fmt.Errorf("no key given") from the usage below, I'm assuming those are expected to have file paths, not actual certificate values, right? I think you would want to use metadata map instead of the authParams map, as the latter contains values resolved from a TriggerAuthentication CRD if present for a particular trigger. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1073 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACC7PP67QF6DXYXNPX22E7DSD7RA7ANCNFSM4QU25HWA> .

Signed-off-by: Anson Au <[email protected]>

Update change log for kedacore#1073 Signed-off-by: Anson Au [email protected]

ansonauhk · 2020-09-06T12:02:04Z

Looking good, could you please write a few tests? For the auth properties parsing. Feel free to look at the PR for an inspiration (Kafka) #1074

Added, please review

ansonauhk · 2020-09-06T12:02:36Z

Could you please update docs: kedacore/keda-docs#231

And add an entry to the Changelog please: https://github.com/kedacore/keda/blob/v2/CHANGELOG.md#v200

all done

ahmelsayed · 2020-09-09T23:38:38Z

No,not from a file. One has to upload the certificates content to the K8S secrets. I wonder how a file path works , as scaledObject is living in a keda namespace. In a enterprise environment, it's not flexible to have file contains secret stored in a shared namespace.

other scaler that use a cert take a certPath parameter in the metadata, and then KEDA users mount the cert as a file using kubernetes secret file volume.

In this PR it seems that you're reading the value from authParams. authParams is constructed here. However, authParams is a map[string]string.

so usually you'd have a secret defined in Kubernetes as

kind: Secret
apiVersion: v1
metadata:
  name: test-secret-name
data:
  key.pem: <base64 of key.pem file>

Other scalers assume that KEDA users will add something like this on the KEDA pod

    volumeMounts:
    - name: certs
      mountPath: "/etc/certs"
      readOnly: true
  volumes:
  - name: certs
    secret:
      secretName: test-secret-name

this will create a file under /etc/certs/key.pem that contains the key. Then the metadata of the scaler just contains:

metadata:
  keyPath: /etc/certs/key.pem

However, with this PR if the user is using TriggerAuthentication with RabbitMQ, then authParam will be a map[string]string

so doing this in parseRabbitMQMetadata()

if val, ok := authParams["key"]; ok {
	meta.key = val
}

then meta.key will contain an invalid string encoding of the key file content

In getConnectionAndChannel() however, it's calling

if ca, err := ioutil.ReadFile(caFile); err == nil {
	cfg.RootCAs.AppendCertsFromPEM(ca)
}

if cert, err := tls.LoadX509KeyPair(certFile, keyFile); err == nil {
	cfg.Certificates = append(cfg.Certificates, cert)
}

which assumes that meta.key and meta.ca are file paths mounted on the KEDA pod itself as mentioned above for the other scalers. You can check the Kafka scaler for an example, but I suspect you need to use tls apis that tabke a byte[] rather than a filePath and call it with []byte(meta.key), and []byte(meta.ca)

That's based on just reading the code of the PR but I could be wrong. Have you been able to test this change on your cluster?

ansonauhk · 2020-09-11T12:55:05Z

Yes, it’s fully tested and working on our cluster. I understand the concept of mounting k8s secret as volume, but it only works if you are creating a pod - keda pod as you mentioned. This is a problem in an enterprise environment or any multi talented environment- Imagine on a Kubernetes cluster, we would install one Keda , under one keda namespace , as a shared service. Each team has their own namespace where their applications and secrets belong to. In this case, it's impossible to have one shared KEDA operator pod,but it has access to all cert/ca/key files from each application. - They won't share it to keda namespace. RabbitMQ Scaler is a scaledobject, which likely will be created by application in their own namespace. That's why, it's more realistic that the scalar loads the content of the cert/ca/key from secrets. I can add support for loading from file, on top of what we have so far. So we can support more use cases.

…

On Thu, 10 Sep 2020 at 7:38 AM, Ahmed ElSayed ***@***.***> wrote: No,not from a file. One has to upload the certificates content to the K8S secrets. I wonder how a file path works , as scaledObject is living in a keda namespace. In a enterprise environment, it's not flexible to have file contains secret stored in a shared namespace. other scaler that use a cert take a certPath parameter in the metadata, and then KEDA users mount the cert as a file using kubernetes secret file volume. In this PR it seems that you're reading the value from authParams. authParams is constructed here <https://github.com/kedacore/keda/blob/41e9978b4683d3bbc0e39860308e645f7513e243/pkg/scaling/resolver/scale_resolvers.go#L208-L227>. However, authParams is a map[string]string. so usually you'd have a secret defined in Kubernetes as kind: Secret apiVersion: v1 metadata: name: test-secret-name data: key.pem: <base64 of key.pem file> Other scalers assume that KEDA users will add something like this on the KEDA pod volumeMounts: - name: certs mountPath: "/etc/certs" readOnly: true volumes: - name: certs secret: secretName: test-secret-name this will create a file under /etc/certs/key.pem that contains the key. Then the metadata of the scaler just contains: metadata: keyPath: /etc/certs/key.pem However, with this PR if the user is using TriggerAuthentication with RabbitMQ, then authParam will be a map[string]string so doing this in parseRabbitMQMetadata() if val, ok := authParams["key"]; ok { meta.key = val } then meta.key will contain an invalid string encoding of the key file content In getConnectionAndChannel() however, it's calling if ca, err := ioutil.ReadFile(caFile); err == nil { cfg.RootCAs.AppendCertsFromPEM(ca) } if cert, err := tls.LoadX509KeyPair(certFile, keyFile); err == nil { cfg.Certificates = append(cfg.Certificates, cert) } which assumes that meta.key and meta.ca are file paths mounted on the KEDA pod itself as mentioned above for the other scalers. That's based on just reading the code of the PR but I could be wrong. Have you been able to test this change on your cluster? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1073 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACC7PPZ7456BI23EQWP4G2TSFAGYVANCNFSM4QU25HWA> .

ansonauhk · 2020-09-16T14:35:26Z

Hi,

Are you happy to accept this PR?

Thanks,
Anson

zroubalik · 2020-09-18T05:55:28Z

@ansonauhk there's a conflict that prevents a merge of this PR.
I am keeping this for @ahmelsayed for review

aman-bansal · 2020-10-16T18:03:49Z

pkg/scalers/rabbitmq_scaler.go

@@ -130,8 +155,27 @@ func parseRabbitMQMetadata(resolvedEnv, metadata, authParams map[string]string)
 	return &meta, nil
 }

-func getConnectionAndChannel(host string) (*amqp.Connection, *amqp.Channel, error) {
-	conn, err := amqp.Dial(host)
+func getConnectionAndChannel(host string, caFile string, certFile string, keyFile string) (*amqp.Connection, *amqp.Channel, error) {


With the recent change, we have added tls_config.go in pkg/util. That is supposed to be one-stop destination to enable tls encryption. Can you please move this to tls_config.go

aman-bansal · 2020-10-16T18:06:39Z

pkg/scalers/rabbitmq_scaler.go

+			cfg.RootCAs.AppendCertsFromPEM(ca)
+		}
+
+		if cert, err := tls.LoadX509KeyPair(certFile, keyFile); err == nil {


In my opinion, this should not be the required parameter. client cert and the key are only required when client authentication at the server is set to REQUIREANDVERIFY. In other cases, it could be empty.

Until and unless I am missing something completely in regards to rabbitmq tls.

aman-bansal · 2020-10-16T18:11:10Z

pkg/scalers/rabbitmq_scaler.go

+		if cert, err := tls.LoadX509KeyPair(certFile, keyFile); err == nil {
+			cfg.Certificates = append(cfg.Certificates, cert)
+		}
+		conn, err = amqp.DialTLS(host, cfg)


This could fail in some cases. So DialTLS has this defined:
if config.TLSClientConfig.ServerName == "" { config.TLSClientConfig.ServerName = uri.Host }

Either specify InsecureSkipVerify or take servername as parameter. @ahmelsayed this is same what I have mentioned in this #1263

clarenceb · 2021-02-23T02:55:00Z

Hi, is this PR still being worked on? It seems to be stalled for a few months now. We have a need for it and would appreciate any update on whether this is now abandoned or still in progress.

jeffhollan · 2021-02-23T05:55:03Z

Looks like some open feedback / issues, as well as now some conflicts around the base that would need to be resolved. Thanks a ton for your help @ansonauhk and this looks pretty close to me. Any interest on clearing out the conflicts so we can ship some of this? Let us know if you have any questions or we can help

ansonauhk · 2021-02-23T10:13:17Z

Sorry for late response. I will take a look at this weekend. 「Jeff Hollan <[email protected]>」在 2021年2月23日週二，下午1:55 寫道：

…

Looks like some open feedback / issues, as well as now some conflicts around the base that would need to be resolved. Thanks a ton for your help @ansonauhk <https://github.com/ansonauhk> and this looks pretty close to me. Any interest on clearing out the conflicts so we can ship some of this? Let us know if you have any questions or we can help — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1073 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACC7PP6MWGVGDEIKYUF5XS3TAM7M5ANCNFSM4QU25HWA> .

glennr8686 · 2021-02-24T02:18:21Z

@ansonauhk, thanks for picking this up. We are in the process of developing and testing Azure AKS scaling based on RabbitMQ queue. Our setup has TLS mandatory (actuallt mTLS), so this is blocking us from proceeding.
(part) RabbitMQ config below.
This is running RabbitMQ Cluster Kubernetes Operator.
https://www.rabbitmq.com/kubernetes/operator/using-operator.html

    additionalConfig: |
      ssl_options.fail_if_no_peer_cert = true
      management.ssl.port = 15671
      management.ssl.certfile = /etc/rabbitmq-tls/tls.crt
      management.ssl.keyfile = /etc/rabbitmq-tls/tls.key
      management.ssl.cacertfile = /etc/rabbitmq-tls/ca.crt```

glennr8686 · 2021-03-16T23:18:05Z

@ansonauhk have you had any time to progress this?

zroubalik · 2021-03-26T09:56:12Z

@ansonauhk any update on this PR please?

ccolonri · 2021-06-07T14:24:42Z

Any updates on this PR?

zroubalik · 2021-06-08T08:26:37Z

This PR is stale, I am closing this.

If anybody wants to follow up with this feature, please open another PR.

mcmcghee · 2021-10-01T15:45:37Z

This might have just killed our keda deployment. Surprised that this is not a feature already and that this PR died.

zroubalik · 2021-10-04T08:55:47Z

@mcmcghee a PR with this feature is more then welcome :)

Add SSL support to RabbitMQ Scaler. This fix kedacore#967

b9f3246

Signed-off-by: Anson Au <[email protected]>

ansonauhk requested review from ahmelsayed and zroubalik as code owners September 3, 2020 14:34

zroubalik changed the title ~~Add SSL support to RabbitMQ Scaler. This fix https://github.com/kedac…~~ [v2] Add SSL support to RabbitMQ Scaler Sep 3, 2020

zroubalik reviewed Sep 3, 2020

View reviewed changes

ahmelsayed reviewed Sep 3, 2020

View reviewed changes

ansonauhk added 3 commits September 4, 2020 09:56

Rabbitmq scaler - improve error message

18797f3

Signed-off-by: Anson Au <[email protected]>

Merge remote-tracking branch 'upstream/v2' into v2

b3a236f

Update test for amqps

1203a69

Signed-off-by: Anson Au <[email protected]>

ansonauhk added a commit to ansonauhk/keda that referenced this pull request Sep 6, 2020

Update CHANGELOG.md

3f1978e

Update change log for kedacore#1073 Signed-off-by: Anson Au [email protected]

ansonauhk mentioned this pull request Sep 6, 2020

Update CHANGELOG.md #1091

Closed

4 tasks

zroubalik assigned ahmelsayed Sep 7, 2020

aman-bansal reviewed Oct 16, 2020

View reviewed changes

zroubalik changed the base branch from v2 to main October 30, 2020 13:22

zroubalik closed this Jun 8, 2021

matan-legit mentioned this pull request Aug 1, 2023

Rabbitmq scaler be used with regexes with amqps protocol #4832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] Add SSL support to RabbitMQ Scaler #1073

[v2] Add SSL support to RabbitMQ Scaler #1073

ansonauhk commented Sep 3, 2020 •

edited by zroubalik

Loading

zroubalik left a comment •

edited

Loading

zroubalik commented Sep 3, 2020

ahmelsayed Sep 3, 2020

ahmelsayed Sep 3, 2020

ansonauhk Sep 6, 2020

ansonauhk commented Sep 4, 2020 via email

ansonauhk commented Sep 6, 2020

ansonauhk commented Sep 6, 2020

ahmelsayed commented Sep 9, 2020 •

edited

Loading

ansonauhk commented Sep 11, 2020 via email

ansonauhk commented Sep 16, 2020

zroubalik commented Sep 18, 2020 •

edited

Loading

aman-bansal Oct 16, 2020

aman-bansal Oct 16, 2020 •

edited

Loading

aman-bansal Oct 16, 2020

clarenceb commented Feb 23, 2021

jeffhollan commented Feb 23, 2021

ansonauhk commented Feb 23, 2021 via email

glennr8686 commented Feb 24, 2021 •

edited

Loading

glennr8686 commented Mar 16, 2021

zroubalik commented Mar 26, 2021

ccolonri commented Jun 7, 2021

zroubalik commented Jun 8, 2021

mcmcghee commented Oct 1, 2021

zroubalik commented Oct 4, 2021

[v2] Add SSL support to RabbitMQ Scaler #1073

[v2] Add SSL support to RabbitMQ Scaler #1073

Conversation

ansonauhk commented Sep 3, 2020 • edited by zroubalik Loading

Checklist

zroubalik left a comment • edited Loading

Choose a reason for hiding this comment

zroubalik commented Sep 3, 2020

ahmelsayed Sep 3, 2020

Choose a reason for hiding this comment

ahmelsayed Sep 3, 2020

Choose a reason for hiding this comment

ansonauhk Sep 6, 2020

Choose a reason for hiding this comment

ansonauhk commented Sep 4, 2020 via email

ansonauhk commented Sep 6, 2020

ansonauhk commented Sep 6, 2020

ahmelsayed commented Sep 9, 2020 • edited Loading

ansonauhk commented Sep 11, 2020 via email

ansonauhk commented Sep 16, 2020

zroubalik commented Sep 18, 2020 • edited Loading

aman-bansal Oct 16, 2020

Choose a reason for hiding this comment

aman-bansal Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

aman-bansal Oct 16, 2020

Choose a reason for hiding this comment

clarenceb commented Feb 23, 2021

jeffhollan commented Feb 23, 2021

ansonauhk commented Feb 23, 2021 via email

glennr8686 commented Feb 24, 2021 • edited Loading

glennr8686 commented Mar 16, 2021

zroubalik commented Mar 26, 2021

ccolonri commented Jun 7, 2021

zroubalik commented Jun 8, 2021

mcmcghee commented Oct 1, 2021

zroubalik commented Oct 4, 2021

ansonauhk commented Sep 3, 2020 •

edited by zroubalik

Loading

zroubalik left a comment •

edited

Loading

ahmelsayed commented Sep 9, 2020 •

edited

Loading

zroubalik commented Sep 18, 2020 •

edited

Loading

aman-bansal Oct 16, 2020 •

edited

Loading

glennr8686 commented Feb 24, 2021 •

edited

Loading