Redesign DBInstance Controller Around Bounded Level-driven Reconciliation - #241
Merged
Conversation
…and interface files
…econciliation steps
…ance - Added ensureReady function to set the DBInstance status to available and update observed generation. - Created ensure_steps.go to manage the ordered steps during DBInstance provisioning. - Implemented runEnsureSteps to execute provisioning steps and handle outcomes. - Developed ensureVM function to ensure the VirtualMachine resource exists and create it if absent. - Added tests for ensureReady, ensureVM, and provisioning steps to validate functionality. - Removed obsolete probe_gate_test.go as its functionality is now covered by new tests. - Introduced stubHarvester for testing, simulating interactions with the Harvester client.
- Added `ensureStorageResize` method to handle cold resizing of VMs based on shape drift. - Introduced `vmShapeDrift` struct to compare VM's declared shape against desired specifications. - Created tests for various scenarios in `ensure_resize_test.go`, including: - No drift satisfied - Class drift requiring VM stop - Waiting for teardown during resize - Applying class and storage changes when VM is down - Handling unsupported storage shrink attempts - Updated `ensure_steps.go` to include the new resize step in the provisioning process. - Refactored existing tests to accommodate changes in the lifecycle management of VMs. - Enhanced `stubHarvester` to track resize calls for testing purposes. - Modified `typed_client.go` and `typed_client_test.go` to reflect changes in volume claim template handling.
- Update `ensurePowerState` to suspend power management during crash-loop conditions, ensuring VMs are not restarted or stopped unnecessarily. - Modify tests to validate that power management behaves correctly under crash-loop conditions, ensuring no VM calls are made while in a halted state. - Enhance `ensureReady` to reflect degraded states accurately, ensuring that the phase remains consistent with health checks. - Introduce new tests to verify that the system correctly handles degraded states and does not trigger unnecessary VM operations. - Implement cleanup steps in the provisioning process to remove cloud-init secrets after VM creation. - Adjust the stub harvester to log operations for better traceability during tests.
- Introduced a new resource package to handle declarative child objects (Service, Endpoints, ServiceMonitor) for monitoring. - Updated DBInstance reconciler to create and manage monitoring resources with controller ownership. - Added owner reference handling for VM creation to ensure proper garbage collection. - Removed legacy monitoring deployment methods from the typed client. - Enhanced tests to verify the creation and ownership of monitoring resources. - Ensured that metrics Service and Endpoints are updated correctly on VM IP changes.
Move credential and TLS material generation out of the Harvester client and into a new internal/credentials package (Resolver.Resolve), decomposing the old monolithic per-instance Secret into three: - pg-<name>-credentials (tenant ns): admin_user/admin_password only - dbi-<uid>-internal (operator ns): repl_password/exporter_password - dbi-<uid>-tls (operator ns, type kubernetes.io/tls): CA + server cert/key Each is get-or-create-once and never regenerated on reentry, preserving the invariant that a booted VM's password/CA can't drift from what the operator holds. status.CACertPEM is removed; the CA is now served via a new pg-<name>-connect Secret (internal/resource.ConnectionSecret) alongside host/port/dbname/jdbcUrl/sslmode, reconciled once status.Endpoint is known. Cloud-init rendering (internal/credentials/cloudinit.go, moved from internal/harvester) now reads from the resolved Material instead of generating its own secrets inline, and is applied via a new internal/resource.CloudInitSecret builder before VM creation. harvester.CreatePostgresVM is slimmed to (VMCreateParams) (vmName, err) with a CloudInitSecretName field — it no longer generates any credential material itself, in both the typed and legacy dynamic clients. Because the two operator-namespace Secrets are cross-namespace from their DBInstance and can't carry an owner reference, reconcileDelete now removes them explicitly: by the recorded status.resources ref first, then a dbaas.opencloud.wso2.com/dbinstance-uid label sweep as a backstop for a ref lost to a status reset. A new --operator-namespace flag (default POD_NAMESPACE via downward API, fallback "dbaas-system") controls where these live.
…ace(PR9) Close a real enforcement gap first: status.appliedSpec never tracked spec.staticNetwork/spec.vmPassword (true since the original commit, not a regression), so immutableDrift() silently allowed editing them post-create. Both are now snapshotted and compared (equality.Semantic.DeepEqual for the struct field). Add +kubebuilder:validation:XValidation "self== oldSelf" transition rules on networkRef/engineVersion/staticNetwork/vmPassword as API-server-level defense-in-depth. Deliberately skip the other five documented-immutable fields (osImage/dbName/masterUsername/port/storageType): immutableDrift() compares them post-defaulting, so a raw CEL rule on the unset spec value would be stricter than today's behavior (e.g. rejecting an explicit port: 5432 after leaving it unset). No enum markers either: dbInstanceClass is validated against a live Go map already, and storageType's real value on Harvester is unconfirmed — enum-constraining either now would either duplicate maintenance or risk locking out the correct value. Remove the legacy dynamic-client Harvester implementation (client.go, probe.go, cmd/harvester_dynamic.go) and the dead DialVMListener method it was built around, both already self-flagged "not used anymore" in the code. TypedClient is now the only implementation, constructed directly in cmd/main.go with no build tag. While touching ClientInterface, drop two more methods that never belonged on a Harvester-specific contract: CreateDataVolume did no I/O at all (pure string formatting, now the plain DataVolumeName function next to buildPostgresVM), and DeleteSecret deleted a plain corev1.Secret via client-go with nothing Harvester-specific about it (now a direct r.Delete call in ensureBootstrapCleanup, matching the pattern deleteOperatorSecrets already used). interface.go now holds only the actual contract: the interface plus the DTOs its methods reference.
…ret, update related tests
… related comments and tests
…atusIfChanged and clean up unused code
…rove comments for clarity
…te method names for consistency
…for clarity, introduce new status for incompatible parameters, and ensure DatabaseReady condition is accurately set during power state and resize operations.
…ng readiness checks and recovery logic
- Introduced a new `finalizeStatus` method to centralize status updates for DBInstance, ensuring consistent handling of Accepted, Ready, and InterventionRequired conditions. - Replaced legacy condition checks with a more robust mechanism that uses ConditionReason for better clarity and maintainability. - Updated tests to reflect changes in condition handling, ensuring that the status phase accurately represents the current state of the DBInstance. - Removed the `markProvisioningFailed` function, consolidating its logic into the new condition management approach. - Enhanced error handling in various steps to ensure that the DBInstance reflects the correct status based on the latest conditions. - Added new tests to validate the behavior of the updated status management logic, ensuring comprehensive coverage of edge cases.
…geChangeRejected and update related logic and tests for clarity
… for resize operations and enhance test coverage for resize lifecycle
…ng for database recovery and update test cases for clarity
…ance comments for clarity on resize lifecycle aggregation
- Add ensureConnectionSecret function to publish tenant connection secrets after database health is established. - Modify ensureCredentials to handle credential changes and requeue if necessary. - Update tests to cover new connection secret logic and ensure proper handling of credentials. - Refactor credential resolver to return a result indicating if the state has changed. - Ensure connection secrets are created only when the database endpoint is known. - Adjust provisioning steps to include connection secret management.
…in DBInstance reconciler
…oud-init and connection secrets
…nstanceStatus and related logic
…init secret management
…alsResolvesAndRecordsRefsWithoutEndpoint
…alse liveness monitor alerts
…ent shell injection test(credentials): add test for cloud-init to ensure shell metacharacters are neutralized fix(controller): add check to prevent VM restart when VMI is gone
…icNetwork after creation in DBInstance
…ApplyBuildersIdempotentWithOwnerRefs
…prove error reporting
… and recovery logic
…on; improve cloud-init YAML formatting
…il VM IP is known
…on and syncReadyCondition for clarity and accuracy
…hecks; enhance error classification for image resolution
…reVM; enhance stub for resize tracking
…ple files for clarity and accuracy
…ng; remove unused code and enhance status patching logic
…and improve logging format
…inalizer handling in Reconcile method
- introduce the ensure.Step interface and Runner - move ensure-step implementations into internal/ensure - centralize step results and dependencies - expose shared condition helpers on DBInstance - move reusable test fixtures into internal/testutil - preserve existing reconciliation behavior and test coverage
- move step-specific tests beside their implementations - retain reconciliation and envtest coverage in the controller package - remove the legacy controller test adapter - centralize shared fixtures in package-local test helpers - clean up stale helpers, comments, and scaffold TODOs
📝 WalkthroughWalkthroughThe database operator now uses a bounded ensure-step reconciler with typed Harvester integration, durable credentials and TLS Secrets, declarative child-resource builders, expanded status conditions, immutable-field validation, lifecycle handling, crash-loop recovery, monitoring reconciliation, deletion cleanup, and extensive unit and envtest coverage. ChangesDBInstance reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DBInstanceReconciler
participant EnsureRunner
participant KubernetesAPI
participant Harvester
Client->>DBInstanceReconciler: create or update DBInstance
DBInstanceReconciler->>EnsureRunner: run ordered ensure steps
EnsureRunner->>KubernetesAPI: resolve credentials and apply child resources
EnsureRunner->>Harvester: resolve image and reconcile VM state
Harvester-->>EnsureRunner: readiness and lifecycle result
EnsureRunner-->>DBInstanceReconciler: outcome and requeue
DBInstanceReconciler->>KubernetesAPI: patch status conditions
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml (1)
141-152: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStale CRD manifest:
make manifestswas not re-run after the type changes. The committed CRD still declaresengineVersion.default: "17",spec.osImage, andstatus.appliedSpec.imageRevision, none of which exist in the current Go types — and the retained default breaks one of the new CEL specs.
database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml#L141-L152: regenerate the CRD soengineVersionlosesdefault: "17"and the orphanedspec.osImage(L208-214) andstatus.appliedSpec.imageRevision(L337-343) properties are dropped.database/api/v1alpha1/dbinstance_types.go#L36-L41: removeosImagefrom the list of immutable fields compared inimmutableDrift(), or restore the field if its removal was unintended.database/internal/controller/dbinstance_cel_validation_test.go#L123-L131: re-verify this spec after regeneration — with the default removed,bareInstance()genuinely leavesengineVersionunset and the one-time set is permitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml` around lines 141 - 152, Regenerate the CRD with make manifests so engineVersion no longer has a "17" default and the obsolete spec.osImage and status.appliedSpec.imageRevision properties are removed. In database/api/v1alpha1/dbinstance_types.go, update immutableDrift() to stop comparing osImage unless the field is intentionally restored. Re-verify the engineVersion CEL validation in dbinstance_cel_validation_test.go so bareInstance() leaves it unset and permits its one-time assignment.
🧹 Nitpick comments (17)
database/internal/ensure/vm.go (1)
131-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover reviewer question in shipped code.
// Do we need this OS Validation?on Line 134 reads as an unresolved discussion note. Either resolve it or turn it into a statement of intent —ensurePreflightalready resolves the image, so this default is just the fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/ensure/vm.go` around lines 131 - 134, Remove the unresolved “Do we need this OS Validation?” comment after the osImage fallback in the relevant ensure flow, or replace it with a concise statement that this assignment provides the default OS image when inst.Spec.OSImage is empty. Preserve the existing fallback behavior.database/internal/resource/servicemonitor.go (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the Prometheus selector label configurable.
release: prometheushardcodes one particular kube-prometheus-stack release name. On a cluster where the stack was installed under any other release name, the ServiceMonitor is created successfully but never selected, so scraping silently never starts. The monitoring step already carries operator-level config (GrafanaBaseURL); threading aServiceMonitorLabels/PrometheusReleaseoption through the same path would avoid the silent failure mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/resource/servicemonitor.go` around lines 52 - 56, Make the Prometheus selector label configurable instead of hardcoding "release": "prometheus" in the ServiceMonitor construction. Add a ServiceMonitorLabels or PrometheusRelease option to the existing operator-level configuration path alongside GrafanaBaseURL, propagate it through the monitoring flow, and use it when populating sm.Labels while preserving the current default for backward compatibility.database/internal/credentials/resolver_test.go (1)
183-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test is currently a near-duplicate of
TestResolveReusesExistingMaterialOnReentry. Its stated scenario (cloud-init Secret gone, durable Secrets survive) isn't actually exercised — nothing is deleted. Either drop it or fold the*m1.TLS != *m2.TLSassertion into the re-entry test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/credentials/resolver_test.go` around lines 183 - 207, Remove TestResolveReusesMaterialWhenOnlyDurableSecretsSurvive because it performs no deletion and duplicates TestResolveReusesExistingMaterialOnReentry. Move its TLS material equality assertion into TestResolveReusesExistingMaterialOnReentry if that coverage is still needed.database/internal/harvester/typed_client_test.go (1)
31-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering
VMCreateParams.Ownerstamping here.ownerRefSliceis only exercised indirectly through the ensure-layer stub; a params-with-Owner case assertingvm.OwnerReferenceswould lock the GC/Owns() contract at the provider boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/harvester/typed_client_test.go` around lines 31 - 46, Extend testVMCreateParams or add a dedicated Owner-populated case to exercise VMCreateParams.Owner, then assert the resulting VM’s OwnerReferences contains the expected owner reference at the provider boundary. Ensure the assertion covers owner stamping through ownerRefSlice and preserves the existing creation behavior.database/internal/testutil/harvester.go (1)
26-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing repave stubs or narrow the
StubHarvestercontract.ClientInterfacedeclaresClearDataVolumeOwnerRef,DeleteDataVolume,DeletePVC, andSwapVMOSDisk, butStubHarvesteronly implements up throughTeardownAll, so it no longer satisfies the documented full interface. Either add the stub methods with a compile-time check or reword the contract to a narrower interface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/testutil/harvester.go` around lines 26 - 58, The StubHarvester contract claims full harvester.ClientInterface compatibility but lacks the repave methods ClearDataVolumeOwnerRef, DeleteDataVolume, DeletePVC, and SwapVMOSDisk. Add stubs for these methods with appropriate injectable errors or no-op behavior matching the interface, and include a compile-time assertion that StubHarvester implements harvester.ClientInterface.database/api/v1alpha1/condition_reason_enforcement_test.go (1)
38-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnforcement only covers three hardcoded, non-recursive directories.
filepath.Glob(dir + "/*.go")skips subpackages, so any new package (or a nested one underinternal/ensure) silently escapes the check. Walkinginternal/andapi/withfilepath.WalkDirwould keep the guard honest as the tree grows.♻️ Optional: walk instead of glob
- dirs := []string{ - filepath.Join(moduleRoot, "api", "v1alpha1"), - filepath.Join(moduleRoot, "internal", "controller"), - filepath.Join(moduleRoot, "internal", "ensure"), - } + roots := []string{ + filepath.Join(moduleRoot, "api"), + filepath.Join(moduleRoot, "internal"), + }…then collect
*.gofiles viafilepath.WalkDiroverroots, keeping the_test.goskip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/api/v1alpha1/condition_reason_enforcement_test.go` around lines 38 - 53, Update the file-discovery logic in the condition-reason enforcement test to recursively walk the configured roots with filepath.WalkDir instead of non-recursive filepath.Glob. Collect all Go files in nested packages, continue skipping files ending in _test.go, and preserve the existing error handling and enforcement behavior.database/internal/controller/watches.go (1)
92-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrack changes in
VirtualMachineInstanceNetworkInterface.IPstoo.KubeVirt keeps
IPas the first address fromIPs; if the primary address stays the same but a dual-stack entry is added or removed, this predicate treats it as unchanged and can skip the reconcile that refreshes endpoint connection data. Compare the sortedIPslabels for each interface name instead of onlyIP.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/watches.go` around lines 92 - 99, The vmiInterfaceIPs helper only tracks each interface’s primary IP and misses changes to additional addresses. Update vmiInterfaceIPs to map interface names to a deterministic representation of their sorted iface.IPs labels, so adding or removing dual-stack entries changes the comparison while preserving stable ordering.database/internal/controller/dbinstance_controller.go (2)
106-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the commented-out defer block.
Status patching now lives in
reconcileInstance/reconcileDelete; the stale comment plus dead block is misleading.♻️ Proposed cleanup
logger.Info("Reconciling", "name", inst.Name, "phase", inst.Status.Phase) - // use defer statement to patch status - - /* defer func() { - if err := r.patchStatusIfChanged(ctx, &inst, &inst); err != nil { - logger.Error(err, "Failed to patch status") - } - }() */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/dbinstance_controller.go` around lines 106 - 113, Remove the commented-out defer block and its accompanying “use defer statement to patch status” comment near the reconciliation logging in the controller; retain the active status patching implemented by reconcileInstance and reconcileDelete.
21-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
osandslicesimports.
database/internal/controller/dbinstance_controller.go:23-24:osandslicesare imported but not referenced in the file, so the package will fail the Go compiler’s unused-import check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/dbinstance_controller.go` around lines 21 - 46, Remove the unused os and slices imports from the import block of the dbinstance controller, leaving all referenced imports unchanged.database/internal/controller/reconcile_instance.go (1)
39-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider joining the patch error with the ensure error.
If a step failed and the status patch also fails,
result.Erris silently dropped, losing the underlying cause in logs.errors.Join(result.Err, patchErr)keeps both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/reconcile_instance.go` around lines 39 - 43, Update the reconciliation return path around patchStatusIfChanged to join its patch error with result.Err using errors.Join, preserving both errors when the status patch and ensure step fail. Return the joined error while keeping result.ControllerResult unchanged.database/internal/controller/reconcile_instance_test.go (1)
295-304: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
StringDataassertions only hold with the fake client.A real API server merges
StringDataintoDataon write, so these checks would fail under envtest. Asserting onData(with the writer normalizing) keeps the test portable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/reconcile_instance_test.go` around lines 295 - 304, Update the tenant credentials assertions in the reconciliation test to inspect tenantCred.Data instead of tenantCred.StringData, since persisted secrets normalize StringData into Data on real API servers. Preserve the existing admin_password presence and ca_cert absence checks, and ensure the test setup writes or normalizes the expected values in Data.database/internal/controller/controller_test_helpers_test.go (2)
31-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest-local copies of production requeue values. Both sites depend on requeue/threshold constants re-declared in the test package rather than sourced from
ensure, so a production timing change leaves the tests asserting stale values (and lets one step's constant stand in for another's).
database/internal/controller/controller_test_helpers_test.go#L31-L38: source these constants from theensurepackage (or add exported values there) instead of re-declaring them.database/internal/controller/reconcile_instance_test.go#L185-L187: assert the connection-secret pass against its own constant rather than reusingcredentialRequeue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/controller_test_helpers_test.go` around lines 31 - 38, Replace the test-local requeue and threshold declarations in controller_test_helpers_test.go with values sourced from the ensure package, exporting the production constants there if necessary. In reconcile_instance_test.go, update the connection-secret assertion to use its dedicated ensure constant rather than credentialRequeue; apply both changes so tests track production values independently.
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnknown step name degrades into a confusing outcome mismatch.
A typo in
nameyieldsTransient(...), so callers report "want Pending" instead of "unknown step". Consider having callers fail fast on that error text — optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/controller/controller_test_helpers_test.go` around lines 54 - 61, Update runEnsureStep to fail fast when no step in ensure.NewDefaultSteps matches name, rather than returning ensure.Transient with an unknown-step error. Make the helper surface the invalid step name directly so callers receive an explicit unknown-step failure instead of an outcome mismatch.database/internal/ensure/bootstrap_cleanup.go (1)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment split by blank line loses the function's
go docsummary.The blank line at Line 45 separates the two comment blocks. Go only attaches the contiguous block immediately preceding the declaration (Lines 46-48) as the doc comment for
Run; the block naming/explainingensureBootstrapCleanup(Lines 42-44) becomes a floating comment that won't show up viago doc/IDE hover.Merge into a single contiguous doc comment
-// ensureBootstrapCleanup redacts the sensitive half of the ephemeral -// cloud-init Secret once the database is provably up. It does NOT delete the -// Secret and does NOT touch the VM. - -// A create/update stops the pass so the next reconcile re-observes the -// persisted redaction. Once the Secret is unchanged, the step is Satisfied and -// generation reconciliation may continue. +// ensureBootstrapCleanup redacts the sensitive half of the ephemeral +// cloud-init Secret once the database is provably up. It does NOT delete the +// Secret and does NOT touch the VM. +// +// A create/update stops the pass so the next reconcile re-observes the +// persisted redaction. Once the Secret is unchanged, the step is Satisfied and +// generation reconciliation may continue. func (r *bootstrapCleanupStep) Run(ctx context.Context, inst *dbaasv1.DBInstance) Result {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/ensure/bootstrap_cleanup.go` around lines 42 - 49, Merge the two comment blocks immediately preceding bootstrapCleanupStep.Run into one contiguous Go doc comment, removing the blank line between them. Preserve all existing explanation while ensuring the comment begins with the Run function’s summary so go doc and IDE hover include the complete documentation.database/internal/ensure/bootstrap_cleanup_test.go (1)
28-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the actual redaction path.
Both tests only cover the "nothing to do" branches (no secret yet / DatabaseReady still false). The core behavior in
bootstrap_cleanup.go(Lines 59-73) — applying the redacted cloud-init Secret, the Pending-on-create/update outcome, the Satisfied-on-no-op outcome, and the Transient-on-Apply-error outcome — has no test exercising it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/ensure/bootstrap_cleanup_test.go` around lines 28 - 48, The tests around ensureBootstrapCleanup need coverage for the actual cloud-init Secret redaction flow. Extend the bootstrap cleanup tests to exercise applying the redacted Secret, asserting Pending for create/update operations, Satisfied when no changes are needed, and Transient when Apply returns an error; anchor these cases to testHarness.ensureBootstrapCleanup and the existing Apply test setup.database/internal/ensure/connection_secret.go (1)
1-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for the connection-secret step.
Every sibling ensure step in this layer (
credentials.go,bootstrap_cleanup.go,health.go,monitoring.go,power.go,generation.go) ships a_test.go, but this file has none in the PR. This step has several branches (endpoint-not-ready, credential-changed, apply-error, apply-changed vs no-op) worth covering, especially theresolved.Changedgate that is easy to regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/ensure/connection_secret.go` around lines 1 - 79, Add unit tests for connectionSecretStep.Run covering the endpoint-not-ready, credential-resolution error, resolved.Changed, apply error, changed-operation, and no-op paths. Follow the existing sibling ensure-step test patterns and verify each branch’s Result, status updates, and requeue behavior, especially the resolved.Changed gate.database/internal/ensure/health.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
dbNamefallback logic across two ensure steps. Both steps repeat the sameinst.Spec.DBName→inst.Namefallback inline; extracting a small shared helper (e.g. inhelpers.go) avoids drift if the default ever changes.
database/internal/ensure/health.go#L153-156: replace the inline fallback with a shareddbNameFor(inst)helper.database/internal/ensure/connection_secret.go#L59-62: replace the inline fallback with the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/internal/ensure/health.go` at line 1, Extract the repeated inst.Spec.DBName-to-inst.Name fallback into a shared dbNameFor(inst) helper, then update the ensure logic in health.go and connection_secret.go to use it instead of inline checks. Preserve the current behavior: return Spec.DBName when set, otherwise use inst.Name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@database/api/v1alpha1/dbinstance_types.go`:
- Around line 36-41: Remove the stale osImage reference from the immutability
comment and ensure its immutableDrift/snapshot handling is not implied or
applied as a supported field. Keep the documented immutable fields aligned with
the actual DBInstanceSpec, AppliedSpec, and CRD definitions.
In `@database/config/manager/manager.yaml`:
- Around line 74-78: Merge the POD_NAMESPACE entry into the existing env list
for the container instead of declaring a second env key. Preserve the existing
BACKING_IMAGE_OS_VERSION entry and append POD_NAMESPACE as another item under
that same env array.
In `@database/internal/controller/dbinstance_controller.go`:
- Around line 212-221: Update the deleteRef closure to reject references with an
empty namespace as well as an empty name before constructing the Secret, so
inputs like “/x” are skipped without calling r.Delete; correct the malformed
“namesapce/name” comment text to “namespace/name”.
In `@database/internal/credentials/cloudinit_test.go`:
- Around line 55-70: The cloud-init test assertions must reflect shell-quoted
password rendering and verify metacharacter safety. Update the password
expectations in the userdata assertion loop of the relevant test, and add a
metacharacter password case patterned after
TestBuildCloudInitBackupConfigNeutralizesShellMetacharacters to confirm
generated values are safely quoted.
In `@database/internal/credentials/cloudinit.go`:
- Around line 313-315: Wrap m.AdminPassword, m.ReplPassword, and
m.ExporterPassword with shellSingleQuote in cloudinit.go, matching the S3
fields. In cloudinit_test.go, update the password expectations to the quoted
form and add a metacharacter-bearing password case alongside
TestBuildCloudInitBackupConfigNeutralizesShellMetacharacters.
- Around line 32-46: Add an EngineVersion string field to
credentials.BootstrapParams, then update the BuildCloudInit call in ensure to
populate it from inst.Spec.EngineVersion. Ensure buildUserData can use
p.EngineVersion when formatting ENGINE_VERSION.
In `@database/internal/credentials/resolver.go`:
- Around line 112-120: Harden Resolver.getOrCreateTenant before adopting an
existing tenant Secret: require the Secret to carry the controller’s expected
owner reference, and reject admin_user/admin_password material from
tenantMaterialFrom when it violates the safe credential charset. Preserve the
existing NotFound creation path, but return an error instead of using
unauthorized or invalid pre-existing credentials.
In `@database/internal/ensure/defaults.go`:
- Around line 63-66: Add an OSImage field to the AppliedSpec status snapshot
type used by the defaults logic, then update defaults.go and defaults_test.go to
use that field for persisting and comparing immutable image state. Regenerate
the associated CRD artifacts so the new status field is represented in the
schema.
In `@database/internal/ensure/vm_test.go`:
- Around line 104-110: The credential assertion in the test incorrectly reads
only cred.StringData, which is write-only with a real API server. Update the
assertion after the credentials secret lookup to validate the admin_password
value from both cred.Data and cred.StringData, matching the behavior of
credentials.get and preserving failure when neither contains a value.
In `@database/internal/harvester/typed_client.go`:
- Around line 363-376: Update ResolveVMImage to reject references beginning with
"/" as ErrVMImageReferenceInvalid before namespace/spec resolution. Ensure
leading-slash inputs return the semantic invalid-reference error rather than
reaching the API lookup, while preserving existing handling for valid
namespace/name and empty-spec references.
In `@database/README.md`:
- Around line 25-32: Update the quickstart command block in the database README
to export KUBECONFIG before the build, install, deploy, and kubectl commands,
ensuring every command uses the Harvester kubeconfig consistently.
---
Outside diff comments:
In `@database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml`:
- Around line 141-152: Regenerate the CRD with make manifests so engineVersion
no longer has a "17" default and the obsolete spec.osImage and
status.appliedSpec.imageRevision properties are removed. In
database/api/v1alpha1/dbinstance_types.go, update immutableDrift() to stop
comparing osImage unless the field is intentionally restored. Re-verify the
engineVersion CEL validation in dbinstance_cel_validation_test.go so
bareInstance() leaves it unset and permits its one-time assignment.
---
Nitpick comments:
In `@database/api/v1alpha1/condition_reason_enforcement_test.go`:
- Around line 38-53: Update the file-discovery logic in the condition-reason
enforcement test to recursively walk the configured roots with filepath.WalkDir
instead of non-recursive filepath.Glob. Collect all Go files in nested packages,
continue skipping files ending in _test.go, and preserve the existing error
handling and enforcement behavior.
In `@database/internal/controller/controller_test_helpers_test.go`:
- Around line 31-38: Replace the test-local requeue and threshold declarations
in controller_test_helpers_test.go with values sourced from the ensure package,
exporting the production constants there if necessary. In
reconcile_instance_test.go, update the connection-secret assertion to use its
dedicated ensure constant rather than credentialRequeue; apply both changes so
tests track production values independently.
- Around line 54-61: Update runEnsureStep to fail fast when no step in
ensure.NewDefaultSteps matches name, rather than returning ensure.Transient with
an unknown-step error. Make the helper surface the invalid step name directly so
callers receive an explicit unknown-step failure instead of an outcome mismatch.
In `@database/internal/controller/dbinstance_controller.go`:
- Around line 106-113: Remove the commented-out defer block and its accompanying
“use defer statement to patch status” comment near the reconciliation logging in
the controller; retain the active status patching implemented by
reconcileInstance and reconcileDelete.
- Around line 21-46: Remove the unused os and slices imports from the import
block of the dbinstance controller, leaving all referenced imports unchanged.
In `@database/internal/controller/reconcile_instance_test.go`:
- Around line 295-304: Update the tenant credentials assertions in the
reconciliation test to inspect tenantCred.Data instead of tenantCred.StringData,
since persisted secrets normalize StringData into Data on real API servers.
Preserve the existing admin_password presence and ca_cert absence checks, and
ensure the test setup writes or normalizes the expected values in Data.
In `@database/internal/controller/reconcile_instance.go`:
- Around line 39-43: Update the reconciliation return path around
patchStatusIfChanged to join its patch error with result.Err using errors.Join,
preserving both errors when the status patch and ensure step fail. Return the
joined error while keeping result.ControllerResult unchanged.
In `@database/internal/controller/watches.go`:
- Around line 92-99: The vmiInterfaceIPs helper only tracks each interface’s
primary IP and misses changes to additional addresses. Update vmiInterfaceIPs to
map interface names to a deterministic representation of their sorted iface.IPs
labels, so adding or removing dual-stack entries changes the comparison while
preserving stable ordering.
In `@database/internal/credentials/resolver_test.go`:
- Around line 183-207: Remove
TestResolveReusesMaterialWhenOnlyDurableSecretsSurvive because it performs no
deletion and duplicates TestResolveReusesExistingMaterialOnReentry. Move its TLS
material equality assertion into TestResolveReusesExistingMaterialOnReentry if
that coverage is still needed.
In `@database/internal/ensure/bootstrap_cleanup_test.go`:
- Around line 28-48: The tests around ensureBootstrapCleanup need coverage for
the actual cloud-init Secret redaction flow. Extend the bootstrap cleanup tests
to exercise applying the redacted Secret, asserting Pending for create/update
operations, Satisfied when no changes are needed, and Transient when Apply
returns an error; anchor these cases to testHarness.ensureBootstrapCleanup and
the existing Apply test setup.
In `@database/internal/ensure/bootstrap_cleanup.go`:
- Around line 42-49: Merge the two comment blocks immediately preceding
bootstrapCleanupStep.Run into one contiguous Go doc comment, removing the blank
line between them. Preserve all existing explanation while ensuring the comment
begins with the Run function’s summary so go doc and IDE hover include the
complete documentation.
In `@database/internal/ensure/connection_secret.go`:
- Around line 1-79: Add unit tests for connectionSecretStep.Run covering the
endpoint-not-ready, credential-resolution error, resolved.Changed, apply error,
changed-operation, and no-op paths. Follow the existing sibling ensure-step test
patterns and verify each branch’s Result, status updates, and requeue behavior,
especially the resolved.Changed gate.
In `@database/internal/ensure/health.go`:
- Line 1: Extract the repeated inst.Spec.DBName-to-inst.Name fallback into a
shared dbNameFor(inst) helper, then update the ensure logic in health.go and
connection_secret.go to use it instead of inline checks. Preserve the current
behavior: return Spec.DBName when set, otherwise use inst.Name.
In `@database/internal/ensure/vm.go`:
- Around line 131-134: Remove the unresolved “Do we need this OS Validation?”
comment after the osImage fallback in the relevant ensure flow, or replace it
with a concise statement that this assignment provides the default OS image when
inst.Spec.OSImage is empty. Preserve the existing fallback behavior.
In `@database/internal/harvester/typed_client_test.go`:
- Around line 31-46: Extend testVMCreateParams or add a dedicated
Owner-populated case to exercise VMCreateParams.Owner, then assert the resulting
VM’s OwnerReferences contains the expected owner reference at the provider
boundary. Ensure the assertion covers owner stamping through ownerRefSlice and
preserves the existing creation behavior.
In `@database/internal/resource/servicemonitor.go`:
- Around line 52-56: Make the Prometheus selector label configurable instead of
hardcoding "release": "prometheus" in the ServiceMonitor construction. Add a
ServiceMonitorLabels or PrometheusRelease option to the existing operator-level
configuration path alongside GrafanaBaseURL, propagate it through the monitoring
flow, and use it when populating sm.Labels while preserving the current default
for backward compatibility.
In `@database/internal/testutil/harvester.go`:
- Around line 26-58: The StubHarvester contract claims full
harvester.ClientInterface compatibility but lacks the repave methods
ClearDataVolumeOwnerRef, DeleteDataVolume, DeletePVC, and SwapVMOSDisk. Add
stubs for these methods with appropriate injectable errors or no-op behavior
matching the interface, and include a compile-time assertion that StubHarvester
implements harvester.ClientInterface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dba921ae-5c37-49a1-9e1e-2e692de5738d
📒 Files selected for processing (93)
database/.gitignoredatabase/README.mddatabase/api/v1alpha1/condition_reason_enforcement_test.godatabase/api/v1alpha1/dbinstance_conditions.godatabase/api/v1alpha1/dbinstance_conditions_test.godatabase/api/v1alpha1/dbinstance_types.godatabase/api/v1alpha1/zz_generated.deepcopy.godatabase/cmd/harvester_dynamic.godatabase/cmd/harvester_typed.godatabase/cmd/main.godatabase/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yamldatabase/config/manager/manager.yamldatabase/config/rbac/role.yamldatabase/go.moddatabase/internal/controller/bootstrap_cleanup_envtest_test.godatabase/internal/controller/controller_test_helpers_test.godatabase/internal/controller/dbinstance_cel_validation_test.godatabase/internal/controller/dbinstance_controller.godatabase/internal/controller/dbinstance_controller_test.godatabase/internal/controller/failed_recovery_test.godatabase/internal/controller/liveness_test.godatabase/internal/controller/probe_gate_test.godatabase/internal/controller/reconcile_crash_loop_test.godatabase/internal/controller/reconcile_delete_test.godatabase/internal/controller/reconcile_instance.godatabase/internal/controller/reconcile_instance_monitoring_test.godatabase/internal/controller/reconcile_instance_test.godatabase/internal/controller/reconcile_power_lifecycle_test.godatabase/internal/controller/resource_secret_builder_envtest_test.godatabase/internal/controller/status.godatabase/internal/controller/status_conditions.godatabase/internal/controller/status_conditions_test.godatabase/internal/controller/status_ready.godatabase/internal/controller/status_ready_test.godatabase/internal/controller/status_test.godatabase/internal/controller/stop_start_test.godatabase/internal/controller/suite_test.godatabase/internal/controller/watches.godatabase/internal/credentials/cloudinit.godatabase/internal/credentials/cloudinit_test.godatabase/internal/credentials/material.godatabase/internal/credentials/material_test.godatabase/internal/credentials/resolver.godatabase/internal/credentials/resolver_test.godatabase/internal/ensure/bootstrap_cleanup.godatabase/internal/ensure/bootstrap_cleanup_test.godatabase/internal/ensure/connection_secret.godatabase/internal/ensure/credentials.godatabase/internal/ensure/credentials_test.godatabase/internal/ensure/defaults.godatabase/internal/ensure/defaults_test.godatabase/internal/ensure/dependencies.godatabase/internal/ensure/errors.godatabase/internal/ensure/errors_test.godatabase/internal/ensure/generation.godatabase/internal/ensure/generation_test.godatabase/internal/ensure/health.godatabase/internal/ensure/health_crashloop_test.godatabase/internal/ensure/health_liveness_test.godatabase/internal/ensure/health_test.godatabase/internal/ensure/helpers.godatabase/internal/ensure/monitoring.godatabase/internal/ensure/monitoring_test.godatabase/internal/ensure/power.godatabase/internal/ensure/power_test.godatabase/internal/ensure/preflight.godatabase/internal/ensure/preflight_test.godatabase/internal/ensure/resize.godatabase/internal/ensure/resize_test.godatabase/internal/ensure/result.godatabase/internal/ensure/result_test.godatabase/internal/ensure/runner.godatabase/internal/ensure/runner_test.godatabase/internal/ensure/step.godatabase/internal/ensure/steps_test_helpers_test.godatabase/internal/ensure/vm.godatabase/internal/ensure/vm_test.godatabase/internal/harvester/client.godatabase/internal/harvester/client_test.godatabase/internal/harvester/interface.godatabase/internal/harvester/typed_client.godatabase/internal/harvester/typed_client_test.godatabase/internal/resource/builder.godatabase/internal/resource/builder_test.godatabase/internal/resource/cloudinit_secret.godatabase/internal/resource/connection_secret.godatabase/internal/resource/metrics_endpoints.godatabase/internal/resource/metrics_service.godatabase/internal/resource/servicemonitor.godatabase/internal/testutil/client.godatabase/internal/testutil/dbinstance.godatabase/internal/testutil/harvester.godatabase/internal/testutil/kubevirt.go
💤 Files with no reviewable changes (8)
- database/cmd/harvester_dynamic.go
- database/cmd/harvester_typed.go
- database/internal/controller/failed_recovery_test.go
- database/internal/controller/probe_gate_test.go
- database/internal/controller/stop_start_test.go
- database/internal/controller/liveness_test.go
- database/internal/harvester/client.go
- database/internal/harvester/client_test.go
Comment on lines
+36
to
+41
| // | ||
| // Of the immutable fields, only networkRef, engineVersion, staticNetwork, and | ||
| // vmPassword carry a CEL "self == oldSelf" rule: the other five (osImage, | ||
| // dbName, masterUsername, port, storageType) are compared post-defaulting in | ||
| // immutableDrift(), so a raw CEL rule on them would be stricter than that | ||
| // check — see immutableDrift's doc comment. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=go '\bOSImage\b|\bImageRevision\b' database/ -C2Repository: wso2/open-cloud-datacenter
Length of output: 4569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate dbinstance_type.yaml and dbinstance_types.go =="
fd -a 'dbinstance_type\.yaml$|dbinstance_types\.go$' database | sed 's#^\./##'
echo
echo "== relevant occurrences in Go types =="
rg -n 'struct \{|AppliedSpec|osImage|OSImage|imageRevision|ImageRevision|XValidation|immutableDrift|Default(' database/api/v1alpha1/dbinstance_types.go database/internal/ensure -C2 || true
echo
echo "== CRD property snippets for spec.osImage and appliedSpec =="
python3 - <<'PY'
from pathlib import Path
p = next(Path('database').rglob('dbinstance_type.yaml')),
p = next(Path('database').rglob('dbinstance_type.yaml'))
txt=p.read_text()
lines=txt.splitlines()
for name in ['osImage', 'ImageRevision', 'imageRevision', 'appliedSpec']:
print(f"\n-- {name} in {p} --")
for i,l in enumerate(lines,1):
if name in l:
start=max(1,i-8); end=min(len(lines),i+18)
for n in range(start,end+1):
print(f"{n:5d}\t{lines[n-1]}")
print()
PYRepository: wso2/open-cloud-datacenter
Length of output: 725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files =="
git ls-files database | grep -E '(^|/)dbinstance_type\.ya?ml$|dbinstance_types\.go$|vm\.go$|defaults\.go$|preflight\.go$' | sort
echo
echo "== Go occurrences =="
rg -n --type=go '\bAppliedSpec\b|\bosImage\b|\bOSImage\b|\bimageRevision\b|\bImageRevision\b|\bXValidation\b|\bimmutableDrift\b|\bDefault\(' database/api/v1alpha1/dbinstance_types.go database/internal/ensure -C2 || true
echo
echo "== CRD type files and relevant property snippets =="
for p in $(git ls-files database | grep -E 'dbinstance_type\.ya?ml$'); do
echo "-- $p --"
rg -n 'osImage|imageRevision|imageRevision|appliedSpec|default: "17"' "$p" -C3 || true
done
echo
echo "== dbinstance_types.go relevant sections =="
sed -n '30,220p' database/api/v1alpha1/dbinstance_types.go
sed -n '380,470p' database/api/v1alpha1/dbinstance_types.goRepository: wso2/open-cloud-datacenter
Length of output: 31329
Fix the stale osImage reference in the immutability comment.
The comment lists osImage as an immutable-spec field, but no OSImage field exists in the Go DBInstanceSpec, AppliedSpec, or database CRD type file. Either add the spec mapping for osImage, or remove it from this comment and apply the drift/snapshot behavior consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/api/v1alpha1/dbinstance_types.go` around lines 36 - 41, Remove the
stale osImage reference from the immutability comment and ensure its
immutableDrift/snapshot handling is not implied or applied as a supported field.
Keep the documented immutable fields aligned with the actual DBInstanceSpec,
AppliedSpec, and CRD definitions.
Comment on lines
+74
to
+78
| env: | ||
| - name: POD_NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Merge POD_NAMESPACE into the existing env list.
The container already defines env at Lines 69-71. Defining it again creates a duplicate YAML key, which can reject the Deployment or overwrite BACKING_IMAGE_OS_VERSION.
Proposed fix
env:
- name: BACKING_IMAGE_OS_VERSION
value: "24.04"
+ - name: POD_NAMESPACE
+ valueFrom:
+ fieldRef:
+ fieldPath: metadata.namespace
image: controller:latest
name: manager
- env:
- - name: POD_NAMESPACE
- valueFrom:
- fieldRef:
- fieldPath: metadata.namespace🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 74-74: duplication of key "env" in mapping
(key-duplicates)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/config/manager/manager.yaml` around lines 74 - 78, Merge the
POD_NAMESPACE entry into the existing env list for the container instead of
declaring a second env key. Preserve the existing BACKING_IMAGE_OS_VERSION entry
and append POD_NAMESPACE as another item under that same env array.
Source: Linters/SAST tools
Comment on lines
+212
to
221
| deleteRef := func(ref string) { | ||
| ns, name, ok := strings.Cut(ref, "/") | ||
| if !ok || name == "" { | ||
| return // If namesapce/name reference is malformed, skip deletion. | ||
| } | ||
| if err := r.Harvester.DeletePVC(ctx, ns, oldOSDisk); err != nil { | ||
| return r.fail(ctx, inst, "RepaveDeleteOSDiskFailed", err) | ||
| sec := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name}} | ||
| if err := r.Delete(ctx, sec); err != nil && !errors.IsNotFound(err) { | ||
| errs = append(errs, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard empty namespace and fix the namesapce typo.
strings.Cut("/x", "/") returns ok == true with an empty namespace, which produces a client error rather than a skip.
🛠️ Proposed fix
deleteRef := func(ref string) {
ns, name, ok := strings.Cut(ref, "/")
- if !ok || name == "" {
- return // If namesapce/name reference is malformed, skip deletion.
+ if !ok || ns == "" || name == "" {
+ return // If the namespace/name reference is malformed, skip deletion.
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deleteRef := func(ref string) { | |
| ns, name, ok := strings.Cut(ref, "/") | |
| if !ok || name == "" { | |
| return // If namesapce/name reference is malformed, skip deletion. | |
| } | |
| if err := r.Harvester.DeletePVC(ctx, ns, oldOSDisk); err != nil { | |
| return r.fail(ctx, inst, "RepaveDeleteOSDiskFailed", err) | |
| sec := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name}} | |
| if err := r.Delete(ctx, sec); err != nil && !errors.IsNotFound(err) { | |
| errs = append(errs, err) | |
| } | |
| } | |
| deleteRef := func(ref string) { | |
| ns, name, ok := strings.Cut(ref, "/") | |
| if !ok || ns == "" || name == "" { | |
| return // If the namespace/name reference is malformed, skip deletion. | |
| } | |
| sec := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name}} | |
| if err := r.Delete(ctx, sec); err != nil && !errors.IsNotFound(err) { | |
| errs = append(errs, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/controller/dbinstance_controller.go` around lines 212 -
221, Update the deleteRef closure to reject references with an empty namespace
as well as an empty name before constructing the Secret, so inputs like “/x” are
skipped without calling r.Delete; correct the malformed “namesapce/name” comment
text to “namespace/name”.
Comment on lines
+55
to
+70
| for _, want := range []string{ | ||
| "INSTANCE_ID=orders", | ||
| "DB_NAME=orders", | ||
| "DB_PORT=5432", | ||
| "MASTER_USER=dbadmin", | ||
| "MASTER_PASSWORD=admin-pw", | ||
| "REPL_PASSWORD=repl-pw", | ||
| "EXPORTER_PASSWORD=exporter-pw", | ||
| "MAX_CONNECTIONS=100", | ||
| `hostssl all all 0.0.0.0/0 scram-sha-256`, | ||
| `hostssl replication all 0.0.0.0/0 scram-sha-256`, | ||
| } { | ||
| if !strings.Contains(userdata, want) { | ||
| t.Errorf("userdata missing %q", want) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
These assertions lock in the unquoted-password rendering.
MASTER_PASSWORD=admin-pw / REPL_PASSWORD=… / EXPORTER_PASSWORD=… assert the raw form. Once the passwords are shell-quoted (see database/internal/credentials/cloudinit.go Lines 313-315), update these to the quoted form and add a metacharacter case mirroring TestBuildCloudInitBackupConfigNeutralizesShellMetacharacters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/credentials/cloudinit_test.go` around lines 55 - 70, The
cloud-init test assertions must reflect shell-quoted password rendering and
verify metacharacter safety. Update the password expectations in the userdata
assertion loop of the relevant test, and add a metacharacter password case
patterned after TestBuildCloudInitBackupConfigNeutralizesShellMetacharacters to
confirm generated values are safely quoted.
Comment on lines
+32
to
+46
| type BootstrapParams struct { | ||
| ID string | ||
| DBName string | ||
| Port int | ||
| MasterUser string | ||
| MaxConnections int | ||
| BackupEnabled bool | ||
| BackupWindow string | ||
| S3Config *dbaasv1.S3BackupConfig | ||
| VMPassword string | ||
| // StaticNetwork, when non-nil, makes the cloud-init netplan use a | ||
| // static IPv4 config instead of DHCP. Used on VLANs without a DHCP | ||
| // server. | ||
| StaticNetwork *dbaasv1.NetworkConfig | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether BootstrapParams declares EngineVersion and who sets it.
fd -t f 'cloudinit.go' database/internal/credentials --exec cat -n {}
rg -nP 'EngineVersion' database --type=go -C2Repository: wso2/open-cloud-datacenter
Length of output: 20107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cloudinit BootstrapParams/BuildCloudInit references =="
rg -n "BootstrapParams|BuildCloudInit|EngineVersion" database/internal/credentials/database/internal/credentials --type=go -C2 || true
echo
echo "== vm.go BuildCloudInit call site context =="
sed -n '170,230p' database/internal/ensure/vm.go | cat -n
echo
echo "== credential package file list =="
git ls-files database/internal/credentials --type fRepository: wso2/open-cloud-datacenter
Length of output: 5047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== credential package files =="
git ls-files 'database/internal/credentials/*'
echo
echo "== BuildCloudInit call site in database/internal/credentials =="
rg -n "BuildCloudInit|BootstrapParams|EngineVersion" database/internal/credentials --type=go -C3
echo
echo "== BuildCloudInit call sites in ensure packages =="
rg -n "credentials\.BuildCloudInit|BuildCloudInit\(" database/internal/ensure --type=go -C5Repository: wso2/open-cloud-datacenter
Length of output: 11909
Compile break: pass the engine version through BootstrapParams.
buildUserData formats p.EngineVersion for ENGINE_VERSION=%s, but credentials.BootstrapParams has no such field and the BuildCloudInit call in database/internal/ensure/vm.go omits it at line 153. Add EngineVersion string to bootstrapParams and populate/from it from inst.Spec.EngineVersion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/credentials/cloudinit.go` around lines 32 - 46, Add an
EngineVersion string field to credentials.BootstrapParams, then update the
BuildCloudInit call in ensure to populate it from inst.Spec.EngineVersion.
Ensure buildUserData can use p.EngineVersion when formatting ENGINE_VERSION.
Comment on lines
+112
to
+120
| func (r *Resolver) getOrCreateTenant(ctx context.Context, inst *dbaasv1.DBInstance) (adminUser, adminPassword string, changed bool, err error) { | ||
| key := types.NamespacedName{Namespace: inst.Namespace, Name: TenantCredentialsSecretName(inst)} | ||
| var sec corev1.Secret | ||
| if getErr := r.Client.Get(ctx, key, &sec); getErr == nil { | ||
| adminUser, adminPassword, err = tenantMaterialFrom(&sec, key) | ||
| return adminUser, adminPassword, false, err | ||
| } else if !apierrors.IsNotFound(getErr) { | ||
| return "", "", false, getErr | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Adopting any pre-existing tenant Secret lets the tenant choose the DB admin identity.
pg-<name>-credentials lives in the tenant namespace and is tenant-writable. If it exists before the first reconcile, tenantMaterialFrom takes both admin_user and admin_password from it and inst.Spec.MasterUsername is silently ignored — the value then flows into cloud-init as MASTER_USER (spliced into CREATE ROLE "${MASTER_USER}") and MASTER_PASSWORD. At minimum, verify the existing Secret carries this controller's owner reference before adopting it, and reject material that doesn't match a safe charset.
🛡️ Sketch
if getErr := r.Client.Get(ctx, key, &sec); getErr == nil {
+ if !metav1.IsControlledBy(&sec, inst) {
+ return "", "", false, fmt.Errorf("credentials secret %s/%s exists but is not owned by this DBInstance", key.Namespace, key.Name)
+ }
adminUser, adminPassword, err = tenantMaterialFrom(&sec, key)
return adminUser, adminPassword, false, err
} else if !apierrors.IsNotFound(getErr) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *Resolver) getOrCreateTenant(ctx context.Context, inst *dbaasv1.DBInstance) (adminUser, adminPassword string, changed bool, err error) { | |
| key := types.NamespacedName{Namespace: inst.Namespace, Name: TenantCredentialsSecretName(inst)} | |
| var sec corev1.Secret | |
| if getErr := r.Client.Get(ctx, key, &sec); getErr == nil { | |
| adminUser, adminPassword, err = tenantMaterialFrom(&sec, key) | |
| return adminUser, adminPassword, false, err | |
| } else if !apierrors.IsNotFound(getErr) { | |
| return "", "", false, getErr | |
| } | |
| func (r *Resolver) getOrCreateTenant(ctx context.Context, inst *dbaasv1.DBInstance) (adminUser, adminPassword string, changed bool, err error) { | |
| key := types.NamespacedName{Namespace: inst.Namespace, Name: TenantCredentialsSecretName(inst)} | |
| var sec corev1.Secret | |
| if getErr := r.Client.Get(ctx, key, &sec); getErr == nil { | |
| if !metav1.IsControlledBy(&sec, inst) { | |
| return "", "", false, fmt.Errorf("credentials secret %s/%s exists but is not owned by this DBInstance", key.Namespace, key.Name) | |
| } | |
| adminUser, adminPassword, err = tenantMaterialFrom(&sec, key) | |
| return adminUser, adminPassword, false, err | |
| } else if !apierrors.IsNotFound(getErr) { | |
| return "", "", false, getErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/credentials/resolver.go` around lines 112 - 120, Harden
Resolver.getOrCreateTenant before adopting an existing tenant Secret: require
the Secret to carry the controller’s expected owner reference, and reject
admin_user/admin_password material from tenantMaterialFrom when it violates the
safe credential charset. Preserve the existing NotFound creation path, but
return an error instead of using unauthorized or invalid pre-existing
credentials.
Comment on lines
+63
to
+66
| appliedOSImage := applied.OSImage | ||
| if appliedOSImage == "" { | ||
| appliedOSImage = defaultOSImage | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore the missing AppliedSpec.OSImage contract.
AppliedSpec has no OSImage field, so applied.OSImage does not compile; database/internal/ensure/defaults_test.go line 64 initializes the same nonexistent field. Add OSImage to the status snapshot and regenerate its CRD artifacts so immutable image drift can be persisted and compared.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/ensure/defaults.go` around lines 63 - 66, Add an OSImage
field to the AppliedSpec status snapshot type used by the defaults logic, then
update defaults.go and defaults_test.go to use that field for persisting and
comparing immutable image state. Regenerate the associated CRD artifacts so the
new status field is represented in the schema.
Comment on lines
+104
to
+110
| var cred corev1.Secret | ||
| if err := r.Get(ctx, types.NamespacedName{Namespace: "tenant-a", Name: credentials.TenantCredentialsSecretName(inst)}, &cred); err != nil { | ||
| t.Fatalf("tenant credentials secret missing: %v", err) | ||
| } | ||
| if cred.StringData["admin_password"] == "" { | ||
| t.Fatal("tenant credentials secret has no admin_password") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assertion depends on fake-client StringData semantics.
A real API server never returns StringData on a read — it's write-only and merged into Data. This assertion only holds because the fake client round-trips StringData verbatim, so the test would silently invert its meaning if this fixture ever moved to envtest. Check both, the way credentials.get does.
💚 Proposed fix
- if cred.StringData["admin_password"] == "" {
+ if len(cred.Data["admin_password"]) == 0 && cred.StringData["admin_password"] == "" {
t.Fatal("tenant credentials secret has no admin_password")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var cred corev1.Secret | |
| if err := r.Get(ctx, types.NamespacedName{Namespace: "tenant-a", Name: credentials.TenantCredentialsSecretName(inst)}, &cred); err != nil { | |
| t.Fatalf("tenant credentials secret missing: %v", err) | |
| } | |
| if cred.StringData["admin_password"] == "" { | |
| t.Fatal("tenant credentials secret has no admin_password") | |
| } | |
| var cred corev1.Secret | |
| if err := r.Get(ctx, types.NamespacedName{Namespace: "tenant-a", Name: credentials.TenantCredentialsSecretName(inst)}, &cred); err != nil { | |
| t.Fatalf("tenant credentials secret missing: %v", err) | |
| } | |
| if len(cred.Data["admin_password"]) == 0 && cred.StringData["admin_password"] == "" { | |
| t.Fatal("tenant credentials secret has no admin_password") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/ensure/vm_test.go` around lines 104 - 110, The credential
assertion in the test incorrectly reads only cred.StringData, which is
write-only with a real API server. Update the assertion after the credentials
secret lookup to validate the admin_password value from both cred.Data and
cred.StringData, matching the behavior of credentials.get and preserving failure
when neither contains a value.
Comment on lines
+363
to
376
| // ResolveVMImage resolves a name or display name and verifies that the image is | ||
| // imported and has the storage class required to clone it. | ||
| func (c *TypedClient) ResolveVMImage(ctx context.Context, ref string) (ResolvedVMImage, error) { | ||
| if ref == "" { | ||
| return ns, name, sc, fmt.Errorf("empty image reference") | ||
| return ResolvedVMImage{}, fmt.Errorf("%w: reference is empty", ErrVMImageReferenceInvalid) | ||
| } | ||
|
|
||
| ns, spec := "default", ref | ||
| if i := strings.Index(ref, "/"); i > 0 { | ||
| ns, spec = ref[:i], ref[i+1:] | ||
| } | ||
| if spec == "" { | ||
| return ns, name, sc, fmt.Errorf("empty image name in reference %q", ref) | ||
| return ResolvedVMImage{}, fmt.Errorf("%w: empty image name in reference %q", ErrVMImageReferenceInvalid, ref) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Leading-slash refs escape the semantic ErrVMImageReferenceInvalid classification. strings.Index(ref, "/") > 0 skips the split for "/ubuntu", so spec keeps the slash and the API Get fails with a generic name-validation error. Preflight then treats it as transient and requeues forever instead of going terminal.
🛠️ Proposed fix
- ns, spec := "default", ref
- if i := strings.Index(ref, "/"); i > 0 {
- ns, spec = ref[:i], ref[i+1:]
- }
- if spec == "" {
+ ns, spec := "default", ref
+ if i := strings.Index(ref, "/"); i >= 0 {
+ ns, spec = ref[:i], ref[i+1:]
+ }
+ if ns == "" || spec == "" || strings.Contains(spec, "/") {
return ResolvedVMImage{}, fmt.Errorf("%w: empty image name in reference %q", ErrVMImageReferenceInvalid, ref)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ResolveVMImage resolves a name or display name and verifies that the image is | |
| // imported and has the storage class required to clone it. | |
| func (c *TypedClient) ResolveVMImage(ctx context.Context, ref string) (ResolvedVMImage, error) { | |
| if ref == "" { | |
| return ns, name, sc, fmt.Errorf("empty image reference") | |
| return ResolvedVMImage{}, fmt.Errorf("%w: reference is empty", ErrVMImageReferenceInvalid) | |
| } | |
| ns, spec := "default", ref | |
| if i := strings.Index(ref, "/"); i > 0 { | |
| ns, spec = ref[:i], ref[i+1:] | |
| } | |
| if spec == "" { | |
| return ns, name, sc, fmt.Errorf("empty image name in reference %q", ref) | |
| return ResolvedVMImage{}, fmt.Errorf("%w: empty image name in reference %q", ErrVMImageReferenceInvalid, ref) | |
| } | |
| // ResolveVMImage resolves a name or display name and verifies that the image is | |
| // imported and has the storage class required to clone it. | |
| func (c *TypedClient) ResolveVMImage(ctx context.Context, ref string) (ResolvedVMImage, error) { | |
| if ref == "" { | |
| return ResolvedVMImage{}, fmt.Errorf("%w: reference is empty", ErrVMImageReferenceInvalid) | |
| } | |
| ns, spec := "default", ref | |
| if i := strings.Index(ref, "/"); i >= 0 { | |
| ns, spec = ref[:i], ref[i+1:] | |
| } | |
| if ns == "" || spec == "" || strings.Contains(spec, "/") { | |
| return ResolvedVMImage{}, fmt.Errorf("%w: empty image name in reference %q", ErrVMImageReferenceInvalid, ref) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/internal/harvester/typed_client.go` around lines 363 - 376, Update
ResolveVMImage to reject references beginning with "/" as
ErrVMImageReferenceInvalid before namespace/spec resolution. Ensure
leading-slash inputs return the semantic invalid-reference error rather than
reaching the API lookup, while preserving existing handling for valid
namespace/name and empty-spec references.
Comment on lines
25
to
32
| ```sh | ||
| # From inside the dbaas/ directory, with kubectl + docker buildx available: | ||
| # From this directory, with kubectl + docker buildx pointed at your Harvester kubeconfig: | ||
| make docker-buildx IMG=<registry>/<name>:<tag> | ||
| KUBECONFIG=<your-harvester-kubeconfig> make install | ||
| KUBECONFIG=<your-harvester-kubeconfig> make deploy IMG=<registry>/<name>:<tag> | ||
| KUBECONFIG=<harvester-kubeconfig> make install | ||
| KUBECONFIG=<harvester-kubeconfig> make deploy IMG=<registry>/<name>:<tag> | ||
|
|
||
| # Then apply a DBInstance — full YAML and walkthrough in USAGE.md | ||
| kubectl apply -f config/samples/dbaas_v1alpha1_dbinstance.yaml | ||
| kubectl get dbi -A -w |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Export the kubeconfig for the entire quickstart.
KUBECONFIG=<harvester-kubeconfig> applies only to the individual make processes; the later kubectl apply and kubectl get commands may use a different default context. Use export KUBECONFIG=... or prefix those commands as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/README.md` around lines 25 - 32, Update the quickstart command block
in the database README to export KUBECONFIG before the build, install, deploy,
and kubectl commands, ensuring every command uses the Harvester kubeconfig
consistently.
HiranAdikari
approved these changes
Jul 27, 2026
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merges Yohan's db-controller redesign (PR #230) with latest operators. Content conflicts resolved in favor of the redesign; obsolete harvester client + tests removed as per the new design. Original PR: #230
Summary by CodeRabbit
New Features
Validation
Documentation