FEATURE: Update Perses Client authentication with Client Credentials Grant #59
Conversation
|
|
||
| func initializePersesClient(baseURL string) (apiClient.ClientInterface, error) { | ||
|
|
||
| bearerToken := os.Getenv(PERSES_TOKEN) |
There was a problem hiding this comment.
To not introduce breaking change, you can keep taking the PERSES_TOKEN env var to set it in the config if provided.
config/config.go
Outdated
|
|
||
| type GlobalConfig struct { | ||
| AppConfig AppConfig `yaml:"app_config"` | ||
| AuthConfig AuthConfig `yaml:"auth_config"` |
There was a problem hiding this comment.
RestConfigClient contains already everything you need to communicate with Perses. The server URL, the token URL, the client id and secret. You don't need all that.
Even AppConfig is useless here. It's only logged for the most part. Please don't introduce unrelated configuration into this PR. this is only about perses creds
main.go
Outdated
|
|
||
| restClient, err := config.NewRESTClient(config.RestConfigClient{ | ||
| func initializePersesAPIClient(baseURL string, authConfig config.AuthConfig) (apiClient.ClientInterface, error) { | ||
| restClient, err := clientConfig.NewRESTClient(clientConfig.RestConfigClient{ |
There was a problem hiding this comment.
you should pass directly a RestConfigClient, from the config.
celian-garcia
left a comment
celian-garcia
left a comment
There was a problem hiding this comment.
Thanks for taking care of this! I like the intent, but I guess it's still a bit rough to be ready to merge.
main.go
Outdated
| flag.StringVar(&configFile, "config", "", "Path to the yaml configuration file") | ||
| flag.StringVar(&persesServerURL, "perses-server-url", "http://localhost:8080", "The Perses backend server URL") | ||
| flag.StringVar(&logLevel, "log-level", "info", "Log level (debug, info, warn, error)") | ||
| flag.StringVar(&transport, "transport", "stdio", "MCP protocol currently supports 'stdio' and 'http-streamable' transport mechanisms") | ||
| flag.StringVar(&transport, "transport", "stdio", "MCP protocol supports 'stdio' and 'streamable-http' transports") | ||
| flag.StringVar(&port, "port", "8000", "Port to run the HTTP Streamable server on") | ||
| flag.BoolVar(&readOnly, "read-only", false, "Restrict the server to read-only operations") | ||
| flag.StringVar(&tokenURL, "token-url", "", "Perses token endpoint URL") | ||
| flag.BoolVar(&readOnly, "read-only", false, "Restrict the server to read-only ops") | ||
| flag.Parse() |
There was a problem hiding this comment.
IMHO since you are introducing the usage of config file managed via perses/common then you can (should?) get rid of the flags that are duplicating the same information.
There was a problem hiding this comment.
But I agree with Celian that changes regarding app config (e.g port & transport) are out of scope of this PR
There was a problem hiding this comment.
Thank You for your input @celian-garcia , @AntoineThebaud
I will take make a separate PR for the config based approach soon.
In the mean time I have updated this PR for you to check.
prakhar29jain
force-pushed
the
feature/auth
branch
2 times, most recently
from
4de7432 to
d75db8b
Compare
…Grant Signed-off-by: Prakhar JAIN <prakhar.jain@amadeus.com>
prakhar29jain
force-pushed
the
feature/auth
branch
from
d75db8b to
b2fd420
Compare
3 participants
Initially the auth used the PERSES_TOKEN directly to initialize a Perses client which was not secure.
Now we are using the client credentials to fetch the token.
Removed manual OAuth token retrieval from main; Perses client now handles auth internally.
Aligned Perses client initialization with configuration structure (HTTPClient, RestConfigClient, OAuth).
Updated main.go to use the new config-based client initialization.