Skip to content

feat: throw exception on problematic owner ference adding in dependent resources #2190

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.

Sign up for GitHub

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 1 commit into from
Jan 9, 2024
Merged

feat: throw exception on problematic owner ference adding in dependent resources #2190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.Locale;
import java.util.Objects;
import java.util.function.Predicate;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import io.fabric8.kubernetes.api.builder.Builder;
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.utils.Serialization;
Expand Down Expand Up @@ -71,6 +74,30 @@ public static String getNameFor(Class<? extends Reconciler> reconcilerClass) {
return getDefaultNameFor(reconcilerClass);
}

public static void checkIfCanAddOwnerReference(HasMetadata owner, HasMetadata resource) {
if (owner instanceof GenericKubernetesResource
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should move this code to the Fabric8 client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, so I would merge this, and will work on that to move it to fabri8 client.
If also @manusa @shawkins thinks it a good idea. Not sure if we should add it as part of an utility, or just simply to .addOwnerReference or both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add it to addOwnerReference, personally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, me too

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be part of HasMetadata#addOwnerReference.

The other suggested alternative would involve exposing the validation method and then calling that method within addOwnerReference too. I don't really see the value of exposing that method.

|| resource instanceof GenericKubernetesResource) {
return;
}
if (owner instanceof Namespaced) {
if (!(resource instanceof Namespaced)) {
throw new OperatorException(
"Cannot add owner reference from a cluster scoped to a namespace scoped resource." +
resourcesIdentifierDescription(owner, resource));
} else if (!Objects.equals(owner.getMetadata().getNamespace(),
resource.getMetadata().getNamespace())) {
throw new OperatorException(
"Cannot add owner reference between two resource in different namespaces." +
resourcesIdentifierDescription(owner, resource));
}
}
}

private static String resourcesIdentifierDescription(HasMetadata owner, HasMetadata resource) {
return " Owner name: " + owner.getMetadata().getName() + " Kind: " + owner.getKind()
+ ", Resource name: " + resource.getMetadata().getName() + " Kind: " + resource.getKind();
}

public static String getNameFor(Reconciler reconciler) {
return getNameFor(reconciler.getClass());
}
Expand Down
2 changes: 2 additions & 0 deletions ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.dsl.Resource;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.config.dependent.Configured;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Constants;
Expand Down Expand Up @@ -236,6 +237,7 @@ protected Resource<R> prepare(Context<P> context, R desired, P primary, String a

protected void addReferenceHandlingMetadata(R desired, P primary) {
if (addOwnerReference()) {
ReconcilerUtils.checkIfCanAddOwnerReference(primary, desired);
desired.addOwnerReference(primary);
} else if (useNonOwnerRefBasedSecondaryToPrimaryMapping()) {
addSecondaryToPrimaryMapperAnnotations(desired, primary);
Expand Down
54 changes: 44 additions & 10 deletions operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ReconcilerUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,12 @@

import org.junit.jupiter.api.Test;

import io.fabric8.kubernetes.api.model.ContainerBuilder;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Namespace;
import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.api.model.Pod;
import io.fabric8.kubernetes.api.model.PodSpec;
import io.fabric8.kubernetes.api.model.PodTemplateSpec;
import io.fabric8.kubernetes.api.model.*;
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
import io.fabric8.kubernetes.api.model.apps.DeploymentSpec;
import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.http.HttpRequest;
Expand All @@ -29,9 +25,7 @@
import static io.javaoperatorsdk.operator.ReconcilerUtils.handleKubernetesClientException;
import static io.javaoperatorsdk.operator.ReconcilerUtils.isFinalizerValid;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -155,6 +149,46 @@ void handleKubernetesExceptionShouldThrowMissingCRDExceptionWhenAppropriate() {
HasMetadata.getFullResourceName(Tomcat.class)));
}


@Test
void checksIfOwnerReferenceCanBeAdded() {
assertThrows(OperatorException.class,
() -> ReconcilerUtils.checkIfCanAddOwnerReference(namespacedResource(),
namespacedResourceFromOtherNamespace()));

assertThrows(OperatorException.class,
() -> ReconcilerUtils.checkIfCanAddOwnerReference(namespacedResource(),
clusterScopedResource()));

assertDoesNotThrow(() -> {
ReconcilerUtils.checkIfCanAddOwnerReference(clusterScopedResource(), clusterScopedResource());
ReconcilerUtils.checkIfCanAddOwnerReference(namespacedResource(), namespacedResource());
});
}

private ClusterRole clusterScopedResource() {
return new ClusterRoleBuilder()
.withMetadata(new ObjectMetaBuilder()
.build())
.build();
}

private ConfigMap namespacedResource() {
return new ConfigMapBuilder()
.withMetadata(new ObjectMetaBuilder()
.withNamespace("testns1")
.build())
.build();
}

private ConfigMap namespacedResourceFromOtherNamespace() {
return new ConfigMapBuilder()
.withMetadata(new ObjectMetaBuilder()
.withNamespace("testns2")
.build())
.build();
}

@Group("tomcatoperator.io")
@Version("v1")
@ShortNames("tc")
Expand Down