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

[extension/oauth2clientauth] Enable dynamically reading ClientID and ClientSecret from files #26310

Merged
merged 7 commits into from
Sep 8, 2023
Merged

[extension/oauth2clientauth] Enable dynamically reading ClientID and ClientSecret from files #26310

merged 7 commits into from
Sep 8, 2023

Conversation

elikatsis
Copy link
Contributor

Description:
This PR implements the feature described in detail in the issue linked below.

In a nutshell, it extends the oauth2clientauth extension to read ClientID and/or ClientSecret from files whenever a new token is needed for the OAuth flow.
As a result, the extension can use updated credentials (when the old ones expire for example) without the need to restart the OTEL collector, as long as the file contents are in sync.

Link to tracking Issue: #26117

Testing:
Apart from the unit testing you can see in the PR, I've tested this feature in two real-life environments:

  1. As a systemd service exporting otlphttp data
  2. A Kubernetes microservice (deployed by an OpenTelemetryCollector CR) exporting otlphttp data

In both cases, the collectors export the data to a service which sits behind an OIDC authentication proxy.
Using the oauth2clientauth extension, the otlphttp exporter hits the authentication provider to issue tokens for the OIDC client and successfully authenticates to the service.

In my cases, the ClientSecret gets rotated quite frequently and there is a stack making sure the ClientID and ClientSecret in the corresponding files are up-to-date.

Documentation:
I have extended the extension's README file. I'm open to more suggestions!

cc @jpkrohling @pavankrish123

@elikatsis elikatsis requested a review from jpkrohling as a code owner August 30, 2023 14:12
@elikatsis elikatsis requested a review from a team August 30, 2023 14:12
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@jpkrohling
Copy link
Member

Can you sort out the CLA before I review this one?

@elikatsis
Copy link
Contributor Author

@jpkrohling CLA ✅

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I have made a few suggestions but this looks very cool! Thank you for the PR.

var err error

if len(filepath) > 0 {
actualValue, err = readCredentialsFile(filepath)
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to create a new var? Perhaps just return the results of the readCredentialsFile if the filepath's len is > 0, otherwise return value, nil ?

Suggested change
actualValue, err = readCredentialsFile(filepath)
return readCredentialsFile(filepath)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point we don't create a new var, we override the actualValue one (that's why we also define err outside the block). This doesn't look optimal at this state (and I'm good with changing it) but it makes things more clear in my opinion when adding more ways of retrieving the "actual value" (as mentioned in the issue, get by executing a command).

I can push the commits with the command part so that you can take a look and decide how you want me to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

as mentioned in the issue, get by executing a command

I missed that, and this specific example is unlikely to get accepted: we probably do not want the collector executing commands.

At this point we don't create a new var, we override the actualValue one

I mean something like this:

func getActualValue(value, cmd, filepath string) (string, error) {
	if len(filepath) > 0 {
		return readCredentialsFile(filepath)
	}

	if len(cmd) > 0 {
		return readCommandOutput(cmd)
	}

	return value, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

By the way, this is a minor improvement, optional for the approval of this PR.

Another take would be this: given that this is in the hot path and the decision will be the same during the whole lifecycle of this component, it could even be better to just use a func as a parameter to this type. Like this:

func NewClientCredentialsConfig(cfg *clientcredentials.Config, clientIDFile, clientSecretFile string) (*clientCredentialsConfig, error) {
	c := &clientCredentialsConfig{
		Config: *cfg,
	}

	if len(clientIDFile) > 0 {
		c.clientIDSource = func() (string, error) {
			return readCredentialsFile(clientIDFile)
		}
	} else {
		c.clientIDSource = func() (string, error) {
			return c.ClientID, nil
		}
	}

	if len(clientSecretFile) > 0 {
		c.clientSecretSource = func() (string, error) {
			return readCredentialsFile(clientSecretFile)
		}
	} else {
		c.clientSecretSource = func() (string, error) {
			return c.ClientSecret, nil
		}
	}

	return c, nil
}

Your createConfig would then call clientID, err := c.clientIDSource() and clientSecret, err := c.clientSecretSource() accordingly.

extension/oauth2clientauthextension/extension_test.go Outdated Show resolved Hide resolved
extension/oauth2clientauthextension/README.md Show resolved Hide resolved
@elikatsis
Copy link
Contributor Author

@jpkrohling thank you very much for your review. I've posted some replies I'd like your feedback on

@jpkrohling
Copy link
Member

LGTM, there's a minor improvement that could be made to the hot path. The only real requirement is to improve the readme. Once that's done, ping me and I'll kick the CI.

Implement a wrapper for clientcredentials.Config in preparation for
custom behavior and logic when issuing new tokens.
Use the locally defined struct clientCredentialsConfig in favor of the
upstream clientcredentials.Config object.
Extend the clientCredentailsConfig to retrieve client ID and/or secret
from files by setting values to the ClientIDFile and ClientSecretFile
fields respectively.

The Client*File field takes precedence over the Client* field.
Leverage the clientCredentialsConfig features to read client ID and/or
secret from a file to the actual extension configuration.
@elikatsis
Copy link
Contributor Author

elikatsis commented Sep 6, 2023

@jpkrohling I made the following changes

  • Extended README
  • Modified tests to not use string comparisons to assert errors
  • Changed getActualValue to immediately return instead of using extra vars
  • [Minor fixup on a comment left from the file: prefix era]

Your suggestion on using client*Source() methods is very interesting. However, I didn't make the change because it feels like it won't help much in case we need something similar for other attributes as well or introduce more options for these attributes. Having the logic to collect values in createConfig() makes it more easy and straight forward if anyone wants to extend it. I also think there isn't any actual performance improvement.
If you prefer it, I will definitely do it, no strong opinion 😃

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM for a first implementation. The next iteration could perhaps use the same approach we have in the Jaeger Remote sampling extension, by watching the file for changes instead of re-reading on every request. I think there's a big performance penalty with this, but I'll accept because:

  1. it's ready and tested by the author
  2. it's a new feature, doesn't impact current users

@elikatsis
Copy link
Contributor Author

@jpkrohling I fixed a typo which made the lint test fail. I ran quite a few more Makefile targets trying to catch things early this time. You'll probably need to trigger the tests again :/

@elikatsis
Copy link
Contributor Author

I see this error in the test

[2023-09-06T17:00:51.035Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

Is it relevant to my PR or it's some infra issue? Hmu if I can do anything to unblock this

@jpkrohling
Copy link
Member

I ran it again and it's passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants