Skip to content

Commit 7104c6f

Browse files
manusamanusa
committed
fix: BuildConfigs.instantiateBinary().fromFile() does not time out
Signed-off-by: Marc Nuri <[email protected]>
1 parent 8b120bf commit 7104c6f

File tree

9 files changed

+229
-54
lines changed

9 files changed

+229
-54
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
* Fix #4723: [java-generator] Fix a race in the use of JavaParser hitting large CRDs
1717
* Fix #4885: addresses a potential hang in the jdk client with exec stream reading
1818
* Fix #4891: address vertx not completely reading exec streams
19+
* Fix #4899: BuildConfigs.instantiateBinary().fromFile() does not time out
1920

2021
#### Improvements
2122
* Fix #4675: adding a fully client side timeout for informer watches

‎kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -412,10 +412,7 @@ public Type getType() {
412412
}
413413
};
414414
CompletableFuture<L> futureAnswer = handleResponse(httpClient, requestBuilder, listTypeReference);
415-
return futureAnswer.thenApply(l -> {
416-
updateApiVersion(l);
417-
return l;
418-
});
415+
return futureAnswer.thenApply(updateApiVersion());
419416
} catch (IOException e) {
420417
throw KubernetesClientException.launderThrowable(forOperationType("list"), e);
421418
}
@@ -832,14 +829,15 @@ protected Class<? extends Config> getConfigType() {
832829
/**
833830
* Updates the list items if they have missing or default apiGroupVersion values and the resource is currently
834831
* using API Groups with custom version strings
835-
*
836-
* @param list Kubernetes resource list
837832
*/
838-
protected void updateApiVersion(KubernetesResourceList<T> list) {
839-
String version = apiVersion;
840-
if (list != null && version != null && version.length() > 0 && list.getItems() != null) {
841-
list.getItems().forEach(this::updateApiVersion);
842-
}
833+
protected UnaryOperator<L> updateApiVersion() {
834+
return list -> {
835+
String version = apiVersion;
836+
if (list != null && version != null && version.length() > 0 && list.getItems() != null) {
837+
list.getItems().forEach(this::updateApiVersion);
838+
}
839+
return list;
840+
};
843841
}
844842

845843
/**

‎kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/OperationContext.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.fabric8.kubernetes.client.Client;
2222
import io.fabric8.kubernetes.client.Config;
2323
import io.fabric8.kubernetes.client.RequestConfig;
24+
import io.fabric8.kubernetes.client.RequestConfigBuilder;
2425
import io.fabric8.kubernetes.client.dsl.FieldValidateable;
2526
import io.fabric8.kubernetes.client.dsl.FieldValidateable.Validation;
2627
import io.fabric8.kubernetes.client.http.HttpClient;
@@ -117,7 +118,7 @@ public OperationContext(Client client, String plural, String namespace, String n
117118
this.forceConflicts = forceConflicts;
118119
this.timeout = timeout;
119120
this.timeoutUnit = timeoutUnit;
120-
this.requestConfig = requestConfig;
121+
this.requestConfig = requestConfig == null ? null : new RequestConfigBuilder(requestConfig).build();
121122
}
122123

123124
private void setFieldsNot(Map<String, String[]> fieldsNot) {

‎kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupport.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -737,9 +737,9 @@ public OperationContext getOperationContext() {
737737
public RequestConfig getRequestConfig() {
738738
RequestConfig result = context.getRequestConfig();
739739
if (result == null && config != null) {
740-
return config.getRequestConfig();
740+
result = config.getRequestConfig();
741741
}
742-
return new RequestConfigBuilder().build();
742+
return new RequestConfigBuilder(result).build();
743743
}
744744

745745
private String getContentTypeFromPatchContextOrDefault(PatchContext patchContext) {

‎kubernetes-client/src/test/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupportTest.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@
1919
import io.fabric8.kubernetes.api.model.PodBuilder;
2020
import io.fabric8.kubernetes.client.Client;
2121
import io.fabric8.kubernetes.client.Config;
22+
import io.fabric8.kubernetes.client.ConfigBuilder;
2223
import io.fabric8.kubernetes.client.KubernetesClientException;
24+
import io.fabric8.kubernetes.client.RequestConfigBuilder;
2325
import io.fabric8.kubernetes.client.http.HttpRequest;
2426
import io.fabric8.kubernetes.client.http.HttpResponse;
2527
import io.fabric8.kubernetes.client.http.StandardHttpRequest;
@@ -50,7 +52,10 @@ class OperationSupportTest {
5052
@BeforeEach
5153
void setUp() {
5254
final OperationContext context = new OperationContext().withClient(mock(Client.class, RETURNS_DEEP_STUBS));
53-
when(context.getClient().getConfiguration()).thenReturn(Config.empty());
55+
final Config globalConfig = new ConfigBuilder(Config.empty())
56+
.withRequestTimeout(313373)
57+
.build();
58+
when(context.getClient().getConfiguration()).thenReturn(globalConfig);
5459
operationSupport = new OperationSupport(context);
5560
}
5661

@@ -170,4 +175,17 @@ void getResourceURLStatus() throws MalformedURLException {
170175
}, "status requires name");
171176
}
172177

178+
@Test
179+
void getRequestConfigReturnsFromGlobalConfigByDefault() {
180+
assertThat(operationSupport.getRequestConfig())
181+
.hasFieldOrPropertyWithValue("requestTimeout", 313373);
182+
}
183+
184+
@Test
185+
void getRequestConfigReturnsFromContextIfPresent() {
186+
assertThat(operationSupport.getOperationContext()
187+
.withRequestConfig(new RequestConfigBuilder().withRequestTimeout(1337).build()).getRequestConfig())
188+
.hasFieldOrPropertyWithValue("requestTimeout", 1337);
189+
}
190+
173191
}

‎kubernetes-itests/src/test/java/io/fabric8/openshift/BuildIT.java‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/**
2+
* Copyright (C) 2015 Red Hat, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.fabric8.openshift;
18+
19+
import io.fabric8.junit.jupiter.api.KubernetesTest;
20+
import io.fabric8.junit.jupiter.api.RequireK8sSupport;
21+
import io.fabric8.kubernetes.api.model.ObjectReferenceBuilder;
22+
import io.fabric8.kubernetes.api.model.Pod;
23+
import io.fabric8.openshift.api.model.Build;
24+
import io.fabric8.openshift.api.model.BuildConfig;
25+
import io.fabric8.openshift.api.model.BuildConfigBuilder;
26+
import io.fabric8.openshift.api.model.BuildOutputBuilder;
27+
import io.fabric8.openshift.api.model.BuildStrategyBuilder;
28+
import io.fabric8.openshift.api.model.ImageStream;
29+
import io.fabric8.openshift.api.model.ImageStreamBuilder;
30+
import io.fabric8.openshift.api.model.SourceBuildStrategyBuilder;
31+
import io.fabric8.openshift.client.OpenShiftClient;
32+
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
33+
import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream;
34+
import org.junit.jupiter.api.AfterEach;
35+
import org.junit.jupiter.api.BeforeEach;
36+
import org.junit.jupiter.api.io.TempDir;
37+
import org.junit.jupiter.params.ParameterizedTest;
38+
import org.junit.jupiter.params.provider.ValueSource;
39+
40+
import java.io.File;
41+
import java.io.FileOutputStream;
42+
import java.io.IOException;
43+
import java.nio.file.Path;
44+
import java.util.UUID;
45+
import java.util.concurrent.TimeUnit;
46+
47+
import static org.assertj.core.api.Assertions.assertThat;
48+
49+
@KubernetesTest(createEphemeralNamespace = false)
50+
@RequireK8sSupport(Build.class)
51+
class BuildIT {
52+
53+
@TempDir
54+
private Path tempDir;
55+
private String imageStreamName;
56+
private String imageStreamTag;
57+
private String buildConfigName;
58+
private String buildName;
59+
OpenShiftClient client;
60+
61+
@BeforeEach
62+
void setUp() {
63+
final String id = UUID.randomUUID().toString().replace("-", "");
64+
imageStreamName = id + "-is";
65+
imageStreamTag = imageStreamName + ":latest";
66+
buildConfigName = id + "-bc";
67+
buildName = id + "-s2i";
68+
}
69+
70+
@AfterEach
71+
void tearDown() {
72+
client.builds().withName(buildName).withGracePeriod(0L).delete();
73+
client.buildConfigs().withName(buildConfigName).withGracePeriod(0L).delete();
74+
client.imageStreams().withName(imageStreamName).withGracePeriod(0L).delete();
75+
}
76+
77+
@ParameterizedTest(name = "OpenShift build from file of size {0} bytes builds ImageStream")
78+
@ValueSource(ints = {
79+
1024,
80+
10485760//, 314572800 // Big file to test-drive upload timeout (disabled for CI)
81+
})
82+
void buildImage(int archiveSize) throws IOException {
83+
// Given
84+
final BuildConfig buildConfig = new BuildConfigBuilder()
85+
.withNewMetadata().withName(buildConfigName).endMetadata()
86+
.withNewSpec()
87+
.withNewSource().withType("Binary").endSource()
88+
.withStrategy(new BuildStrategyBuilder()
89+
.withType("Source")
90+
.withSourceStrategy(new SourceBuildStrategyBuilder()
91+
.withFrom(new ObjectReferenceBuilder()
92+
.withKind("DockerImage")
93+
.withName("quay.io/jkube/jkube-java:0.0.17")
94+
.build())
95+
.build())
96+
.build())
97+
.withOutput(new BuildOutputBuilder()
98+
.withTo(new ObjectReferenceBuilder()
99+
.withKind("ImageStreamTag")
100+
.withName(imageStreamTag)
101+
.build())
102+
.build())
103+
.endSpec().build();
104+
final ImageStream is = new ImageStreamBuilder()
105+
.withNewMetadata().withName(imageStreamName).endMetadata()
106+
.withNewSpec().withNewLookupPolicy().withLocal(true).endLookupPolicy().endSpec()
107+
.build();
108+
client.resourceList(buildConfig, is).create();
109+
// When
110+
final Build build = client.buildConfigs().withName(buildConfigName).instantiateBinary()
111+
.fromFile(prepareTarFile(archiveSize));
112+
// Then
113+
client.pods().withName(build.getMetadata().getName() + "-build")
114+
.waitUntilReady(60, TimeUnit.SECONDS);
115+
final Pod buildPod = client.pods()
116+
.waitUntilCondition(pod -> !pod.getStatus().getPhase().equals("Running"), 60, TimeUnit.SECONDS);
117+
assertThat(buildPod)
118+
.hasFieldOrPropertyWithValue("status.phase", "Succeeded");
119+
}
120+
121+
private File prepareTarFile(int size) throws IOException {
122+
final File tarFile = tempDir.resolve("test-" + size + ".tar").toFile();
123+
try (FileOutputStream fos = new FileOutputStream(tarFile);
124+
TarArchiveOutputStream tarArchiveOutputStream = new TarArchiveOutputStream(fos)) {
125+
tarArchiveOutputStream.setBigNumberMode(TarArchiveOutputStream.BIGNUMBER_POSIX);
126+
tarArchiveOutputStream.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX);
127+
final TarArchiveEntry file = new TarArchiveEntry("deployments/file.txt");
128+
file.setMode(TarArchiveEntry.DEFAULT_FILE_MODE);
129+
file.setSize(size);
130+
tarArchiveOutputStream.putArchiveEntry(file);
131+
tarArchiveOutputStream.write(new byte[(int) size]);
132+
tarArchiveOutputStream.closeArchiveEntry();
133+
134+
}
135+
return tarFile;
136+
}
137+
}

‎openshift-client/pom.xml‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,13 @@
6161
<groupId>io.fabric8</groupId>
6262
<artifactId>openshift-client-api</artifactId>
6363
</dependency>
64-
64+
6565
<dependency>
6666
<groupId>com.github.mifmif</groupId>
6767
<artifactId>generex</artifactId>
6868
<version>${generex.version}</version>
6969
</dependency>
70-
70+
7171
<!-- Compile Only Dependencies -->
7272
<dependency>
7373
<groupId>io.sundr</groupId>
@@ -78,7 +78,7 @@
7878
<groupId>io.sundr</groupId>
7979
<artifactId>transform-annotations</artifactId>
8080
</dependency>
81-
81+
8282
<dependency>
8383
<groupId>org.projectlombok</groupId>
8484
<artifactId>lombok</artifactId>
@@ -100,7 +100,7 @@
100100
<type>test-jar</type>
101101
<scope>test</scope>
102102
</dependency>
103-
103+
104104
<dependency>
105105
<groupId>io.fabric8</groupId>
106106
<artifactId>kubernetes-client-api</artifactId>
@@ -128,14 +128,13 @@
128128
</dependency>
129129

130130
<dependency>
131-
<groupId>org.junit.jupiter</groupId>
132-
<artifactId>junit-jupiter-migrationsupport</artifactId>
131+
<groupId>org.mockito</groupId>
132+
<artifactId>mockito-core</artifactId>
133133
<scope>test</scope>
134134
</dependency>
135-
136135
<dependency>
137-
<groupId>org.mockito</groupId>
138-
<artifactId>mockito-core</artifactId>
136+
<groupId>org.assertj</groupId>
137+
<artifactId>assertj-core</artifactId>
139138
<scope>test</scope>
140139
</dependency>
141140
</dependencies>

‎openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/build/BuildConfigOperationsImpl.java‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import io.fabric8.kubernetes.api.model.EventList;
2121
import io.fabric8.kubernetes.client.Client;
2222
import io.fabric8.kubernetes.client.KubernetesClientException;
23+
import io.fabric8.kubernetes.client.RequestConfigBuilder;
2324
import io.fabric8.kubernetes.client.dsl.Triggerable;
2425
import io.fabric8.kubernetes.client.dsl.Typeable;
2526
import io.fabric8.kubernetes.client.dsl.internal.HasMetadataOperation;
@@ -66,8 +67,6 @@ public class BuildConfigOperationsImpl
6667
CommitterAuthorMessageAsFileTimeoutInputStreamable<Build> {
6768

6869
private static final Logger logger = LoggerFactory.getLogger(BuildConfigOperationsImpl.class);
69-
public static final String BUILD_CONFIG_LABEL = "openshift.io/build-config.name";
70-
public static final String BUILD_CONFIG_ANNOTATION = "openshift.io/build-config.name";
7170

7271
private final BuildConfigOperationContext buildConfigOperationContext;
7372
private final String secret;
@@ -82,7 +81,8 @@ public class BuildConfigOperationsImpl
8281
private final String asFile;
8382

8483
public BuildConfigOperationsImpl(Client client) {
85-
this(new BuildConfigOperationContext(), HasMetadataOperationsImpl.defaultContext(client));
84+
this(new BuildConfigOperationContext(), HasMetadataOperationsImpl.defaultContext(client).withRequestConfig(
85+
new RequestConfigBuilder(client.getConfiguration().getRequestConfig()).withRequestTimeout(0).build()));
8686
}
8787

8888
public BuildConfigOperationsImpl(BuildConfigOperationContext context, OperationContext superContext) {
@@ -241,7 +241,11 @@ public CommitterEmailable<AuthorMessageAsFileTimeoutInputStreamable<Build>> with
241241

242242
@Override
243243
public BuildConfigOperationsImpl withTimeout(long timeout, TimeUnit unit) {
244-
return new BuildConfigOperationsImpl(getContext(), context.withTimeout(timeout, unit));
244+
return new BuildConfigOperationsImpl(getContext(), context
245+
.withTimeout(timeout, unit)
246+
.withRequestConfig(new RequestConfigBuilder(context.getRequestConfig())
247+
.withRequestTimeout((int) unit.toMillis(timeout))
248+
.build()));
245249
}
246250

247251
@Override
@@ -257,8 +261,8 @@ public Typeable<Triggerable<WebHookTrigger, Void>> withSecret(String secret) {
257261
protected Build submitToApiServer(InputStream inputStream, long contentLength) {
258262
try {
259263
HttpClient newClient = this.httpClient.newBuilder()
260-
.readTimeout(this.context.getTimeout(), this.context.getTimeoutUnit())
261-
.writeTimeout(this.context.getTimeout(), this.context.getTimeoutUnit())
264+
.readTimeout(getOperationContext().getTimeout(), getOperationContext().getTimeoutUnit())
265+
.writeTimeout(getOperationContext().getTimeout(), getOperationContext().getTimeoutUnit())
262266
.build();
263267
HttpRequest.Builder requestBuilder = this.httpClient.newHttpRequestBuilder()
264268
.post("application/octet-stream", inputStream, contentLength)
@@ -270,7 +274,7 @@ public Type getType() {
270274
return Build.class;
271275
}
272276
}));
273-
} catch (Throwable e) {
277+
} catch (Exception e) {
274278
// TODO: better determine which exception this should occur on
275279
// otherwise we need to have the httpclient api open up to the notion
276280
// of a RequestBody/BodyPublisher

0 commit comments

Comments
 (0)