Simplified plugin impl, removed unreliable `onResume` by jglick · Pull Request #152 · jenkinsci/ssh-agent-plugin

jglick · 2024-07-29T23:34:27Z

Cleaning up after #48.

jglick · 2024-07-29T23:35:07Z

pom.xml

    <changelist>999999-SNAPSHOT</changelist>
    <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
-    <jenkins.version>2.387.3</jenkins.version>
+    <jenkins.baseline>2.440</jenkins.baseline>


Just updating to a recommended baseline while we are here.

jglick · 2024-07-29T23:35:40Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/LauncherProvider.java

- * might change in between usages and where it is not possible to propagate
- * the object in other ways.


We can just pass the Launcher to each call.

jglick · 2024-07-29T23:36:22Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/RemoteAgent.java

-import java.io.IOException;
-
-/**
- * Need an interface in order to export the object from the channel.


Not sure if this was actually true prior to #48, but it is certainly not true now—this interface was not being passed to Channel.export.

jglick · 2024-07-29T23:36:48Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/RemoteAgentFactory.java

-import hudson.model.TaskListener;
-
-/**
- * Extension point for ssh-agent providers.


When there is just a single impl for years, this was unnecessary complexity.

jglick · 2024-07-29T23:37:22Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentBuildWrapper.java

-         * @throws Throwable if things go wrong.
-         * @deprecated use {@link #SSHAgentEnvironment(hudson.Launcher, hudson.model.BuildListener, java.util.List)}
-         */
-        @Deprecated


No reason to keep these long-deprecated overloads around; who would call them?

jglick · 2024-07-29T23:38:22Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentBuildWrapper.java

-            Map<String, Throwable> faults = new LinkedHashMap<>();
-            for (RemoteAgentFactory factory : Jenkins.get().getExtensionList(RemoteAgentFactory.class)) {
-                if (factory.isSupported(launcher, listener)) {
-                    try {
-                        listener.getLogger().println("[ssh-agent]   " + factory.getDisplayName());
-                        agent = factory.start(new SingletonLauncherProvider(launcher), listener,
-                                workspace != null ? SSHAgentStepExecution.tempDir(workspace) : null);
-                        break;
-                    } catch (Throwable t) {
-                        faults.put(factory.getDisplayName(), t);
-                    }
-                }
-            }
-            if (agent == null) {
-                listener.getLogger().println("[ssh-agent] FATAL: Could not find a suitable ssh-agent provider");
-                listener.getLogger().println("[ssh-agent] Diagnostic report");
-                for (Map.Entry<String, Throwable> fault : faults.entrySet()) {
-                    listener.getLogger().println("[ssh-agent] * " + fault.getKey());
-                    StringWriter sw = new StringWriter();
-                    fault.getValue().printStackTrace(new PrintWriter(sw));
-                    for (String line : StringUtils.split(sw.toString(), "\n")) {
-                        listener.getLogger().println("[ssh-agent]     " + line);
-                    }
-                }
-                throw new RuntimeException("[ssh-agent] Could not find a suitable ssh-agent provider.");
-            }
-            this.agent = agent;
+            agent = new ExecRemoteAgent(launcher, listener);


When there is no list of factories to consult, the code is simpler and the error handling should be as well: either running ssh-agent works, or it does not.

jglick · 2024-07-29T23:39:22Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentBuildWrapper.java

        @Override
        public void buildEnvVars(Map<String, String> env) {
-            env.put("SSH_AUTH_SOCK", agent.getSocket());
+            env.putAll(agent.getEnv());


Probably only this one variable really needs to be passed to nested steps, but ssh-agent(1) suggests you just eval the env, so why not?

jglick · 2024-07-29T23:39:42Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentStep.java

-public class SSHAgentStep extends AbstractStepImpl implements Serializable {
-
-    private static final long serialVersionUID = 1L;
+public class SSHAgentStep extends Step {


Getting rid of long-deprecated Guice usage.

jglick · 2024-07-29T23:39:54Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentStep.java

            return new StandardUsernameListBoxModel()
                    .includeMatchingAs(
-                            item instanceof Queue.Task ? Tasks.getAuthenticationOf((Queue.Task)item) : ACL.SYSTEM,
+                            item instanceof Queue.Task ? Tasks.getAuthenticationOf2((Queue.Task)item) : ACL.SYSTEM2,


Spring Security

jglick · 2024-07-29T23:42:53Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentStepExecution.java

+        } catch (Exception x) {
+            cause.addSuppressed(x);
        }
+        super.stop(cause);


Probably better, so we pass along interruptions and so on as is. Otherwise I think I have seen builds whose terminating error was supposedly running ssh-agent -k, when this was just a side effect.

jglick · 2024-07-29T23:45:33Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentStepExecution.java

-    @Override
-    public void onResume() {
-        super.onResume();
        try {
-            purgeSockets();
-            initRemoteAgent();


This never made sense, and probably contributed to the instability of the plugin. Why would we stop and restart the SSH agent process, running on some agent computer, just because the controller restarted?

jglick · 2024-07-29T23:47:53Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentStepExecution.java

-
-    /**
-     * Purges all socket files created previously.
-     * Especially useful when Jenkins is restarted during the execution of this step.


Something else dating to #4 but of unclear purpose. We already call ssh-agent -k, which deletes the socket file.

jglick · 2024-07-29T23:48:25Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/SSHAgentStepExecution.java

-            File socket = new File(it.next());
-            if (socket.exists()) {


Furthermore, this was looking up the socket path on the controller filesystem, where it would never exist. So this was dead code AFAICT.

jglick · 2024-07-29T23:49:18Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/exec/ExecRemoteAgent.java

+ * Runs a native SSH agent installed on a system.
 */
-public class ExecRemoteAgent implements RemoteAgent {
+public final class ExecRemoteAgent implements Serializable {


OK to serialize now that this merely contains a Map<String, String>, and means that the StepExecution does not need to recreate it.

jglick · 2024-07-29T23:50:19Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/exec/ExecRemoteAgentFactory.java

-    @Override
-    public boolean isSupported(Launcher launcher, final TaskListener listener) {
-        try {             
-            int status = launcher.launch().cmds("ssh-agent", "-k").quiet(true).start().joinWithTimeout(1, TimeUnit.MINUTES, listener);


Pointless overhead; we are just going to run it for real shortly afterwards.

MikeyCarter · 2024-07-30T14:27:05Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/LauncherProvider.java

This file being replaced by something. I've got failures all over the place with the removal of this.

Can you be more specific please? Do you have a Pipeline build which was in the middle of sshagent at the time of an upgrade? I do not think there is a functional test for that yet…

After the server is restarted, with the latest release from a few hours ago of ssh-agent, I have other plugins failing on start because they are looking for LauncherProvider class. Notice in your latest merges/release this class has been removed.

So trying to determine if this LauncherProvider has been replaced with something so I can at the very least alter my own plugin to use the new setup.

other plugins failing on start because they are looking for LauncherProvider

Which plugins? There are no outside references to this class that I am aware of.

if this LauncherProvider has been replaced with something

No.

So yes there are some outside references. I think I see how to modify my own code by looking at the changes. If I figure out the other plugins I'll let you know, chances are if your unaware it's going to be something unregistered.

I do not think there is a functional test for [a Pipeline build which was in the middle of sshagent at the time of an upgrade] yet

Tried to write one, but it looked hard since the plugin depends on a specific process running. Tested interactively and the step survived upgrade.

MikeyCarter · 2024-07-30T14:27:05Z

src/main/java/com/cloudbees/jenkins/plugins/sshagent/LauncherProvider.java

This file being replaced by something. I've got failures all over the place with the removal of this.

Can you be more specific please? Do you have a Pipeline build which was in the middle of sshagent at the time of an upgrade? I do not think there is a functional test for that yet…

After the server is restarted, with the latest release from a few hours ago of ssh-agent, I have other plugins failing on start because they are looking for LauncherProvider class. Notice in your latest merges/release this class has been removed.

So trying to determine if this LauncherProvider has been replaced with something so I can at the very least alter my own plugin to use the new setup.

other plugins failing on start because they are looking for LauncherProvider

Which plugins? There are no outside references to this class that I am aware of.

if this LauncherProvider has been replaced with something

No.

So yes there are some outside references. I think I see how to modify my own code by looking at the changes. If I figure out the other plugins I'll let you know, chances are if your unaware it's going to be something unregistered.

I do not think there is a functional test for [a Pipeline build which was in the middle of sshagent at the time of an upgrade] yet

Tried to write one, but it looked hard since the plugin depends on a specific process running. Tested interactively and the step survived upgrade.

Simplified plugin impl

c84ce1c

jglick added the bug label Jul 29, 2024

jglick commented Jul 29, 2024

View reviewed changes

Better StepExecution.stop impl

8508172

jglick commented Jul 29, 2024

View reviewed changes

jglick added 2 commits July 29, 2024 20:51

Defensive null check as in jenkinsci#134

17ba8bd

jenkinsci/bom#3408 etc. are too new, go back a little bit

87c5c6d

jglick changed the title ~~Simplified plugin impl~~ Simplified plugin impl, removed unreliable onResume Jul 30, 2024

jglick marked this pull request as ready for review July 30, 2024 01:23

jglick requested a review from a team as a code owner July 30, 2024 01:23

julieheard approved these changes Jul 30, 2024

View reviewed changes

jglick merged commit 8933585 into jenkinsci:master Jul 30, 2024

jglick deleted the overhaul branch July 30, 2024 12:13

This was referenced Jul 30, 2024

JENKINS-44308 Avoid killing pre-existing ssh-agent #20

Closed

Overhauled docs #153

Merged

MikeyCarter reviewed Jul 30, 2024

View reviewed changes

jglick mentioned this pull request Aug 9, 2024

Avoid infinite loops due to corrupted flow graphs in some cases and improve resumption error handling jenkinsci/workflow-api-plugin#349

Merged

6 tasks

MikeyCarter reviewed Jul 30, 2024

View reviewed changes

jglick mentioned this pull request Aug 9, 2024

Avoid infinite loops due to corrupted flow graphs in some cases and improve resumption error handling jenkinsci/workflow-api-plugin#349

Merged

6 tasks

		* might change in between usages and where it is not possible to propagate
		* the object in other ways.

Uh oh!

Conversation

jglick commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jglick Jul 29, 2024 •

edited

Loading