Skip to content

Commit 47fb5f1

Browse files
metacosmmetacosm
authored
feat: improve handling & reporting around shared CSV metadata (#610)
* feat: log bundle assignment for each reconciler * feat: fail when reconcilers use the same bundle without shared metadata Theoretically, it should be possible to gather all the metadata pieces with the same name and create an aggregated version, only throwing an error in case of conflicting information but it's much cleaner and maintainable to define explicitly shared CSV metadata.
1 parent 1b4af7a commit 47fb5f1

File tree

11 files changed

+248
-38
lines changed

11 files changed

+248
-38
lines changed

‎bundle-generator/deployment/src/main/java/io/quarkiverse/operatorsdk/bundle/deployment/BundleProcessor.java‎

Lines changed: 76 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,12 @@
1111
import java.util.LinkedList;
1212
import java.util.List;
1313
import java.util.Map;
14-
import java.util.Objects;
1514
import java.util.Optional;
1615
import java.util.function.Function;
1716
import java.util.stream.Collectors;
1817

1918
import org.jboss.jandex.AnnotationInstance;
2019
import org.jboss.jandex.AnnotationValue;
21-
import org.jboss.jandex.ClassInfo;
2220
import org.jboss.jandex.DotName;
2321
import org.jboss.jandex.IndexView;
2422
import org.jboss.logging.Logger;
@@ -61,8 +59,8 @@ CSVMetadataBuildItem gatherCSVMetadata(ApplicationInfoBuildItem configuration,
6159
BundleGenerationConfiguration bundleConfiguration,
6260
CombinedIndexBuildItem combinedIndexBuildItem) {
6361
final var index = combinedIndexBuildItem.getIndex();
64-
final var operatorCsvMetadata = getCSVMetadataForOperator(
65-
bundleConfiguration.packageName.orElse(configuration.getName()), index);
62+
final var defaultName = bundleConfiguration.packageName.orElse(configuration.getName());
63+
final var sharedMetadataHolders = getSharedMetadataHolders(defaultName, index);
6664
final var csvGroups = new HashMap<CSVMetadataHolder, List<ReconcilerAugmentedClassInfo>>();
6765

6866
ClassUtils.getKnownReconcilers(index, log)
@@ -72,14 +70,52 @@ CSVMetadataBuildItem gatherCSVMetadata(ApplicationInfoBuildItem configuration,
7270
log.debugv("Processing reconciler: {0}", name);
7371

7472
// Check whether the reconciler must be shipped using a custom bundle
75-
CSVMetadataHolder csvMetadata = getCsvMetadataFromAnnotation(operatorCsvMetadata,
76-
reconcilerInfo.classInfo()).orElse(operatorCsvMetadata);
73+
final var csvMetadataAnnotation = reconcilerInfo.classInfo()
74+
.declaredAnnotation(CSV_METADATA);
75+
final var maybeCSVMetadataName = getCSVMetadataName(csvMetadataAnnotation);
76+
final var csvMetadataName = maybeCSVMetadataName.orElse(defaultName);
77+
78+
var csvMetadata = sharedMetadataHolders.get(csvMetadataName);
79+
if (csvMetadata == null) {
80+
final var origin = reconcilerInfo.classInfo().name().toString();
81+
82+
if (!csvMetadataName.equals(defaultName)) {
83+
final var maybeExistingOrigin = csvGroups.keySet().stream()
84+
.filter(mh -> mh.name.equals(csvMetadataName))
85+
.map(CSVMetadataHolder::getOrigin)
86+
.findFirst();
87+
if (maybeExistingOrigin.isPresent()) {
88+
throw new IllegalStateException("Reconcilers '" + maybeExistingOrigin.get()
89+
+ "' and '" + origin
90+
+ "' are using the same bundle name '" + csvMetadataName
91+
+ "' but no SharedCSVMetadata implementation with that name exists. Please create a SharedCSVMetadata with that name to have one single source of truth and reference it via CSVMetadata annotations using that name on your reconcilers.");
92+
}
93+
}
94+
csvMetadata = createMetadataHolder(csvMetadataAnnotation,
95+
new CSVMetadataHolder(csvMetadataName, origin));
96+
}
97+
log.infov("Assigning ''{0}'' reconciler to {1}",
98+
reconcilerInfo.nameOrFailIfUnset(),
99+
getMetadataOriginInformation(csvMetadataAnnotation, maybeCSVMetadataName, csvMetadata));
100+
77101
csvGroups.computeIfAbsent(csvMetadata, m -> new ArrayList<>()).add(reconcilerInfo);
78102
});
79103

80104
return new CSVMetadataBuildItem(csvGroups);
81105
}
82106

107+
private String getMetadataOriginInformation(AnnotationInstance csvMetadataAnnotation, Optional<String> csvMetadataName,
108+
CSVMetadataHolder metadataHolder) {
109+
final var isDefault = csvMetadataAnnotation == null;
110+
final var actualName = metadataHolder.name;
111+
if (isDefault) {
112+
return "default bundle automatically named '" + actualName + "'";
113+
} else {
114+
return "bundle " + (csvMetadataName.isEmpty() ? "automatically " : "") + "named '"
115+
+ actualName + "' defined by '" + metadataHolder.getOrigin() + "'";
116+
}
117+
}
118+
83119
@SuppressWarnings("unused")
84120
@BuildStep
85121
void generateBundle(ApplicationInfoBuildItem configuration,
@@ -169,27 +205,44 @@ void generateBundle(ApplicationInfoBuildItem configuration,
169205
}
170206
}
171207

172-
private CSVMetadataHolder getCSVMetadataForOperator(String name, IndexView index) {
173-
CSVMetadataHolder csvMetadata = new CSVMetadataHolder(name);
174-
csvMetadata = aggregateMetadataFromSharedCsvMetadata(csvMetadata, index);
175-
return csvMetadata;
208+
private Map<String, CSVMetadataHolder> getSharedMetadataHolders(String name, IndexView index) {
209+
CSVMetadataHolder csvMetadata = new CSVMetadataHolder(name, "default");
210+
final var sharedMetadataImpls = index.getAllKnownImplementors(SHARED_CSV_METADATA);
211+
final var result = new HashMap<String, CSVMetadataHolder>(sharedMetadataImpls.size() + 1);
212+
sharedMetadataImpls.forEach(sharedMetadataImpl -> {
213+
final var csvMetadataAnn = sharedMetadataImpl.declaredAnnotation(CSV_METADATA);
214+
if (csvMetadataAnn != null) {
215+
final var origin = sharedMetadataImpl.name().toString();
216+
final var metadataHolder = createMetadataHolder(csvMetadataAnn, csvMetadata, origin);
217+
final var existing = result.get(metadataHolder.name);
218+
if (existing != null) {
219+
throw new IllegalStateException(
220+
"Only one SharedCSVMetadata named " + metadataHolder.name
221+
+ " can be defined. Was defined on (at least): " + existing.getOrigin() + " and " + origin);
222+
}
223+
result.put(metadataHolder.name, metadataHolder);
224+
}
225+
});
226+
return result;
176227
}
177228

178-
private Optional<CSVMetadataHolder> getCsvMetadataFromAnnotation(CSVMetadataHolder holder, ClassInfo info) {
179-
return Optional.ofNullable(info.declaredAnnotation(CSV_METADATA))
180-
.map(annotationInstance -> createMetadataHolder(annotationInstance, holder));
229+
private Optional<String> getCSVMetadataName(AnnotationInstance csvMetadataAnnotation) {
230+
return Optional.ofNullable(csvMetadataAnnotation)
231+
.map(annotation -> annotation.value("name"))
232+
.map(AnnotationValue::asString);
181233
}
182234

183-
private CSVMetadataHolder aggregateMetadataFromSharedCsvMetadata(CSVMetadataHolder holder, IndexView index) {
184-
return index.getAllKnownImplementors(SHARED_CSV_METADATA).stream()
185-
.map(t -> t.declaredAnnotation(CSV_METADATA))
186-
.filter(Objects::nonNull)
187-
.map(annotation -> createMetadataHolder(annotation, holder))
188-
.findFirst()
189-
.orElse(holder);
235+
private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata,
236+
CSVMetadataHolder mh) {
237+
return createMetadataHolder(csvMetadata, mh, mh.getOrigin());
190238
}
191239

192-
private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, CSVMetadataHolder mh) {
240+
private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, CSVMetadataHolder mh,
241+
String origin) {
242+
if (csvMetadata == null) {
243+
return mh;
244+
}
245+
193246
final var providerField = csvMetadata.value("provider");
194247
String providerName = null;
195248
String providerURL = null;
@@ -363,6 +416,7 @@ private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, C
363416
icon,
364417
installModes,
365418
permissionRules,
366-
requiredCRDs);
419+
requiredCRDs,
420+
origin);
367421
}
368422
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/MultipleOperatorsBundleTest.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@
2121

2222
public class MultipleOperatorsBundleTest {
2323

24-
private static final String BUNDLE = "bundle";
25-
2624
@RegisterExtension
2725
static final QuarkusProdModeTest config = new QuarkusProdModeTest()
2826
.withApplicationRoot((jar) -> jar
@@ -38,7 +36,7 @@ public class MultipleOperatorsBundleTest {
3836

3937
@Test
4038
public void shouldWriteBundleForTheOperators() throws IOException {
41-
final var bundle = prodModeTestResults.getBuildDir().resolve(BUNDLE);
39+
final var bundle = prodModeTestResults.getBuildDir().resolve(Utils.BUNDLE);
4240
checkBundleFor(bundle, "first-operator", First.class);
4341
checkBundleFor(bundle, "second-operator", Second.class);
4442

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/MultipleReconcilerWithSharedMetadataBundleTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package io.quarkiverse.operatorsdk.bundle;
2+
3+
import static io.quarkiverse.operatorsdk.bundle.Utils.checkBundleFor;
4+
5+
import java.io.IOException;
6+
7+
import org.junit.jupiter.api.Test;
8+
import org.junit.jupiter.api.extension.RegisterExtension;
9+
10+
import io.quarkiverse.operatorsdk.bundle.sources.AReconciler;
11+
import io.quarkiverse.operatorsdk.bundle.sources.BReconciler;
12+
import io.quarkiverse.operatorsdk.bundle.sources.CReconciler;
13+
import io.quarkus.test.ProdBuildResults;
14+
import io.quarkus.test.ProdModeTestResults;
15+
import io.quarkus.test.QuarkusProdModeTest;
16+
17+
public class MultipleReconcilerWithSharedMetadataBundleTest {
18+
19+
public static final String APPLICATION_NAME = "application-name";
20+
@RegisterExtension
21+
static final QuarkusProdModeTest config = new QuarkusProdModeTest()
22+
.withApplicationRoot((jar) -> jar.addClasses(AReconciler.class, BReconciler.class, CReconciler.class))
23+
.setApplicationName(APPLICATION_NAME);
24+
25+
@SuppressWarnings("unused")
26+
@ProdBuildResults
27+
private ProdModeTestResults prodModeTestResults;
28+
29+
@Test
30+
public void shouldWriteBundleForTheOperators() throws IOException {
31+
final var bundle = prodModeTestResults.getBuildDir().resolve(Utils.BUNDLE);
32+
checkBundleFor(bundle, "shared", null);
33+
// reconcilers with no csv metadata should use the application name
34+
checkBundleFor(bundle, APPLICATION_NAME, null);
35+
}
36+
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/MultipleReconcilerWithoutSharedMetadataBundleTest.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package io.quarkiverse.operatorsdk.bundle;
2+
3+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
4+
import static org.junit.jupiter.api.Assertions.assertTrue;
5+
import static org.junit.jupiter.api.Assertions.fail;
6+
7+
import org.junit.jupiter.api.Test;
8+
import org.junit.jupiter.api.extension.RegisterExtension;
9+
10+
import io.quarkiverse.operatorsdk.bundle.sources.DuplicatedBundleNameWithoutSharedCSVMetadata1;
11+
import io.quarkiverse.operatorsdk.bundle.sources.DuplicatedBundleNameWithoutSharedCSVMetadata2;
12+
import io.quarkus.test.ProdBuildResults;
13+
import io.quarkus.test.ProdModeTestResults;
14+
import io.quarkus.test.QuarkusProdModeTest;
15+
16+
public class MultipleReconcilerWithoutSharedMetadataBundleTest {
17+
18+
public static final String APPLICATION_NAME = "incorrect-metadata-sharing";
19+
@RegisterExtension
20+
static final QuarkusProdModeTest config = new QuarkusProdModeTest()
21+
.withApplicationRoot((jar) -> jar.addClasses(
22+
DuplicatedBundleNameWithoutSharedCSVMetadata1.class, DuplicatedBundleNameWithoutSharedCSVMetadata2.class))
23+
.setApplicationName(APPLICATION_NAME)
24+
.assertBuildException(e -> {
25+
// exception should be a runtime exception wrapping a build exception wrapping our own exception
26+
assertInstanceOf(IllegalStateException.class, e.getCause().getCause());
27+
assertTrue(e.getMessage().contains(DuplicatedBundleNameWithoutSharedCSVMetadata1.class.getName()));
28+
assertTrue(e.getMessage()
29+
.contains(DuplicatedBundleNameWithoutSharedCSVMetadata2.class.getName()));
30+
});
31+
32+
@SuppressWarnings("unused")
33+
@ProdBuildResults
34+
private ProdModeTestResults prodModeTestResults;
35+
36+
@Test
37+
public void shouldWriteBundleForTheOperators() {
38+
fail("Should have failed at build time");
39+
}
40+
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/Utils.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
public class Utils {
1212

13+
static final String BUNDLE = "bundle";
14+
1315
static void checkBundleFor(Path bundle, String operatorName,
1416
Class<? extends HasMetadata> resourceClass) {
1517
final var operatorManifests = bundle.resolve(operatorName);

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/sources/AReconciler.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package io.quarkiverse.operatorsdk.bundle.sources;
2+
3+
import io.fabric8.kubernetes.api.model.ConfigMap;
4+
import io.javaoperatorsdk.operator.api.reconciler.Context;
5+
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
6+
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
7+
import io.quarkiverse.operatorsdk.bundle.runtime.CSVMetadata;
8+
import io.quarkiverse.operatorsdk.bundle.runtime.SharedCSVMetadata;
9+
10+
@CSVMetadata(name = "shared", version = "0.0.1")
11+
public class AReconciler implements Reconciler<ConfigMap>, SharedCSVMetadata {
12+
13+
@Override
14+
public UpdateControl<ConfigMap> reconcile(ConfigMap configMap, Context<ConfigMap> context)
15+
throws Exception {
16+
return null;
17+
}
18+
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/sources/BReconciler.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package io.quarkiverse.operatorsdk.bundle.sources;
2+
3+
import io.fabric8.kubernetes.api.model.Service;
4+
import io.javaoperatorsdk.operator.api.reconciler.Context;
5+
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
6+
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
7+
import io.quarkiverse.operatorsdk.bundle.runtime.CSVMetadata;
8+
9+
@CSVMetadata(name = "shared", version = "0.0.2")
10+
public class BReconciler implements Reconciler<Service> {
11+
12+
@Override
13+
public UpdateControl<Service> reconcile(Service service, Context<Service> context)
14+
throws Exception {
15+
return null;
16+
}
17+
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/sources/CReconciler.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package io.quarkiverse.operatorsdk.bundle.sources;
2+
3+
import io.fabric8.kubernetes.api.model.ServiceAccount;
4+
import io.javaoperatorsdk.operator.api.reconciler.Context;
5+
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
6+
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
7+
8+
public class CReconciler implements Reconciler<ServiceAccount> {
9+
10+
@Override
11+
public UpdateControl<ServiceAccount> reconcile(ServiceAccount serviceAccount,
12+
Context<ServiceAccount> context) throws Exception {
13+
return null;
14+
}
15+
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/sources/DuplicatedBundleNameWithoutSharedCSVMetadata1.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package io.quarkiverse.operatorsdk.bundle.sources;
2+
3+
import io.fabric8.openshift.api.model.Role;
4+
import io.javaoperatorsdk.operator.api.reconciler.Context;
5+
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
6+
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
7+
import io.quarkiverse.operatorsdk.bundle.runtime.CSVMetadata;
8+
9+
@CSVMetadata(name = "illegal")
10+
public class DuplicatedBundleNameWithoutSharedCSVMetadata1 implements Reconciler<Role> {
11+
12+
@Override
13+
public UpdateControl<Role> reconcile(Role role, Context<Role> context) throws Exception {
14+
return null;
15+
}
16+
}

‎bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/sources/DuplicatedBundleNameWithoutSharedCSVMetadata2.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package io.quarkiverse.operatorsdk.bundle.sources;
2+
3+
import io.fabric8.openshift.api.model.RoleBinding;
4+
import io.javaoperatorsdk.operator.api.reconciler.Context;
5+
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
6+
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
7+
import io.quarkiverse.operatorsdk.bundle.runtime.CSVMetadata;
8+
9+
@CSVMetadata(name = "illegal")
10+
public class DuplicatedBundleNameWithoutSharedCSVMetadata2 implements Reconciler<RoleBinding> {
11+
12+
@Override
13+
public UpdateControl<RoleBinding> reconcile(RoleBinding roleBinding, Context<RoleBinding> context)
14+
throws Exception {
15+
return null;
16+
}
17+
}

0 commit comments

Comments
 (0)