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

fix: configure ssh connect timeout value #2394

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

pearsonradu
Copy link
Contributor

@pearsonradu pearsonradu commented Mar 15, 2024

Modify PropertyBasedSshSessionFactory and FileBasedSshSessionFactory to configure SshConstants.CONNECT_TIMEOUT based on the supplied timeout property.

This is a work in progress and I'm looking for feedback.

I created #2393 as a temporary solution to #2256 (comment).

What I'm really after is setting the SSH read timeout for Git.

I'm using the CONNECT_TIMEOUT key from JGit to hopefully accomplish this.

@spencergibb
Copy link
Member

Can you please provide a better description than WIP, otherwise it may be better to work on this in your own branch before you submit a PR.

@pearsonradu
Copy link
Contributor Author

Can you please provide a better description than WIP, otherwise it may be better to work on this in your own branch before you submit a PR.

Sorry, I hit draft by accident. Please let me know if you require additional details.

@pearsonradu pearsonradu changed the title wip fix: configure the ssh read timeout Mar 15, 2024
@pearsonradu
Copy link
Contributor Author

I'm looking for feedback on:

  • Am I understanding this JGit property correctly? For example, will a request timeout on a fetch call?
  • Is this the correct class to make this change? There is also FileBasedSshSessionFactory that may need changes.

@ryanjbaxter
Copy link
Contributor

It seems to make sense on my reading of the code, but you would have to test it

@pearsonradu
Copy link
Contributor Author

Testing the timeout is a bit challenging because I don't have any malfunctioning Git servers and I wouldn't be able to introduce lag if I stood one up.

Sadly, the SshConstants.CONNECT_TIMEOUT is processed as Duration#ofSeconds so I can't use milliseconds to test the timeout easily. I modified the JGit code to use Duration#ofMillis, updated Spring Cloud Config to use my locally generated version and was able to see a timeout on clone.

Without modifying the JGit code, SshdSessionFactory#getSession can be overridden in PropertyBasedSshSessionFactory with the below example. The tms parameter is used in connect if the SshConstants.CONNECTION_TIMEOUT is not provided. Luckily the tms parameter is already processed as milliseconds so it doesn't require modifying the JGit source. I was able to see the same timeout as modifying the JGit code directly.

To me this is enough to show the flag is working, unfortunately I can't witness fetch timeouts on subsequent calls to Spring Cloud Config Server because the timeout will fail on startup. Please let me know if I'm ok to proceed and I'll look into adding tests to Spring Cloud Config, but it is unlikely I'll be able to test something so fine grained like this in the project.

    @Override
	public SshdSession getSession(URIish uri, CredentialsProvider credentialsProvider, FS fs, int tms) throws TransportException {
		return super.getSession(uri, credentialsProvider, fs, 50);
	}
