Add Azure monitor scaler

[melmaliacone]

Fixes #155

Majority of the code in azure_monitor.go taken from azure-k8s-metrics-adapter.

ScaledObject I used for testing authentication from metadata (minus the credentials)

apiVersion: keda.k8s.io/v1alpha1
kind: ScaledObject
metadata:
  name: order-processor-scaler
  labels:
    app: order-processor
    deploymentName: order-processor
spec:
  scaleTargetRef:
    deploymentName: order-processor
  minReplicaCount: 1 # Change to define how many minimum replicas you want
  maxReplicaCount: 10
  triggers:
  - type: azure-monitor
    metadata:
      resourceURI: Microsoft.ContainerService/managedClusters/melconeKeda # Required. URI to the Azure resource
      tenantId: xxxx # Required. Used for authentication
      subscriptionId: xxxx # Required. Used for determining the full resource URI
      resourceGroupName: melconeKeda # Required. Used for determining the full resource URI
      metricName: kube_pod_status_ready # Required. Officially supported metric from https://docs.microsoft.com/en-us/azure/azure-monitor/platform/metrics-supported
      metricFilter: namespace eq 'default' # Optional. Used to define more specific part of the resource. Options for filter listed under the dimensions column in https://docs.microsoft.com/en-us/azure/azure-monitor/platform/metrics-supported
      metricAggregationInterval: '00:15:00' # Optional. Interval over which the metric values should be aggregated and reported
      metricAggregationType: Average # Required. Aggregation type to use for metric based on what is supported https://docs.microsoft.com/en-us/azure/azure-monitor/platform/metrics-supported
      targetValue: '1' # Required. Target threshold for the HPA.
      activeDirectoryClientId: CLIENT_ID_ENV_VAR # Optional. Used when no trigger auth is linked
      activeDirectoryClientPassword: CLIENT_PASSWORD_ENV_VAR # Optional. Used when no trigger auth is linked

Here's the file I used for testing custom metricAggregationInterval and authentication with authParams and resolvedEnv

apiVersion: v1
kind: Secret
metadata:
  name: azure-monitor-secrets
data:
  activeDirectoryClientId: xxx
  activeDirectoryClientPassword: xxx
---
apiVersion: keda.k8s.io/v1alpha1
kind: TriggerAuthentication
metadata: 
  name: azure-monitor-trigger-auth
spec:
  secretTargetRef:
    - parameter: activeDirectoryClientId
      name: azure-monitor-secrets
      key: activeDirectoryClientId
    - parameter: activeDirectoryClientPassword
      name: azure-monitor-secrets
      key: activeDirectoryClientPassword
---
apiVersion: keda.k8s.io/v1alpha1
kind: ScaledObject
metadata:
  name: azure-monitor-scaler
  labels:
    app: azure-monitor-example
    deploymentName: azure-monitor-example
spec:
  scaleTargetRef:
    deploymentName: azure-monitor-example
  minReplicaCount: 1 # Change to define how many minimum replicas you want
  maxReplicaCount: 10
  triggers:
  - type: azure-monitor
    metadata:
      resourceURI: Microsoft.ContainerService/managedClusters/melconeKeda # Required. URI to the Azure resource
      resourceGroupName: melconeKeda # Required. Used for determining the full resource URI
      metricName: kube_pod_status_ready # Required. Officially supported metric from https://docs.microsoft.com/en-us/azure/azure-monitor/platform/metrics-supported
      metricFilter: namespace eq 'default' # Optional. Used to define more specific part of the resource, in this example a queue in SB namespace
      metricAggregationInterval: '1:15:10' # Optional. Interval over which the metric values should be aggregated and reported
      metricAggregationType: Average # Required. Aggregation type to use for metric based on what is supported https://docs.microsoft.com/en-us/azure/azure-monitor/platform/metrics-supported
      targetValue: '1' # Required. Target threshold for the HPA.
    authenticationRef:
      name: azure-monitor-trigger-auth

TODO

• Allow for metricAggregationInterval other than 5 minute default
• Implement authentication with resolvedEnv or authParams
• Add test
• Test more Azure Monitor metrics

[tomkerkhove]

Thanks for your PR!

A few small questions:

• Will tenantId & subscriptionId be part of the parameters that I can configure via TriggerAuth?
• Can we rename the ad prefix to activeDirectory so that folks know what it stands for?
• Last week in the standup you've mentioned that you were going to use the structured approach that was proposed but now it's using a flat list - What made you change?
  • Just asking to understand, it's not an issue. My personal pref is to structure it more to make it easier to use but that's just my opinion so not a blocker.

[iremmats]

From a happy user! :)

Agree with @tomkerkhove about the prefix. It should be aad which is the common abbrevation for Azure Active Directory.

Principle should be Principal.

I would prefer aadClientId and aadClientSecret instead of using Principal.

[melmaliacone]

• @tomkerkhove, I asked about this in standup, but my understanding is that KEDA does not currently support the structured format. I agree with you and prefer that. I think it's much cleaner and easier to read. Jeff Hollan said he was going to open an issue to look into supporting it going forward so we're not forced to use the flat list. Here's the issue Jeff opened.

• I'll change the service principal prefix.

• For the TriggerAuth I plan to have tenantID, subscriptionID, servicePrincipalID & servicePrincipalPassword. I also plan to add podIdentity as well but that will be the last thing I do from my to do list.

• @iremmats, good catch on the principle v. principal. I also agree with you that clientID and clientPassword would be better given that those are usually the terms used when referencing them.

Update: all the above resolved after most recent commit.

[melmaliacone]

@dimberman, @rasavant-ms, @ahmelsayed for review please

[melmaliacone]

@ahmelsayed @dimberman, there are 3 more commits for you all to look at. I ran into some git troubles so ignore the force pushes and base branch changes. Didn't realize that you can't rebase against master mid pull request without those changes showing up.

[tomkerkhove]

• @tomkerkhove, I asked about this in standup, but my understanding is that KEDA does not currently support the structured format. I agree with you and prefer that. I think it's much cleaner and easier to read. Jeff Hollan said he was going to open an issue to look into supporting it going forward so we're not forced to use the flat list. Here's the issue Jeff opened.

Awesome, I presume we'll wait with merging the PR then or?

• I'll change the service principal prefix.
• For the TriggerAuth I plan to have tenantID, subscriptionID, servicePrincipalID & servicePrincipalPassword. I also plan to add podIdentity as well but that will be the last thing I do from my to do list.

Thanks 🙌

[tomkerkhove]

Would you mind opening a PR to https://github.com/kedacore/keda-docs as well please?

[melmaliacone]

Would you mind opening a PR to https://github.com/kedacore/keda-docs as well please?

@tomkerkhove For sure. It's next on my list after I finish my other to dos.

Awesome, I presume we'll wait with merging the PR then or?

I think the plan is not to wait since I suspect (from my initial digging around in the codebase) that the nested format will not be an easy thing to fix. But I'm new to contributing so I could be wrong. I know for sure that there was no talk of waiting on my pr until the nesting format is supported.

[melmaliacone]

@lee0c for review

[melmaliacone]

@ahmelsayed, refactored the code to break it into smaller functions. Couldn't decide whether I should have getAzureMetric as a separate function. I decided to keep it since the result of the request is a float64 rather than an int32. Let me know what you think.

[ahmelsayed]

@melmaliacone the change LGTM. are you still working on the pod identity and tests? a metadata parsing validation test would be great since it's easy to regress those.

Pod identity can be a separate PR, but it's up to you if you're already working on it.

[melmaliacone]

@melmaliacone the change LGTM. are you still working on the pod identity and tests? a metadata parsing validation test would be great since it's easy to regress those.

Pod identity can be a separate PR, but it's up to you if you're already working on it.

I’m close to being done with the test so I think I’ll make pod identity a separate pr so we can get this merged sooner.

[tomkerkhove]

• @tomkerkhove, I asked about this in standup, but my understanding is that KEDA does not currently support the structured format. I agree with you and prefer that. I think it's much cleaner and easier to read. Jeff Hollan said he was going to open an issue to look into supporting it going forward so we're not forced to use the flat list. Here's the issue Jeff opened.

What's the status of #590? If we merge this PR before this is implemented then we already have a breaking change on the trigger spec side which is not ideal.

Personally, I would wait until #590 is resolved.

What do you think @jeffhollan @ahmelsayed?

[ahmelsayed]

I don't really know how a union/sum type could be implemented in go's type system. If this were json marshal/unmarshal, I think we'd need to use a json.RawMessage and a custom unmarshal method, but I have no idea how the controller/client code generation would handle that. Maybe someone more familiar with that might know.

[jeffhollan]

Not positive which comment the union/sum was in response to - but regardless to @tomkerkhove I don't think we should wait for #590 primarily because it's just proposed right now and not even aware if anyone is willing to pick up and size of work it would be. I want it - but I don't think it would make sense to block this scaler on it. Would be a future enhancement if / when #590 is resolved.

EDIT: Ah - TIL about union/sum type. So yes we'd still need to sort out #590 specifics with how metdata works today

[ahmelsayed]

@jeffhollan yes I was referring to me not knowing how to implement #590. sorry for the cryptic reply.

If my understanding is correct, conceptually we want metadata to be one of many possible types based on the scaler or trigger type, which is what I was referring to as a sum/union type. I don't think that can be represented in Go/operator-sdk/kubernetes code-gen (as far as I know at least, but correct me please if it's possible).

I agree with you that we shouldn't block this PR on #590.

[tomkerkhove]

It's ok for me to go ahead but think we'll need #613 over time.

If my understanding is correct, conceptually we want metadata to be one of many possible types based on the scaler or trigger type, which is what I was referring to as a sum/union type. I don't think that can be represented in Go/operator-sdk/kubernetes code-gen (as far as I know at least, but correct me please if it's possible).

Isn't it a matter of how the spec (config) is mapped to internal object? I don't know Go but would surprise me if you can't map two config formats into an internal format?

[melmaliacone]

I got rid of any references to resolvedEnv since I was not actually using it correctly. @ahmelsayed, I know you and I discussed providing support for that. Thoughts? There's also the option of discussing it more and me including it in the podIdentity PR.

[melmaliacone]

All done, but would like to do some more testing with different Azure Monitor metrics. I'll remove the WIP when I'm done.

[melmaliacone]

Added support for resolvedEnv and addressed any other PR comments