Replace operator-sdk run bundle with catalog-based OLM deployment #2078
Conversation
WalkthroughAdds a catalog-based OLM deployment flow: new public variable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Comment |
openshift-ci
bot
added
the
approved
The operator-sdk run bundle command has limitations and a pending PR (#7040) that is not getting merged. This replaces it with a catalog-based approach using opm and direct OLM resource creation. Changes: - Add CATALOG_SOURCE_NAME variable for configurability - Update deploy-olm to build catalog using existing catalog-build/catalog-push - Create CatalogSource with grpcPodConfig.securityContextConfig: restricted - Create OperatorGroup and Subscription directly via oc apply - Wait for CatalogSource READY, InstallPlan, and CSV Succeeded states - Simplify undeploy-olm to remove operator-sdk cleanup dependency - Update create-sts-subscription helper to use DEFAULT_CHANNEL variable - Update deploy-olm-stsflow console URL to use DEFAULT_CHANNEL variable Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
kaovilai
force-pushed
the
deploy-olm-opm-edition
branch
from
64e9ab7 to
4f6a2c6
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the deprecated operator-sdk run bundle command with a catalog-based OLM deployment approach. The change addresses limitations with the operator-sdk tooling and provides more control over the deployment process through direct OLM resource management.
Changes:
- Introduces catalog-based deployment using
opmand direct OLM resource creation - Adds configurable
CATALOG_SOURCE_NAMEandBUNDLE_CHANNELvariables for deployment flexibility - Implements comprehensive wait logic for CatalogSource, InstallPlan, and CSV readiness states
- Simplifies cleanup by removing dependency on
operator-sdk cleanupcommand
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "Timeout waiting for InstallPlan"; \ | ||
| exit 1; \ | ||
| fi | ||
| @echo "Waiting for CSV to exist..." |
There was a problem hiding this comment.
When the InstallPlan timeout is reached, there's no diagnostic information printed to help troubleshoot the issue. Consider adding diagnostic output similar to the CatalogSource timeout handling (line 572) to display the Subscription status or relevant resources that could help identify why the InstallPlan wasn't created.
| @echo "Waiting for CSV to exist..." | |
| echo "Timeout waiting for InstallPlan"; \ | |
| echo "Subscription details (oadp-operator in namespace $(OADP_TEST_NAMESPACE)):"; \ | |
| $(OC_CLI) get subscription oadp-operator -n $(OADP_TEST_NAMESPACE) -o yaml || true; \ | |
| echo "Existing InstallPlans in namespace $(OADP_TEST_NAMESPACE):"; \ | |
| $(OC_CLI) get installplan -n $(OADP_TEST_NAMESPACE) || true; \ |
| fi | ||
| @echo "Waiting for CSV to be ready..." | ||
| @CSV_NAME=$$($(OC_CLI) get subscription oadp-operator -n $(OADP_TEST_NAMESPACE) -o jsonpath='{.status.currentCSV}' 2>/dev/null); \ | ||
| if [ -n "$$CSV_NAME" ]; then \ |
There was a problem hiding this comment.
Similar to the InstallPlan timeout, when the CSV existence timeout is reached, there's no diagnostic information printed. Consider adding diagnostic output to show the Subscription status or other relevant information to help troubleshoot why the CSV wasn't created.
| # Delete CSV with OADP label | ||
| -$(OC_CLI) delete csv -l operators.coreos.com/oadp-operator.$(OADP_TEST_NAMESPACE) -n $(OADP_TEST_NAMESPACE) --ignore-not-found=true | ||
| -$(OC_CLI) delete catalogsource oadp-operator-catalog -n $(OADP_TEST_NAMESPACE) --ignore-not-found=true | ||
| # Delete any CSV starting with oadp-operator |
There was a problem hiding this comment.
The cleanup pipeline here passes OADP_TEST_NAMESPACE and CATALOG_SOURCE_NAME into $(OC_CLI) and grep without quoting or escaping (e.g. $(OC_CLI) get subscription -n $(OADP_TEST_NAMESPACE) ... | grep "$(CATALOG_SOURCE_NAME)" | ...). If these values are ever derived from environment variables or other untrusted inputs, an attacker could inject shell metacharacters or command substitutions to execute arbitrary commands during undeploy, potentially impacting the CI runner or cluster resources. To mitigate this, treat these variables as untrusted and quote/escape them appropriately in all shell and grep invocations so that their contents cannot be interpreted as part of the shell syntax or options.
| echo " sourceNamespace: $(OADP_TEST_NAMESPACE)" >> $(1) && \ | ||
| echo " installPlanApproval: Automatic" >> $(1) && \ | ||
| echo " config:" >> $(1) && \ |
There was a problem hiding this comment.
The create-sts-subscription helper echoes a Subscription YAML that directly injects BUNDLE_CHANNEL and CATALOG_SOURCE_NAME into a double-quoted shell string (e.g. echo " channel: $(BUNDLE_CHANNEL)" >> $(1)), which allows shell command substitution/backtick expansion if these values are influenced by environment variables or other untrusted input. This can be abused to run arbitrary commands in the shell used to generate the YAML, even before the manifest is applied to the cluster. To reduce this risk, ensure these variables are safely quoted/escaped in the shell context or written via safer mechanisms (such as here-documents) so that their contents are treated as data only.
| @echo "===========================================================================" | ||
| @echo "Open the following URL in your browser to trigger the standardized flow UI:" | ||
| @echo "" | ||
| @CONSOLE_URL=$$($(OC_CLI) get route console -n openshift-console -o jsonpath='{.spec.host}'); \ | ||
| echo "https://$$CONSOLE_URL/operatorhub/ns/$(OADP_TEST_NAMESPACE)?keyword=oadp-operator&details-item=oadp-operator-oadp-operator-catalog-$(OADP_TEST_NAMESPACE)&channel=operator-sdk-run-bundle&version=$(VERSION)" | ||
| echo "https://$$CONSOLE_URL/operatorhub/ns/$(OADP_TEST_NAMESPACE)?keyword=oadp-operator&details-item=oadp-operator-$(CATALOG_SOURCE_NAME)-$(OADP_TEST_NAMESPACE)&channel=$(DEFAULT_CHANNEL)&version=$(VERSION)" | ||
| @echo "" | ||
| @echo "===========================================================================" | ||
|
|
There was a problem hiding this comment.
In deploy-olm-stsflow, both the subscription deletion pipeline and the final echo of the console URL interpolate OADP_TEST_NAMESPACE, CATALOG_SOURCE_NAME, and BUNDLE_CHANNEL directly into shell commands without quoting or escaping. If any of these values are sourced from environment variables or other untrusted inputs, an attacker could inject shell metacharacters or command substitutions (e.g. via backticks or $(...)) to execute arbitrary commands when this target is run. To harden this path, treat these variables as untrusted and ensure they are safely quoted/escaped in shell invocations so they cannot alter the command structure.
| @echo "Waiting for CatalogSource to be ready..." | ||
| @timeout=120; \ | ||
| while [ $$timeout -gt 0 ]; do \ | ||
| STATE=$$($(OC_CLI) get catalogsource $(CATALOG_SOURCE_NAME) -n $(OADP_TEST_NAMESPACE) -o jsonpath='{.status.connectionState.lastObservedState}' 2>/dev/null); \ | ||
| if [ "$$STATE" = "READY" ]; then \ | ||
| echo "CatalogSource is ready"; \ | ||
| break; \ | ||
| fi; \ | ||
| echo -n "."; \ | ||
| sleep 2; \ | ||
| timeout=$$((timeout-2)); \ | ||
| done; \ | ||
| if [ $$timeout -le 0 ]; then \ | ||
| echo "Timeout waiting for CatalogSource"; \ | ||
| $(OC_CLI) get catalogsource $(CATALOG_SOURCE_NAME) -n $(OADP_TEST_NAMESPACE) -o yaml; \ | ||
| exit 1; \ | ||
| fi | ||
| # Create OperatorGroup if not exists | ||
| @echo "Checking OperatorGroup..." | ||
| @OG_COUNT=$$($(OC_CLI) get operatorgroup -n $(OADP_TEST_NAMESPACE) --no-headers 2>/dev/null | wc -l | tr -d ' '); \ | ||
| if [ "$$OG_COUNT" -eq "0" ]; then \ | ||
| echo "Creating OperatorGroup..."; \ | ||
| echo -e "apiVersion: operators.coreos.com/v1\nkind: OperatorGroup\nmetadata:\n name: oadp-operator-group\n namespace: $(OADP_TEST_NAMESPACE)\nspec:\n targetNamespaces:\n - $(OADP_TEST_NAMESPACE)" | $(OC_CLI) apply -f -; \ | ||
| else \ | ||
| echo "OperatorGroup already exists"; \ | ||
| fi | ||
| # Create Subscription | ||
| @echo "Creating Subscription..." | ||
| @echo -e "apiVersion: operators.coreos.com/v1alpha1\nkind: Subscription\nmetadata:\n name: oadp-operator\n namespace: $(OADP_TEST_NAMESPACE)\nspec:\n channel: $(DEFAULT_CHANNEL)\n name: oadp-operator\n source: $(CATALOG_SOURCE_NAME)\n sourceNamespace: $(OADP_TEST_NAMESPACE)\n installPlanApproval: Automatic" | $(OC_CLI) apply -f - | ||
| # Wait for operator to be ready | ||
| @echo "Waiting for InstallPlan to be created..." | ||
| @timeout=60; \ |
There was a problem hiding this comment.
Several shell commands in this block interpolate OADP_TEST_NAMESPACE, CATALOG_SOURCE_NAME, and BUNDLE_CHANNEL directly into the shell without quoting or escaping, for example in the echo -e ... | $(OC_CLI) apply -f - lines and $(OC_CLI) get catalogsource $(CATALOG_SOURCE_NAME) -n $(OADP_TEST_NAMESPACE) loop. If any of these values can be influenced via environment variables or other untrusted inputs, an attacker could inject shell metacharacters or command substitutions (e.g. backticks or $(...)) to execute arbitrary commands in the developer/CI environment or against the cluster. To harden this, ensure these variables are safely quoted/escaped in shell context (or constructed via safer mechanisms like here-documents) so their contents cannot change the shell command structure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 716-726: The console URL printed by the deploy-olm-stsflow target
uses DEFAULT_CHANNEL which may include surrounding quotes and thus yields
channel="dev"; update the URL construction to strip any double quotes from
DEFAULT_CHANNEL before embedding it. Modify the shell snippet that sets
CONSOLE_URL and echoes the final URL (the lines assigning CONSOLE_URL and the
echo that includes $(DEFAULT_CHANNEL)) so that you run a tiny shell substitution
to remove " characters from DEFAULT_CHANNEL (for example via tr or shell
parameter expansion) and use that cleaned value for the channel parameter.
| .PHONY: deploy-olm-stsflow | ||
| deploy-olm-stsflow: deploy-olm ## Deploy via OLM then uninstall CSV/Subscription and provide console URL for standardized flow | ||
| @echo "Uninstalling CSV and Subscription to trigger standardized flow UI..." | ||
| -$(OC_CLI) get subscription -n $(OADP_TEST_NAMESPACE) -o name | xargs -I {} $(OC_CLI) get {} -n $(OADP_TEST_NAMESPACE) -o jsonpath='{.metadata.name}{"\t"}{.spec.source}{"\n"}' | grep "oadp-operator-catalog" | cut -f1 | xargs -I {} $(OC_CLI) delete subscription {} -n $(OADP_TEST_NAMESPACE) --ignore-not-found=true | ||
| -$(OC_CLI) get subscription -n $(OADP_TEST_NAMESPACE) -o name 2>/dev/null | xargs -I {} $(OC_CLI) get {} -n $(OADP_TEST_NAMESPACE) -o jsonpath='{.metadata.name}{"\t"}{.spec.source}{"\n"}' 2>/dev/null | grep "$(CATALOG_SOURCE_NAME)" | cut -f1 | xargs -I {} $(OC_CLI) delete subscription {} -n $(OADP_TEST_NAMESPACE) --ignore-not-found=true || true | ||
| -$(OC_CLI) delete csv oadp-operator.v$(VERSION) -n $(OADP_TEST_NAMESPACE) --ignore-not-found=true | ||
| @echo "" | ||
| @echo "===========================================================================" | ||
| @echo "Open the following URL in your browser to trigger the standardized flow UI:" | ||
| @echo "" | ||
| @CONSOLE_URL=$$($(OC_CLI) get route console -n openshift-console -o jsonpath='{.spec.host}'); \ | ||
| echo "https://$$CONSOLE_URL/operatorhub/ns/$(OADP_TEST_NAMESPACE)?keyword=oadp-operator&details-item=oadp-operator-oadp-operator-catalog-$(OADP_TEST_NAMESPACE)&channel=operator-sdk-run-bundle&version=$(VERSION)" | ||
| echo "https://$$CONSOLE_URL/operatorhub/ns/$(OADP_TEST_NAMESPACE)?keyword=oadp-operator&details-item=oadp-operator-$(CATALOG_SOURCE_NAME)-$(OADP_TEST_NAMESPACE)&channel=$(DEFAULT_CHANNEL)&version=$(VERSION)" |
There was a problem hiding this comment.
Strip quotes from DEFAULT_CHANNEL in the console URL.
Line 726 will emit channel="dev" because DEFAULT_CHANNEL includes quotes, which can break the OperatorHub URL parsing.
🛠️ Proposed fix
@@
-DEFAULT_CHANNEL = "dev"
+DEFAULT_CHANNEL = "dev"
+DEFAULT_CHANNEL_VALUE := $(subst ",,$(DEFAULT_CHANNEL))@@
-echo "https://$$CONSOLE_URL/operatorhub/ns/$(OADP_TEST_NAMESPACE)?keyword=oadp-operator&details-item=oadp-operator-$(CATALOG_SOURCE_NAME)-$(OADP_TEST_NAMESPACE)&channel=$(DEFAULT_CHANNEL)&version=$(VERSION)"
+echo "https://$$CONSOLE_URL/operatorhub/ns/$(OADP_TEST_NAMESPACE)?keyword=oadp-operator&details-item=oadp-operator-$(CATALOG_SOURCE_NAME)-$(OADP_TEST_NAMESPACE)&channel=$(DEFAULT_CHANNEL_VALUE)&version=$(VERSION)"🤖 Prompt for AI Agents
In `@Makefile` around lines 716 - 726, The console URL printed by the
deploy-olm-stsflow target uses DEFAULT_CHANNEL which may include surrounding
quotes and thus yields channel="dev"; update the URL construction to strip any
double quotes from DEFAULT_CHANNEL before embedding it. Modify the shell snippet
that sets CONSOLE_URL and echoes the final URL (the lines assigning CONSOLE_URL
and the echo that includes $(DEFAULT_CHANNEL)) so that you run a tiny shell
substitution to remove " characters from DEFAULT_CHANNEL (for example via tr or
shell parameter expansion) and use that cleaned value for the channel parameter.
well that is a new error.. |
|
#itworksonmypc Go another round prow. /retest-required |
|
|
/test 4.21-e2e-test-kubevirt-aws HCO timeout |
|
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kaovilai: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/retest-required |
|
guess 4.22 is optional. need one more ack |
2 participants
The operator-sdk run bundle command has limitations and a pending PR (#7040)
that is not getting merged. This replaces it with a catalog-based approach
using opm and direct OLM resource creation.
Changes:
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Why the changes were made
#2067 mentioned some issues with catalogsource pod creation by operator-sdk cli
How to test the changes made