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

feature[v2]: adding documentation regarding authentication in metric scalers #260

Merged
merged 3 commits into from
Oct 15, 2020

Conversation

aman-bansal
Copy link
Contributor

@aman-bansal aman-bansal commented Sep 15, 2020

This is related to kedacore/keda#1137 . Authentication methods for metric scaler api has been added. Currently supported authModes are API key-based authentication, Basic Authentication and TLS authentication.

Relates to kedacore/keda#1082
Relates to kedacore/keda#1083
Relates to kedacore/keda#1084

Signed-off-by: aman-bansal [email protected]

@tomkerkhove
Copy link
Member

I'll review it ASAP but can you update the trigger spec as well please?
https://github.com/kedacore/keda-docs/pull/260/files#diff-2079ef1d5bc7dbe2c2470f43d8749c87R18

@aman-bansal
Copy link
Contributor Author

I'll review it ASAP but can you update the trigger spec as well please?
https://github.com/kedacore/keda-docs/pull/260/files#diff-2079ef1d5bc7dbe2c2470f43d8749c87R18

Hey didn't get this part? Did it miss something in the documentation?

@tomkerkhove
Copy link
Member

I've missed the implementation side of things but presume we do not only support authentication parameters but define it in https://github.com/kedacore/keda-docs/pull/260/files#diff-2079ef1d5bc7dbe2c2470f43d8749c87R19-R24 as well by using FromEnv for secrets and literal values such as authMode?

content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Show resolved Hide resolved
content/docs/2.0/scalers/metrics-api.md Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Just as an example @aman-bansal:
We currently have this sample:

apiVersion: v1
kind: Secret
metadata:
  name: keda-metric-api-secret
  namespace: default
data:
  authMode: "apiKeyAuth"
  apiKey: "APIKEY" 
  method: "query"
  keyParamName: "QUERY_KEY"
---
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: keda-metric-api-creds
  namespace: default
spec:
  secretTargetRef:
    - parameter: authMode
      name: keda-metric-api-secret
      key: authMode
    - parameter: apiKey
      name: keda-metric-api-secret
      key: apiKey
    - parameter: method
      name: keda-metric-api-secret
      key: method
    - parameter: keyParamName
      name: keda-metric-api-secret
      key: keyParamName
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: http-scaledobject
  namespace: keda
  labels:
    deploymentName: dummy
spec:
  maxReplicaCount: 12
  scaleTargetRef:
    name: dummy
  triggers:
    - type: metrics-api
      metadata:
        targetValue: "7"
        url: "http://api:3232/components/stats"
        valueLocation: 'components.worker.tasks'
      authenticationRef:
        name: keda-metric-api-creds

But I'd expect to be able to define it like this:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: http-scaledobject
  namespace: keda
  labels:
    deploymentName: dummy
spec:
  maxReplicaCount: 12
  scaleTargetRef:
    name: dummy
  triggers:
    - type: metrics-api
      metadata:
        targetValue: "7"
        url: "http://api:3232/components/stats"
        valueLocation: 'components.worker.tasks'
        authMode: apiKeyAuth
        method: query
        keyParamName: QUERY_KEY
      authenticationRef:
        name: keda-metric-api-creds

@aman-bansal
Copy link
Contributor Author

Guys I am on leave for two weeks. Will be back next week. Will continue with this after that :)

@zroubalik
Copy link
Member

@aman-bansal could you please update and rebase this PR? thanks!

@aman-bansal
Copy link
Contributor Author

metadata:
        targetValue: "7"
        url: "http://api:3232/components/stats"
        valueLocation: 'components.worker.tasks'
        authMode: apiKeyAuth
        method: query
        keyParamName: QUERY_KEY

Okay, so what I understand from this is only critical security params like username or password should be passed via auth params. Other parameters should use basic metadata map.

@tomkerkhove is my understanding correct?

@tomkerkhove
Copy link
Member

metadata:
        targetValue: "7"
        url: "http://api:3232/components/stats"
        valueLocation: 'components.worker.tasks'
        authMode: apiKeyAuth
        method: query
        keyParamName: QUERY_KEY

Okay, so what I understand from this is only critical security params like username or password should be passed via auth params. Other parameters should use basic metadata map.

@tomkerkhove is my understanding correct?

Yes, TriggerAuthentication is only for secrets

@aman-bansal
Copy link
Contributor Author

@tomkerkhove I have updated the docs. Now, these changes require an update of the scaler itself. Here is the PR for that kedacore/keda#1258

@zroubalik
Copy link
Member

@tomkerkhove I have updated the docs. Now, these changes require an update of the scaler itself. Here is the PR for that kedacore/keda#1258

The PR is merged, I'll keep this PR for Tom's review.

Thanks @aman-bansal !

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tomkerkhove tomkerkhove merged commit 27d6940 into kedacore:master Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants