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 2, 2020
1 parent 3458dcb commit aa651fa
Show file tree
Hide file tree
Showing 7 changed files with 187 additions and 229 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -3,6 +3,7 @@
### 4.11-SNAPSHOT

#### Bugs
* Fix #2445: ConfigMap and other resources are replaced

#### Improvements

Expand Down
180 changes: 58 additions & 122 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 @@ -474,29 +439,28 @@ public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withLabelSelec
return this;
}

// Deprecated as the underlying implementation does not align with the arguments anymore.
// It is possible to negate multiple values with the same key, e.g.:
// foo != bar , foo != baz
// To support this a multi-value map is needed, as a regular map would override the key with the new value.
/**
* @deprecated as the underlying implementation does not align with the arguments anymore.
* It is possible to negate multiple values with the same key, e.g.:
* foo != bar , foo != baz
* 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 @@ -540,16 +504,17 @@ public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withField(Stri
return this;
}

// Deprecated as the underlying implementation does not align with the arguments fully.
// Method is created to have a similar API as `withoutLabels`, but should eventually be replaced with something
// better for the same reasons.
// It is possible to negate multiple values with the same key, e.g.:
// foo != bar , foo != baz
// To support this a multi-value map is needed, as a regular map would override the key with the new value.
/**
* @deprecated as the underlying implementation does not align with the arguments fully.
* Method is created to have a similar API as `withoutLabels`, but should eventually be replaced
* with something better for the same reasons.
* It is possible to negate multiple values with the same key, e.g.:
* foo != bar , foo != baz
* 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 +620,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 +665,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 +716,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 +818,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 +871,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 +993,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 +1039,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

0 comments on commit aa651fa

Please sign in to comment.