[JENKINS-38831] Add support for credential tracking in SecretBuildWrapper and BindingStep#27
Conversation
…er and BindingStep
|
This is currently setup to track the instances of builds using the credential, would we want this to be that granular or do we just want the Job? |
jglick
changed the title
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Binary-incompatible, which is easily fixed I think; otherwise looks LGTM.
|
|
||
| /** Type token. */ | ||
| protected abstract Class<C> type(); | ||
| public abstract Class<C> type(); |
There was a problem hiding this comment.
Binary-incompatible for external implementations, I think.
Probably solvable by adding a public final method to MultiBinding to set up the tracking.
| unbinders.add(environment.getUnbinder()); | ||
| overrides.putAll(environment.getValues()); | ||
| IdCredentials id = CredentialsProvider.findCredentialById(binding.getCredentialsId(), binding.type(), run); | ||
| CredentialsProvider.track(run, id); |
There was a problem hiding this comment.
Perhaps also desirable to call track(Node, Credentials). (For BindingStep, just inject a Node; for SecretBuildWrapper, use the method on AbstractBuild IIRC.) @stephenc any guidance?
There was a problem hiding this comment.
So track(Node, C) is to track when the credential is used to launch that node. Not the case here. We already have tracking between the Run and the Node(s) it ran on.
There was a problem hiding this comment.
Well, not in the case of Pipeline builds generally, but I see your point—track(Node, Credentials) is irrelevant here. I will file a PR to fix the Javadoc.
stephenc
left a comment
stephenc
left a comment
There was a problem hiding this comment.
🐛 the intent is that tracking should be done at the point of lookup not by doing a second lookup
| MultiBinding.MultiEnvironment environment = binding.bind(run, workspace, launcher, listener); | ||
| unbinders.add(environment.getUnbinder()); | ||
| overrides.putAll(environment.getValues()); | ||
| IdCredentials id = CredentialsProvider.findCredentialById(binding.getCredentialsId(), binding.type(), run); |
There was a problem hiding this comment.
Ugh! The tracking should be done when the credential is actually looked up... not by doing a second look up... as that second look-up may trigger a second logged call to an external credentials management system. Much better if we can have it as part of the binding... May require a thread local to capture the Run context
| final List<MultiBinding.MultiEnvironment> m = new ArrayList<MultiBinding.MultiEnvironment>(); | ||
| for (MultiBinding binding : bindings) { | ||
| m.add(binding.bind(build, build.getWorkspace(), launcher, listener)); | ||
| IdCredentials id = CredentialsProvider.findCredentialById(binding.getCredentialsId(), binding.type(), build); |
There was a problem hiding this comment.
Ugh! Please no... one lookup only
…edential lookups.
|
The jenkins hook seems to be stuck, can someone give it a kick :) |
|
Is there anything else needed for this PR? |
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Buglet regarding timing of track call, otherwise good.
| import hudson.model.Descriptor; | ||
| import hudson.model.Run; | ||
| import hudson.model.TaskListener; | ||
| import hudson.model.*; |
There was a problem hiding this comment.
Please revert and use explicit imports.
| */ | ||
| protected final @Nonnull C getCredentials(@Nonnull Run<?,?> build) throws IOException { | ||
| IdCredentials cred = CredentialsProvider.findCredentialById(credentialsId, IdCredentials.class, build); | ||
| CredentialsProvider.track(build, cred); |
There was a problem hiding this comment.
Better moved to the if block that returns a value.
|
|
||
| import com.cloudbees.plugins.credentials.CredentialsProvider; | ||
| import com.cloudbees.plugins.credentials.CredentialsScope; | ||
| import com.cloudbees.plugins.credentials.*; |
There was a problem hiding this comment.
Ditto, please retain explicit imports.
|
@jglick updated from feedback. |
3 participants
JENKINS-38831