Repository navigation
Conversation
knative-prow
Bot
added
size/XXL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4558 +/- ##
==========================================
+ Coverage 30.10% 30.70% +0.60%
==========================================
Files 294 294
Lines 19543 19591 +48
==========================================
+ Hits 5883 6016 +133
+ Misses 13207 13117 -90
- Partials 453 458 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
|
CC @twoGiants |
@qswinson could you clarify this some more? Currently, Knative is managing the rest of the KEDA integration for these resources, so I'm a little hesitant to have this one edge case for the AWS bits around the trigger auth, especially since this would then need to be managed per-trigger by the user Also re: different set of permissions - doesn't Eventing still need permissions to through AWS IAM to authenticate to these clusters anyway? I could be missing something here, so please correct me anywhere you see fit. Also cc @matzew @twoGiants for their thoughts |
There was a problem hiding this comment.
Hi @qswinson!
Great to see you back and thanks again for your new contribution. Keep those contributions coming! 🥇
I have noticed quite big additions in test code and added my comments to it. It would be great if you could address them.
And I think hard coding the awsRegion is not the way to go. I would opt for a generic way of adding trigger metadata key-value pairs. Then its the users responsibility to use awsRegion for the key and us-west-1 for the value. Not sure how though 😆. Maybe something like that autoscaling.eventing.knative.dev/trigger-metadata-awsRegion: "us-west-1". This part autoscaling.eventing.knative.dev/trigger-metadata- is the one that doesn't change and after that the generic one.
And I have one last little request to you. After you finish your re-work I would kindly ask you to fully cleane up the code, proofread it one last time and address all PR review comments before you request the next review. That would be really helpful! Thank you 😃 👍!
Let me know if I can support in any way. You can also find me on the CNCF slack. Happy coding!
There was a problem hiding this comment.
I see you added quite a few tests here to cover the new logic you introduced in config.go. Thank you for taking care of code coverage 😸 👍. Keep adding tests whenever you add new logic.
Now lets dive a bit deeper. Unfortunately the test code does not have the code quality I'd expect. But we will get there, no worries. I can support you, just let me know if something is not clear.
I have a few questions about the tests you wrote:
- Why did you created a this test file here? There was none before because the config is tested implicitly in a different place.
- Do you know where config can be tested implicitly or how to figure that out?
- When you write unit tests for a class or a function always cover those things: conditions (if), loops (for, while), calculations/operations and polymorphism. Now you added a few new default values and extended the existing condition. It would be enough to write a test for each new condition case and one test for the default values. Why didn't you do that?
| ) | ||
|
|
||
| func TestDefaultConfig(t *testing.T) { | ||
| config := defaultConfig() |
There was a problem hiding this comment.
I would avoid package level tests when you can. The rule of thumb is: prefer public api tests over package scoped tests. Add package level tests only if its really needed. Here it isn't.
| } | ||
|
|
||
| // Helper function to check if a string contains a substring | ||
| func contains(s, substr string) bool { |
There was a problem hiding this comment.
Small hint, Go standard library contains this function => strings.Contains. Just use that :)
There was a problem hiding this comment.
Again a big thanks for keeping the test coverage high! I really appreciate it 😸 👍.
This is also lot of code here and I am afraid it does need a bit of re-work. I have a few similar questions as before, it would really help me understand you thought process better if you address them 😸 :
- Why did you create this test file? There was none before which could an indication that it is tested somewhere else.
- Why did you create so many test cases in
TestGenerateScaleTriggerswhich testsGenerateScaleTriggersand that has only a few conditions? - Why on the other hand you created so few test cases in
TestGenerateTriggerAuthenticationwhich testsGenerateTriggerAuthenticationwhich is huge with a ton of conditions? - Why are you writing so many test cases for
SetAutoscalingAnnotationswhich has barely any logic at all?
| // AutoscalingAwsRegion is the annotation that refers to the AWS region of the MSK cluster. | ||
| // KEDA requires this to be set when using AWS IAM authentication as an explicit property | ||
| // in the trigger.metadata section. | ||
| AutoscalingAwsRegion = Autoscaling + "/aws-region" |
| // DefaultLagThreshold is the default value for AutoscalingLagThreshold. | ||
| DefaultLagThreshold = 100 | ||
| // DefaultAuthenticationName is the default value for AutoscalingAuthenticationName. | ||
| DefaultAuthenticationName = "" |
There was a problem hiding this comment.
Empty values for defaults could also be no defaults then.
| AutoscalerAuthentication: map[string]string{ | ||
| AutoscalingAuthenticationName: DefaultAuthenticationName, | ||
| AutoscalingAuthenticationKind: DefaultAuthenticationKind, | ||
| AutoscalingAwsRegion: DefaultAwsRegion, |
There was a problem hiding this comment.
As mentioned above. I would not do this. Probably better a generic way to add trigger metadata key-value pairs.
| AutoscalingLagThreshold: DefaultLagThreshold, | ||
| }, | ||
| AutoscalerAuthentication: map[string]string{ | ||
| AutoscalingAuthenticationName: DefaultAuthenticationName, |
There was a problem hiding this comment.
Default empty means you don't have to set them at all.
| } | ||
|
|
||
| if *awsRegion != "" { | ||
| triggerMetadata["awsRegion"] = *awsRegion |
There was a problem hiding this comment.
Same here. Don't think hard coding awsRegion is a good idea. A more generic way is better.
| return &i32, nil | ||
| } | ||
|
|
||
| func GetStringValueFromMap(dict map[string]string, key string, defaultValue string) (*string, error) { |
There was a problem hiding this comment.
This returns either the defaultValue or an empty string or nil. Why empty string or nil? Does it really need the defaultValue if it's an empty string?
Can you elaborate? Thx! 👍
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qswinson The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This separates the authentication concerns in a platform agnostic manner. Right now KEDA authentication for Kafka must be the same as KNative authentication for Kafka. That makes it difficult to track what api calls are coming from which service regardless of authentication type or Kafka hosting provider. By giving the end user the option to use a separate authentication mechanism for Keda versus KNative, end users who need to track that kind of usage have the tools to do so.
The permission issue comes down to IAM role permission policies and trust relationship policies. The role used by the Keda service account must be granted permission to assume every role that is referenced in a TriggerAuthentication resource and every role referenced in a TriggerAuthentication resource must have a trust relationship policy that grants the Keda service account permission to assume it. If KNative Kafka Controller creates the TriggerAuthentication using the IAM role configured for the broker, then that role has to include the keda service account in its trust relationship. The Keda role must include broker IAM role in its permission policy. This then gives the Keda pod permission to do all IAM permissions granted to the Knative Kafka broker. The diagram below illustrates how defining an external TriggerAuthentication or ClusterTriggerAuthentication separates the tight coupling of the KNative broker role and the Keda role. |
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
Bot
added
lifecycle/stale
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
Bot
added
the
lifecycle/stale
Follow up for #4516 comment
Proposed Changes
New Feature
Add optional KEDA autoscaler configuration to specify externally managed TriggerAuthentication or ClusterTriggerAuthentication resource. These are specified through the
config-kafka-autoscalerConfigMap or annotations on the scaled resource.The annotation names:
autoscaling.eventing.knative.dev/authentication-name: The name of the KEDA resource.autoscaling.eventing.knative.dev/authentication-kind: TriggerAuthentication or ClusterTriggerAuthentication. Only required if the KEDA resource is ClusterTriggerAuthentication.autoscaling.eventing.knative.dev/aws-region: Required if using AWS IAM authentication for MSK. The KEDA Kafka source requires this property to be defined in thetrigger.metadata. KEDA does not recognize this property in the authentication resource as it does with properties likesasl.The primary use case for this feature is to support KEDA autoscaling when using AWS IAM authentication for MSK. This allows a different role to be used for the KEDA operator with a different set of permissions from the KNative Eventing services. By allowing the user to supply an externally managed resource, the complexity of configuring this KEDA authentication is removed from the KNative code and gives more flexibility to the user.
Release Note
Docs
WIP