Stacktrace
  2024-03-18T12:08:55.007-04:00  WARN 68152 --- [           main] .c.s.e.MultipleJGitEnvironmentRepository : Error occured cloning to base directory.

  org.eclipse.jgit.api.errors.TransportException: <REMOVED>: DefaultConnectFuture[[email protected]/<REMOVED>:22]: Failed to get operation result within specified timeout: 5 msec
      at org.eclipse.jgit.api.FetchCommand.call(FetchCommand.java:249) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.api.CloneCommand.fetch(CloneCommand.java:319) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.api.CloneCommand.call(CloneCommand.java:189) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.springframework.cloud.config.server.environment.JGitEnvironmentRepository.cloneToBasedir(JGitEnvironmentRepository.java:657) ~[classes/:na]
      at org.springframework.cloud.config.server.environment.JGitEnvironmentRepository.initClonedRepository(JGitEnvironmentRepository.java:362) ~[classes/:na]
      at org.springframework.cloud.config.server.environment.JGitEnvironmentRepository.afterPropertiesSet(JGitEnvironmentRepository.java:283) ~[classes/:na]
      at org.springframework.cloud.config.server.environment.MultipleJGitEnvironmentRepository.afterPropertiesSet(MultipleJGitEnvironmentRepository.java:71) ~[classes/:na]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods(AbstractAutowireCapableBeanFactory.java:1833) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1782) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:600) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:522) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:326) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:324) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.config.DependencyDescriptor.resolveCandidate(DependencyDescriptor.java:254) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.addCandidateEntry(DefaultListableBeanFactory.java:1689) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1653) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveMultipleBeanCollection(DefaultListableBeanFactory.java:1543) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveMultipleBeans(DefaultListableBeanFactory.java:1511) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1392) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1353) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:904) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:782) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:542) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1335) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1165) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:562) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:522) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:326) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:324) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.config.DependencyDescriptor.resolveCandidate(DependencyDescriptor.java:254) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1443) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1353) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:904) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:782) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:542) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1335) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1165) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:562) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:522) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:326) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:324) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.config.DependencyDescriptor.resolveCandidate(DependencyDescriptor.java:254) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.addCandidateEntry(DefaultListableBeanFactory.java:1689) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1653) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveMultipleBeanMap(DefaultListableBeanFactory.java:1575) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveMultipleBeans(DefaultListableBeanFactory.java:1514) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1392) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1353) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:904) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:782) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:542) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1335) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1165) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:562) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:522) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:326) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:324) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:975) ~[spring-beans-6.1.5.jar:6.1.5]
      at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:962) ~[spring-context-6.1.5.jar:6.1.5]
      at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:624) ~[spring-context-6.1.5.jar:6.1.5]
      at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:146) ~[spring-boot-3.2.4-20240318.160536-50.jar:3.2.4-SNAPSHOT]
      at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:754) ~[spring-boot-3.2.4-20240318.160536-50.jar:3.2.4-SNAPSHOT]
      at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:456) ~[spring-boot-3.2.4-20240318.160536-50.jar:3.2.4-SNAPSHOT]
      at org.springframework.boot.SpringApplication.run(SpringApplication.java:334) ~[spring-boot-3.2.4-20240318.160536-50.jar:3.2.4-SNAPSHOT]
      at org.springframework.boot.SpringApplication.run(SpringApplication.java:1354) ~[spring-boot-3.2.4-20240318.160536-50.jar:3.2.4-SNAPSHOT]
      at org.springframework.boot.SpringApplication.run(SpringApplication.java:1343) ~[spring-boot-3.2.4-20240318.160536-50.jar:3.2.4-SNAPSHOT]
      at sample.Application.main(Application.java:37) ~[classes/:na]
  Caused by: org.eclipse.jgit.errors.TransportException: <REMOVED>: DefaultConnectFuture[[email protected]/<REMOVED>:22]: Failed to get operation result within specified timeout: 5 msec
      at org.eclipse.jgit.transport.sshd.SshdSessionFactory.getSession(SshdSessionFactory.java:267) ~[org.eclipse.jgit.ssh.apache-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.springframework.cloud.config.server.ssh.PropertyBasedSshSessionFactory.getSession(PropertyBasedSshSessionFactory.java:74) ~[classes/:na]
      at org.springframework.cloud.config.server.ssh.PropertyBasedSshSessionFactory.getSession(PropertyBasedSshSessionFactory.java:61) ~[classes/:na]
      at org.eclipse.jgit.transport.SshTransport.getSession(SshTransport.java:107) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.TransportGitSsh$SshFetchConnection.<init>(TransportGitSsh.java:279) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.TransportGitSsh.openFetch(TransportGitSsh.java:152) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.FetchProcess.executeImp(FetchProcess.java:153) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.FetchProcess.execute(FetchProcess.java:105) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.Transport.fetch(Transport.java:1482) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.api.FetchCommand.call(FetchCommand.java:238) ~[org.eclipse.jgit-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      ... 74 common frames omitted
  Caused by: org.apache.sshd.common.SshException: DefaultConnectFuture[[email protected]/<REMOVED>:22]: Failed to get operation result within specified timeout: 5 msec
      at org.apache.sshd.common.future.AbstractSshFuture.lambda$verifyResult$1(AbstractSshFuture.java:114) ~[sshd-osgi-2.12.0.jar:2.12.0]
      at org.apache.sshd.common.future.AbstractSshFuture.formatExceptionMessage(AbstractSshFuture.java:206) ~[sshd-osgi-2.12.0.jar:2.12.0]
      at org.apache.sshd.common.future.AbstractSshFuture.verifyResult(AbstractSshFuture.java:114) ~[sshd-osgi-2.12.0.jar:2.12.0]
      at org.apache.sshd.client.future.DefaultConnectFuture.verify(DefaultConnectFuture.java:55) ~[sshd-osgi-2.12.0.jar:2.12.0]
      at org.apache.sshd.client.future.DefaultConnectFuture.verify(DefaultConnectFuture.java:36) ~[sshd-osgi-2.12.0.jar:2.12.0]
      at org.eclipse.jgit.transport.sshd.SshdSession.connect(SshdSession.java:216) ~[org.eclipse.jgit.ssh.apache-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.sshd.SshdSession.connect(SshdSession.java:147) ~[org.eclipse.jgit.ssh.apache-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.sshd.SshdSession.connect(SshdSession.java:101) ~[org.eclipse.jgit.ssh.apache-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      at org.eclipse.jgit.transport.sshd.SshdSessionFactory.getSession(SshdSessionFactory.java:260) ~[org.eclipse.jgit.ssh.apache-6.10.0-SNAPSHOT.jar:6.10.0-SNAPSHOT]
      ... 83 common frames omitted
  Caused by: java.util.concurrent.TimeoutException: Timed out after 5 msec
      at org.apache.sshd.common.future.AbstractSshFuture.verifyResult(AbstractSshFuture.java:113) ~[sshd-osgi-2.12.0.jar:2.12.0]
      ... 89 common frames omitted

Modify `PropertyBasedSshSessionFactory` and `FileBasedSshSessionFactory` to configure `SshConstants.CONNECT_TIMEOUT` based on the supplied timeout property
@pearsonradu pearsonradu changed the title fix: configure the ssh read timeout fix: configure ssh connect timeout value Mar 18, 2024
@pearsonradu pearsonradu marked this pull request as ready for review March 25, 2024 11:51
@ryanjbaxter ryanjbaxter added this to the 4.1.2 milestone Mar 28, 2024
@ryanjbaxter ryanjbaxter merged commit de0c0e1 into spring-cloud:main Mar 28, 2024
1 check passed
@pearsonradu pearsonradu deleted the refresh-timeout branch March 28, 2024 18:10
@marnee01
Copy link
Contributor

marnee01 commented May 30, 2024

We've always had a concern about knowing if and how we can set timeout for SSH. If it's now supporting configuration of a SSH read and/or connection time (and didn't support it before?), was there a corresponding documentation update that explains how to configure it? It wasn't obvious to me by scanning over the code changes.

@pearsonradu
Copy link
Contributor Author

We've always had a concern about knowing if and how we can set timeout for SSH. If it's now supporting configuration of a SSH read and/or connection time (and didn't support it before?), was there a corresponding documentation update that explains how to configure it? It wasn't obvious to me by scanning over the code changes.

Documentation was added in #2396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants