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 short-lived tokens instead of Access key #426

Merged
merged 9 commits into from
Apr 19, 2024
Merged

Conversation

alextu
Copy link
Contributor

@alextu alextu commented Apr 11, 2024

Generate a short lived token before each build and use it in the env var instead of the access key.
A new UI parameter is introduced to control the tokens expiry:

image

Testing done

Submitter checklist

Preview Give feedback

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual
@alextu alextu requested a review from welandaz April 11, 2024 09:04
alextu added 2 commits April 11, 2024 12:03

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual
Copy link
Member

@welandaz welandaz left a comment

Choose a reason for hiding this comment

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

Nice! 🎉

@alextu alextu requested a review from jthurne April 11, 2024 16:09
DevelocityLogger logger = new DevelocityLogger(listener);
if (secretKey != null && DevelocityAccessKey.isValid(secretKey.getPlainText())) {
shortLivedToken = tokenClient.get(InjectionConfig.get().getServer(),
DevelocityAccessKey.parse(secretKey.getPlainText()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless I'm mistaken, this parse method assumes that the access key contains a single server=key entry, but the isValid method above will pass for a key with server1=key1;server2=key2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, then we need to extract the hostname from the DV server URL and try to find the corresponding key

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual
tokenClient.get(serverUrl, k, InjectionConfig.get().getShortLivedTokenExpiry()))
.filter(Optional::isPresent)
.map(k -> Secret.fromString(k.get().getRawAccessKey()))
.orElse(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we fail to get a short-lived access token, do we want to fail the build, or fall back to some reasonable behavior?

In our own dogfooding case, we decided to not set the access key and let the build fail to use various Develocity features like build caching and build scan publishing. However, we had to explicitly disable Test Distribution (which will otherwise fail if it fails to connect to Develocity). I wonder if we should do something similar here.

I also wonder if it would make sense to add a configuration option to allow users to decide if they want the build to fail if it can't get an access token, or to continue without Develocity.

Copy link
Contributor Author

@alextu alextu Apr 15, 2024

Choose a reason for hiding this comment

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

I would not fail the build as the injection applies to potentially hundreds/thousands of projects. We can check later if we want to introduce a configuration option to do that (and research if it's even possible to fail the build from this hook).
For now the behavior is the same as you described, except we did not implement any specific behavior for Test Distribution since we do not configure it with the injection (nor build cache btw).

url = url + "?expiresInHours=" + expiry;
}

Request request = new Request.Builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend implementing some form of a retry mechanism. That way, if the Develocity instance is down briefly, the build has a chance of still working.

We found a retry mechanism to be quite helpful in our own dogfooding of short-lived access tokens.

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual
alextu added 4 commits April 16, 2024 09:09

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual

Verified

This commit was signed with the committer’s verified signature.
alextu Alexis Tual
@alextu alextu merged commit 27994bc into master Apr 19, 2024
20 of 22 checks passed
@alextu alextu deleted the atual/short-lived-token branch April 19, 2024 14:33
@basil
Copy link
Member

basil commented May 15, 2024

Nice fix! JENKINS-73082 would also be nice to fix.

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

5 participants