Log error when caCert is not empty and no certificates were successfully parsed by AppendCertsFromPEM

[raonitimo]

Proposal

AppendCertsFromPEM attempts to parse a series of PEM encoded certificates and reports whether any certificates were successfully parsed.

We test if caCert is not empty and invoke AppendCertsFromPEM here but we don't check if at least one certificate was successfully parsed.

Use-Case

I was trying to setup an external scaledObject with TLS but the doc is a bit misleading with caCert - Location of a Certificate Authority (CA) certificate to use for the GRPC connection to authenticate with. (Optional).

I was mounting the PEM certificate file in the operator and setting caCert to the path, I didn't get any errors.

Reading the code, I think the caCert expects a string with a PEM formatted certificate. It'd be nice to have an example as well.

Is this a feature you are interested in implementing yourself?

Yes

Anything else?

Happy to raise a PR to the code and the docs as well if it sounds about right.

[JorTurFer]

I think that adding validations is always good (and even more if you are willing to contribute :)).
You are correct, that code expects a PEM certificate but I'm not totally sure if the error is raised there or in this code https://github.com/dttung2905/keda/blob/dbb906ff4d43172d512457a64e60e6ae02875140/pkg/scalers/external_scaler.go#L307-L327
External scaler has 2 approach for providing the CA, the old one in place and the new one via adding the CA in KEDA directly. I have the feeling that the error you sent is related with the old approach (but maybe I'm wrong).
Could you share your ScaledObject?

[raonitimo]

I think that adding validations is always good (and even more if you are willing to contribute :)). You are correct, that code expects a PEM certificate but I'm not totally sure if the error is raised there or in this code dttung2905/keda@dbb906f/pkg/scalers/external_scaler.go#L307-L327

Hi @JorTurFer, thanks for looking into this issue.

IIUC, the code path we're talking about starting from the code you mentioned, is:

NewTLSConfig -> NewTLSConfigWithPassword -> AppendCertsFromPEM.

So, to raise the error here, we'd have to at lease check the return of AppendCertsFromPEM https://github.com/dttung2905/keda/blob/main/pkg/util/tls_config.go#L64 to return an error in NewTLSConfigWithPassword and propagate it.

IMO, it's simpler to just raise the error in NewTLSConfigWithPassword after invoking AppendCertsFromPEM. Similar to what's done for decryptClientKey and X509KeyPair. WDYT?

External scaler has 2 approach for providing the CA, the old one in place and the new one via adding the CA in KEDA directly. I have the feeling that the error you sent is related with the old approach (but maybe I'm wrong). Could you share your ScaledObject?

I'm pretty sure I'm using the new approach.

This was my first attempt for an ScaledObject and the operator didn't log any errors:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  labels:
    scaledobject.keda.sh/name: alb-scaledob-tls
  name: alb-scaledob-tls
spec:
  advanced:
    horizontalPodAutoscalerConfig:
      behavior:
        scaleDown:
          stabilizationWindowSeconds: 600
        scaleUp:
          stabilizationWindowSeconds: 300
  pollingInterval: 10
  scaleTargetRef:
    name: nginx
  triggers:
  - metadata:
      labels: app.canva.k8s/component=nginx
      metricType: albRequestsPerPod
      scalerAddress: alb-scaler.a-alb-scaler.svc.cluster.local:80
      targetValue: "100"
      caCert: "/certs/ca.crt"
    type: external

And no errors are logged. This is what I think it expects:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  labels:
    scaledobject.keda.sh/name: alb-scaledob-tls
  name: alb-scaledob-tls
spec:
  advanced:
    horizontalPodAutoscalerConfig:
      behavior:
        scaleDown:
          stabilizationWindowSeconds: 600
        scaleUp:
          stabilizationWindowSeconds: 300
  pollingInterval: 10
  scaleTargetRef:
    name: nginx
  triggers:
  - metadata:
      caCert: |
         -----BEGIN CERTIFICATE-----
         MIIC/TCCAeWgAwIBAgIRALXx9KbECqvXUuRVUYKp3VIwDQYJKoZIhvcNAQELBQAw
         GDEWMBQGA1UEAxMNa2VkYS1vcGVyYXRvcjAeFw0yMzEwMTEwODQwMjJaFw0yNDEw
         MTAwODQwMjJaMBgxFjAUBgNVBAMTDWtlZGEtb3BlcmF0b3IwggEiMA0GCSqGSIb3
         DQEBAQUAA4IBDwAwggEKAoIBAQCZf8LTNNBIIZCK1WkREhgtvQ3xJ2FSeqp9Po2m
         xldrWWp3bxfSl6zFcvuI0/bCQf8JsyJxY40Zot3cccsSP6SEPyzY6WKxr73ZkWb2
         gY8qRyp3or1Gqs7W/apZFfoWUrM49ZHWfpgrEDXIlrBx9VOSfoWrpV1bGFbZ82LG
         SNtqgNx5LC8ZH13PWeK1V647ApGPlX+SRwshjUPNRl7j8mKfipioczwdsw7nRINt
         OMjUpdyJcUW1+xcwoVjD9f+ueJQfiSAGvtXMrWYAWVsdgcYEIpyBXr3sW8GJxmNa
         kQ5Swh8JOPAzrkw6rtWuFMB4oNN/NaWb+aPkGBi+7qHp+hMBAgMBAAGjQjBAMA4G
         A1UdDwEB/wQEAwICpDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSe12Dmh8Qi
         6QQlNFvmIN67a7cInDANBgkqhkiG9w0BAQsFAAOCAQEAWG1McULhBeuga3ym8pm2
         IZrz7Yy5DNP7KBwdafpxAswvqII61LJ6Yysxaq7wYn6wiN7I+EBr2W91QUF8ZUBv
         4Tej6um4kWXcB1SXHku7N8GcXpuZ8q4q4ObLLwEeikQmrcbDr2ho2iIcMZDQFndc
         qhcknMvc9X78IYp5upxDTPx0RmSY3TqN6bVEadmHpO7z7u7vazhBYyOaP8rxrJmY
         F2+e07eJXzkLwfBwlpn14XvvMlnLiFFvgGFnfn58C+NNL3gKeBgAa2vIUYc07pzL
         24LJ+demV+g/JybKpL8Cdx9gXjhxqL9Pz9bf9LNeq4x7JJtP541jfwzHrgHYFJUM
         AQ==
         -----END CERTIFICATE-----
      metricType: albRequestsPerPod
      scalerAddress: alb-scaler.a-alb-scaler.svc.cluster.local:80
      targetValue: "100"
    type: external

Or maybe even:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  labels:
    scaledobject.keda.sh/name: alb-scaledob-tls
  name: alb-scaledob-tls
spec:
  advanced:
    horizontalPodAutoscalerConfig:
      behavior:
        scaleDown:
          stabilizationWindowSeconds: 600
        scaleUp:
          stabilizationWindowSeconds: 300
  pollingInterval: 10
  scaleTargetRef:
    name: nginx
  triggers:
  - metadata:
      caCert: "MIIC/TCCAeWgAwIBAgIRALXx9KbECqvXUuRVUYKp3VIwDQYJKoZIhvcNAQELBQAwGDEWMBQGA1UEAxMNa2VkYS1vcGVyYXRvcjAeFw0yMzEwMTEwODQwMjJaFw0yNDEwMTAwODQwMjJaMBgxFjAUBgNVBAMTDWtlZGEtb3BlcmF0b3IwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCZf8LTNNBIIZCK1WkREhgtvQ3xJ2FSeqp9Po2mxldrWWp3bxfSl6zFcvuI0/bCQf8JsyJxY40Zot3cccsSP6SEPyzY6WKxr73ZkWb2gY8qRyp3or1Gqs7W/apZFfoWUrM49ZHWfpgrEDXIlrBx9VOSfoWrpV1bGFbZ82LGSNtqgNx5LC8ZH13PWeK1V647ApGPlX+..."
      metricType: albRequestsPerPod
      scalerAddress: alb-scaler.a-alb-scaler.svc.cluster.local:80
      targetValue: "100"
    type: external

[JorTurFer]

I have checked the code and it looks like documentation is outdated because certificate suff are recovered from TriggerAuthentication and not from trigger metadata, that's why you can't get them (I'll update it later on).
As you are only setting the `caCert, you have 2 options to do it:

• Locally with TriggerAuthentication
• Globally for all KEDA scalers

For doing it locally, you need to define a secret and use them via TriggerAuthentication:

apiVersion: v1
kind: Secret
metadata:
  name: certificate
data:
  ca: "YOUR_CA_IN_SECRET"
---
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: keda-trigger-auth
spec:
  secretTargetRef:
  - parameter: caCert
    name: certificate
    key: ca
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  labels:
    scaledobject.keda.sh/name: alb-scaledob-tls
  name: alb-scaledob-tls
spec:
  advanced:
    horizontalPodAutoscalerConfig:
      behavior:
        scaleDown:
          stabilizationWindowSeconds: 600
        scaleUp:
          stabilizationWindowSeconds: 300
  pollingInterval: 10
  scaleTargetRef:
    name: nginx
  triggers:
  - metadata:      
      metricType: albRequestsPerPod
      scalerAddress: alb-scaler.a-alb-scaler.svc.cluster.local:80
      targetValue: "100"
    type: external
    authenticationRef:
      name: keda-trigger-auth

For adding it globally, you have to mount all the CA that you want under the path /custom/ca within KEDA operator pod

[JorTurFer]

This is the fix to the docs BTW: kedacore/keda-docs#1248

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[zroubalik]

@JorTurFer can we close this?

[JorTurFer]

Yeah, I think so as there isn't any activity from the OP