Enhance bootstrap launcher to download artifacts using global credentials. #1991

greg-a-atkinson · 2021-03-07T22:32:13Z

Possible implementation for issue: #1774

Within my organisation we use a private authenticated Artifactory instance to serve all internal and external repositories. Ideally we would like to retrieve artifacts through this repository and not via a proxy. (A proxy requires domain credentials, while Artifactory only requires a user specific Artifactry API key, which more secure for adding to configuration files/properties/variables than domain credentials.)

When I configure the following mirror.properties:

central.from=https://repo1.maven.org/maven2
central.to=https://private.artifactory.com/artifactory/mavencentral
jcenter.from=https://jcenter.bintray.com
jcenter.to=https://private.artifactory.com/artifactory/jcenter
typesafe.from=https://repo.typesafe.com/typesafe/ivy-releases
typesafe.to=https://private.artifactory.com/artifactory/typesafe-ivy-releases

Most artifact retrievals succeed, but artifacts retrieved by the bootstrap launcher fail. For example, the Scala Metals plugin for VSCode fails to download various artifacts due to 401 Unauthorized errors when the bootstrap launcher does not use the configured global credentials:

Java home: C:\Users\user\working\tools\jdk1.8.0_261-x64
Metals version: 0.9.10
Error while downloading https://private.artifactory.com/artifactory/virtual-java/com/chuusai/shapeless_2.12/2.3.3/shapeless_2.12-2.3.3.jar: Server returned HTTP response code: 401 for URL: https://private.artifactory.com/artifactory/virtual-java/com/chuusai/shapeless_2.12/2.3.3/shapeless_2.12-2.3.3.jar, ignoring it
Error while downloading https://private.artifactory.com/artifactory/virtual-java/com/github/alexarchambault/case-app-annotations_2.12/2.0.0-M9/case-app-annotations_2.12-2.0.0-M9.jar: Server returned HTTP response code: 401 for URL: https://private.artifactory.com/artifactory/virtual-java/com/github/alexarchambault/case-app-annotations_2.12/2.0.0-M9/case-app-annotations_2.12-2.0.0-M9.jar, ignoring it
Error while downloading https://private.artifactory.com/artifactory/virtual-scala/com.lightbend/emoji_2.12/1.2.1/jars/emoji_2.12.jar: Server returned HTTP response code: 401 for URL: https://private.artifactory.com/artifactory/virtual-scala/com.lightbend/emoji_2.12/1.2.1/jars/emoji_2.12.jar, ignoring it
Error while downloading https://private.artifactory.com/artifactory/virtual-java/com/github/alexarchambault/argonaut-shapeless_6.2_2.12/1.2.0-M11/argonaut-shapeless_6.2_2.12-1.2.0-M11.jar: Server returned HTTP response code: 401 for URL: https://private.artifactory.com/artifactory/virtual-java/com/github/alexarchambault/argonaut-shapeless_6.2_2.12/1.2.0-M11/argonaut-shapeless_6.2_2.12-1.2.0-M11.jar, ignoring it

The bootstrap launcher is a Java-only module, which makes sense as it will only have use of the core Java libraries before any dependencies are downloaded, but as a consequence it does not have access to the current Scala-based Credentials functionality. If ideally global credentials is used from bootstraps, the this could be implemented in a number of ways, each with different pros and cons:

The bootstrap-launcher module is enhanced with just enough Java-based functionality to utilize the global credentials without any changes to the existing, out-of-scope Scala-based credentials functionality. Pros: No change to the the Scala Credentials API or implementation. Cons: Some duplication.
A new or exisitng Java-only module, like paths, could be enhanced with the complete credentials functionality, the Scala API updated to delegate to the Java implementation and the boostrap-launcher updated to utilize the common credentials functionality. Pros: A single credentials implementation. Cons: Change, compatibility and larger initiative.
Something else?

Is there a preferred implementation in mind?

What do you think?

Happy to discuss.

alexarchambault · 2021-03-08T14:56:12Z

Thanks a lot for taking a cut at this!

About how to add this, I think your point 2. should be the way to go, in the long term. The mirror logic is already implemented this way, via paths (whose name should probably be changed now, as it does more than just dealing with paths). But it doesn't seem straightforward without breaking binary compatibility, yes. And the companion objects of coursier.credentials.{Credentials, DirectCredentials, FileCredentials} make it harder to write binary compatibility equivalents in Java.

I guess we can leave it as is in a first time (so your point 1., and the current state of the PR), and work on making this PR mergeable. And at a later point in time, either find a way to use the Java-based Credentials everywhere, or wait for the next version bump and break binary compatibility.

In order to run the integration tests you added, we need to start an authenticated mirror repository, right? Do you think you can add that? (I've got to run right now, but there are examples of Sonatype instances being spawn in the code, I can point them later today, if needed).

greg-a-atkinson · 2021-03-08T15:49:56Z

Thanks for your reply. I agree.

Implementation (1) is a subset and requirement for implementation (2) so I thought it a reasonable place to start. In fact I had ported more of the code than is included in the pull request, but chopped it down to what was absolutely necessary.

I'll have a look at spinning up a Sonatype instance in the integration test. I had seen logging for this in the test suite, so I'm sure I can find an example. I developed the change against an Artifactory instance I spun up in a container so, I expect integrating with an in-memory repository should be straight forward. Thanks for the suggestion.

alexarchambault · 2021-03-08T21:07:01Z

Spawning an Artifactory would be fine too. A Sonatype server is spawned via docker around here for example.

By the way, if it's more practical, I think it should be do-able to write the tests in Scala (while keeping the main module in Java). And DockerServer.scala could be moved there if needed.

greg-a-atkinson · 2021-03-08T23:32:51Z

I found AuthenticatedProxyTests, CentralNexus2ProxyTests and CentralNexus3ProxyTests depending on DockerServer.scala, but also found AuthenticationTests and HttpAuthenticationTests simply depending on the environment variables TEST_REPOSITORY, TEST_REPOSITORY_USER and TEST_REPOSITORY_PASSWORD.

I updated DownloadTest to utilise the test repository environment variables and these seemed to do the trick.

What do you think of the updated pull request?

greg-a-atkinson · 2021-03-09T09:26:16Z

Yes and it should be possible to convert the tests. Let me update the PR...

greg-a-atkinson · 2021-03-09T16:56:09Z

Hi @alexarchambault.

The DownloadTest integration test has been updated to test authentication by accessing the TEST_REPOSITORY and the tests are in Scala while the main source tree is Java-only.

Please let me know if there's anything else.

Thanks

alexarchambault

Thanks @greg-a-atkinson!

Looks good overall, I only left some minor comments.

alexarchambault · 2021-03-10T12:39:31Z

build.sbt

+    libraryDependencies  += Deps.collectionCompat % Test,
+    libraryDependencies  += Deps.java8Compat % Test,


NIT Let's merge those:

Suggested change

libraryDependencies += Deps.collectionCompat % Test,

libraryDependencies += Deps.java8Compat % Test,

libraryDependencies ++= Seq(

Deps.collectionCompat % Test,

Deps.java8Compat % Test

),

alexarchambault · 2021-03-10T12:39:59Z

modules/bootstrap-launcher/src/it/scala/coursier/bootstrap/launcher/DownloadTests.scala

@@ -0,0 +1,105 @@
+package coursier.bootstrap.launcher;


NIT

Suggested change

package coursier.bootstrap.launcher;

package coursier.bootstrap.launcher

alexarchambault · 2021-03-10T12:40:45Z

modules/bootstrap-launcher/src/it/scala/coursier/bootstrap/launcher/DownloadTests.scala

+          val remoteUrls = Seq(
+            new URL("https://repo1.maven.org/maven2/com/chuusai/shapeless_2.12/2.3.3/shapeless_2.12-2.3.3.jar")
+          )
+          val localUrls = Download.getLocalURLs(remoteUrls.asJava).asScala;


Suggested change

val localUrls = Download.getLocalURLs(remoteUrls.asJava).asScala;

val localUrls = Download.getLocalURLs(remoteUrls.asJava).asScala

alexarchambault · 2021-03-10T12:55:12Z

project/Deps.scala

@@ -20,6 +21,7 @@ object Deps {
  def caseApp = "com.github.alexarchambault" %% "case-app" % "2.0.0"
  def catsCore = "org.typelevel" %% "cats-core" % "2.2.0"
  def collectionCompat = "org.scala-lang.modules" %% "scala-collection-compat" % versions.collectionCompat
+  def java8Compat = "org.scala-lang.modules" %% "scala-java8-compat" % versions.java8Compat


No need to factor the version, as it's only used once.

Suggested change

def java8Compat = "org.scala-lang.modules" %% "scala-java8-compat" % versions.java8Compat

def java8Compat = "org.scala-lang.modules" %% "scala-java8-compat" % "0.9.1"

Also, this line should be moved between http4sDsl and jimfs, to keep dependencies sorted.

alexarchambault · 2021-03-10T13:19:38Z

modules/bootstrap-launcher/src/it/scala/coursier/bootstrap/launcher/DownloadTests.scala

+    test("public URL") {
+      withTmpDir { dir =>
+        try {
+          Download.setCacheDir(dir.toFile)


I think the tests might run in parallel, which would be a problem here… To work around that, we can remove the static initializer on Download, and make its methods non-static. #2000 does that, don't hesitate to rebase your work on it.

It should allow to just do

val download = new Download(dir.toFile, …) download.getLocalURLs(…)

so that each test has its own Download instance.

This is fine with me. I see that the bootstrap-launcher doesn't enforce source and binary compatibility. Are you going to merge #2000? Then I'll rebase.

Thanks

There's no binary compatibility checks for bootstrap-launcher, as it's not meant to be used as a library (the same applies to the cli module). I just merged #2000, just feel free to rebase.

That's done: branch rebased and review comments applied. Please let me know if I missed anything. Thanks

alexarchambault · 2021-03-11T10:14:05Z

modules/bootstrap-launcher/src/it/scala/coursier/bootstrap/launcher/DownloadTests.scala

+        val remoteUrls = Seq(
+          new URL("https://repo1.maven.org/maven2/com/chuusai/shapeless_2.12/2.3.3/shapeless_2.12-2.3.3.jar")
+        )
+        val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.emptyList())


Shouldn't dir be used here?

Suggested change

val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.emptyList())

val download = new Download(1, dir, Collections.emptyList())

Thanks for double checking. Yes, CachePath.defaultCacheDirectory() should have been dir.toFile. PR updated and checks out.

alexarchambault · 2021-03-11T10:14:26Z

modules/bootstrap-launcher/src/it/scala/coursier/bootstrap/launcher/DownloadTests.scala

+          new URL(s"$testRepository/com/abc/test/0.1/test-0.1.pom"
+            .replaceFirst(testRepositoryHost, s"$testRepositoryUser:$testRepositoryPassword@$testRepositoryHost"))
+        )
+        val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.emptyList())


Same as above:

Suggested change

val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.emptyList())

val download = new Download(1, dir, Collections.emptyList())

alexarchambault · 2021-03-11T10:14:49Z

modules/bootstrap-launcher/src/it/scala/coursier/bootstrap/launcher/DownloadTests.scala

+        val remoteUrls = Seq(
+          new URL(s"$testRepository/com/abc/test/0.1/test-0.1.pom")
+        )
+        val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.singletonList(


Same as above:

Suggested change

val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.singletonList(

val download = new Download(1, dir, Collections.singletonList(

…ials.

alexarchambault

Thanks @greg-a-atkinson!

greg-a-atkinson · 2021-03-11T12:51:03Z

Great. Thank you @alexarchambault

greg-a-atkinson mentioned this pull request Mar 7, 2021

cs bootstrap / cs install launcher script not working with private -r repositories #1774

Closed

greg-a-atkinson force-pushed the bootstrap_credentials branch from 1cf70ad to d738f5c Compare March 7, 2021 22:43

greg-a-atkinson mentioned this pull request Mar 7, 2021

Abandoned pull request. #1990

Closed

greg-a-atkinson force-pushed the bootstrap_credentials branch 2 times, most recently from 0fd2afe to 1162d51 Compare March 7, 2021 23:03

greg-a-atkinson force-pushed the bootstrap_credentials branch 8 times, most recently from 00ec61f to 0540311 Compare March 8, 2021 23:10

greg-a-atkinson force-pushed the bootstrap_credentials branch 3 times, most recently from c1fe79b to 4d48fbd Compare March 9, 2021 15:29

alexarchambault mentioned this pull request Mar 10, 2021

No more static initializer in Download (bootstrap-launcher) #2000

Merged

alexarchambault reviewed Mar 10, 2021

View reviewed changes

greg-a-atkinson force-pushed the bootstrap_credentials branch 3 times, most recently from 69b40a0 to 8078a1e Compare March 10, 2021 22:22

alexarchambault reviewed Mar 11, 2021

View reviewed changes

Enhance bootstrap launcher to download artifacts using global credent…

1562eba

…ials.

greg-a-atkinson force-pushed the bootstrap_credentials branch from d0ae1e7 to 1562eba Compare March 11, 2021 11:32

nit

723e592

alexarchambault approved these changes Mar 11, 2021

View reviewed changes

alexarchambault merged commit 2ec1545 into coursier:master Mar 11, 2021

greg-a-atkinson mentioned this pull request Mar 11, 2021

Upgrade Coursier from 2.0.12 to 2.0.13 for bootstrap launcher enhancement to download artifacts using global credentials. scalameta/metals#2597

Merged

greg-a-atkinson deleted the bootstrap_credentials branch March 12, 2021 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance bootstrap launcher to download artifacts using global credentials. #1991

Enhance bootstrap launcher to download artifacts using global credentials. #1991

greg-a-atkinson commented Mar 7, 2021 •

edited

alexarchambault commented Mar 8, 2021

greg-a-atkinson commented Mar 8, 2021

alexarchambault commented Mar 8, 2021

greg-a-atkinson commented Mar 8, 2021

greg-a-atkinson commented Mar 9, 2021

greg-a-atkinson commented Mar 9, 2021

alexarchambault left a comment

alexarchambault Mar 10, 2021

alexarchambault Mar 10, 2021

alexarchambault Mar 10, 2021

alexarchambault Mar 10, 2021

alexarchambault Mar 10, 2021

alexarchambault Mar 10, 2021

greg-a-atkinson Mar 10, 2021

alexarchambault Mar 10, 2021

greg-a-atkinson Mar 10, 2021

alexarchambault Mar 11, 2021

greg-a-atkinson Mar 11, 2021

alexarchambault Mar 11, 2021

alexarchambault Mar 11, 2021

alexarchambault left a comment

greg-a-atkinson commented Mar 11, 2021

		libraryDependencies += Deps.collectionCompat % Test,
		libraryDependencies += Deps.java8Compat % Test,

	package coursier.bootstrap.launcher;
	package coursier.bootstrap.launcher

	val localUrls = Download.getLocalURLs(remoteUrls.asJava).asScala;
	val localUrls = Download.getLocalURLs(remoteUrls.asJava).asScala

	def java8Compat = "org.scala-lang.modules" %% "scala-java8-compat" % versions.java8Compat
	def java8Compat = "org.scala-lang.modules" %% "scala-java8-compat" % "0.9.1"

	val download = new Download(1, CachePath.defaultCacheDirectory(), Collections.emptyList())
	val download = new Download(1, dir, Collections.emptyList())

Enhance bootstrap launcher to download artifacts using global credentials. #1991

Enhance bootstrap launcher to download artifacts using global credentials. #1991

Conversation

greg-a-atkinson commented Mar 7, 2021 • edited

alexarchambault commented Mar 8, 2021

greg-a-atkinson commented Mar 8, 2021

alexarchambault commented Mar 8, 2021

greg-a-atkinson commented Mar 8, 2021

greg-a-atkinson commented Mar 9, 2021

greg-a-atkinson commented Mar 9, 2021

alexarchambault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexarchambault left a comment

Choose a reason for hiding this comment

greg-a-atkinson commented Mar 11, 2021

greg-a-atkinson commented Mar 7, 2021 •

edited