feature[v2]: enabling authentication for metric api scaler

[aman-bansal]

Signed-off-by: aman-bansal bansalaman2905@gmail.com

This PR is to add authentication for metric_api_scaler. Three methods have been added. They will be identified on the basis of authMode parameter.

1. API Key Based auth: auth mode would apiKeyAuth. API key needs to be provided. Default behaviour would be to send it in headers. This can be changed via method and header/query param keys can be changed via keyParamName.

2. Basic Auth: auth mode would basicAuth. Username is compulsory. Password isn't. This is based on experience over basic auth. Some people just put api key as the username to ease out authentication at their end.

3. Client Certification: auth mode would tlsAuth. cert, cacert and key needs to be provided.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Tests have been added
• A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
• Changelog has been updated

Relates to #1082 #1083 #1084

[Edit]
Docs PR attached kedacore/keda-docs#260

[aman-bansal]

@tomkerkhove please review this PR and do suggest if I missed something. This will enable metric api scaler to authentication via api key, basic auth, and client certificates. In the meantime, I am working on docs.

[zroubalik]

Thanks, minor nits

[turbaszek]

@aman-bansal have you tested it locally? If not, I can help over the weekend or you may find this useful: turbaszek/keda-example#1 (sorry for no docs)

[zroubalik]

@aman-bansal have you tested it locally? If not, I can help over the weekend or you may find this useful: turbaszek/keda-example#1 (sorry for no docs)

We should have e2e test for this scaler probably

[aman-bansal]

@aman-bansal have you tested it locally? If not, I can help over the weekend or you may find this useful: turbaszek/keda-example#1 (sorry for no docs)

This is great! I was actually looking for ways to test this. I'll try to create one sample for each authorization. If there is any doubt I will raise in this thread itself.

[aman-bansal]

@aman-bansal have you tested it locally? If not, I can help over the weekend or you may find this useful: turbaszek/keda-example#1 (sorry for no docs)

We should have e2e test for this scaler probably

This will be tricky. We have to add one sample app which can provide support for each authorization method. Will need a bit help in this case.

[turbaszek]

We should have e2e test for this scaler probably

@zroubalik I agree that e2e would be awesome. Do we have any documentation on how we approach such testing?

This will be tricky. We have to add one sample app which can provide support for each authorization method. Will need a bit help in this case.

I would say that one app with 4 endpoints, each with different auth methods should serve the purpose. And I'm happy to help with doing this :)

[zroubalik]

We should have e2e test for this scaler probably

@zroubalik I agree that e2e would be awesome. Do we have any documentation on how we approach such testing?

This will be tricky. We have to add one sample app which can provide support for each authorization method. Will need a bit help in this case.

I would say that one app with 4 endpoints, each with different auth methods should serve the purpose. And I'm happy to help with doing this :)

Thanks! Great, it can definititely be a separate PR :)

[aman-bansal]

Took time but finally, build HTTP authentication example, I will add the documentation in some time. But do check if you get the time. https://github.com/aman-bansal/keda-http-auth-example

[turbaszek]

I'm not 100% but I think we should add authenticationType to two other methods, otherwise go will resolve the type from assigned value as string?

[aman-bansal]

oh didn't know that automatic type assignment works for zero indexed type declaration i.e. with iota. In testing, this didn't get caught because of its string representation.

[turbaszek]

Few nits but otherwise it looks good to me! 👏

[aman-bansal]

Few nits but otherwise it looks good to me! 👏

Thank you so much @turbaszek. Great learning. Will be more careful with future PRs. :)

[turbaszek]

According to code scanning

InsecureSkipVerify should not be used in production code.

@tomkerkhove how should we address this feedback?

[ahmelsayed]

I'm not sure why we need to skip verification if we're setting the RootCAs on the config. I haven't tried it, but I'd think it shouldn't be needed. Maybe we just need to append the system ca certs first

caCertPool, err := x509.SystemCertPool()
if err != nil {
    ....
}
caCertPool.AppendCertsFromPEM([]byte(caCert))
...

source

[turbaszek]

@aman-bansal what do you think?

[aman-bansal]

Hey, I am back finally.
So I believe we shouldn't use this because keda when running in the production systems this could lead to issues. But should we make this configurable?

Secondly, I believe the system ca cert is not required for this. We need system ca cert only if using self-signed cert. I believe we should let the clients specify if they need this via configuration. Because some clients might need this.

[zroubalik]

@aman-bansal I agree

[turbaszek]

@tomkerkhove @zroubalik is there any blocker to merging this PR other than rebasing it?

[tomkerkhove]

I'll leave it up to @ahmelsayed @zroubalik for reviewing this

[aman-bansal]

@ahmelsayed @zroubalik can we have a final review over this. I am back in 100% capacity and would love to complete this so that I can pick something next :).

[zroubalik]

@aman-bansal could you please rebase this PR, there's a conflict. I think that we are good to go with the merge and then we can address the config.InsecureSkipVerify = true problem in a separate issue.

[aman-bansal]

@zroubalik @ahmelsayed Merge conflicts have been resolved.

[zroubalik]

LGTM thanks @aman-bansal !