Skip to content

Commit

Permalink
fix: ConfigMap and other resources are replaced
Browse files Browse the repository at this point in the history 
- Reverted **behavior** of BaseOperation#createOrReplace method (and similar)
  to the previous behavior.
  API will always be hit, POST to try to create, and PUT in case POST fails.
  This reverts the behavior trying to mimic kubectl and not performing
  the second PUT request in case the resource is identical to the server version.
  • Loading branch information
@manusa
manusa committed Sep 1, 2020
1 parent 3458dcb commit 777c885
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 121 deletions.
156 changes: 44 additions & 112 deletions kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java
View file
Expand Up @@ -16,7 +16,6 @@
package io.fabric8.kubernetes.client.dsl.base;

import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil;
import io.fabric8.kubernetes.client.utils.ResourceCompare;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -131,26 +130,8 @@ protected BaseOperation(OperationContext ctx) {
this.watchRetryBackoffMultiplier = ctx.getWatchRetryBackoffMultiplier();
}

/**
* Returns the name and falls back to the item name.
* @param item The item.
* @param name The name to check.
* @param <T>
* @return
*/
private static <T> String name(T item, String name) {
if (name != null && !name.isEmpty()) {
return name;
} else if (item instanceof HasMetadata) {
HasMetadata h = (HasMetadata) item;
return h.getMetadata() != null ? h.getMetadata().getName() : null;
}
return null;
}


public BaseOperation<T, L, D, R> newInstance(OperationContext context) {
return new BaseOperation<T, L, D, R>(context);
return new BaseOperation<>(context);
}

/**
Expand Down Expand Up @@ -188,14 +169,8 @@ private void addQueryStringParam(HttpUrl.Builder requestUrlBuilder, String name,
@Override
public T get() {
try {
T answer = getMandatory();
if (answer instanceof HasMetadata) {
HasMetadata hasMetadata = (HasMetadata) answer;
updateApiVersion(hasMetadata);
} else if (answer instanceof KubernetesResourceList) {
KubernetesResourceList list = (KubernetesResourceList) answer;
updateApiVersion(list);
}
final T answer = getMandatory();
updateApiVersion(answer);
return answer;
} catch (KubernetesClientException e) {
if (e.getCode() != HttpURLConnection.HTTP_NOT_FOUND) {
Expand All @@ -206,19 +181,13 @@ public T get() {
}

@Override
public T require() throws ResourceNotFoundException {
public T require() {
try {
T answer = getMandatory();
if (answer == null) {
throw new ResourceNotFoundException("The resource you request doesn't exist or couldn't be fetched.");
}
if (answer instanceof HasMetadata) {
HasMetadata hasMetadata = (HasMetadata) answer;
updateApiVersion(hasMetadata);
} else if (answer instanceof KubernetesResourceList) {
KubernetesResourceList list = (KubernetesResourceList) answer;
updateApiVersion(list);
}
updateApiVersion(answer);
return answer;
} catch (KubernetesClientException e) {
if (e.getCode() != HttpURLConnection.HTTP_NOT_FOUND) {
Expand Down Expand Up @@ -265,7 +234,7 @@ public RootPaths getRootPaths() {
}

@Override
public D edit() throws KubernetesClientException {
public D edit() {
throw new KubernetesClientException("Cannot edit read-only resources");
}

Expand Down Expand Up @@ -332,8 +301,9 @@ public Gettable<T> fromServer() {
return newInstance(context.withReloadingFromServer(true));
}

@SafeVarargs
@Override
public T create(T... resources) throws KubernetesClientException {
public final T create(T... resources) {
try {
if (resources.length > 1) {
throw new IllegalArgumentException("Too many items to create.");
Expand Down Expand Up @@ -363,7 +333,7 @@ public T create(T resource) {
}

@Override
public D createNew() throws KubernetesClientException {
public D createNew() {
final Function<T, T> visitor = resource -> {
try {
return create(resource);
Expand All @@ -381,7 +351,7 @@ public D createNew() throws KubernetesClientException {


@Override
public D createOrReplaceWithNew() throws KubernetesClientException {
public D createOrReplaceWithNew() {
final Function<T, T> visitor = resource -> {
try {
return createOrReplace(resource);
Expand All @@ -397,8 +367,9 @@ public D createOrReplaceWithNew() throws KubernetesClientException {
}
}

@SafeVarargs
@Override
public T createOrReplace(T... items) {
public final T createOrReplace(T... items) {
T itemToCreateOrReplace = getItem();
if (items.length > 1) {
throw new IllegalArgumentException("Too many items to create.");
Expand All @@ -423,16 +394,10 @@ public T createOrReplace(T... items) {
if (exception.getCode() != HttpURLConnection.HTTP_CONFLICT) {
throw exception;
}

// Conflict; Do Replace
T itemFromServer = fromServer().get();
if (ResourceCompare.equals(itemFromServer, itemToCreateOrReplace)) {
// Nothing changed, ignore
return itemToCreateOrReplace;
} else {
KubernetesResourceUtil.setResourceVersion(itemToCreateOrReplace, KubernetesResourceUtil.getResourceVersion(itemFromServer));
return replace(itemToCreateOrReplace);
}
final T itemFromServer = fromServer().get();
KubernetesResourceUtil.setResourceVersion(itemToCreateOrReplace, KubernetesResourceUtil.getResourceVersion(itemFromServer));
return replace(itemToCreateOrReplace);
}
}

Expand Down Expand Up @@ -480,23 +445,20 @@ public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withLabelSelec
// To support this a multi-value map is needed, as a regular map would override the key with the new value.
@Override
@Deprecated
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withoutLabels(Map<String, String> labels) throws
KubernetesClientException {
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withoutLabels(Map<String, String> labels) {
// Re-use "withoutLabel" to convert values from String to String[]
labels.forEach(this::withoutLabel);
return this;
}

@Override
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withLabelIn(String key, String... values) throws
KubernetesClientException {
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withLabelIn(String key, String... values) {
labelsIn.put(key, values);
return this;
}

@Override
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withLabelNotIn(String key, String... values) throws
KubernetesClientException {
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withLabelNotIn(String key, String... values) {
labelsNotIn.put(key, values);
return this;
}
Expand Down Expand Up @@ -548,8 +510,7 @@ public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withField(Stri
// To support this a multi-value map is needed, as a regular map would override the key with the new value.
@Override
@Deprecated
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withoutFields(Map<String, String> fields) throws
KubernetesClientException {
public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withoutFields(Map<String, String> fields) {
// Re-use "withoutField" to convert values from String to String[]
labels.forEach(this::withoutField);
return this;
Expand Down Expand Up @@ -655,7 +616,7 @@ public String getFieldQueryParam() {
return sb.toString();
}

public L list() throws KubernetesClientException {
public L list() {
try {
return listRequestHelper(getResourceUrl(namespace, name));
} catch (IOException e) {
Expand Down Expand Up @@ -700,35 +661,30 @@ public Boolean delete() {
}
}


@SafeVarargs
@Override
public Boolean delete(T... items) {
public final Boolean delete(T... items) {
return delete(Arrays.asList(items));
}

@Override
public Boolean delete(List<T> items) {
boolean deleted = true;
if (items != null) {
for (T item : items) {
if (item == null) {
for (T toDelete : items) {
if (toDelete == null) {
continue;
}
updateApiVersionResource(item);
updateApiVersion(toDelete);

try {
R op;

if (item instanceof HasMetadata
&& ((HasMetadata) item).getMetadata() != null
&& ((HasMetadata) item).getMetadata().getName() != null
&& !((HasMetadata) item).getMetadata().getName().isEmpty()) {
op = (R) inNamespace(checkNamespace(item)).withName(((HasMetadata) item).getMetadata().getName());
if (toDelete.getMetadata() != null
&& toDelete.getMetadata().getName() != null
&& !toDelete.getMetadata().getName().isEmpty()) {
deleted &= inNamespace(checkNamespace(toDelete)).withName(toDelete.getMetadata().getName()).delete();
} else {
op = (R) withItem(item);
deleted &= withItem(toDelete).delete();
}

deleted &= op.delete();
} catch (KubernetesClientException e) {
if (e.getCode() != HttpURLConnection.HTTP_NOT_FOUND) {
throw e;
Expand Down Expand Up @@ -756,7 +712,7 @@ public BaseOperation<T, L, D, R> withItem(T item) {
void deleteThis() {
try {
if (item != null) {
updateApiVersionResource(item);
updateApiVersion(item);
handleDelete(item, gracePeriodSeconds, propagationPolicy, cascading);
} else {
handleDelete(getResourceUrl(), gracePeriodSeconds, propagationPolicy, cascading);
Expand Down Expand Up @@ -858,27 +814,27 @@ public boolean isResourceNamespaced() {
return Utils.isResourceNamespaced(getType());
}

protected T handleResponse(Request.Builder requestBuilder) throws ExecutionException, InterruptedException, KubernetesClientException, IOException {
protected T handleResponse(Request.Builder requestBuilder) throws ExecutionException, InterruptedException, IOException {
return handleResponse(requestBuilder, getType());
}

protected T handleCreate(T resource) throws ExecutionException, InterruptedException, KubernetesClientException, IOException {
updateApiVersionResource(resource);
protected T handleCreate(T resource) throws ExecutionException, InterruptedException, IOException {
updateApiVersion(resource);
return handleCreate(resource, getType());
}

protected T handleReplace(T updated) throws ExecutionException, InterruptedException, KubernetesClientException, IOException {
updateApiVersionResource(updated);
protected T handleReplace(T updated) throws ExecutionException, InterruptedException, IOException {
updateApiVersion(updated);
return handleReplace(updated, getType());
}

protected T handlePatch(T current, T updated) throws ExecutionException, InterruptedException, KubernetesClientException, IOException {
updateApiVersionResource(updated);
protected T handlePatch(T current, T updated) throws ExecutionException, InterruptedException, IOException {
updateApiVersion(updated);
return handlePatch(current, updated, getType());
}

protected T handlePatch(T current, Map<String, Object> patchedUpdate) throws ExecutionException, InterruptedException, IOException {
updateApiVersionResource(current);
updateApiVersion(current);
return handlePatch(current, patchedUpdate, getType());
}

Expand Down Expand Up @@ -911,7 +867,7 @@ protected Status handleDeploymentRollback(DeploymentRollback deploymentRollback)

protected T handleGet(URL resourceUrl) throws InterruptedException, ExecutionException, IOException {
T answer = handleGet(resourceUrl, getType());
updateApiVersionResource(answer);
updateApiVersion(answer);
return answer;
}

Expand Down Expand Up @@ -1033,42 +989,19 @@ protected Class<? extends Config> getConfigType() {
return Config.class;
}

/**
* Updates the list or single item if it has a missing or incorrect apiGroupVersion
*
* @param resource resource object
*/
protected void updateApiVersionResource(Object resource) {
if (resource instanceof HasMetadata) {
HasMetadata hasMetadata = (HasMetadata) resource;
updateApiVersion(hasMetadata);
} else if (resource instanceof KubernetesResourceList) {
KubernetesResourceList list = (KubernetesResourceList) resource;
updateApiVersion(list);
}
}

/**
* Updates the list items if they have missing or default apiGroupVersion values and the resource is currently
* using API Groups with custom version strings
*
* @param list Kubernetes resource list
*/
protected void updateApiVersion(KubernetesResourceList list) {
protected void updateApiVersion(KubernetesResourceList<T> list) {
String version = getApiVersion();
if (list != null && version != null && version.length() > 0) {
List items = list.getItems();
if (items != null) {
for (Object item : items) {
if (item instanceof HasMetadata) {
updateApiVersion((HasMetadata) item);
}
}
}
if (list != null && version != null && version.length() > 0 && list.getItems() != null) {
list.getItems().forEach(this::updateApiVersion);
}
}


/**
* Updates the resource if it has missing or default apiGroupVersion values and the resource is currently
* using API Groups with custom version strings
Expand Down Expand Up @@ -1102,8 +1035,7 @@ public boolean isApiGroup() {

@Override
public Boolean isReady() {
T i = get();
return i instanceof HasMetadata && Readiness.isReady((HasMetadata) i);
return Readiness.isReady(get());
}

@Override
Expand Down
5 changes: 0 additions & 5 deletions ...client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java
View file
Expand Up @@ -58,7 +58,6 @@
import io.fabric8.kubernetes.client.dsl.base.OperationSupport;
import io.fabric8.kubernetes.client.handlers.KubernetesListHandler;
import io.fabric8.kubernetes.client.internal.readiness.Readiness;
import io.fabric8.kubernetes.client.utils.ResourceCompare;
import okhttp3.OkHttpClient;

public class NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl extends OperationSupport implements
Expand Down Expand Up @@ -153,16 +152,12 @@ public HasMetadata createOrReplace() {
}

// Conflict; check deleteExisting flag otherwise replace
HasMetadata r = h.reload(client, config, meta.getMetadata().getNamespace(), meta);
if (Boolean.TRUE.equals(deletingExisting)) {
Boolean deleted = h.delete(client, config, namespaceToUse, propagationPolicy, meta);
if (Boolean.FALSE.equals(deleted)) {
throw new KubernetesClientException("Failed to delete existing item:" + meta);
}
return h.create(client, config, namespaceToUse, meta);
} else if (ResourceCompare.equals(r, meta)) {
LOGGER.debug("Item has not changed. Skipping");
return meta;
} else {
KubernetesResourceUtil.setResourceVersion(meta, resourceVersion);
return h.replace(client, config, namespaceToUse, meta);
Expand Down
4 changes: 0 additions & 4 deletions ...nt/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java
View file
Expand Up @@ -276,15 +276,12 @@ public List<HasMetadata> createOrReplace() {
}

// Conflict; check deleteExisting flag otherwise replace
HasMetadata r = h.reload(client, config, meta.getMetadata().getNamespace(), meta);
if (Boolean.TRUE.equals(deletingExisting)) {
Boolean deleted = h.delete(client, config, namespaceToUse, propagationPolicy, meta);
if (Boolean.FALSE.equals(deleted)) {
throw new KubernetesClientException("Failed to delete existing item:" + meta);
}
result.add(h.create(client, config, namespaceToUse, meta));
} else if (ResourceCompare.equals(r, meta)) {
LOGGER.debug("Item has not changed. Skipping");
} else {
KubernetesResourceUtil.setResourceVersion(meta, resourceVersion);
result.add(h.replace(client, config, namespaceToUse, meta));
Expand All @@ -299,7 +296,6 @@ public Waitable<List<HasMetadata>, HasMetadata> createOrReplaceAnd() {
return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, deletingExisting, visitors, createOrReplace(), inputStream, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier);
}


@Override
public Boolean delete() {
//First pass check before deleting
Expand Down

0 comments on commit 777c885

Please sign in to comment.