Allow per-query tenant configuration #85
Conversation
Feat. hawkular#62 Notes: - There's a limitation with templating vars. I'm not sure that grafana allows to edit that screen layout. A solution for later would be to somehow integrate the tenant in the query string, but that requires a little more reflection, so I'll open another ticket for that - In the same time I've added the ability to fetch availability metrics in annotations (very little work was needed)
|
"Don't merge yet" => I must add unit tests first |
also some refactoring in tests for easier/more readable assertions on requests
|
I've done some refactoring on tests, splitting the huge "datasource_spec" into several files. Maybe it's going to be easier to review commit by commit |
src/datasource.js
Outdated
| // If tenants is unknown at this point (when having per-query tenants) | ||
| // We do a more basic check to status endpoint | ||
| // Else, it's full connectivity with tenant check | ||
| const endpoint = this.isTenantPerQuery ? '/status' : '/metrics'; |
There was a problem hiding this comment.
The thing is that the status endpoint might be reachable even without the proper credentials
There was a problem hiding this comment.
You're right, but I can't see other way round. In openshift for example authentication is closely linked to the tenant, so it cannot be checked anyway at this step.
|
Then maybe a note should indicate that testing the datasource does not
validate credentials when tenant is not set.
2017-07-25 17:42 GMT+02:00 Joel Takvorian <notifications@github.com>:
… ***@***.**** commented on this pull request.
------------------------------
In src/datasource.js
<#85 (comment)>
:
> @@ -74,31 +89,35 @@ export class HawkularDatasource {
}
testDatasource() {
+ // If tenants is unknown at this point (when having per-query tenants)
+ // We do a more basic check to status endpoint
+ // Else, it's full connectivity with tenant check
+ const endpoint = this.isTenantPerQuery ? '/status' : '/metrics';
You're right, but I can't see other way round. In openshift for example
authentication is closely linked to the tenant, so it cannot be checked
anyway at this step.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#85 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABbltuS7dmuq9rlVW7K6HVb39HdBxAz4ks5sRgzsgaJpZM4OhUqO>
.
|
Unlike /status this endpoint requires auth in basic-auth mode, though not with openshift (special message is then displayed)
|
@tsegismont I think I've found a good compromise, testing on "/" endpoint is better than "/status" as I noticed that in this case authentication is required in basic-auth mode. |
|
Have you checked what happens when tenant is query specific and authentication is misconfigured? Is the error message displayed on the graph top-left corner informative? |
|
It's informative... as long as the server provides informative response. If I put a valid oauth token but coupled with unauthorized tenant, it shows a 403 forbidden as expected. However if I put an invalid oauth token, I get a 500 internal server error, which is not very meaningful. It seems to be an Hawkular issue, because I don't see the same behaviour when reaching OS rest api For example, Anyway this is a server-side issue. I'll investigate more on that but I think it shouldn't block this PR. |
|
PS: the error is displayed in the upper left corner as you expect |
|
Alright. I don't have any other comment. 👍 |
|
Thanks, by the way JIRA has been filed for the error 500: https://issues.jboss.org/browse/HWKMETRICS-709 |
|
when do you plan to release this one so we can pull it through docker.io ? |
|
@ljuaneda sounds like a good task for a friday afternoon |
|
Done: https://hub.docker.com/r/hawkular/hawkular-grafana-datasource/tags/ |
|
Ok thanks, I'll a quick test and let you know if there is any pb |
|
hi how do put tenant (ocp projects) into templating ? |
|
It doesnt seem possible to set the tenant from a variable... |
|
@alekonkoisp @ljuaneda no, you cannot put a tenant into templating unfortunately (I mentioned it in the first bullet point of this PR above). Grafana API doesn't allow to customize the templating view, so you still need to have a fixed tenant in DS config to use templating I fear :( |
|
Sorry, If we misunderstood, I was not talking about the templating view. |
|
I see, I misunderstood. I've just opened an issue ( #90 ) |
Feat. #62
Notes: