Add initial local auth implementation #3

saswatamcode · 2022-02-22T11:57:41Z

This PR adds the initial local authentication implementation for obsctl,

Adds context api add and context api rm functionality, which saves an Observatorium API instance with a given name or hostname, locally in .config/obs/config.json`
Adds login functionality, which saves configuration for a tenant, including OIDC config and token, also in .config/obs/config.json
Adds logout command which removes locally saved tenant config
Adds context switch <api name>/<tenant name> and context current functionality for viewing and changing context
Adds context list to display all locally saved contexts
Adds get operations whose output is pretty printed
Add set operation to set Rule files (obsctl metrics set --rule.file=)

TODOs (which will be handled in subsequent PRs),

Add way to hook in with oapi fetcher
Add flags for URL query params for API
Add query functionality
Add docs and intergration tests w/ api

Signed-off-by: Saswata Mukherjee <[email protected]>

Signed-off-by: Prem Saraswat <[email protected]>

Signed-off-by: Saswata Mukherjee <[email protected]>

bill3tt · 2022-02-28T18:43:52Z

@saswatamcode FYI there's a bug in the way that we are constructing URLs. I think we're missing a leading slash.

❯ obsctl metrics get rules.raw
Error: fetching: Get "https://observatorium.api.stage.openshift.comapi/metrics/v1/rhobs/api/v1/rules/raw": dial tcp: lookup observatorium.api.stage.openshift.comapi: no such host

This was the login command I used

obsctl login --api observatorium.api.stage.openshift.com --oidc.audience observatorium --oidc.client-id <client-id> --oidc.client-secret <client-secret> --oidc.issuer-url https://sso.redhat.com/auth/realms/redhat-external --tenant rhobs

pkg/config/auth.go

bill3tt · 2022-02-28T18:55:48Z

❯ obsctl metrics get labelvalues
Error: indent JSON invalid character 'p' after top-level value

bill3tt · 2022-02-28T18:58:08Z

My other suggestion here would be to add an explicit log line, possibly at error level, indicating to the user when a command is not yet implemented.

❯ obsctl metrics query ALERTS
info: query called

bill3tt · 2022-02-28T19:12:36Z

Struggling to login with certain URLs 🤔

❯ obsctl login --api observatorium.api.stage.openshift.com --oidc.audience observatorium --oidc.client-id <client-id> --oidc.client-secret <client-secret> --oidc.issuer-url https://sso.redhat.com/auth/realms/redhat-external --tenant rhobs
Error: adding new api: observatorium.api.stage.openshift.com is not a valid URL

Which is weird because if I then try and run

❯ obsctl login --api http://observatorium.api.stage.openshift.com ... 
Error: api with name http://observatorium.api.stage.openshift.com doesn't exist

But then when I re-run the original command it passes successfully 🤷

bill3tt · 2022-02-28T19:16:59Z

@saswatamcode I think this is a fantastic start and already I can see how this is going to make our lives much easier when interacting with observatorium 🎉 however, there are a couple of rough edges that are reducing my enjoyment of this CLI that we should get ironed out :)

saswatamcode · 2022-03-01T02:45:48Z

@ianbillett thanks for testing this out! I'll try and tackle those rough edges!

obsctl metrics get labelvalues
Error: indent JSON invalid character 'p' after top-level value

Labelvalues requires a name flag as per API, will mark that flag as required.

❯ obsctl login --api observatorium.api.stage.openshift.com --oidc.audience observatorium --oidc.client-id --oidc.client-secret --oidc.issuer-url https://sso.redhat.com/auth/realms/redhat-external --tenant rhobs
Error: adding new api: observatorium.api.stage.openshift.com is not a valid URL

For the login api flag, one can pass an already saved API name or just the URL which it will save as a new API. There's a bug in that step so let me fix. 🙂

Edit: Fixed and added a context list command as well.
Also, I noticed that each operation is generating a new access token, so changed that to reuse tokens as well!

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g

Great work @saswatamcode 👏 I have added some comments, I'd be happy to give it a test spin soon as well!

pkg/config/config.go

pkg/cmd/context.go

pkg/config/config.go

pkg/cmd/login.go

pkg/cmd/logout.go

pkg/cmd/metrics.go

pkg/config/auth.go

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g

Just tested obsctl = 🤩

I left few suggestions, perhaps they do not need addressing now but in future PRs.

I'd only request changing the auth.go filename, otherwise good to go 🚢

matej-g · 2022-03-03T10:20:54Z

pkg/cmd/context.go

+				return err
+			}
+
+			for k, v := range conf.APIs {


If we have API's specified, but no tenants, is that a valid context? It was a bit confusing since only APIs were listed when I had no tenants added - but that means those contexts are not valid / usable, right?

Context means an API name and a tenant name using which we can do an operation (will add docs soon to clarify this well 🙂). An API can exist in the config without a tenant (although you can't make requests like that), but a tenant cannot exist without an API.

So in this case, it means they aren't usable. You need to add a tenant under your already added API.

I think I understand that, but then we're listing 'unusable' contexts, which is not useful for the user.

Hmm, that's correct. Let's add an issue once this is merged and figure out a nice way to display contexts in an easily understandable way in future PRs. Maybe adding a " no tenant, cannot use yet" text beside the API name which has no tenants would be reasonable? 🙂

matej-g · 2022-03-03T10:22:21Z

pkg/cmd/context.go

+	}
+
+	listCmd := &cobra.Command{
+		Use:   "list",


I was also wondering whether it would make sense to add list command for APIs as well?

The list command lists out all possible context in the form of,

API1: - tenant1 - tenant2 API2: - tenant1 - tenant2

So might not need a separate list command for only API.
But one thing that I would want to add is a verbose mode by which we can print out all details or all tenant and APIs.

I think it still might be useful, especially if you have no context defined so far or if you want a higher-level view of which APIs you currently have in your config. If we can add and remove an entity, then it makes to me to list as well. But yes, it's achievable through context command to a degree, so it's not a blocker.

Let's iterate on a future PR! 🙂

pkg/cmd/context.go

pkg/config/auth.go

pkg/config/config.go

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g

🏅

onprem

LGTM!

saswatamcode and others added 6 commits February 22, 2022 17:21

Add initial local auth implementation

81353b3

Signed-off-by: Saswata Mukherjee <[email protected]>

Fix lint

f28ceed

Signed-off-by: Saswata Mukherjee <[email protected]>

Update comment

566c4eb

Signed-off-by: Saswata Mukherjee <[email protected]>

simplify config file implementation

0d7b560

Signed-off-by: Prem Saraswat <[email protected]>

Add tests for config pkg

99aeee0

Signed-off-by: Saswata Mukherjee <[email protected]>

Add logout command and use client + some cleanup

582e5ac

Signed-off-by: Saswata Mukherjee <[email protected]>

saswatamcode marked this pull request as ready for review February 27, 2022 05:28

saswatamcode changed the title ~~WIP: Add initial local auth implementation~~ Add initial local auth implementation Feb 27, 2022

Pretty print output

7948e09

Signed-off-by: Saswata Mukherjee <[email protected]>

saswatamcode force-pushed the auth-save branch from cd74709 to 7948e09 Compare February 28, 2022 05:03

Add debug logs

6a2c64b

Signed-off-by: Saswata Mukherjee <[email protected]>

bill3tt reviewed Feb 28, 2022

View reviewed changes

pkg/config/auth.go Outdated Show resolved Hide resolved

saswatamcode added 5 commits March 1, 2022 10:09

Implement suggestions: fix login, labelvalues & unimplemented

6005674

Signed-off-by: Saswata Mukherjee <[email protected]>

Add context list command

c554f87

Signed-off-by: Saswata Mukherjee <[email protected]>

Fix lint

25eea28

Signed-off-by: Saswata Mukherjee <[email protected]>

Add functionality to reuse token

d8d7e18

Signed-off-by: Saswata Mukherjee <[email protected]>

Add rules set command

2f8d8d1

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g reviewed Mar 1, 2022

View reviewed changes

saswatamcode added 3 commits March 2, 2022 09:35

Implement suggestions

fa6c6d6

Signed-off-by: Saswata Mukherjee <[email protected]>

Add defaults for single API config

b9cfe21

Signed-off-by: Saswata Mukherjee <[email protected]>

Remove string types for readability

6c659ec

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g reviewed Mar 3, 2022

View reviewed changes

Rename auth.go to request.go

e1dd11d

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g approved these changes Mar 3, 2022

View reviewed changes

onprem approved these changes Mar 10, 2022

View reviewed changes

onprem merged commit 1f8c2ab into observatorium:main Mar 10, 2022

saswatamcode mentioned this pull request Mar 10, 2022

Do not list unusable contexts in context list #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial local auth implementation #3

Add initial local auth implementation #3

saswatamcode commented Feb 22, 2022 •

edited

Loading

bill3tt commented Feb 28, 2022 •

edited

Loading

bill3tt commented Feb 28, 2022

bill3tt commented Feb 28, 2022

bill3tt commented Feb 28, 2022 •

edited

Loading

bill3tt commented Feb 28, 2022 •

edited

Loading

saswatamcode commented Mar 1, 2022 •

edited

Loading

matej-g left a comment

matej-g left a comment

matej-g Mar 3, 2022

saswatamcode Mar 3, 2022

matej-g Mar 3, 2022

saswatamcode Mar 3, 2022

matej-g Mar 3, 2022

saswatamcode Mar 3, 2022

matej-g Mar 3, 2022

saswatamcode Mar 3, 2022

matej-g left a comment

onprem left a comment

Add initial local auth implementation #3

Add initial local auth implementation #3

Conversation

saswatamcode commented Feb 22, 2022 • edited Loading

bill3tt commented Feb 28, 2022 • edited Loading

bill3tt commented Feb 28, 2022

bill3tt commented Feb 28, 2022

bill3tt commented Feb 28, 2022 • edited Loading

bill3tt commented Feb 28, 2022 • edited Loading

saswatamcode commented Mar 1, 2022 • edited Loading

matej-g left a comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

onprem left a comment

Choose a reason for hiding this comment

saswatamcode commented Feb 22, 2022 •

edited

Loading

bill3tt commented Feb 28, 2022 •

edited

Loading

bill3tt commented Feb 28, 2022 •

edited

Loading

bill3tt commented Feb 28, 2022 •

edited

Loading

saswatamcode commented Mar 1, 2022 •

edited

Loading