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.

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

feat: improve OLM support #851

Merged
merged 8 commits into from
Mar 19, 2024
7 changes: 4 additions & 3 deletions .github/workflows/build-for-quarkus-version.yml
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,11 @@ jobs:
- name: Install Operator Lifecycle Manager and Operator SDK into Kind
run: operator-sdk olm install --version v0.23.0

- name: Validate OLM ExposedApp bundle
# Joke sample currently doesn't validate with OLM v1 because it bundles required Joke CRD, which v1 thinks should be owned
- name: Validate OLM bundles (excluding Joke sample)
run: |
cd samples/exposedapp
operator-sdk bundle validate target/bundle/quarkus-operator-sdk-samples-exposedapp/ --select-optional suite=operatorframework
cd samples/
find . -type d -name bundle | grep -v joke | xargs -I {} find {} -type d -maxdepth 1 -mindepth 1 | xargs -I {} operator-sdk bundle validate {} --select-optional suite=operatorframework

- name: Run Joke sample using Quarkus DEV mode
run: |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,29 @@
* gradle) name. This name can be used to assign reconcilers to the same bundle by creating a {@link SharedCSVMetadata}
* implementation bearing a {@link CSVMetadata} annotation specifying the CSV metadata to be shared among reconcilers
* assigned to that named bundle.
*
* @deprecated Use {@link #bundleName()} and {@link #csvName()} instead as previously this method was being used for both
* values resulting in confusion or even problems setting the correct CSV name as recommended by OLM. See
* <a href='https://github.com/quarkiverse/quarkus-operator-sdk/issues/738'>this issue</a> for a more detailed
* discussion.
*/
@Deprecated
String name() default "";

/**
* The name which should be used for the generated bundle. If not provided, the name is derived from the project's (maven or
* gradle) name. This name can be used to assign reconcilers to the same bundle by creating a {@link SharedCSVMetadata}
* implementation bearing a {@link CSVMetadata} annotation specifying the CSV metadata to be shared among reconcilers
* assigned to that named bundle.
*/
String bundleName() default "";

/**
* The name used in the CSV metadata stanza. If not provided, this will default to {@code <bundle name>.v<version>}
* as recommended by OLM.
*/
String csvName() default "";

/**
* Extra annotations that should be added to the CSV metadata.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ public static List<ManifestsBuilder> prepareGeneration(BundleGenerationConfigura
var missing = addCRDManifestBuilder(crds, builders, csvMetadata, csvBuilder.getOwnedCRs());
if (!missing.isEmpty()) {
throw new IllegalStateException(
"Missing owned CRD data for resources: " + missing + " for bundle: " + csvMetadata.name);
"Missing owned CRD data for resources: " + missing + " for bundle: " + csvMetadata.bundleName);
}
// output required CRDs in the manifest, output a warning in case we're missing some
missing = addCRDManifestBuilder(crds, builders, csvMetadata, csvBuilder.getRequiredCRs());
if (!missing.isEmpty()) {
log.warnv("Missing required CRD data for resources: {0} for bundle: {1}", missing, csvMetadata.name);
log.warnv("Missing required CRD data for resources: {0} for bundle: {1}", missing, csvMetadata.bundleName);
}
}

Expand All @@ -93,7 +93,7 @@ private static HashSet<String> addCRDManifestBuilder(Map<String, CRDInfo> crds,

private static SortedMap<String, String> generateBundleLabels(CSVMetadataHolder csvMetadata,
BundleGenerationConfiguration bundleConfiguration, Version version) {
var packageName = bundleConfiguration.packageName.orElse(csvMetadata.name);
var packageName = bundleConfiguration.packageName.orElse(csvMetadata.bundleName);

SortedMap<String, String> values = new TreeMap<>();
values.put(join(BUNDLE_PREFIX, CHANNEL, DEFAULT, ANNOTATIONS_VERSION),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@
import io.quarkus.deployment.builditem.ApplicationInfoBuildItem;
import io.quarkus.deployment.builditem.CombinedIndexBuildItem;
import io.quarkus.deployment.builditem.GeneratedFileSystemResourceBuildItem;
import io.quarkus.deployment.pkg.builditem.JarBuildItem;
import io.quarkus.deployment.pkg.builditem.OutputTargetBuildItem;
import io.quarkus.kubernetes.deployment.KubernetesCommonHelper;
import io.quarkus.kubernetes.deployment.KubernetesConfig;
import io.quarkus.kubernetes.deployment.ResourceNameUtil;

Expand All @@ -51,6 +53,7 @@ public class BundleProcessor {
private static final DotName SHARED_CSV_METADATA = DotName.createSimple(SharedCSVMetadata.class.getName());
private static final DotName CSV_METADATA = DotName.createSimple(CSVMetadata.class.getName());
private static final String BUNDLE = "bundle";
private static final String DEFAULT_PROVIDER_NAME = System.getProperty("user.name");
public static final String CRD_DISPLAY_NAME = "CRD_DISPLAY_NAME";
public static final String CRD_DESCRIPTION = "CRD_DESCRIPTION";

Expand All @@ -69,7 +72,8 @@ public boolean getAsBoolean() {
CSVMetadataBuildItem gatherCSVMetadata(KubernetesConfig kubernetesConfig,
ApplicationInfoBuildItem appConfiguration,
BundleGenerationConfiguration bundleConfiguration,
CombinedIndexBuildItem combinedIndexBuildItem) {
CombinedIndexBuildItem combinedIndexBuildItem,
JarBuildItem jarBuildItem) {
final var index = combinedIndexBuildItem.getIndex();
final var defaultName = bundleConfiguration.packageName
.orElse(ResourceNameUtil.getResourceName(kubernetesConfig, appConfiguration));
Expand All @@ -94,31 +98,37 @@ CSVMetadataBuildItem gatherCSVMetadata(KubernetesConfig kubernetesConfig,
// Check whether the reconciler must be shipped using a custom bundle
final var csvMetadataAnnotation = reconcilerInfo.classInfo()
.declaredAnnotation(CSV_METADATA);
final var maybeCSVMetadataName = getCSVMetadataName(csvMetadataAnnotation);
final var csvMetadataName = maybeCSVMetadataName.orElse(defaultName);
final var sharedMetadataName = getBundleName(csvMetadataAnnotation, defaultName);
final var isNameInferred = defaultName.equals(sharedMetadataName);

var csvMetadata = sharedMetadataHolders.get(csvMetadataName);
var csvMetadata = sharedMetadataHolders.get(sharedMetadataName);
if (csvMetadata == null) {
final var origin = reconcilerInfo.classInfo().name().toString();

if (!csvMetadataName.equals(defaultName)) {
if (!sharedMetadataName.equals(defaultName)) {
final var maybeExistingOrigin = csvGroups.keySet().stream()
.filter(mh -> mh.name.equals(csvMetadataName))
.filter(mh -> mh.bundleName.equals(sharedMetadataName))
.map(CSVMetadataHolder::getOrigin)
.findFirst();
if (maybeExistingOrigin.isPresent()) {
throw new IllegalStateException("Reconcilers '" + maybeExistingOrigin.get()
+ "' and '" + origin
+ "' are using the same bundle name '" + csvMetadataName
+ "' are using the same bundle name '" + sharedMetadataName
+ "' 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.");
}
}
csvMetadata = createMetadataHolder(csvMetadataAnnotation,
new CSVMetadataHolder(csvMetadataName, defaultVersion, defaultReplaces, origin));
new CSVMetadataHolder(sharedMetadataName, defaultVersion, defaultReplaces,
DEFAULT_PROVIDER_NAME, origin));
if (DEFAULT_PROVIDER_NAME.equals(csvMetadata.providerName)) {
log.warnv(
"It is recommended that you provide a provider name provided for {0}: ''{1}'' was used as default value.",
origin, DEFAULT_PROVIDER_NAME);
}
}
log.infov("Assigning ''{0}'' reconciler to {1}",
reconcilerInfo.nameOrFailIfUnset(),
getMetadataOriginInformation(csvMetadataAnnotation, maybeCSVMetadataName, csvMetadata));
getMetadataOriginInformation(csvMetadataAnnotation, isNameInferred, csvMetadata));

csvGroups.computeIfAbsent(csvMetadata, m -> new ArrayList<>()).add(
augmentReconcilerInfo(reconcilerInfo));
Expand All @@ -127,6 +137,37 @@ CSVMetadataBuildItem gatherCSVMetadata(KubernetesConfig kubernetesConfig,
return new CSVMetadataBuildItem(csvGroups);
}

private static String getDefaultProviderURLFromSCMInfo(ApplicationInfoBuildItem appConfiguration,
JarBuildItem jarBuildItem) {
final var maybeProject = KubernetesCommonHelper.createProject(appConfiguration, Optional.empty(),
Copy link
Contributor

Choose a reason for hiding this comment

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

@metacosm need to be extremely careful here. This class uses Dekorate to get the URL info directly from local git config. Personally I consider this unexpected (compared to using VCS info from pom configuration) and it was also the source of the recent Quarkus CVE .

jarBuildItem.getPath());
return maybeProject.map(project -> {
final var scmInfo = project.getScmInfo();
if (scmInfo != null) {
var origin = scmInfo.getRemote().get("origin");
Copy link
Contributor

Choose a reason for hiding this comment

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

Again.. same as with that CVE. We should make sure that this can be explicitly overridable -- there are cases when origin might point actually be a fork which one doesn't want to publicly advertise (e.g. when builds are for whatever reason using a mirror URL or when you simply have manual steps in your workflow)

Copy link

Choose a reason for hiding this comment

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

I absolutely must echo @jcechace concern here. In almost all use cases, origin for me is my fork, and upstream refers to the public repository.

Copy link
Member Author

@metacosm metacosm Mar 19, 2024

Choose a reason for hiding this comment

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

Yes, it's very tricky so maybe I'll just remove this and let people deal with it using @CSVMetadata.Provider instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again.. same as with that CVE. We should make sure that this can be explicitly overridable -- there are cases when origin might point actually be a fork which one doesn't want to publicly advertise (e.g. when builds are for whatever reason using a mirror URL or when you simply have manual steps in your workflow)

Note that this is for the default value and is overridable using @CSVMetadata and its provider field. That said, this computed value might be faulty in many situations so it's best to remove it.

if (origin != null) {
try {
int atSign = origin.indexOf('@');
if (atSign > 0) {
origin = origin.substring(atSign + 1);
origin = origin.replaceFirst(":", "/");
origin = "https://" + origin;
}

int dotGit = origin.indexOf(".git");
if (dotGit > 0 && dotGit < origin.length() - 1) {
origin = origin.substring(0, dotGit);
}
return origin;
} catch (Exception e) {
log.warnv("Couldn't parse SCM information: {0}", origin);
}
}
}
return null;
}).orElse(null);
}

private static ReconcilerAugmentedClassInfo augmentReconcilerInfo(
ReconcilerAugmentedClassInfo reconcilerInfo) {
// if primary resource is a CR, check if it is annotated with CSVMetadata and augment it if it is
Expand Down Expand Up @@ -161,14 +202,14 @@ private static void augmentResourceInfoIfCR(ReconciledAugmentedClassInfo<?> reco
}
}

private String getMetadataOriginInformation(AnnotationInstance csvMetadataAnnotation, Optional<String> csvMetadataName,
private String getMetadataOriginInformation(AnnotationInstance csvMetadataAnnotation, boolean isNameInferred,
CSVMetadataHolder metadataHolder) {
final var isDefault = csvMetadataAnnotation == null;
final var actualName = metadataHolder.name;
final var actualName = metadataHolder.bundleName;
if (isDefault) {
return "default bundle automatically named '" + actualName + "'";
} else {
return "bundle " + (csvMetadataName.isEmpty() ? "automatically " : "") + "named '"
return "bundle " + (isNameInferred ? "automatically " : "") + "named '"
+ actualName + "' defined by '" + metadataHolder.getOrigin() + "'";
}
}
Expand Down Expand Up @@ -256,30 +297,40 @@ void generateBundle(ApplicationInfoBuildItem configuration,

private Map<String, CSVMetadataHolder> getSharedMetadataHolders(String name, String version, String defaultReplaces,
IndexView index) {
CSVMetadataHolder csvMetadata = new CSVMetadataHolder(name, version, defaultReplaces, "default");
CSVMetadataHolder csvMetadata = new CSVMetadataHolder(name, version, defaultReplaces, DEFAULT_PROVIDER_NAME,
"default");
final var sharedMetadataImpls = index.getAllKnownImplementors(SHARED_CSV_METADATA);
final var result = new HashMap<String, CSVMetadataHolder>(sharedMetadataImpls.size() + 1);
sharedMetadataImpls.forEach(sharedMetadataImpl -> {
final var csvMetadataAnn = sharedMetadataImpl.declaredAnnotation(CSV_METADATA);
if (csvMetadataAnn != null) {
final var origin = sharedMetadataImpl.name().toString();
final var metadataHolder = createMetadataHolder(csvMetadataAnn, csvMetadata, origin);
final var existing = result.get(metadataHolder.name);
final var existing = result.get(metadataHolder.bundleName);
if (existing != null) {
throw new IllegalStateException(
"Only one SharedCSVMetadata named " + metadataHolder.name
"Only one SharedCSVMetadata named " + metadataHolder.bundleName
+ " can be defined. Was defined on (at least): " + existing.getOrigin() + " and " + origin);
}
result.put(metadataHolder.name, metadataHolder);
result.put(metadataHolder.bundleName, metadataHolder);
}
});
return result;
}

private Optional<String> getCSVMetadataName(AnnotationInstance csvMetadataAnnotation) {
return Optional.ofNullable(csvMetadataAnnotation)
.map(annotation -> annotation.value("name"))
.map(AnnotationValue::asString);
private static String getBundleName(AnnotationInstance csvMetadata, String defaultName) {
if (csvMetadata == null) {
return defaultName;
} else {
final var bundleName = csvMetadata.value("bundleName");
if (bundleName != null) {
return bundleName.asString();
} else {
return Optional.ofNullable(csvMetadata.value("name"))
.map(AnnotationValue::asString)
.orElse(defaultName);
}
}
}

private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata,
Expand All @@ -294,8 +345,8 @@ private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, C
}

final var providerField = csvMetadata.value("provider");
String providerName = null;
String providerURL = null;
String providerName = mh.providerName;
String providerURL = mh.providerURL;
if (providerField != null) {
final var provider = providerField.asNested();
providerName = ConfigurationUtils.annotationValueOrDefault(provider, "name",
Expand Down Expand Up @@ -441,8 +492,9 @@ private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, C
}

return new CSVMetadataHolder(
ConfigurationUtils.annotationValueOrDefault(csvMetadata, "name",
AnnotationValue::asString, () -> mh.name),
getBundleName(csvMetadata, mh.bundleName),
ConfigurationUtils.annotationValueOrDefault(csvMetadata, "csvName",
AnnotationValue::asString, () -> mh.csvName),
ConfigurationUtils.annotationValueOrDefault(csvMetadata, "description",
AnnotationValue::asString, () -> mh.description),
ConfigurationUtils.annotationValueOrDefault(csvMetadata, "displayName",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public CsvManifestsBuilder(CSVMetadataHolder metadata, BuildTimeOperatorConfigur

csvBuilder = new ClusterServiceVersionBuilder();

final var metadataBuilder = csvBuilder.withNewMetadata().withName(getName());
final var metadataBuilder = csvBuilder.withNewMetadata().withName(metadata.csvName);
if (metadata.annotations != null) {
metadataBuilder.addToAnnotations("olm.skipRange", metadata.annotations.skipRange);
metadataBuilder.addToAnnotations("containerImage", metadata.annotations.containerImage);
Expand All @@ -72,7 +72,7 @@ public CsvManifestsBuilder(CSVMetadataHolder metadata, BuildTimeOperatorConfigur
final var csvSpecBuilder = csvBuilder
.editOrNewSpec()
.withDescription(metadata.description)
.withDisplayName(defaultIfEmpty(metadata.displayName, getName()))
.withDisplayName(defaultIfEmpty(metadata.displayName, metadata.csvName))
.withKeywords(metadata.keywords)
.withReplaces(metadata.replaces)
.withVersion(metadata.version)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public abstract byte[] getManifestData(List<ServiceAccount> serviceAccounts, Lis
public abstract String getManifestType();

public String getName() {
return metadata.name;
return metadata.bundleName;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public void shouldWriteBundleEvenWhenCsvMetadataIsNotUsed() throws IOException {
assertEquals(name, deployment.getName());
// by default, we shouldn't output the version label in the selector match labels as the default controlling this should be overridden by KubernetesLabelConfigOverrider
assertNull(deployment.getSpec().getSelector().getMatchLabels().get("app.kubernetes.io/version"));
assertEquals(System.getProperty("user.name"), csv.getSpec().getProvider().getName());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.List;
import java.util.Optional;

import io.quarkiverse.operatorsdk.annotations.CSVMetadata;
import io.quarkus.runtime.annotations.ConfigItem;
import io.quarkus.runtime.annotations.ConfigRoot;

Expand All @@ -28,7 +29,10 @@ public class BundleGenerationConfiguration {

/**
* The name of the package that bundle belongs to.
*
* @deprecated Use {@link CSVMetadata#bundleName()} instead
*/
@Deprecated(forRemoval = true)
@ConfigItem
public Optional<String> packageName;

Expand Down
Loading
Loading