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

Use OAuth token instead of http basic auth for prometheus access #62

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

csibbitt
Copy link
Contributor

@csibbitt csibbitt commented Dec 5, 2023

This change is required in order to match changes in STO where we will stop allowing http basic auth for access from grafana to prometheus, and use an oauth token from a new restricted service account instead.

Depends-On: infrawatch/service-telemetry-operator#549

This change is required in order to match changes in STO[1] where we will stop
allowing http basic auth for access from grafana to prometheus, and use an
oauth token from a new restricted service account instead.

[1] infrawatch/service-telemetry-operator#549
@csibbitt
Copy link
Contributor Author

csibbitt commented Dec 5, 2023

NOTE: This change will not work until infrawatch/service-telemetry-operator#549 merges, and will not work with existing STF releases! (Fixed)

I tested this locally against an environment deployed from my STO PR branch like so: ansible localhost --module-name include_role --args name=client_side_tests and was able to see successful prometheus queries using the token.

(Edit: This is fixed below) Before we merge this, I'm noticing that we may have a problem. While this change is required in order to test the new code that will soon be on the STO master branch, there are no "stable-1.5"-like release branches in this repo, so if this merges, you will no longer be able to use it to test the release version of STF. Creating the branch is trivial, but I have not yet looked at what will be required to adjust the jenkins jobs to use the correct branch at the correct time, and feel like this is way outside my scope at the moment. Looking for help from QE to figure out how to handle this.

CC: @ayefimov-1 @myadla

@elfiesmelfie
Copy link
Collaborator

NOTE: This change will not work until infrawatch/service-telemetry-operator#549 merges, and will not work with existing STF releases!

I tested this locally against an environment deployed from my STO PR branch like so: ansible localhost --module-name include_role --args name=client_side_tests and was able to see successful prometheus queries using the token.

Before we merge this, I'm noticing that we may have a problem. While this change is required in order to test the new code that will soon be on the STO master branch, there are no "stable-1.5"-like release branches in this repo, so if this merges, you will no longer be able to use it to test the release version of STF. Creating the branch is trivial, but I have not yet looked at what will be required to adjust the jenkins jobs to use the correct branch at the correct time, and feel like this is way outside my scope at the moment. Looking for help from QE to figure out how to handle this.

CC: @ayefimov-1 @myadla

I would suggest adding a var to determine whether to use user/pass or get a token.
prom_auth_method = token|pass

Also, a branch would be a good idea, but we still have a temporary issue trying to figure out how to point jenkins at the right branch.

ansible.builtin.shell:
cmd: |
oc get secret default-prometheus-htpasswd -ojson | jq '.data.auth, .data.password' | sed 's/"//g'
register: prom_secret
oc create token stf-prometheus-reader
Copy link
Collaborator

@elfiesmelfie elfiesmelfie Dec 5, 2023

Choose a reason for hiding this comment

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

For backwards-compatibility, I suggest keeping the "get password" and "get user" tasks, and stick them in a block, guarded with a when: X.

The new token generation can be in a complementary block (i.e. when: not X)

To keep the curl tasks clean, I suggest adding a "prom_auth_string" that would be either -u "{{ prom_user }}:{{ prom_pass }}" or -H "Authorization: Bearer {{prom_token}}"

@csibbitt
Copy link
Contributor Author

csibbitt commented Dec 5, 2023

Thanks, @elfiesmelfie , I'll add a variable that selects between the methods as a temporary measure until the branch awareness can be sorted out.

changed_when: false
- when: prom_auth_method == 'password'
block:
- name: "Get the default-prometheus-htpasswd secret"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This diff is unfair; every one of these blocks was in the existing code! ;) The changes are mostly indenting.

@csibbitt
Copy link
Contributor Author

csibbitt commented Dec 6, 2023

I haven't looked into why, but the task names are never printed during execution so it's hard to follow; nevertheless, here is my evidence of testing:

Token auth

STF from csibbitt/STF-1549_prometheus_token_auth branch

$ ansible localhost --module-name include_role --args name=client_side_tests
[...]
localhost | SUCCESS | rc=0 >>
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   136    0    63  100    73    431    500 --:--:-- --:--:-- --:--:--   931

Password auth

STF from stable-1.5 branch

Fails with token auth:

$ ansible localhost --module-name include_role --args name=client_side_tests
[...]
localhost | FAILED | rc=1 >>
error: failed to create token: serviceaccounts "stf-prometheus-reader" not foundnon-zero return code

Works with password auth:

$ ansible localhost --module-name include_role --args name=client_side_tests -e prom_auth_method=password
[...]
localhost | SUCCESS | rc=0 >>
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   136    0    63  100    73    140    162 --:--:-- --:--:-- --:--:--   302

I've also gone ahead and created the stable-1.5 branch in this repo based on the current master commit so that we are ready if the jobs can be made branch-aware. This workaround should never really need to merge there, as we should be in token-only mode as of STF 1.5.4; but even if we do merge it to that branch everything will still work.

@csibbitt csibbitt force-pushed the csibbitt/STF-1549_token_auth_for_prometheus branch from bdad069 to 24b8ecc Compare December 6, 2023 23:07
Copy link
Contributor

@ayefimov-1 ayefimov-1 left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@csibbitt csibbitt merged commit db8ce42 into master Dec 12, 2023
4 checks passed
@csibbitt csibbitt deleted the csibbitt/STF-1549_token_auth_for_prometheus branch December 12, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants