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

Allow properties from ConfigMap in TriggerAuthentication #4830

Closed
novicr opened this issue Jul 29, 2023 · 17 comments · Fixed by #5111
Closed

Allow properties from ConfigMap in TriggerAuthentication #4830

novicr opened this issue Jul 29, 2023 · 17 comments · Fixed by #5111
Assignees

Comments

@novicr
Copy link
Contributor

novicr commented Jul 29, 2023

Currently TriggerAuthentication allows specifying properties from multiple sources including Secrets and Environment. In some cases information like username is already maintained in ConfigMap's. Would be useful to allow specifying properties defined in ConfigMap when defining TriggerAuthentication

@JorTurFer
Copy link
Member

Hello,
In one hand, I'm not 100% sure if we should allow this option because it could produce that users store secrets in cofigMaps (at least now KEDA doesn't allow it).
In the other hand, things like username aren't sensitive so if we read them from TriggerAuthentication, supporting configMaps sounds reasonable.
WDTY @kedacore/keda-core-contributors ?

@novicr
Copy link
Contributor Author

novicr commented Aug 2, 2023

Note, trigger authentication also allows settings like oauthTokenEndpointUri + scope. These are not secrets and are likely to already be present in application configmap's. Current requirement means that people have to copy them into a Secret object.

@trzejos
Copy link

trzejos commented Aug 9, 2023

Other operators out there like trust-manager generate ConfigMaps rather than Secrets, and it should be preferable to reference the configmap directly rather than having to sync it to a secret. This can be useful for setting the ca parameter on the kafka scaler for example.

@tomkerkhove
Copy link
Member

I think this is a reasonable thing to add and wonder how this did not come up earlier.

Even though it's not secure, it still has valid use-cases and storing secrets in environment variables is not secure either and still we support it. I'm in favor of this.

@stale
Copy link

stale bot commented Oct 9, 2023

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 Oct 9, 2023
@novicr
Copy link
Contributor Author

novicr commented Oct 10, 2023

This issue went stale. Seems there's agreement that allowing auth properties via ConfigMap would be useful and appropriate. Could this be picked up please?

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Oct 10, 2023
@JorTurFer
Copy link
Member

I'm still not sure if we should allow this, but I don't have any strong opinion, if someone is willing to add it, it's fine to me

@zroubalik
Copy link
Member

I agree, Secret is not more secure than ConfigMap. Let's do this.

@tomkerkhove
Copy link
Member

@novicr Are you willing to contribute it?

@wozniakjan
Copy link
Member

hey @novicr I'd be happy to help, let me know if you intend to tackle the implementation as well or prefer to hand it over.

@tomkerkhove wrt implementation, what do you envision as desired API change, just adding array of ConfigMapTargetRef below SecretTargetRef?

// +optional
SecretTargetRef []AuthSecretTargetRef `json:"secretTargetRef,omitempty"`

Or more generic with corev1.ObjectReference with jsonpath so people can use arbitrary CRDs in the future?

@tomkerkhove
Copy link
Member

I'd personally go with ConfigMapTargetRef to have an easy to use API

@novicr
Copy link
Contributor Author

novicr commented Oct 16, 2023

Thanks for confirming. Unfortunately I probably won't be able to pick this up for couple of months. If someone is willing to take it, would be great. Otherwise, I'll take it when I can.

@wozniakjan
Copy link
Member

If someone is willing to take it, would be great.

in that case, will try to take a look at it this weekend
cc: @zroubalik

@Sathya-omnius
Copy link

Sathya-omnius commented Mar 6, 2024

Hi Team,
we have a config-map created in templates , the Rabbitmq queue name is present in this configmap, can some one guide me how to access this queue name in Rabbitmq keda triggers.
apiVersion: v1
kind: ConfigMap
metadata:
name: amqp-settings
data:
max_priority: "1"
prediction_postprocess_result_queue: xyz

I tried using this from Keda documentation but the below code is throwing the exception
configMapTargetRef: # Optional.

  • parameter: connectionString # Required - Defined by the scale trigger
    name: my-keda-configmap-resource-name # Required.
    key: azure-storage-connectionstring # Required.

can some one help how to consume the configmap data in to keda config trigers.

@wozniakjan
Copy link
Member

I am not sure I fully understand your issue but I think you should configure the RabbitMQ queue name directly in the ScaledObject as described in https://keda.sh/docs/2.13/scalers/rabbitmq-queue/. This particular PR just allows setting authentication parameters in a ConfigMap for a certain TriggerAuthentication.

@Sathya-omnius
Copy link

HI,
Thanks for the reply, yes we can directly configure the queuename in scaled object but in our scenario the queue name is dynamic and will be present in configmap so I need to read the queuename from configmap and assign the value.

@wozniakjan
Copy link
Member

I don't think setting other than authentication parameters for TriggerAuthentication from ConfigMap is supported at the moment. But can you configure your automation that changes dynamically the ConfigMap to change the ScaledObject directly too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants