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: ensures kind is set for generic resource lists #5731

Merged
merged 2 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### 6.11-SNAPSHOT

#### Bugs
* Fix #5729: ensure that kind is set for generic resource lists
* Fix #3032: JUnit5 Kubernetes Extension works with Nested tests

#### Improvements
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.fabric8.kubernetes.api.builder.Visitor;
import io.fabric8.kubernetes.api.model.DefaultKubernetesResourceList;
import io.fabric8.kubernetes.api.model.DeletionPropagation;
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.KubernetesResource;
import io.fabric8.kubernetes.api.model.KubernetesResourceList;
Expand Down Expand Up @@ -438,7 +439,7 @@ public Type getType() {
}
};
CompletableFuture<L> futureAnswer = handleResponse(httpClient, requestBuilder, listTypeReference);
return futureAnswer.thenApply(updateApiVersion());
return futureAnswer.thenApply(this::updateListItems);
} catch (IOException e) {
throw KubernetesClientException.launderThrowable(forOperationType("list"), e);
}
Expand Down Expand Up @@ -873,16 +874,22 @@ protected Class<? extends Config> getConfigType() {

/**
* Updates the list items if they have missing or default apiGroupVersion values and the resource is currently
* using API Groups with custom version strings
* using API Groups with custom version strings, or if they are generic and lack a kind
*/
protected UnaryOperator<L> updateApiVersion() {
return list -> {
String version = apiVersion;
if (list != null && version != null && version.length() > 0 && list.getItems() != null) {
list.getItems().forEach(this::updateApiVersion);
protected L updateListItems(L list) {
if (list != null && list.getItems() != null) {
boolean updateApiVersion = Utils.isNotNullOrEmpty(apiVersion);
boolean updateKind = GenericKubernetesResource.class.isAssignableFrom(getType());
if (updateApiVersion || updateKind) {
for (T item : list.getItems()) {
updateApiVersion(item);
if (updateKind && item != null && item.getKind() == null) {
((GenericKubernetesResource) item).setKind(getKind());
}
}
}
return list;
};
}
return list;
}

/**
Expand All @@ -893,11 +900,11 @@ protected UnaryOperator<L> updateApiVersion() {
*/
protected void updateApiVersion(HasMetadata hasMetadata) {
String version = apiVersion;
if (hasMetadata != null && version != null && version.length() > 0) {
if (hasMetadata != null && Utils.isNotNullOrEmpty(version)) {
String current = hasMetadata.getApiVersion();
// lets overwrite the api version if its currently missing, the resource uses an API Group with '/'
// or we have the default of 'v1' when using an API group version like 'build.openshift.io/v1'
if (current == null || "v1".equals(current) || current.indexOf('/') < 0 && version.indexOf('/') > 0) {
if (current == null || "v1".equals(current) || (current.indexOf('/') < 0 && version.indexOf('/') > 0)) {
hasMetadata.setApiVersion(version);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,14 @@ public boolean isResourceNamespaced() {
return rdc.isNamespaceScoped();
}

@Override
public OperationContext getOperationContext() {
return this.context;
}

@Override
public String getKind() {
return rdc.getKind();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -116,7 +117,7 @@ public void onClose(Executor executor) {

BaseClient(final HttpClient httpClient, Config config, ExecutorSupplier executorSupplier,
KubernetesSerialization kubernetesSerialization) {
this.closable = Collections.newSetFromMap(new WeakHashMap<>());
this.closable = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>()));
Copy link
Member

Choose a reason for hiding this comment

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

Do these changes have anything to do with this PR?

this.closed = new CompletableFuture<>();
this.config = config;
this.httpClient = httpClient;
Expand Down Expand Up @@ -149,21 +150,25 @@ protected void setDerivedFields() {
}

@Override
public synchronized void close() {
public void close() {
if (closed.complete(null) && logger.isDebugEnabled()) {
logger.debug(
"The client and associated httpclient {} have been closed, the usage of this or any client using the httpclient will not work after this",
httpClient.getClass().getName());
}
httpClient.close();
closable.forEach(c -> {
List<AutoCloseable> toClose = null;
synchronized (closable) {
toClose = new ArrayList<>(closable);
closable.clear();
}
toClose.forEach(c -> {
try {
c.close();
} catch (Exception e) {
logger.warn("Error closing resource", e);
}
});
closable.clear();
if (this.executorSupplier != null) {
this.executorSupplier.onClose(executor);
this.executorSupplier = null;
Expand Down Expand Up @@ -412,14 +417,16 @@ public KubernetesSerialization getKubernetesSerialization() {
return kubernetesSerialization;
}

public synchronized void addToCloseable(AutoCloseable closeable) {
if (this.closed.isDone()) {
throw new KubernetesClientException("Client is already closed");
public void addToCloseable(AutoCloseable closeable) {
synchronized (closeable) {
if (this.closed.isDone()) {
throw new KubernetesClientException("Client is already closed");
}
this.closable.add(closeable);
}
this.closable.add(closeable);
}

public synchronized void removeFromCloseable(AutoCloseable closeable) {
public void removeFromCloseable(AutoCloseable closeable) {
this.closable.remove(closeable);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ void testGenericBuiltinResourceWithoutMetadata() {

MixedOperation<GenericKubernetesResource, GenericKubernetesResourceList, Resource<GenericKubernetesResource>> resources = client
.genericKubernetesResources("v1", "ConfigMap");
assertTrue(!resources.list().getItems().isEmpty());
List<GenericKubernetesResource> items = resources.list().getItems();
assertTrue(!items.isEmpty());
assertTrue(items.stream().allMatch(g -> g.getKind().equals("ConfigMap")));
}

@Test
Expand Down
Loading