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

Keep credentials cached across build commands. #16822

Closed
wants to merge 2 commits into from

Conversation

tjgq
Copy link
Contributor

@tjgq tjgq commented Nov 22, 2022

When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it.

@tjgq tjgq requested a review from a team as a code owner November 22, 2022 20:06
@ShreeM01 ShreeM01 added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Nov 22, 2022
@tjgq tjgq force-pushed the credential-cache branch 2 times, most recently from cb02a7c to c836978 Compare November 22, 2022 21:32
When using a credential helper, the lifetime of the credential cache is
currently tied to an individual command, which causes the helper to be called
for every command resulting in poor incremental build latency for builds
using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule,
I've introduced a new CredentialModule whose sole purpose is to provide access
to it.
@tjgq
Copy link
Contributor Author

tjgq commented Nov 22, 2022

cc @Yannic

Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

Mostly drive-by comment, will take a closer look tomorrow.

I think if we cache credentials accross invocations, we need to make the helper part of the cache key

@tjgq
Copy link
Contributor Author

tjgq commented Nov 23, 2022

Mostly drive-by comment, will take a closer look tomorrow.

I think if we cache credentials accross invocations, we need to make the helper part of the cache key

Could you elaborate on the scenario you're concerned about? It's not immediately obvious to me that changing the helper necessarily invalidates credentials that were obtained before.

@Override
public void beforeCommand(CommandEnvironment env) {
// Update the cache expiration policy according to the command options.
AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class);
Copy link
Member

Choose a reason for hiding this comment

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

Probably clear the cache when the command is clean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's probably expected behavior for bazel clean. It also gives us a reliable way to bust the cache. Added a test and amended the flag description to document this behavior.

Caffeine.newBuilder().expireAfterWrite(Duration.ZERO).build();

/** Returns the credential cache. */
public Cache<URI, ImmutableMap<String, ImmutableList<String>>> getCredentialCache() {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether this is the best way to provide the cache. For now it seems fine as we have limited scope but if we ever want to share the cache for more modules (e.g. for bzlmod), please consider adding a provider to ServerBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged; I'd prefer to wait until repository fetching is wired up to see how the whole thing looks.

I think in any case we'd still want this module to host the beforeCommand logic (otherwise we'd have to arbitrarily choose one of the many modules that will eventually use credentials, making the scope unclear).

Copy link
Contributor

Choose a reason for hiding this comment

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

@Yannic
Copy link
Contributor

Yannic commented Nov 23, 2022

Mostly drive-by comment, will take a closer look tomorrow.
I think if we cache credentials accross invocations, we need to make the helper part of the cache key

Could you elaborate on the scenario you're concerned about? It's not immediately obvious to me that changing the helper necessarily invalidates credentials that were obtained before.

Let's say someone does a build with --credential_helper=foo, then the cache would be populated with credentials for example.com from foo. In the next build, they change to --credential_helper=bar (also against example.com). I would expect bar to be always called since we changed the flag, but it won't necessarily because the helper isn't part of the cache key and there are credentials for example.com in the cache. I guess Bazel would run bar if the RPC with the credentials from foo fail, but it seems very much reasonable that Bazel would invalidate all credentials that come from a different helper than what's configured for the current invocation.

@tjgq
Copy link
Contributor Author

tjgq commented Nov 23, 2022

Mostly drive-by comment, will take a closer look tomorrow.
I think if we cache credentials accross invocations, we need to make the helper part of the cache key

Could you elaborate on the scenario you're concerned about? It's not immediately obvious to me that changing the helper necessarily invalidates credentials that were obtained before.

Let's say someone does a build with --credential_helper=foo, then the cache would be populated with credentials for example.com from foo. In the next build, they change to --credential_helper=bar (also against example.com). I would expect bar to be always called since we changed the flag, but it won't necessarily because the helper isn't part of the cache key and there are credentials for example.com in the cache. I guess Bazel would run bar if the RPC with the credentials from foo fail, but it seems very much reasonable that Bazel would invalidate all credentials that come from a different helper than what's configured for the current invocation.

I think it can be argued both ways: if I'm replacing foo with bar but they both call the same external service, I know that the cached credentials are still good.

But assuming we do want to make it part of the key: how far do we want to go? Are you proposing to only take the name of the helper into account, or also the executable it points to, the values of environment variables, etc?

Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

I will let you and Yannic decide whether to include helper inside the key. Other part LGTM.

Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

I'm fine with starting with just the URI as cache key. Shouldn't be too hard to change if a more concrete use case comes up. I also expect that we need to change the key in any case once we tackle scopes for credentials from helpers.

@tjgq
Copy link
Contributor Author

tjgq commented Nov 28, 2022

I'm fine with starting with just the URI as cache key. Shouldn't be too hard to change if a more concrete use case comes up. I also expect that we need to change the key in any case once we tackle scopes for credentials from helpers.

Thanks, I'll merge this as-is then. I agree that we're going to have to think more carefully about it (cache invalidation: one of the two famously hard problems in compsci :))

tjgq added a commit to tjgq/bazel that referenced this pull request Nov 29, 2022
When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it.

Closes bazelbuild#16822.

PiperOrigin-RevId: 491598103
Change-Id: Ib668954b635a0e9498f0a7418707d6a2dfae0265
tjgq added a commit to tjgq/bazel that referenced this pull request Nov 29, 2022
When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it.

Closes bazelbuild#16822.

PiperOrigin-RevId: 491598103
Change-Id: Ib668954b635a0e9498f0a7418707d6a2dfae0265
tjgq added a commit to tjgq/bazel that referenced this pull request Nov 29, 2022
When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it.

Closes bazelbuild#16822.

PiperOrigin-RevId: 491598103
Change-Id: Ib668954b635a0e9498f0a7418707d6a2dfae0265
ShreeM01 added a commit that referenced this pull request Nov 29, 2022
When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it.

Closes #16822.

PiperOrigin-RevId: 491598103
Change-Id: Ib668954b635a0e9498f0a7418707d6a2dfae0265

Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
ShreeM01 pushed a commit that referenced this pull request Nov 29, 2022
When using a credential helper, the lifetime of the credential cache is currently tied to an individual command, which causes the helper to be called for every command resulting in poor incremental build latency for builds using a non-trivial helper.

Since the cache must be shared by RemoteModule and BazelBuildServiceModule, I've introduced a new CredentialModule whose sole purpose is to provide access to it.

Closes #16822.

PiperOrigin-RevId: 491598103
Change-Id: Ib668954b635a0e9498f0a7418707d6a2dfae0265
@tjgq tjgq deleted the credential-cache branch December 8, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants