Skip to content

🌱 Add test-operator with custom controller #2085

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Jul 9, 2025

Description

Re-opening #2070 and moving test-operator to Helm chart to facilitate the bundle manifest generation.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2025
Copy link

netlify bot commented Jul 9, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 3b06657
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/686fd1529b24a70008b21aeb
😎 Deploy Preview https://deploy-preview-2085--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

openshift-ci bot commented Jul 9, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign perdasilva for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@perdasilva perdasilva changed the title Test operator 🌱 Add test-operator with custom controller Jul 9, 2025
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.48%. Comparing base (e02c8de) to head (3b06657).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2085      +/-   ##
==========================================
+ Coverage   73.37%   73.48%   +0.10%     
==========================================
  Files          77       78       +1     
  Lines        7076     7241     +165     
==========================================
+ Hits         5192     5321     +129     
- Misses       1543     1568      +25     
- Partials      341      352      +11     
Flag Coverage Δ
e2e 43.71% <ø> (-1.20%) ⬇️
experimental-e2e 50.04% <ø> (-1.12%) ⬇️
unit 58.74% <ø> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@perdasilva perdasilva force-pushed the test-operator branch 2 times, most recently from 68faebc to 9ea948a Compare July 9, 2025 19:13
Makefile Outdated
Comment on lines 251 to 277
.PHONY: test-operator-test-unit
test-operator-test-unit: $(SETUP_ENVTEST) envtest-k8s-bins
KUBEBUILDER_ASSETS="$(shell $(SETUP_ENVTEST) use -p path $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE))" \
CGO_ENABLED=1 go test -tags '$(GO_BUILD_TAGS)' -count=1 -race -short $(TEST_OPERATOR_DIR)/...

.PHONY: test-operator-build-controllers
test-operator-build-controllers:
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/images/controllers/test-operator/v1.0.0/manager $(TEST_OPERATOR_DIR)/v1/cmd/main.go
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/images/controllers/test-operator/v2.0.0/manager $(TEST_OPERATOR_DIR)/v2/cmd/main.go

.PHONY: test-operator-generate-manifests
test-operator-generate-manifests: $(CONTROLLER_GEN)
# generate resources crds
$(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) crd paths="$(TEST_OPERATOR_DIR)/api/v1/..." output:dir=$(TEST_OPERATOR_DIR)/charts/v1/crds
$(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) crd paths="$(TEST_OPERATOR_DIR)/api/..." output:dir=$(TEST_OPERATOR_DIR)/charts/v2/crds

# generate other resources
$(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) rbac:roleName=test-operator-manager-role paths="$(TEST_OPERATOR_DIR)/v1/..." output:dir=$(TEST_OPERATOR_DIR)/charts/v1/templates
$(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) rbac:roleName=test-operator-manager-role webhook paths=".$(TEST_OPERATOR_DIR)/v2/..." output:dir=$(TEST_OPERATOR_DIR)/charts/v2/templates

.PHONY: test-operator-generate-bundles
test-operator-generate-bundles: test-operator-generate-manifests $(OPERATOR_SDK)
# generate bundles
helm template --include-crds $(TEST_OPERATOR_DIR)/charts/v1 | $(OPERATOR_SDK) generate bundle -q --channels beta --version 1.0.0 --package test --output-dir ./testdata/images/bundles/test-operator/v1.0.0
helm template --include-crds --set=configmap.shouldNotTemplate=true $(TEST_OPERATOR_DIR)/charts/v1 | $(OPERATOR_SDK) generate bundle -q --channels beta --version 1.3.0 --package test --output-dir ./testdata/images/bundles/test-operator/v1.3.0
helm template --include-crds $(TEST_OPERATOR_DIR)/charts/v2 | $(OPERATOR_SDK) generate bundle -q --channels beta --version 2.0.0 --package test --output-dir ./testdata/images/bundles/test-operator/v2.0.0
rm -rf bundle.Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

All above should be part of the sample
If we use the tooling we do not need this code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end the only difference between both approaches is how you deal with changes to the underlying operator code:

kubebuilder approach:

some bash script:

kubebuilder init
kubebuilder create ...
kubebuilder ...
commmands to mutate generated code: cp, sed, patch, etc.
operator-sdk generate bundle

this approach:

build the operator how you want once in-tree as a helm chart modify and render at will using a familiar methodology (the exact same we use for o-c). Modifications can be done directly to the fixture, no need to figure out how modify generated code the right way to get the things you need in there. Basically, use kubebuilder/operator-sdk to scaffold a helm project, remove cruft, modify directly when needed.

Ultimately, that's the issue for me with the automated approach, you end up transferring complexity to how the modifications are done to save on resource files that can be generated. Except, once we're closer to the helm world, we'll need to keep those resources around anyway...so I really don't get what we get out of simulating the operator developer flow...=S

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with is:

goes against important separation-of-concerns principles.
It introduces coupling that will make these components harder to maintain over time.
Keeping this logic self-contained in the sample is key to preserving clarity and long-term maintainability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's coupling in any case. In the automated case, we are coupled to the tools that we use to generate the foundation on top of which we will build out the operators. If the foundation changes, all layers on top of that will be affected, and that to me that, afaics, will be far more of a maintenance headache. If there are any changes in the generated scaffolding, it will be like doing a rebase. And in any case, the fixtures we will build will be in function of the project. So, there's coupling there too.

Given that the goals are the same in both approaches, I don't think they differ very much when it comes to separation of concerns.

To me, the core of the difference between both approaches are how in how we make changes to preexisting test-operator code/resources. In one case, we generate the scaffolding once, and make changes directly to the files and produce the artifacts from that source of truth. In the other, we generate the scaffolding and replay changes on top of it to get it to the desired state and produce the artifacts from that source of truth.

This is the only dimension in which I see there's any material difference between the approaches.

@@ -0,0 +1,23 @@
name: testdata
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 10, 2025

Choose a reason for hiding this comment

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

Can we discuss the case scenarios:

The test-operator supports AllNamespaces, SingleNamespace, and OwnNamespace install modes.

  • v1.0.0: is just the crd and the existing configmap resource
  • v1.3.0: has the same crd as v1.0.0 and the updated configmap previously part of v2.0.0
  • v2.0.0: has validating, mutating and conversion webhooks

The DELTA between v1.0.0 and v2.0.0 is clear
V2.0.0 should introduce webhooks

But what is the DELTA between v1.0.0 and v1.3.0
What do we want to cover with v1.3.0?
Do we want to add any changes that would not be a breaking change?
What scenarios do we need to cover?

IHMO it should be:

  • v1.0.0 - Initial basic project ( no metrics. no networkpolicies )
  • v.1.1.0 - Enable network policy, metrics, etc (not a breaking change)
  • v2.0.0 - Has webhooks and v2 CRD -> migration from v1 to V2 using spoke

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.3.0 is just to harmonize with what's already there. It's something that can definitely be revisited. Right now, 2.0.0 includes this configmap. We can probably just collapse it to 1.0.0 and do the check. I think it's just a sanity check to make sure that annotations that include template syntax don't get affected.

Copy link
Contributor Author

@perdasilva perdasilva Jul 10, 2025

Choose a reason for hiding this comment

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

I think the list you put there is fine. I don't even think we need the 1.1.0 version. We only care about what is in the payload insofar as it interacts with OLM features/behavior. I.e. whether it has network policies, exposes metrics, uses HA, etc. isn't important if it doesn't matter to OLM. We don't need to add the complexity.

When we get to things like probing the state of the installation, or want to test some boxcutter features etc, then I think this will become more necessary. For now, if it stamps out a configmap (or any other resource) just to say it stamped out a resource from the bundle, that's enough imo. I think v1.0.0 and v2.0.0 give us what we need for now.

@perdasilva perdasilva force-pushed the test-operator branch 2 times, most recently from 32805f9 to 6c6b367 Compare July 10, 2025 14:34
Signed-off-by: Per Goncalves da Silva <[email protected]>
Per Goncalves da Silva added 4 commits July 10, 2025 16:42
Signed-off-by: Per Goncalves da Silva <[email protected]>
Signed-off-by: Per Goncalves da Silva <[email protected]>
Signed-off-by: Per Goncalves da Silva <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants