feature[v2]: enabling authentication for metric api scaler #1137
Conversation
aman-bansal
force-pushed
the
metric_scaler_auth
branch
from
308ce84 to
2805ffc
Compare
|
@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. |
|
@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 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. |
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. |
@zroubalik I agree that e2e would be awesome. Do we have any documentation on how we approach such testing?
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
force-pushed
the
metric_scaler_auth
branch
from
56539f9 to
8abe00d
Compare
|
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 |
pkg/scalers/metrics_api_scaler.go
Outdated
| apiKeyAuth authenticationType = "apiKeyAuth" | ||
| basicAuth = "basicAuth" | ||
| tlsAuth = "tlsAuth" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thank you so much @turbaszek. Great learning. Will be more careful with future PRs. :) |
| caCertPool := x509.NewCertPool() | ||
| caCertPool.AppendCertsFromPEM([]byte(caCert)) | ||
| config.RootCAs = caCertPool | ||
| config.InsecureSkipVerify = true |
There was a problem hiding this comment.
According to code scanning
InsecureSkipVerify should not be used in production code.
@tomkerkhove how should we address this feedback?
There was a problem hiding this comment.
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))
...There was a problem hiding this comment.
@aman-bansal what do you think?
There was a problem hiding this comment.
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.
|
@tomkerkhove @zroubalik is there any blocker to merging this PR other than rebasing it? |
|
I'll leave it up to @ahmelsayed @zroubalik for reviewing this |
|
@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 :). |
|
@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 |
Signed-off-by: aman-bansal <[email protected]>
Signed-off-by: aman-bansal <[email protected]>
Signed-off-by: aman-bansal <[email protected]>
Signed-off-by: aman-bansal <[email protected]>
aman-bansal
force-pushed
the
metric_scaler_auth
branch
2 times, most recently
from
9cf397d to
06d18f1
Compare
Signed-off-by: aman-bansal <[email protected]>
aman-bansal
force-pushed
the
metric_scaler_auth
branch
from
06d18f1 to
0e4583a
Compare
|
@zroubalik @ahmelsayed Merge conflicts have been resolved. |
Signed-off-by: aman-bansal <[email protected]>
Signed-off-by: Zbynek Roubalik <[email protected]>
Signed-off-by: aman-bansal [email protected]
This PR is to add authentication for metric_api_scaler. Three methods have been added. They will be identified on the basis of
authModeparameter.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.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.Client Certification: auth mode would
tlsAuth. cert, cacert and key needs to be provided.Checklist
Relates to #1082 #1083 #1084
[Edit]
Docs PR attached kedacore/keda-docs#260