Skip to content

feat: generic matcher for Kubernetes dependent resource #945

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

Merged
merged 6 commits into from
Feb 16, 2022
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
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package io.javaoperatorsdk.operator;

import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Locale;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.client.utils.Serialization;
import io.javaoperatorsdk.operator.api.reconciler.Constants;
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
Expand Down Expand Up @@ -111,7 +114,15 @@ public static Object getSpec(HasMetadata resource) {
Method getSpecMethod = resource.getClass().getMethod("getSpec");
return getSpecMethod.invoke(resource);
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
throw new IllegalStateException(e);
throw new IllegalStateException("No spec found on resource", e);
}
}

public static <T> T loadYaml(Class<T> clazz, Class loader, String yaml) {
try (InputStream is = loader.getResourceAsStream(yaml)) {
return Serialization.unmarshal(is, clazz);
} catch (IOException ex) {
throw new IllegalStateException("Cannot find yaml on classpath: " + yaml);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
/** An interface from which to retrieve configuration information. */
public interface ConfigurationService {

Cloner DEFAULT_CLONER = new Cloner() {
private final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
ObjectMapper OBJECT_MAPPER = new ObjectMapper();

Cloner DEFAULT_CLONER = new Cloner() {
@Override
public HasMetadata clone(HasMetadata object) {
try {
Expand Down Expand Up @@ -122,4 +122,8 @@ default ExecutorService getExecutorService() {
default boolean closeClientOnStop() {
return true;
}

default ObjectMapper getObjectMapper() {
return OBJECT_MAPPER;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.zjsonpatch.JsonDiff;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.reconciler.Context;

import com.fasterxml.jackson.databind.ObjectMapper;

public class DesiredValueMatcher implements ResourceMatcher {

private final ObjectMapper objectMapper;

public DesiredValueMatcher(ObjectMapper objectMapper) {
this.objectMapper = objectMapper;
}

@Override
public boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context) {
if (actualResource instanceof Secret) {
return ResourceComparators.compareSecretData((Secret) desiredResource,
(Secret) actualResource);
}
if (actualResource instanceof ConfigMap) {
return ResourceComparators.compareConfigMapData((ConfigMap) desiredResource,
(ConfigMap) actualResource);
}
// reflection will be replaced by this:
// https://github.com/fabric8io/kubernetes-client/issues/3816
var desiredSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(desiredResource));
var actualSpecNode = objectMapper.valueToTree(ReconcilerUtils.getSpec(actualResource));
var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode);
for (int i = 0; i < diffJsonPatch.size(); i++) {
String operation = diffJsonPatch.get(i).get("op").asText();
if (!operation.equals("add")) {
return false;
}
}
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.config.ConfigurationService;
import io.javaoperatorsdk.operator.api.config.Utils;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
Expand All @@ -32,6 +31,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
protected KubernetesClient client;
private InformerEventSource<R, P> informerEventSource;
private boolean addOwnerReference;
protected ResourceMatcher resourceMatcher;

@Override
public void configureWith(KubernetesDependentResourceConfig config) {
Expand All @@ -40,7 +40,7 @@ public void configureWith(KubernetesDependentResourceConfig config) {
}

@SuppressWarnings("unchecked")
private void configureWith(ConfigurationService service, String labelSelector,
private void configureWith(ConfigurationService configService, String labelSelector,
Set<String> namespaces, boolean addOwnerReference) {
final var primaryResourcesRetriever =
(this instanceof PrimaryResourcesRetriever) ? (PrimaryResourcesRetriever<R>) this
Expand All @@ -50,26 +50,28 @@ private void configureWith(ConfigurationService service, String labelSelector,
? (AssociatedSecondaryResourceIdentifier<P>) this
: ResourceID::fromResource;
InformerConfiguration<R, P> ic =
InformerConfiguration.from(service, resourceType())
InformerConfiguration.from(configService, resourceType())
.withLabelSelector(labelSelector)
.withNamespaces(namespaces)
.withPrimaryResourcesRetriever(primaryResourcesRetriever)
.withAssociatedSecondaryResourceIdentifier(secondaryResourceIdentifier)
.build();
this.addOwnerReference = addOwnerReference;
informerEventSource = new InformerEventSource<>(ic, client);
configureWith(configService, new InformerEventSource<>(ic, client), addOwnerReference);
}

/**
* Use to share informers between event more resources.
*
*
* @param configurationService get configs
* @param informerEventSource informer to use
* @param addOwnerReference to the created resource
*/
public void configureWith(InformerEventSource<R, P> informerEventSource,
public void configureWith(ConfigurationService configurationService,
InformerEventSource<R, P> informerEventSource,
boolean addOwnerReference) {
this.informerEventSource = informerEventSource;
this.addOwnerReference = addOwnerReference;
initResourceMatcherIfNotSet(configurationService);
}

protected void beforeCreateOrUpdate(R desired, P primary) {
Expand All @@ -79,8 +81,8 @@ protected void beforeCreateOrUpdate(R desired, P primary) {
}

@Override
protected boolean match(R actual, R target, Context context) {
return ReconcilerUtils.specsEqual(actual, target);
protected boolean match(R actualResource, R desiredResource, Context context) {
return resourceMatcher.match(actualResource, desiredResource, context);
}

@SuppressWarnings("unchecked")
Expand All @@ -107,6 +109,7 @@ protected R update(R actual, R target, P primary, Context context) {

@Override
public Optional<EventSource> eventSource(EventSourceContext<P> context) {
initResourceMatcherIfNotSet(context.getConfigurationService());
if (informerEventSource == null) {
configureWith(context.getConfigurationService(), null, null,
KubernetesDependent.ADD_OWNER_REFERENCE_DEFAULT);
Expand Down Expand Up @@ -144,4 +147,16 @@ public Optional<R> getResource(P primaryResource) {
public void setKubernetesClient(KubernetesClient kubernetesClient) {
this.client = kubernetesClient;
}

/**
* Override this method to configure resource matcher
*
* @param configurationService config service to mainly access object mapper
*/
protected void initResourceMatcherIfNotSet(ConfigurationService configurationService) {
if (resourceMatcher == null) {
resourceMatcher = new DesiredValueMatcher(configurationService.getObjectMapper());
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import java.util.Objects;

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.api.model.Secret;

public class ResourceComparators {

public static boolean compareConfigMapData(ConfigMap c1, ConfigMap c2) {
return Objects.equals(c1.getData(), c2.getData()) &&
Objects.equals(c1.getBinaryData(), c2.getBinaryData());
}

public static boolean compareSecretData(Secret s1, Secret s2) {
return Objects.equals(s1.getType(), s2.getType()) &&
Objects.equals(s1.getData(), s2.getData()) &&
Objects.equals(s1.getStringData(), s2.getStringData());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;

public interface ResourceMatcher {

boolean match(HasMetadata actualResource, HasMetadata desiredResource, Context context);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import org.junit.jupiter.api.Test;

import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.javaoperatorsdk.operator.ReconcilerUtils;

import com.fasterxml.jackson.databind.ObjectMapper;

import static org.assertj.core.api.Assertions.assertThat;

class DesiredValueMatcherTest {

DesiredValueMatcher desiredValueMatcher = new DesiredValueMatcher(new ObjectMapper());

@Test
void checksIfDesiredValuesAreTheSame() {
var target1 = createDeployment();
var desired1 = createDeployment();
assertThat(desiredValueMatcher.match(target1, desired1, null)).isTrue();

var target2 = createDeployment();
var desired2 = createDeployment();
target2.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val");
assertThat(desiredValueMatcher.match(target2, desired2, null))
.withFailMessage("Additive changes should be ok")
.isTrue();

var target3 = createDeployment();
var desired3 = createDeployment();
desired3.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val");
assertThat(desiredValueMatcher.match(target3, desired3, null))
.withFailMessage("Removed value should not be ok")
.isFalse();

var target4 = createDeployment();
var desired4 = createDeployment();
target4.getSpec().setReplicas(2);
assertThat(desiredValueMatcher.match(target4, desired4, null))
.withFailMessage("Changed values are not ok")
.isFalse();
}

Deployment createDeployment() {
Deployment deployment =
ReconcilerUtils.loadYaml(
Deployment.class, DesiredValueMatcherTest.class, "nginx-deployment.yaml");
return deployment;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
apiVersion: apps/v1 # for versions before 1.9.0 use apps/v1beta2
kind: Deployment
metadata:
name: "test"
spec:
progressDeadlineSeconds: 600
revisionHistoryLimit: 10
selector:
matchLabels:
app: "test-dependent"
replicas: 1
template:
metadata:
labels:
app: "test-dependent"
spec:
containers:
- name: nginx
image: nginx:1.17.0
ports:
- containerPort: 80
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.util.List;
import java.util.Objects;

import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.client.KubernetesClient;
Expand Down Expand Up @@ -72,14 +71,5 @@ protected Deployment desired(StandaloneDependentTestCustomResource primary, Cont
deployment.getMetadata().setNamespace(primary.getMetadata().getNamespace());
return deployment;
}

@Override
protected boolean match(Deployment actual, Deployment target, Context context) {
return Objects.equals(actual.getSpec().getReplicas(), target.getSpec().getReplicas()) &&
actual.getSpec().getTemplate().getSpec().getContainers().get(0).getImage()
.equals(
target.getSpec().getTemplate().getSpec().getContainers().get(0).getImage());
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,4 @@ protected Deployment desired(Tomcat tomcat, Context context) {
.build();
return deployment;
}

@Override
public boolean match(Deployment fetched, Deployment target, Context context) {
return fetched.getSpec().getTemplate().getSpec().getContainers().get(0).getImage()
.equals(target.getSpec().getTemplate().getSpec().getContainers().get(0).getImage());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context context) {
WebPageStatus status = new WebPageStatus();

waitUntilConfigMapAvailable(webPage);
status.setHtmlConfigMap(configMapDR.getResource(webPage).get().getMetadata().getName());
status.setHtmlConfigMap(configMapDR.getResource(webPage).orElseThrow().getMetadata().getName());
status.setAreWeGood("Yes!");
status.setErrorMessage(null);
webPage.setStatus(status);
Expand Down Expand Up @@ -114,12 +114,6 @@ protected Deployment desired(WebPage webPage, Context context) {
return deployment;
}

@Override
protected boolean match(Deployment actual, Deployment target, Context context) {
// todo comparator
return true;
}

@Override
protected Class<Deployment> resourceType() {
return Deployment.class;
Expand All @@ -140,11 +134,6 @@ protected Service desired(WebPage webPage, Context context) {
return service;
}

protected boolean match(Service actual, Service target, Context context) {
// todo comparator
return true;
}

@Override
protected Class<Service> resourceType() {
return Service.class;
Expand Down