roachtest: grafana annotations read creds from cloud storage #125571
Conversation
|
This change is |
DarrylWong
force-pushed
the
annotation-secret-manager
branch
2 times, most recently
from
ba372dc to
a976db7
Compare
|
@nameisbhaskar I cleaned up some of the logging and changed the error handling a bit in |
DarrylWong
force-pushed
the
annotation-secret-manager
branch
from
a976db7 to
5b55193
Compare
DarrylWong
changed the title
DarrylWong
force-pushed
the
annotation-secret-manager
branch
from
5b55193 to
b32947f
Compare
There was a problem hiding this comment.
LGTM in terms of code reuse; just have some naming questions.
This will allow users to have access to Grafana annotations out of the box locally
I'm missing something in my understanding of how things are set up -- why does this change imply that everyone will have access out of the box? Doesn't it still require users to set environment variables locally?
| grafanaAudience := os.Getenv(ServiceAccountAudience) | ||
| if grafanaAudience == "" { | ||
| return nil, errors.Newf("%s env variable was not found", ServiceAccountAudience) | ||
| if _, err := promhelperclient.SetPromHelperCredsEnv(ctx, false); err != nil { |
There was a problem hiding this comment.
Documentation for newGrafanaClient could be updated to mention the function that reads credentials from env (maybe SetPromHelperCredsEnv), as ServiceAccountJson no longer exists.
There was a problem hiding this comment.
Also, the code reuse is good, but the fact that Grafana authentication works by calling promhelperclient.SetPromHelperCredsEnv is confusing because it suggests promhelperclient is involved.
There was a problem hiding this comment.
Ah yes good point, maybe this should live in a separate identity package since this is really non specific to prom/grafana/etc., just anything that uses google idp.
| serviceAccountJson = "PROM_HELPER_SERVICE_ACCOUNT_JSON" | ||
| serviceAccountAudience = "PROM_HELPER_SERVICE_ACCOUNT_AUDIENCE" | ||
| ServiceAccountJson = "PROM_HELPER_SERVICE_ACCOUNT_JSON" | ||
| ServiceAccountAudience = "PROM_HELPER_SERVICE_ACCOUNT_AUDIENCE" |
There was a problem hiding this comment.
Should these have more generic names? It's strange to me that we'll need to say that in order to use roachprod grafana-annotation, you'll need to set the PROM_HELPER_SERVICE_ACCOUNT_JSON env var.
In the case of number 3, it will set the environment variables locally after reading. It's a bit weird to me that the function doesn't just return the credentials and we need to refetch from the env, but I kept it as is to minimize changes. |
DarrylWong
force-pushed
the
annotation-secret-manager
branch
3 times, most recently
from
9a8dc1f to
788b1bc
Compare
|
I've extracted the common logic into a |
DarrylWong
force-pushed
the
annotation-secret-manager
branch
from
788b1bc to
3953c8f
Compare
As of cockroachdb#124099 we now store the service account creds in cloud storage. We already use this to access prometheus when generating dynamic configs. This change does the same for Grafana annotations by extracting the common logic into a helper. This will allow users to have access to Grafana annotations out of the box locally, and limit the amount of benign but potentially confusing warnings about invalid credentials. Epic: none Fixes: none Release note: none
DarrylWong
force-pushed
the
annotation-secret-manager
branch
from
3953c8f to
079a0f5
Compare
|
TFTRs! bors r=herkolategan, renatolabs |
As of #124099 we now store the service account creds in cloud storage. We already use this to access prometheus when generating dynamic configs. This change does the same for Grafana annotations by extracting the common logic into a helper.
This will allow users to have access to Grafana annotations out of the box locally, and limit the amount of benign but potentially confusing warnings about invalid credentials.
Release note: none
Fixes: none
Epic: none