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.

Sign up for GitHub

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

Jump to bottom

Revert Google HTTP Client upgrade #1980

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Revert Google HTTP Client upgrade #1980

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0eb91da
Revert Google HTTP Client upgrade
chanseokoh Sep 12, 2019
cb3a279
Fix broken build.gradle
chanseokoh Sep 12, 2019
bf0ddbb
Exclude google-http-client from google-auth-library-oauth2-http
chanseokoh Sep 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 1 addition & 24 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,10 @@ subprojects {
// For Google libraries, check <http-client-bom.version>, <google.auth.version>, <guava.version>,
// ... in https:/googleapis/google-cloud-java/blob/master/google-cloud-clients/pom.xml
// for best compatibility.
GOOGLE_HTTP_CLIENT: '1.31.0',
GOOGLE_HTTP_CLIENT_APACHE_V2: '1.31.0',
GOOGLE_HTTP_CLIENT: '1.27.0',
GOOGLE_AUTH_LIBRARY_OAUTH2_HTTP: '0.16.2',
GUAVA: '28.0-jre',

// TODO: remove once https:/googleapis/google-http-java-client/issues/795 is fixed and released.
// Forcing to downgrade this to 4.5.6 fixes https:/GoogleContainerTools/jib/issues/1914
// However, #795 and upgrading httpclient alone may not fix #1914. We may need to explicitly disable URI
// normalization as discussed in #795.
APACHE_HTTP_CLIENT_OVERRIDE: '4.5.6',
COMMONS_COMPRESS: '1.18',
JACKSON_DATABIND: '2.9.9.2',
ASM: '7.0',
Expand All @@ -67,23 +61,6 @@ subprojects {
SYSTEM_RULES: '1.19.0',
]

// Use this to ensure we correctly override transitive dependencies
// TODO: There might be a plugin that does this
task ensureTransitiveDependencyOverrides {
chanseokoh marked this conversation as resolved.
Show resolved Hide resolved
def rules = ["httpclient": dependencyVersions.APACHE_HTTP_CLIENT_OVERRIDE]
doLast {
configurations.runtimeClasspath.resolvedConfiguration.resolvedArtifacts.each { artifact ->
def dependency = artifact.moduleVersion.id
if (rules[dependency.name] && rules[dependency.name] != dependency.version) {
throw new GradleException(
dependency.name + " version error in " + project
+ ", expected:" + rules[dependency.name]
+ ", found:" + dependency.version);
}
}
}
}
compileJava.dependsOn ensureTransitiveDependencyOverrides
/* PROJECT DEPENDENCY VERSIONS */

/* NULLAWAY */
Expand Down
8 changes: 1 addition & 7 deletions jib-core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,10 @@ plugins {
}

dependencies {
implementation("com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}") {
exclude group: "org.apache.httpcomponents", module: "httpclient"
}
implementation("com.google.http-client:google-http-client-apache-v2:${dependencyVersions.GOOGLE_HTTP_CLIENT_APACHE_V2}") {
exclude group: "org.apache.httpcomponents", module: "httpclient"
}
implementation "com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}"
implementation("com.google.auth:google-auth-library-oauth2-http:${dependencyVersions.GOOGLE_AUTH_LIBRARY_OAUTH2_HTTP}") {
exclude group: "org.apache.httpcomponents", module: "httpclient"
Copy link
Member

Choose a reason for hiding this comment

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

does this still create a conflict?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized it pulls in google-http-client and upgrading 1.27.0 to 1.30.1, so I think I should exclude google-http-client.

+--- com.google.auth:google-auth-library-oauth2-http:0.16.2 
|    +--- com.google.auth:google-auth-library-credentials:0.16.2
|    +--- com.google.http-client:google-http-client:1.30.1 (*)
|    +--- com.google.http-client:google-http-client-jackson2:1.30.1
|    |    +--- com.google.http-client:google-http-client:1.30.1 (*)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if that'll break at runtime then?

Copy link
Member Author

Choose a reason for hiding this comment

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

https:/googleapis/google-cloud-java/blob/v0.80.0/google-cloud-clients/pom.xml recommends

guava.version 26.0 (down from 28.0)
http-client.version 1.27.0 (from 1.31.0)
google.auth.version 0.12.0 (from 0.16.2)

I'll first trying excluding google-http-client from google-auth-library-oauth2-http rather than downgrading it. Will do some test if ADC works on GCB.

Copy link
Member Author

Choose a reason for hiding this comment

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

ADC on GCB works at least.

}
implementation "org.apache.httpcomponents:httpclient:${dependencyVersions.APACHE_HTTP_CLIENT_OVERRIDE}"

implementation "org.apache.commons:commons-compress:${dependencyVersions.COMMONS_COMPRESS}"
implementation "com.google.guava:guava:${dependencyVersions.GUAVA}"
Expand Down
56 changes: 44 additions & 12 deletions jib-core/src/main/java/com/google/cloud/tools/jib/http/Connection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@
import com.google.api.client.http.HttpRequestFactory;
import com.google.api.client.http.HttpResponse;
import com.google.api.client.http.HttpTransport;
import com.google.api.client.http.apache.v2.ApacheHttpTransport;
import com.google.api.client.util.SslUtils;
import com.google.api.client.http.apache.ApacheHttpTransport;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import java.io.Closeable;
Expand All @@ -32,8 +31,9 @@
import java.security.GeneralSecurityException;
import java.util.function.Function;
import javax.annotation.Nullable;
import org.apache.http.conn.ssl.NoopHostnameVerifier;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.impl.client.DefaultHttpClient;

/**
* Sends an HTTP {@link Request} and stores the {@link Response}. Clients should not send more than
Expand Down Expand Up @@ -61,7 +61,9 @@ public static Function<URL, Connection> getConnectionFactory() {
//
// A new ApacheHttpTransport needs to be created for each connection because otherwise HTTP
// connection persistence causes the connection to throw NoHttpResponseException.
return url -> new Connection(url, new ApacheHttpTransport());
ApacheHttpTransport transport = new ApacheHttpTransport();
addProxyCredentials(transport);
return url -> new Connection(url, transport);
}

/**
Expand All @@ -72,14 +74,44 @@ public static Function<URL, Connection> getConnectionFactory() {
*/
public static Function<URL, Connection> getInsecureConnectionFactory()
throws GeneralSecurityException {
HttpClientBuilder httpClientBuilder =
ApacheHttpTransport.newDefaultHttpClientBuilder()
.setSSLSocketFactory(null) // creates new factory with the SSLContext given below
.setSSLContext(SslUtils.trustAllSSLContext())
.setSSLHostnameVerifier(new NoopHostnameVerifier());

// Do not use NetHttpTransport. See comments in getConnectionFactory for details.
return url -> new Connection(url, new ApacheHttpTransport(httpClientBuilder.build()));
ApacheHttpTransport transport =
new ApacheHttpTransport.Builder().doNotValidateCertificate().build();
addProxyCredentials(transport);
return url -> new Connection(url, transport);
}

/**
* Registers proxy credentials onto transport client, in order to deal with proxies that require
* basic authentication.
*
* @param transport Apache HTTP transport
*/
@VisibleForTesting
static void addProxyCredentials(ApacheHttpTransport transport) {
addProxyCredentials(transport, "https");
addProxyCredentials(transport, "http");
}

private static void addProxyCredentials(ApacheHttpTransport transport, String protocol) {
Preconditions.checkArgument(protocol.equals("http") || protocol.equals("https"));

String proxyHost = System.getProperty(protocol + ".proxyHost");
String proxyUser = System.getProperty(protocol + ".proxyUser");
String proxyPassword = System.getProperty(protocol + ".proxyPassword");
if (proxyHost == null || proxyUser == null || proxyPassword == null) {
return;
}

String defaultProxyPort = protocol.equals("http") ? "80" : "443";
int proxyPort = Integer.parseInt(System.getProperty(protocol + ".proxyPort", defaultProxyPort));

DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
httpClient
.getCredentialsProvider()
.setCredentials(
new AuthScope(proxyHost, proxyPort),
new UsernamePasswordCredentials(proxyUser, proxyPassword));
}

private HttpRequestFactory requestFactory;
Expand Down
167 changes: 167 additions & 0 deletions ...ore/src/test/java/com/google/cloud/tools/jib/http/ConnectionWithProxyCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/*
* Copyright 2018 Google LLC.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/

package com.google.cloud.tools.jib.http;

import com.google.api.client.http.apache.ApacheHttpTransport;
import com.google.common.collect.ImmutableList;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.Credentials;
import org.apache.http.impl.client.DefaultHttpClient;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

/** Tests for {@link Connection} with setting proxy credentials. */
public class ConnectionWithProxyCredentialsTest {

private static final ImmutableList<String> proxyProperties =
ImmutableList.of(
"http.proxyHost",
"http.proxyPort",
"http.proxyUser",
"http.proxyPassword",
"https.proxyHost",
"https.proxyPort",
"https.proxyUser",
"https.proxyPassword");

// HashMap to allow saving null values.
private final HashMap<String, String> savedProperties = new HashMap<>();

private final ApacheHttpTransport transport = new ApacheHttpTransport();

@Before
public void setUp() {
proxyProperties.stream().forEach(key -> savedProperties.put(key, System.getProperty(key)));
proxyProperties.stream().forEach(key -> System.clearProperty(key));
}

@After
public void tearDown() {
Consumer<Map.Entry<String, String>> restoreProperty =
entry -> {
if (entry.getValue() == null) {
System.clearProperty(entry.getKey());
} else {
System.setProperty(entry.getKey(), entry.getValue());
}
};
savedProperties.entrySet().stream().forEach(restoreProperty);
}

@Test
public void testAddProxyCredentials_undefined() {
Connection.addProxyCredentials(transport);
DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
Credentials credentials = httpClient.getCredentialsProvider().getCredentials(AuthScope.ANY);
Assert.assertNull(credentials);
}

@Test
public void testAddProxyCredentials() {
System.setProperty("http.proxyHost", "http://localhost");
System.setProperty("http.proxyPort", "1080");
System.setProperty("http.proxyUser", "user");
System.setProperty("http.proxyPassword", "pass");

System.setProperty("https.proxyHost", "https://host.com");
System.setProperty("https.proxyPort", "1443");
System.setProperty("https.proxyUser", "s-user");
System.setProperty("https.proxyPassword", "s-pass");

Connection.addProxyCredentials(transport);
DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
Credentials httpCredentials =
httpClient.getCredentialsProvider().getCredentials(new AuthScope("http://localhost", 1080));
Assert.assertEquals("user", httpCredentials.getUserPrincipal().getName());
Assert.assertEquals("pass", httpCredentials.getPassword());

Credentials httpsCredentials =
httpClient.getCredentialsProvider().getCredentials(new AuthScope("https://host.com", 1443));
Assert.assertEquals("s-user", httpsCredentials.getUserPrincipal().getName());
Assert.assertEquals("s-pass", httpsCredentials.getPassword());
}

@Test
public void testAddProxyCredentials_defaultPorts() {
System.setProperty("http.proxyHost", "http://localhost");
System.setProperty("http.proxyUser", "user");
System.setProperty("http.proxyPassword", "pass");

System.setProperty("https.proxyHost", "https://host.com");
System.setProperty("https.proxyUser", "s-user");
System.setProperty("https.proxyPassword", "s-pass");

Connection.addProxyCredentials(transport);
DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
Credentials httpCredentials =
httpClient.getCredentialsProvider().getCredentials(new AuthScope("http://localhost", 80));
Assert.assertEquals("user", httpCredentials.getUserPrincipal().getName());
Assert.assertEquals("pass", httpCredentials.getPassword());

Credentials httpsCredentials =
httpClient.getCredentialsProvider().getCredentials(new AuthScope("https://host.com", 443));
Assert.assertEquals("s-user", httpsCredentials.getUserPrincipal().getName());
Assert.assertEquals("s-pass", httpsCredentials.getPassword());
}

@Test
public void testAddProxyCredentials_hostUndefined() {
System.setProperty("http.proxyUser", "user");
System.setProperty("http.proxyPassword", "pass");

System.setProperty("https.proxyUser", "s-user");
System.setProperty("https.proxyPassword", "s-pass");

Connection.addProxyCredentials(transport);
DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
Credentials credentials = httpClient.getCredentialsProvider().getCredentials(AuthScope.ANY);
Assert.assertNull(credentials);
}

@Test
public void testAddProxyCredentials_userUndefined() {
System.setProperty("http.proxyHost", "http://localhost");
System.setProperty("http.proxyPassword", "pass");

System.setProperty("https.proxyHost", "https://host.com");
System.setProperty("https.proxyPassword", "s-pass");

Connection.addProxyCredentials(transport);
DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
Credentials credentials = httpClient.getCredentialsProvider().getCredentials(AuthScope.ANY);
Assert.assertNull(credentials);
}

@Test
public void testAddProxyCredentials_passwordUndefined() {
System.setProperty("http.proxyHost", "http://localhost");
System.setProperty("http.proxyUser", "user");

System.setProperty("https.proxyHost", "https://host.com");
System.setProperty("https.proxyUser", "s-user");

Connection.addProxyCredentials(transport);
DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
Credentials credentials = httpClient.getCredentialsProvider().getCredentials(AuthScope.ANY);
Assert.assertNull(credentials);
}
}
2 changes: 1 addition & 1 deletion jib-core/src/test/java/com/google/cloud/tools/jib/http/MockConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package com.google.cloud.tools.jib.http;

import com.google.api.client.http.GenericUrl;
import com.google.api.client.http.apache.v2.ApacheHttpTransport;
import com.google.api.client.http.apache.ApacheHttpTransport;
import java.io.IOException;
import java.util.function.BiFunction;

Expand Down
6 changes: 1 addition & 5 deletions jib-gradle-plugin/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,10 @@ repositories {
}

dependencies {

sourceProject project(":jib-core")
sourceProject project(":jib-plugins-common")

implementation ("com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}") {
exclude group: "org.apache.httpcomponents", module: "httpclient"
}
implementation "org.apache.httpcomponents:httpclient:${dependencyVersions.APACHE_HTTP_CLIENT_OVERRIDE}"
implementation "com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}"
Copy link
Member

Choose a reason for hiding this comment

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

you can remove this, and guava, and jackson

Copy link
Member Author

Choose a reason for hiding this comment

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

Can there be any possibility that removing these is OK with compilation but causes trouble when the user actually uses our plugin by defining Jib in their builds, like not pulling in required dependencies at their run-time?

Copy link
Member

@loosebazooka loosebazooka Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

those dependencies are inherited as runtime implementation from the parent modules.

Copy link
Member

@loosebazooka loosebazooka Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

the reason dependencies still need to be jib-plugins-common is because that library doesn't bring in jib-core as a "sourceProject" (which is defined by us as included). This might be easier to explain in person though since it's all proprietary.

Copy link
Member

Choose a reason for hiding this comment

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

although now that I look at it again, it's pretty dumb, jib-plugins-common shouldn't have to redefine those dependencies. There must be a better way, I'll fix that later.

Copy link
Member Author

Choose a reason for hiding this comment

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

And as of now, jib-plugins-common defines google-http-client, guava, and jackson-databind but not google-auth-library-oauth2-http. Why is that?

Copy link
Member

@loosebazooka loosebazooka Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

it's because it is defined as a regular project dependency (the gradle) way, it doesn't need to include google-auth.... because it doesn't use it.

a dependency defined as implementation on a project is exposed to dependency project as runtime. So they can't use it as a compile time dependency unless they redefine it. The way sourceProject works is that it takes anything defined as implementation and makes it available as implementation on the dependent project as well.

Given that this is confusing, it might be an area to improve the build.


implementation "com.google.guava:guava:${dependencyVersions.GUAVA}"
implementation "com.fasterxml.jackson.core:jackson-databind:${dependencyVersions.JACKSON_DATABIND}"
Expand Down
5 changes: 1 addition & 4 deletions jib-plugins-common/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
dependencies {
implementation project(':jib-core')
implementation ("com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}") {
exclude group: "org.apache.httpcomponents", module: "httpclient"
}
implementation "org.apache.httpcomponents:httpclient:${dependencyVersions.APACHE_HTTP_CLIENT_OVERRIDE}"
implementation "com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}"
implementation "com.google.guava:guava:${dependencyVersions.GUAVA}"
implementation "com.fasterxml.jackson.core:jackson-databind:${dependencyVersions.JACKSON_DATABIND}"

Expand Down