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.

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

improve: simpler api for adding additional CRD file #2574

Merged
merged 4 commits into from
Nov 19, 2024
Merged

improve: simpler api for adding additional CRD file #2574

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc0aabf
improve: simpler api for adding additional CRD file
csviri Nov 14, 2024
72309ce
format
csviri Nov 14, 2024
208d1d2
fix
csviri Nov 14, 2024
e0ae283
improve: more crd in one file
csviri Nov 15, 2024
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
76 changes: 46 additions & 30 deletions ...k-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.io.ByteArrayInputStream;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
Expand All @@ -21,6 +22,7 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.LocalPortForward;
Expand All @@ -45,7 +47,7 @@ public class LocallyRunOperatorExtension extends AbstractOperatorExtension {
private final List<LocalPortForward> localPortForwards;
private final List<Class<? extends CustomResource>> additionalCustomResourceDefinitions;
private final Map<Reconciler, RegisteredController> registeredControllers;
private final Map<String, String> crdMappings;
private final List<String> additionalCrds;

private LocallyRunOperatorExtension(
List<ReconcilerSpec> reconcilers,
Expand All @@ -60,7 +62,7 @@ private LocallyRunOperatorExtension(
Consumer<ConfigurationServiceOverrider> configurationServiceOverrider,
Function<ExtensionContext, String> namespaceNameSupplier,
Function<ExtensionContext, String> perClassNamespaceNameSupplier,
Map<String, String> crdMappings) {
List<String> additionalCrds) {
super(
infrastructure,
infrastructureTimeout,
Expand All @@ -80,7 +82,7 @@ private LocallyRunOperatorExtension(
: overrider -> overrider.withKubernetesClient(kubernetesClient);
this.operator = new Operator(configurationServiceOverrider);
this.registeredControllers = new HashMap<>();
this.crdMappings = crdMappings;
this.additionalCrds = additionalCrds;
}

/**
Expand Down Expand Up @@ -119,6 +121,10 @@ public static void applyCrd(String resourceTypeName, KubernetesClient client) {
}
}

public static void applyCrd(CustomResourceDefinition crd, KubernetesClient client) {
client.resource(crd).serverSideApply();
}

private static void applyCrd(InputStream is, String path, KubernetesClient client) {
try {
if (is == null) {
Expand All @@ -138,6 +144,17 @@ private static void applyCrd(InputStream is, String path, KubernetesClient clien
}
}

public static CustomResourceDefinition parseCrd(String path, KubernetesClient client) {
try (InputStream is = new FileInputStream(path)) {
return (CustomResourceDefinition) client.load(new ByteArrayInputStream(is.readAllBytes()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should handle better weird cases: if there are more than just one resource in the file, if the CRD is not a v1 spec, if the resource in the file is not a CRD are the most obvious cases where this might fail.

Copy link
Collaborator Author

@csviri csviri Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update it to handle more CRD in file. I don't think we should support other CRD version that v1, note that this is not the resource definition version inside, CRD api version itself is stable for very long time. (In actual kubernetes release oldel versions are not even supported)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but the APT version of the CRD generator generates v1beta1 as well (and in fact, you can see that these versions are generated in our tests). It would actually be useful not to generate the v1beta1 version to save some time on the tests since we have a lot of CRDs to generate.

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, agree, we should fix that, thus not generate those anymore.

.items().get(0);
} catch (FileNotFoundException e) {
throw new RuntimeException(e);
} catch (IOException e) {
throw new RuntimeException(e);
}
}

private Stream<Reconciler> reconcilers() {
return reconcilers.stream().map(reconcilerSpec -> reconcilerSpec.reconciler);
}
Expand Down Expand Up @@ -190,7 +207,7 @@ protected void before(ExtensionContext context) {
}

additionalCustomResourceDefinitions.forEach(this::applyCrd);

Map<String, CustomResourceDefinition> unappliedCRDs = getAdditionalCRDsFromFiles();
for (var ref : reconcilers) {
final var config = operator.getConfigurationService().getConfigurationFor(ref.reconciler);
final var oconfig = override(config);
Expand All @@ -207,29 +224,40 @@ protected void before(ExtensionContext context) {
ref.controllerConfigurationOverrider.accept(oconfig);
}

final var unapplied = new HashMap<>(crdMappings);
final var resourceTypeName = ReconcilerUtils.getResourceTypeName(resourceClass);
// only try to apply a CRD for the reconciler if it is associated to a CR
if (CustomResource.class.isAssignableFrom(resourceClass)) {
applyCrd(resourceTypeName);
unapplied.remove(resourceTypeName);
if (unappliedCRDs.get(resourceTypeName) != null) {
applyCrd(resourceTypeName);
unappliedCRDs.remove(resourceTypeName);
} else {
applyCrd(resourceClass);
}
}

// apply yet unapplied CRDs
unapplied.keySet().forEach(this::applyCrd);

var registeredController = this.operator.register(ref.reconciler, oconfig.build());
registeredControllers.put(ref.reconciler, registeredController);
}
unappliedCRDs.keySet().forEach(this::applyCrd);

LOGGER.debug("Starting the operator locally");
this.operator.start();
}

private Map<String, CustomResourceDefinition> getAdditionalCRDsFromFiles() {
Map<String, CustomResourceDefinition> crdMappings = new HashMap<>();
additionalCrds.forEach(p -> {
var crd = parseCrd(p, getKubernetesClient());
crdMappings.put(crd.getMetadata().getName(), crd);
});
return crdMappings;
}

/**
* Applies the CRD associated with the specified custom resource, first checking if a CRD has been
* manually specified using {@link Builder#withCRDMapping(Class, String)}, otherwise assuming that
* its CRD should be found in the standard location as explained in
* manually specified using {@link Builder#withAdditionalCRD(String)}, otherwise assuming that its
* CRD should be found in the standard location as explained in
* {@link LocallyRunOperatorExtension#applyCrd(String, KubernetesClient)}
*
* @param crClass the custom resource class for which we want to apply the CRD
Expand All @@ -239,16 +267,7 @@ public void applyCrd(Class<? extends CustomResource> crClass) {
}

public void applyCrd(String resourceTypeName) {
final var path = crdMappings.get(resourceTypeName);
if (path != null) {
try (InputStream inputStream = new FileInputStream(path)) {
applyCrd(inputStream, path, getKubernetesClient());
} catch (IOException e) {
throw new IllegalStateException("Cannot apply CRD yaml: " + path, e);
}
} else {
applyCrd(resourceTypeName, getKubernetesClient());
}
applyCrd(resourceTypeName, getKubernetesClient());
}

@Override
Expand Down Expand Up @@ -277,6 +296,7 @@ public static class Builder extends AbstractBuilder<Builder> {
private final List<PortForwardSpec> portForwards;
private final List<Class<? extends CustomResource>> additionalCustomResourceDefinitions;
private final Map<String, String> crdMappings;
private final List<String> additionalCRDs = new ArrayList<>();
private KubernetesClient kubernetesClient;

protected Builder() {
Expand Down Expand Up @@ -339,13 +359,8 @@ public Builder withAdditionalCustomResourceDefinition(
return this;
}

public Builder withCRDMapping(Class<? extends CustomResource> customResourceClass,
String path) {
return withCRDMapping(ReconcilerUtils.getResourceTypeName(customResourceClass), path);
}

public Builder withCRDMapping(String resourceTypeName, String path) {
crdMappings.put(resourceTypeName, path);
public Builder withAdditionalCRD(String path) {
additionalCRDs.add(path);
return this;
}

Expand All @@ -360,8 +375,9 @@ public LocallyRunOperatorExtension build() {
waitForNamespaceDeletion,
oneNamespacePerClass,
kubernetesClient,
configurationServiceOverrider, namespaceNameSupplier, perClassNamespaceNameSupplier,
crdMappings);
configurationServiceOverrider, namespaceNameSupplier,
perClassNamespaceNameSupplier,
additionalCRDs);
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...ator-framework/src/test/java/io/javaoperatorsdk/operator/CRDMappingInTestExtensionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class CRDMappingInTestExtensionIT {
LocallyRunOperatorExtension operator =
LocallyRunOperatorExtension.builder()
.withReconciler(new TestReconciler())
.withCRDMapping("tests.crd.example", "src/test/crd/test.crd")
.withAdditionalCRD("src/test/resources/crd/test.crd")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was explicitly not put in resources to check that the file could be accessed from a "random" part of the file system, not just from the classpath.

Copy link
Collaborator Author

@csviri csviri Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see, isn't it trivial? I would rather stick with this, but no strong opinion, if you insist I can change it back.

.build();

@Test
Expand Down
0 operator-framework/src/test/crd/test.crd → ...framework/src/test/resources/crd/test.crd
View file
Open in desktop
File renamed without changes.
Loading