-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: add file-based authentication support for ClusterTriggerAuthentication #7082
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
104a39e to
690655c
Compare
|
/run-e2e file_based_auth_test |
|
in a quick look I do see errors in the e2e: ERROR Failed to get TriggerAuthentication "controller": "scaledobject", "controllerGroup": "keda.sh", "controllerKind": "ScaledObject", "ScaledObject": "name":"file-based-auth-test-so","namespace":"file-based-auth-test-ns", "namespace": "file-based-auth-test-ns", "name": "file-based-auth-test-so", "reconcileID": "0d8eea8-5d05-4d***-be5f-0ed760b04a4b", "error": "TriggerAuthentication.keda.sh "file-auth" not found"*** |
Thanks for taking a look! I just pushed a fix in d80bb2e and was able to run it successfully locally now. |
|
/run-e2e file_based_auth_test |
|
Could you take a look at the test log? Because it looks like that the tests not have been executed. |
|
Hello |
|
sorry, I closed the PR by mistake clicking on comment button 🤦 |
Thanks @rickbrouwer I see the tests executed successfully here. Isn't that the correct log?
@JorTurFer Thank you for your feedback and for sharing your concerns about storing credentials on disk. To address the bits about needing pod restarts when credentials on disk change: in Kubernetes, most volume types support dynamic updates without requiring a restart. For my use case with The proposed feature lets us avoid forking KEDA as our solution is proprietary, hence there is no easy way to add a new config source. Forking KEDA would also introduce long-term challenges for us with compatibility and maintainability as it'll likely require backporting upstream changes, so we’re aiming for a solution that aligns with the ecosystem. Our use case with Grafana involves long-lived credentials across multiple clouds, so frequent secret changes aren’t a primary concern. However, the proposed filesystem-based approach still supports dynamic updates if needed (e.g., via external tools updating I’d love to hear your thoughts on this approach, thanks again for your time and for maintaining KEDA! |
|
@rickbrouwer @JorTurFer could you please take another look here? Thank you! |
5884a88 to
ffc2afb
Compare
|
Hi, In the other hand, I agree with the point of not building your custom KEDA's flavor just to support your secrets source and I'm wondering if it's worth for KEDA supporting a gRPC interface to query secrets to "external" secret sources (like we do with external scaler but for secrets), this could bring the option to implement just a proxy between the secrets solution and KEDA implementing the gRPC interface 🤔 @kedacore/keda-core-contributors @kedacore/keda-core-maintainers |
Thank you @JorTurFer for the discussion, I can see how the provided example of using @kedacore/keda-core-contributors @kedacore/keda-core-maintainers could you please chime in here? 🙇 |
|
What about passing the path as argument during startup? This can be useful to change it in the future and makes less relevant the current path that we decide because users can choose it for themselves.
The point is that ClusterTriggerAuthentication uses the same API as TriggerAuthentication (and makese sense) , so you'd need to limit that API by RBAC too or the exposition vector is there. Thoughts @kedacore/keda-core-contributors @kedacore/keda-core-maintainers? |
I can add that in if that's the mechanism you feel comfortable with, let me know when consensus is reached with the rest of the maintainers and I'll add it.
https://github.com/kedacore/keda/pull/7082/files#diff-8c546d2eb95a297e897b699d6103324875de92d89c7e2519182c011c68317b57R247-R250 intentionally limits the
Friendly nudge @kedacore/keda-core-contributors @kedacore/keda-core-maintainers 🙇 |
That's a good point, IMHO we should avoid shipping this only for clustertrigger, this is an auth method that can be leveraged using both apis |
Apologies if this wasn't clear from the beginning but I don't think this functionality should be extended to the namespaced Here's a diagram to illustrate: flowchart TD
subgraph "User Namespace (team-a)"
TA[TriggerAuthentication]
FileUser[File: /creds/team-a.json]
TA --> FileUser
end
subgraph "KEDA Namespace (keda)"
OP[KEDA Operator Pod]
Vol[emptyDir Volume]
FileKeda[File: /keda/creds/shared.json]
OP --> Vol --> FileKeda
end
FileUser -->|Cross-namespace mount?| No[NOT POSSIBLE]
TA -.->|Sync to keda NS?| No
OP -->|Reads file directly| FileKeda
style No fill:#fee,stroke:#f66,stroke-width:2px
There's a few caveats that make using
Can we please merge this as-is? Path whitelisting via startup arg can come in now (if agreed upon) or as a follow-up but I have been rebasing this PR for over a month and would like to be able to set timeline expectations with stakeholders internally. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, but as currently implemented by the PR, this will allow anyone who can create ScaledObject to gain root access to the whole KEDA instance. That is not something we can merge because users expect a certain level of isolation and they depend on it. Telling KEDA users that if they want to upgrade, they need OPA with new rules is just not desirable.
But I think with a few tweaks this feature might be mergeable:
- KEDA will need a new command-line argument, which could be called
filepath-auth-root-path. ThefilePathconfigured inClusterTriggerAuthenticationwill be treated as a relative path under thefilepath-auth-root-paththat is ensured to always expand under thefilepath-auth-root-path - The API should not be in
ScaledObjectorScaledJobbut instead inClusterTriggerAuthentication/TriggerAuthentication.
Thank you @wozniakjan, I incorporated your feedback. Let me know if it needs further adjustments. Could you please trigger the e2e tests? |
|
/run-e2e file_based_auth_test |
wozniakjan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great!
e2e test related nit below, but maybe first wait for other @kedacore/keda-maintainers to chime in
| } | ||
|
|
||
| // Update the deployment | ||
| _, err = kc.AppsV1().Deployments("keda").Update(context.Background(), operatorDeployment, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho this could part of the KEDA e2e test setup to avoid KEDA restarts during executing e2e tests, wdyt @kedacore/keda-maintainers ?
keda/tests/utils/setup_test.go
Lines 252 to 253 in 76812bc
| out, err := ExecuteCommandWithDirAndEnv("make deploy", "../..", envVars) | |
| require.NoErrorf(t, err, "error deploying KEDA - %s", err) |
keda/config/e2e/patch_operator.yml
Lines 1 to 15 in 76812bc
| - op: add | |
| path: /spec/template/spec/containers/0/volumeMounts/1 | |
| value: | |
| name: custom-cas | |
| mountPath: /custom/ca | |
| readOnly: true | |
| - op: add | |
| path: /spec/template/spec/volumes/1 | |
| value: | |
| name: custom-cas | |
| secret: | |
| defaultMode: 420 | |
| secretName: custom-cas | |
| optional: true |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Thanks @wozniakjan, I just rebased again to resolve conflicts. Did you receive any feedback from @kedacore/keda-core-maintainers? Happy to make adjustments as needed, thanks! |
…ication We're using a custom secret solution that decrypts credentials via an init container and writes the resulting credentials to a shared emptyDir. The current `AuthenticationRef` is limiting as it forces us to store the credential in a `Secret`/`ConfigMap` instead and we'd like to avoid doing that due to the fact that we cannot use these safely in our environment. Happy to discuss viable alternatives but for now the implementation looks as follows: • Add optional FilePath field to AuthenticationRef for reading auth params from mounted files • Implement readAuthParamsFromFile helper to parse JSON credentials • Modify resolveAuthRef to handle file-based auth when FilePath is specified • Add comprehensive tests for new functionality • Maintain backward compatibility with existing authentication methods Fix kedacore#7083 Signed-off-by: Jonas-Taha El Sesiy <[email protected]>
Signed-off-by: Jonas-Taha El Sesiy <[email protected]>
Friendly nudge @wozniakjan, thank you! |
I don't have any new feedback, I think this approach should be safe and not bypass any RBAC or security guarantees from KEDA. I'd personally not patch the deployment of KEDA operator mid e2e tests (as mentioned here) and rather set the path when KEDA is deployed. But at least one more maintainer has to review this, otherwise it won't be merged. |
We're using a custom secret solution that decrypts credentials via an init container and writes the resulting credentials to a shared emptyDir. The current
AuthenticationRefis limiting as it forces us to store the credential in aSecret/ConfigMapinstead and we'd like to avoid doing that due to the fact that we cannot use these safely in our environment.Happy to discuss viable alternatives but for now the implementation looks as follows:
• Add optional FilePath field to AuthenticationRef for reading auth params from mounted files
• Implement readAuthParamsFromFile helper to parse JSON credentials
• Modify resolveAuthRef to handle file-based auth when FilePath is specified
• Add tests for new functionality
• Maintain backward compatibility with existing authentication methods
For namespaced
TriggerAuthentication, users will need to use existingconfigMapTargetReforsecretTargetRefas file-mounting across namespaces (outside of the operator namespace that is) is impractical and provides no benefit over existing functionality.Checklist
Fixes #7083
Relates to #
Note: I will open a docs PR shortly after a KEDA member confirms that they are accepting this change.