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

feat(HMS-3416): Switch to service accounts for dev auth #792

Merged
merged 1 commit into from
May 9, 2024

Conversation

ezr-ondrej
Copy link
Member

For developement we have been using basic auth to authenticate against stage services. This switches the auth to Service accounts.

@ezr-ondrej ezr-ondrej requested a review from lzap May 3, 2024 19:06
data.Add("client_id", clientId)
data.Add("client_secret", clientSecret)

req, err := http.NewRequest(http.MethodPost, provider.Endpoint().TokenURL, strings.NewReader(data.Encode()))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is me being bit fancy just to resolve a static URL, for which we need extra module 🤔

provider.Endpoint().TokenURL is quite static value.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Normally, I would insist on caching the token but it has some drawbacks too and since this is only for dev setups, doing it on every request is fine. Thanks.

}
res.Body.Close()
json.Unmarshal(body, &token)

Copy link
Member

Choose a reason for hiding this comment

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

Check return value, also use defer to ensure body is closed.

if err != nil {
return err
}
zerolog.Ctx(ctx).Warn().Msgf("Service account authentication: %s", clientID)
Copy link
Member

Choose a reason for hiding this comment

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

This should not be warning, maybe info or debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is warning, because we don't want that to happen in Stage/Prod, so this makes it easier to spot in case by some accident we allow it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or that was the thinking why it was a warning for basic auth, I've kept the level.

@ezr-ondrej ezr-ondrej force-pushed the service_accounts branch 2 times, most recently from bb56af5 to fe4810d Compare May 6, 2024 08:22
@ezr-ondrej
Copy link
Member Author

I would insist on caching the token but it has some drawbacks too and since this is only for dev setups

Yeah exactly, I was thinking about it, but then decided against it for 1) I was lazy and 2) probably not as necessary for dev only.

@ezr-ondrej ezr-ondrej force-pushed the service_accounts branch 2 times, most recently from 2d274a1 to b143986 Compare May 6, 2024 08:28
For developement we have been using basic auth to authenticate against stage services.
This switches the auth to Service accounts.
@ezr-ondrej
Copy link
Member Author

/retest

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Tested with sources, thanks. Took me a while to prepare my new dev setup, apologies.

@lzap lzap merged commit 808f6a7 into RHEnVision:main May 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants