feat(dis-psql-operator): add subnet management #2628
Conversation
📝 WalkthroughWalkthroughAdds a Database CRD and controller that allocates VNet subnets, Azure subnet-fetch utilities and test fakes, Kind-based e2e workflow and image tooling, updated build/Dockerfile/Makefile for podman/Kind, RBAC and kustomize CRD manifests, and Go module dependency updates for Azure SDK and logging. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client/API
participant K8s as Kubernetes API
participant Reconciler as DatabaseReconciler
participant Catalog as SubnetCatalog
User->>K8s: Create Database CR
K8s->>Reconciler: Deliver Reconcile(Request)
Reconciler->>K8s: Get Database
alt SubnetCIDR present
Reconciler-->>K8s: Done
else
Reconciler->>K8s: List Databases
K8s-->>Reconciler: Database list
Reconciler->>Catalog: FirstFreeSubnet(usedCIDRs)
alt Subnet available
Catalog-->>Reconciler: SubnetInfo(CIDR)
Reconciler->>K8s: Update Database.Status.SubnetCIDR
K8s-->>Reconciler: Status updated
else
Catalog-->>Reconciler: Error (none free)
Reconciler-->>K8s: Requeue / Log error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Tried as much as I could to make a simple PR, but there's still a lot of code, so please take a look, must be some mistakes here and there @tjololo. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
services/dis-pgsql-operator/internal/network/subnets_test.go (2)
5-31: NewSubnetCatalog tests validate ordering and basic input well; consider asserting copy semantics.The test nicely checks ordering and validation behavior. Since
All()is intended to expose catalog contents, it may be worth adding a small test that mutates the returned slice and asserts the internal catalog isn’t affected (i.e.,All()returns a defensive copy), to lock in that contract.
54-117: FirstFreeSubnet tests cover main paths; optional extra edge cases could harden behavior.The three tests (no used, some used, all used) give good confidence in the selection logic and catalog ordering. If you want to further harden it, consider adding coverage for:
- An empty catalog (
NewSubnetCatalog(nil)/[]SubnetInfo{}) passed toFirstFreeSubnet.usedcontaining CIDRs not present in the catalog (should effectively be ignored).
These are minor, optional additions.services/dis-pgsql-operator/config/rbac/database_admin_role.yaml (1)
1-27: Wildcard verbs are acceptable for an admin helper role but may be worth narrowing for clarity/compliance.Given this is an admin-style helper role,
verbs: ['*']ondatabasesis defensible, but it does trigger scanners and hides the exact operations needed. If you want to align with tools like Checkov and make intent explicit, you could replace'*'with the standard CRUD/watch verbs used by the manager role (create, get, list, watch, update, patch, delete), keepingdatabases/statustogetonly.- resources: - - databases - verbs: - - '*' + resources: + - databases + verbs: + - create + - delete + - get + - list + - patch + - update + - watchservices/dis-pgsql-operator/config/manager/kustomization.yaml (1)
1-8: Kustomization wiring is correct; verify that the localhoste2eimage is overridden outside Kind.Defining this as a proper Kustomization with an
imagesblock is good. Since it defaults thecontrollerimage tolocalhost/dis-pgsql-operator:e2e, please confirm that your higher‑level environment overlays (Flux, prod kustomize, etc.) override this image so non‑Kind deployments don’t accidentally reference a localhost/e2e image. Based on learnings, this should normally be handled via environment‑specific kustomize overlays.services/dis-pgsql-operator/config/rbac/kustomization.yaml (1)
21-27: Helper Database Admin/Editor/Viewer roles are wired correctly; consider separating them if you want a lean default.The added roles and comments look good and will be available for cluster admins without affecting the operator directly. If you prefer to avoid installing helper ClusterRoles by default, you could move these into a separate kustomize overlay that operators can opt into.
services/dis-pgsql-operator/.dockerignore (1)
2-3: Consider ignoring more non-build directories to keep Docker context smallOnly
bin/is ignored now, so.git, test artifacts, and other large directories (if present) will be sent in the build context. That’s harmless but can slow builds and cache invalidation. Consider optionally ignoring additional non-runtime paths (e.g. VCS metadata, local build/test output) if they’re not needed by the Docker build.services/dis-pgsql-operator/Dockerfile (1)
14-18: Explicit COPY set looks reasonable; consider whether azfakes should ship in prod imageCopying only
cmd/main.go,api/,internal/, andtest/azfakes/is a nice tightening of the build context. If theazfakespackage is only ever used when running with--use-az-fakesin Kind/e2e and never in production, consider moving it behind a build tag or a separate test-only binary to keep the production image smaller. If you intentionally share the same image for both prod and Kind, current approach is fine.services/dis-pgsql-operator/config/kind/manager_kind_patch.yaml (1)
1-20: Patch is suitable for Kind; consider adding basic securityContextUsing
imagePullPolicy: Never,--use-az-fakes, and fake Azure env values is exactly what you want for a Kind/e2e overlay and clearly avoids real cloud calls.Given the static-analysis hints about privilege escalation/root, you might optionally add a minimal
securityContextto this container here (if not already set in the base Deployment), e.g.:securityContext: allowPrivilegeEscalation: false runAsNonRoot: trueto keep linters and cluster policies happy, even in test environments.
services/dis-pgsql-operator/internal/controller/database_controller_test.go (1)
16-125: Well-structured controller tests; consider minor resilience tweaksThe tests exercise the reconciler end-to-end and use
Eventuallywith explicit timeouts/polling, which is solid. Two small, optional improvements:
- To reduce duplication and make future refactors easier, you could derive
types.NamespacedNamefrom theDatabaseobjects (db.Name,db.Namespace) instead of hardcoding"db1"/"db2"in theGetcalls.- If you ever enable Ginkgo parallelism for this suite, consider either making resource names unique per spec or adding explicit cleanup to avoid cross-spec interference.
services/dis-pgsql-operator/test/utils/utils.go (1)
136-162: Archive-based kind image loading is sound; minor doc nitThe
KIND_IMAGE_ARCHIVEbranch is a clean extension and keeps the old docker-image path intact. One small, optional tweak:
- The function name/comment still suggest it always uses the image “name”, but in archive mode the
nameparameter is ignored. You might clarify the comment to mention that behavior explicitly to avoid confusion for future callers.services/dis-pgsql-operator/internal/controller/suite_test.go (1)
38-92: Envtest suite and subnet catalog wiring look good; avoid double directory scanThe suite wiring (scheme registration, CRD loading, fixed
SubnetCatalog, and manager startup) looks solid and should make the controller tests reliable.A tiny, optional improvement:
getFirstFoundEnvTestBinaryDir()is called twice in theifat Lines 55–58, which re-reads the directory and can log the same error twice. Caching the result in a local variable avoids the duplicate work/logging:- // Retrieve the first found binary directory to allow running tests from IDEs - if getFirstFoundEnvTestBinaryDir() != "" { - testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir() - } + // Retrieve the first found binary directory to allow running tests from IDEs + if dir := getFirstFoundEnvTestBinaryDir(); dir != "" { + testEnv.BinaryAssetsDirectory = dir + }Also applies to: 101-115
services/dis-pgsql-operator/internal/network/subnets.go (2)
70-76: Consider adding a clarifying comment.The logic to skip empty strings in the used set is correct, but a brief comment explaining why (e.g., "empty strings represent databases without allocated subnets") would improve readability.
120-128: Consider logging skipped subnets.Subnets with nil properties or AddressPrefix are silently skipped. While this is defensive, it could hide Azure configuration issues. Consider logging a warning when skipping subnets.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/dis-pgsql-operator/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
services/dis-pgsql-operator/.dockerignore(1 hunks)services/dis-pgsql-operator/Dockerfile(1 hunks)services/dis-pgsql-operator/Makefile(5 hunks)services/dis-pgsql-operator/PROJECT(1 hunks)services/dis-pgsql-operator/api/v1alpha1/database_types.go(1 hunks)services/dis-pgsql-operator/api/v1alpha1/groupversion_info.go(1 hunks)services/dis-pgsql-operator/api/v1alpha1/zz_generated.deepcopy.go(1 hunks)services/dis-pgsql-operator/cmd/main.go(6 hunks)services/dis-pgsql-operator/config/crd/bases/storage.dis.altinn.cloud_databases.yaml(1 hunks)services/dis-pgsql-operator/config/crd/kustomization.yaml(1 hunks)services/dis-pgsql-operator/config/crd/kustomizeconfig.yaml(1 hunks)services/dis-pgsql-operator/config/default/kustomization.yaml(1 hunks)services/dis-pgsql-operator/config/kind/kustomization.yaml(1 hunks)services/dis-pgsql-operator/config/kind/manager_kind_patch.yaml(1 hunks)services/dis-pgsql-operator/config/manager/kustomization.yaml(1 hunks)services/dis-pgsql-operator/config/rbac/database_admin_role.yaml(1 hunks)services/dis-pgsql-operator/config/rbac/database_editor_role.yaml(1 hunks)services/dis-pgsql-operator/config/rbac/database_viewer_role.yaml(1 hunks)services/dis-pgsql-operator/config/rbac/kustomization.yaml(1 hunks)services/dis-pgsql-operator/config/rbac/role.yaml(1 hunks)services/dis-pgsql-operator/config/samples/kustomization.yaml(1 hunks)services/dis-pgsql-operator/config/samples/storage_v1alpha1_database.yaml(1 hunks)services/dis-pgsql-operator/go.mod(5 hunks)services/dis-pgsql-operator/internal/controller/database_controller.go(1 hunks)services/dis-pgsql-operator/internal/controller/database_controller_test.go(1 hunks)services/dis-pgsql-operator/internal/controller/suite_test.go(1 hunks)services/dis-pgsql-operator/internal/network/subnets.go(1 hunks)services/dis-pgsql-operator/internal/network/subnets_test.go(1 hunks)services/dis-pgsql-operator/test/azfakes/env.go(1 hunks)services/dis-pgsql-operator/test/azfakes/network_fakes.go(1 hunks)services/dis-pgsql-operator/test/e2e/e2e_suite_test.go(1 hunks)services/dis-pgsql-operator/test/e2e/e2e_test.go(1 hunks)services/dis-pgsql-operator/test/utils/utils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-08T06:08:49.970Z
Learnt from: tjololo
Repo: Altinn/altinn-platform PR: 1606
File: services/dis-identity-operator/config/manager/manager.yaml:66-66
Timestamp: 2025-05-08T06:08:49.970Z
Learning: In Altinn Kubernetes operators, image references in deployment manifests are handled through Kustomize customization rather than being hardcoded in the base YAML templates, which keeps the base manifests generic and enables environment-specific configuration.
Applied to files:
services/dis-pgsql-operator/config/kind/kustomization.yamlservices/dis-pgsql-operator/config/crd/kustomization.yamlservices/dis-pgsql-operator/config/manager/kustomization.yamlservices/dis-pgsql-operator/config/crd/kustomizeconfig.yamlservices/dis-pgsql-operator/config/default/kustomization.yamlservices/dis-pgsql-operator/Makefile
📚 Learning: 2025-10-27T14:29:47.584Z
Learnt from: tjololo
Repo: Altinn/altinn-platform PR: 2405
File: flux/blackbox-exporter/apps/helmrelease-patch.yaml:1-18
Timestamp: 2025-10-27T14:29:47.584Z
Learning: In the Altinn platform repository, FluxCD Kustomization resources (kind: Kustomization, apiVersion: kustomize.toolkit.fluxcd.io/v1) use postBuild.substitute for variable replacement, with variables referenced using ${VAR_NAME} syntax in patch files and manifests. The Flux Kustomization resources are typically defined in flux-kustomize.yaml files separate from the kustomize kustomization.yaml files.
Applied to files:
services/dis-pgsql-operator/config/crd/kustomization.yamlservices/dis-pgsql-operator/config/crd/kustomizeconfig.yaml
📚 Learning: 2025-08-12T11:30:54.035Z
Learnt from: tjololo
Repo: Altinn/altinn-platform PR: 2041
File: flux/dis-apim/flux-kustomize.yaml:21-21
Timestamp: 2025-08-12T11:30:54.035Z
Learning: In the Altinn dis-apim-operator, the image tag `latest` is used temporarily in flux/dis-apim/flux-kustomize.yaml until proper versioned releases of the controller are available, at which point they plan to implement version pinning.
Applied to files:
services/dis-pgsql-operator/config/manager/kustomization.yamlservices/dis-pgsql-operator/Makefile
📚 Learning: 2025-09-05T07:37:27.185Z
Learnt from: tjololo
Repo: Altinn/altinn-platform PR: 2169
File: flux/linkerd/helmrelease.yaml:102-102
Timestamp: 2025-09-05T07:37:27.185Z
Learning: In Altinn's Kubernetes setup, environment variable substitution in YAML manifests (like ${DEFAULT_INBOUND_POLICY:=all-unauthenticated}) is handled by Kustomize, not by shell expansion or Flux directly.
Applied to files:
services/dis-pgsql-operator/config/crd/kustomizeconfig.yaml
📚 Learning: 2025-08-15T08:58:01.562Z
Learnt from: tjololo
Repo: Altinn/altinn-platform PR: 2072
File: services/dis-apim-operator/config/default/kustomization.yaml:127-163
Timestamp: 2025-08-15T08:58:01.562Z
Learning: Kubebuilder 4.7.1 scaffolds kustomize fieldPath entries with leading dots (e.g., `.metadata.name`, `.spec.dnsNames.0`). Both formats with and without leading dots are valid in kustomize, but the scaffold-generated format should be trusted as it's verified upstream by kubebuilder maintainers.
Applied to files:
services/dis-pgsql-operator/config/crd/kustomizeconfig.yaml
📚 Learning: 2025-08-28T13:59:14.257Z
Learnt from: sduranc
Repo: Altinn/altinn-platform PR: 2150
File: services/lakmus/cmd/main.go:99-99
Timestamp: 2025-08-28T13:59:14.257Z
Learning: The scanAndUpdateSecretGauges function in services/lakmus/cmd/main.go takes exactly 3 parameters: ctx, subscriptionID, and cred. The armOpts and secOpts are handled as global variables within the function implementation, not passed as parameters.
Applied to files:
services/dis-pgsql-operator/cmd/main.go
📚 Learning: 2025-08-15T08:44:09.677Z
Learnt from: tjololo
Repo: Altinn/altinn-platform PR: 2072
File: services/dis-apim-operator/.devcontainer/devcontainer.json:3-3
Timestamp: 2025-08-15T08:44:09.677Z
Learning: In the Altinn dis-apim-operator project, the team prefers using unpinned/latest Go image tags (e.g., "golang:1.24") in devcontainer configurations over pinned versions, prioritizing convenience and automatic updates over strict reproducibility.
Applied to files:
services/dis-pgsql-operator/Makefile
🧬 Code graph analysis (6)
services/dis-pgsql-operator/internal/controller/database_controller_test.go (1)
services/dis-pgsql-operator/api/v1alpha1/database_types.go (3)
Database(58-72)DatabaseSpec(19-30)DatabaseAuth(8-16)
services/dis-pgsql-operator/internal/network/subnets_test.go (1)
services/dis-pgsql-operator/internal/network/subnets.go (2)
SubnetInfo(14-17)NewSubnetCatalog(31-56)
services/dis-pgsql-operator/internal/controller/suite_test.go (2)
services/dis-pgsql-operator/api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)services/dis-pgsql-operator/internal/network/subnets.go (3)
NewSubnetCatalog(31-56)SubnetInfo(14-17)SubnetCatalog(24-26)
services/dis-pgsql-operator/api/v1alpha1/database_types.go (2)
services/lakmus/manifests/imports/k8s.ts (2)
ObjectMeta(11399-11519)ListMeta(11711-11740)services/dis-pgsql-operator/api/v1alpha1/groupversion_info.go (1)
SchemeBuilder(32-32)
services/dis-pgsql-operator/internal/controller/database_controller.go (2)
services/dis-pgsql-operator/internal/network/subnets.go (1)
SubnetCatalog(24-26)services/dis-pgsql-operator/api/v1alpha1/database_types.go (2)
Database(58-72)DatabaseList(77-81)
services/dis-pgsql-operator/api/v1alpha1/zz_generated.deepcopy.go (1)
services/dis-pgsql-operator/api/v1alpha1/database_types.go (5)
Database(58-72)DatabaseAuth(8-16)DatabaseList(77-81)DatabaseSpec(19-30)DatabaseStatus(33-52)
🪛 Checkov (3.2.334)
services/dis-pgsql-operator/config/rbac/database_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
services/dis-pgsql-operator/config/kind/manager_kind_patch.yaml
[medium] 1-20: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-20: Minimize the admission of root containers
(CKV_K8S_23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build, scan and release / Build and scan Docker image
- GitHub Check: Run tests on Ubuntu
- GitHub Check: Run linter on Ubuntu
- GitHub Check: Analyze (go)
🔇 Additional comments (35)
services/dis-pgsql-operator/go.mod (4)
12-14: Version alignment is correct — no compatibility issues found.The web search confirms that controller-runtime v0.22.x (including v0.22.4) targets k8s.io/* v0.34.x. The pinned versions in go.mod (k8s.io/apimachinery v0.34.1, k8s.io/client-go v0.34.1, and sigs.k8s.io/controller-runtime v0.22.4) follow the documented compatibility matrix and are properly aligned.
6-8: Azure SDK versions are mutually compatible—no action needed.Based on verification, the three Azure SDK modules specified (azcore v1.20.0, azidentity v1.13.1, and armnetwork/v7 v7.2.0) are compatible:
- All three versions are present in the
go.sumfile with successful resolution, confirminggo mod tidyexecuted without conflicts.- azidentity v1.13.1 is built against azcore v1.x and is compatible with v1.20.0 (both maintain v1.x semver stability).
- armnetwork v7.2.0 and azidentity v1.13.1 are from the same Azure SDK release cycle and are designed to work together.
- Transitive dependencies (azidentity/cache v0.3.2, internal v1.11.2, resourcemanager/internal v3.1.0) are consistent and properly resolved.
No version conflicts, incompatibilities, or alignment issues exist.
6-9: No known unpatched security vulnerabilities detected in the specified dependency versions.The web search confirms:
golang.org/x/crypto v0.42.0 — Not affected by tracked CVEs. It contains the patch for CVE-2023-48795 (fixed in v0.17.0) and later vulnerability fixes.
golang.org/x/net v0.44.0 — Not affected by the listed advisories (CVE-2025-22872, CVE-2025-22870, CVE-2024-45338). All patched versions are earlier than v0.44.0.
Azure SDK versions (azcore v1.20.0, azidentity v1.13.1, armnetwork/v7 v7.2.0) — No specific vulnerabilities identified.
While newer patch releases may be available (e.g., golang.org/x/crypto v0.52+), the current versions contain known security patches and are not known to have unpatched CVEs. The versions are mature and stable, though they lack the latest enhancements and bug fixes available in more recent releases. Updating to the latest versions is a maintenance best practice for staying within the active support window, but not an urgent security requirement based on current vulnerability data.
3-3: Consider upgrading to Go 1.25.4, the current stable version.Go 1.24.6 is a valid release (released Aug 6, 2025), but Go 1.25.4 is the current stable version as of October 31, 2025. While 1.24.6 remains supported, upgrading aligns with keeping dependencies current. Given the project's preference for latest versions (as seen in other operators), consider updating the go directive from 1.24.6 to 1.25.4.
services/dis-pgsql-operator/config/rbac/role.yaml (1)
7-26: Manager RBAC for Database CRD looks appropriately scoped.The rule set (CRUD+watch on
databasesand get/patch/update ondatabases/status) is consistent with typical controller needs and avoids unnecessary wildcards. No issues from my side.services/dis-pgsql-operator/test/e2e/e2e_test.go (1)
71-75: Switch todeploy-kindfits the Kind e2e flow; just confirm cleanup symmetry withundeploy.Using
make deploy-kind IMG=...here makes sense for Kind-based e2e. Please double‑check thatdeploy-kinduses the same kustomization/overlays asundeploy/uninstall, so the AfterAll cleanup fully removes what was deployed by this test suite.services/dis-pgsql-operator/config/default/kustomization.yaml (1)
17-19: Including../crdin default kustomization makes the deployment self‑contained.Adding the CRD resource here ensures a
config/defaultapply includes the Database CRD without relying on a separate step. The extra apply alongsidemake installis idempotent, so this looks good.services/dis-pgsql-operator/config/samples/kustomization.yaml (1)
1-4: Sample kustomization for Database CRD looks standard.Referencing
storage_v1alpha1_database.yamlwith the scaffold marker matches Kubebuilder patterns; no changes needed.services/dis-pgsql-operator/test/e2e/e2e_suite_test.go (1)
43-46: Verify image tag consistency between build, kind load, and deploy-kind
projectImageis used both formake docker-build IMG=…andLoadImageToKindClusterWithName. Please double-check that anymake deploy-kindor kustomize image overrides use the same tag (localhost/dis-pgsql-operator:v0.0.1) so the Deployment in the Kind cluster actually runs the image built and loaded here.services/dis-pgsql-operator/config/samples/storage_v1alpha1_database.yaml (1)
1-23: Sample CRs look good; just confirm they match CRD validationThe two
Databasesamples are well-formed and useful for quick testing. Please just confirm that:
spec.serverType: devis one of the allowed values in the CRD schema, andspec.version: 17matches the accepted type/format (e.g. integer vs string)so that
kubectl applyon these examples won’t fail validation.services/dis-pgsql-operator/Dockerfile (2)
20-26: Build flags look good; verify TARGETOS/TARGETARCH wiring in your build pipeline
go build -trimpath -ldflags="-s -w" -o manager cmd/main.gois fine and matches common patterns for minimal binaries. With:RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ go build ...please ensure:
TARGETOS/TARGETARCHare correctly set by your Docker/Buildx invocation when doing multi-arch builds, and- for local single-arch
docker build, the resulting GOOS/GOARCH combination still matches the platform you intend to run in Kind/cluster.This is mostly a wiring/CI concern, but worth confirming so e2e images match cluster platform expectations.
1-3: Thegolang:1.24base image exists and is properly aligned with your Go toolchain.The Docker Hub golang image includes the
golang:1.24tag. Yourgo.moddeclaresgo 1.24.6, which is compatible with the genericgolang:1.24tag in the Dockerfile. No version misalignment or build break risk has been identified.services/dis-pgsql-operator/config/kind/kustomization.yaml (1)
1-12: Kind overlay wiring looks correctReferencing
../defaultand applyingmanager_kind_patch.yamltodis-pgsql-operator-controller-managerin thedis-pgsql-operator-systemnamespace matches the established pattern of using Kustomize overlays for environment-specific tweaks instead of hardcoding in base manifests. Based on learnings, this aligns well with how other Altinn operators are configured.services/dis-pgsql-operator/config/crd/kustomization.yaml (1)
1-16: CRD kustomization matches kubebuilder conventionsThis CRD kustomization referencing
bases/storage.dis.altinn.cloud_databases.yamland leaving webhook/config sections scaffolded but commented is in line with kubebuilder defaults and the pattern of invoking it fromconfig/default. Looks good as a base for the Database CRD.services/dis-pgsql-operator/test/azfakes/network_fakes.go (1)
1-43: Code implementation is correct and follows Azure SDK v7 fake patternsVerification confirms
SubnetsServerOneVNetcorrectly implements the fake pattern for the Azure Go SDK v7. TheNewListPagercallback signature matches the expectedSubnetsClient.NewListPagerinterface, andazfake.PagerResponder[armnetwork.SubnetsClientListResponse]is the standard generic type fromazcore/fakefor building paginated fake responses. The single-page response viaresp.AddPage(http.StatusOK, page, nil)follows the documented Azure SDK fake pager pattern.The implementation is production-ready for test scenarios.
services/dis-pgsql-operator/config/crd/kustomizeconfig.yaml (1)
1-19: Kustomize CRD substitutions look correctThe name/namespace and annotation var-reference wiring for the CRD conversion webhook follows the expected kustomize patterns and should work fine with kubebuilder-scaffolded CRDs.
services/dis-pgsql-operator/config/rbac/database_viewer_role.yaml (1)
1-29: Viewer ClusterRole is appropriately scoped read-only accessThe
database-viewer-rolerules (get/list/watch ondatabases, get ondatabases/status) provide the expected read-only surface without mutation verbs, which is appropriate for monitoring/visibility use-cases.services/dis-pgsql-operator/config/rbac/database_editor_role.yaml (1)
1-33: Editor ClusterRole matches expected CRUD responsibilitiesThe
database-editor-rolegrants the usual create/read/update/delete verbs ondatabaseswhile limitingdatabases/statustoget, which is a sensible balance between management capabilities and protection of controller-owned status.services/dis-pgsql-operator/cmd/main.go (1)
59-101: Let me verify the Azure SDK documentation for nil options handling:
github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork v7 NewSubnetsClient documentationservices/dis-pgsql-operator/test/azfakes/env.go (1)
10-38: Code patterns verified against Azure SDK documentation—implementation is correct.The review's verification requests have been confirmed:
armnetwork.NewSubnetsClientacceptsnil *arm.ClientOptions✓
Microsoft samples show calling the constructor withnilfor options.Custom
TransportviaClientOptionsis the intended pattern ✓
The code correctly embedsazcore.ClientOptionsinarm.ClientOptionsand sets theTransportfield, matching Azure SDK guidance.Non-nil
*arm.ClientOptionsin both branches is appropriate ✓
While the SDK acceptsnil, returning a constructed (possibly empty)ClientOptionsin both branches is a valid, defensive approach. The code ensures callers do not need nil checks.The implementation is sound and the approval stands.
services/dis-pgsql-operator/PROJECT (1)
11-20: LGTM!The Database CRD resource configuration follows standard Kubebuilder scaffolding conventions.
services/dis-pgsql-operator/api/v1alpha1/groupversion_info.go (1)
27-36: LGTM!The API group version registration follows Kubebuilder conventions correctly.
services/dis-pgsql-operator/config/crd/bases/storage.dis.altinn.cloud_databases.yaml (1)
1-159: LGTM!The CRD definition is well-structured with appropriate validation rules and follows Kubernetes API conventions.
services/dis-pgsql-operator/internal/controller/database_controller.go (3)
34-66: LGTM - Good concurrency control.The reconcile logic is correct. The single-threaded reconciler (Line 109) prevents concurrent subnet allocations, and standard Kubernetes optimistic concurrency handles any race conditions during status updates.
68-102: LGTM!The subnet allocation logic correctly lists all Databases, collects used subnets, and allocates the first available subnet. Error handling is appropriate.
104-112: LGTM - Appropriate concurrency design.The single-threaded reconciler is a reasonable design choice for subnet allocation, prioritizing correctness over scalability. For database provisioning (an infrequent operation), this trade-off is appropriate.
services/dis-pgsql-operator/Makefile (2)
2-3: LGTM!The image tag and container tool changes align with Kind-based testing infrastructure and team preferences for using podman in development environments.
Also applies to: 18-18
92-95: LGTM!The new Kind-based testing targets provide a complete workflow for building, loading, and testing the operator in an isolated environment.
Also applies to: 159-163, 184-187
services/dis-pgsql-operator/internal/network/subnets.go (3)
31-56: LGTM!The subnet catalog constructor includes appropriate validation for empty CIDRs and duplicates, and correctly preserves input order.
88-92: LGTM!Defensive copying prevents external modifications to the internal catalog state.
7-9: Azure SDK versions are current and secure—no vulnerabilities detected.The declared versions in
go.mod(azcore v1.20.0, armnetwork/v7 v7.2.0) are the latest stable releases. Azcore v1.20.0 was released in November 2025 with no known CVEs. Armnetwork/v7 v7.2.0 is the current stable version with no security advisories. The also-declared azidentity v1.13.1 is well past the CVE-2024-35255 fix threshold (v1.6.0+). All dependencies are secure.services/dis-pgsql-operator/api/v1alpha1/database_types.go (3)
8-30: LGTM!The DatabaseAuth and DatabaseSpec types have appropriate validation markers and follow Kubernetes CRD best practices.
33-52: LGTM!The DatabaseStatus structure correctly uses optional fields and standard metav1.Condition for status reporting.
58-85: LGTM!The Database and DatabaseList types follow standard Kubernetes CRD patterns with proper kubebuilder markers for root objects and status subresources.
services/dis-pgsql-operator/api/v1alpha1/zz_generated.deepcopy.go (1)
1-138: LGTM!Auto-generated deepcopy methods follow standard Kubernetes code generation patterns and correctly handle all fields including slices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/dis-pgsql-operator/cmd/main.go (2)
251-258: LGTM! Controller initialization is correct.The DatabaseReconciler is properly initialized with the fetched subnet catalog. The static catalog approach (loaded once at startup) is intentional per the controller design.
Consider adding a brief comment before Line 245 explaining the design decision:
// Fetch the subnet catalog once at startup. The catalog is static for the lifetime // of the operator to avoid repeated Azure API calls during reconciliation. subnetCatalog, err := network.FetchSubnetCatalog(ctx, subscriptionID, resourceGroup, vnetName, cred, armOpts)
63-63: Consider extracting Azure setup logic into a helper function.The main() function continues to grow in complexity with this PR. While the existing
gocyclosuppression indicates this is a known issue, extracting the Azure authentication and subnet catalog setup (lines 216-249) into a helper function would improve readability and testability.Example refactor:
// setupAzureSubnetCatalog initializes Azure credentials and fetches the subnet catalog. func setupAzureSubnetCatalog( useFakes bool, subscriptionID, resourceGroup, vnetName string, ) (*network.SubnetCatalog, error) { var cred azcore.TokenCredential var armOpts *arm.ClientOptions var err error if useFakes { srv := azfakes.SubnetsServerOneVNet([]string{ "10.100.1.0/28", "10.100.1.16/28", "10.100.1.32/28", }) env := azfakes.NewNetworkEnv(srv) cred = env.Cred armOpts = env.ARM setupLog.Info("running with Azure SDK fakes for armnetwork") } else { cred, err = azidentity.NewDefaultAzureCredential(nil) if err != nil { return nil, fmt.Errorf("azure credential error: %w", err) } armOpts = nil } if subscriptionID == "" || resourceGroup == "" || vnetName == "" { return nil, fmt.Errorf("subscription-id, resource-group and vnet-name are required") } ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() return network.FetchSubnetCatalog(ctx, subscriptionID, resourceGroup, vnetName, cred, armOpts) }Then in main():
subnetCatalog, err := setupAzureSubnetCatalog(useFakes, subscriptionID, resourceGroup, vnetName) if err != nil { setupLog.Error(err, "failed to setup Azure subnet catalog") os.Exit(1) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
services/dis-pgsql-operator/cmd/main.go(6 hunks)services/dis-pgsql-operator/internal/controller/database_controller.go(1 hunks)services/dis-pgsql-operator/internal/network/subnets_test.go(1 hunks)services/dis-pgsql-operator/test/azfakes/network_fakes.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/dis-pgsql-operator/internal/network/subnets_test.go
- services/dis-pgsql-operator/internal/controller/database_controller.go
- services/dis-pgsql-operator/test/azfakes/network_fakes.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T13:59:14.257Z
Learnt from: sduranc
Repo: Altinn/altinn-platform PR: 2150
File: services/lakmus/cmd/main.go:99-99
Timestamp: 2025-08-28T13:59:14.257Z
Learning: The scanAndUpdateSecretGauges function in services/lakmus/cmd/main.go takes exactly 3 parameters: ctx, subscriptionID, and cred. The armOpts and secOpts are handled as global variables within the function implementation, not passed as parameters.
Applied to files:
services/dis-pgsql-operator/cmd/main.go
🧬 Code graph analysis (1)
services/dis-pgsql-operator/cmd/main.go (5)
services/dis-pgsql-operator/api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)services/dis-pgsql-operator/test/azfakes/network_fakes.go (1)
SubnetsServerOneVNet(15-46)services/dis-pgsql-operator/test/azfakes/env.go (1)
NewNetworkEnv(19-38)services/dis-pgsql-operator/internal/network/subnets.go (2)
FetchSubnetCatalog(96-138)SubnetCatalog(24-26)services/dis-pgsql-operator/internal/controller/database_controller.go (1)
DatabaseReconciler(21-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, scan and release / Build and scan Docker image
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
services/dis-pgsql-operator/cmd/main.go (5)
19-49: LGTM! Clean import additions.The new imports are well-organized and all appear necessary for the Azure integration and subnet catalog functionality.
59-59: LGTM! Correct schema registration.Registering the v1alpha1 storage API types follows the standard kubebuilder pattern.
96-116: LGTM! Good flag design with environment variable fallbacks.The Azure configuration flags follow Go best practices by allowing environment variables as defaults with flag overrides. The
use-az-fakesflag is particularly useful for local development and testing.
216-234: LGTM! Clean credential handling for fake and production modes.The conditional Azure credential setup is well-designed:
- Fake mode provides a controlled test environment with predefined subnets
- Production mode uses
NewDefaultAzureCredential, which is the Azure SDK best practice- Error handling is appropriate
241-249: Verify the 30-second timeout is adequate for production.The subnet catalog fetch logic is correct with proper context handling and error management. However, the 30-second timeout may be insufficient if Azure API experiences throttling or network delays.
Consider monitoring startup times in production to confirm this timeout is appropriate. If timeouts occur, you may need to increase it or add retry logic.
| if subscriptionID == "" || resourceGroup == "" || vnetName == "" { | ||
| setupLog.Error(fmt.Errorf("missing config"), "subscription-id, resource-group and vnet-name are required") | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Improve error message specificity.
The validation correctly checks required Azure parameters, but the error message doesn't indicate which specific parameter(s) are missing, making troubleshooting harder.
Consider this improvement:
- if subscriptionID == "" || resourceGroup == "" || vnetName == "" {
- setupLog.Error(fmt.Errorf("missing config"), "subscription-id, resource-group and vnet-name are required")
+ var missing []string
+ if subscriptionID == "" {
+ missing = append(missing, "subscription-id")
+ }
+ if resourceGroup == "" {
+ missing = append(missing, "resource-group")
+ }
+ if vnetName == "" {
+ missing = append(missing, "vnet-name")
+ }
+ if len(missing) > 0 {
+ setupLog.Error(fmt.Errorf("missing required Azure configuration: %v", missing), "config validation failed")
os.Exit(1)
}📝 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.
| if subscriptionID == "" || resourceGroup == "" || vnetName == "" { | |
| setupLog.Error(fmt.Errorf("missing config"), "subscription-id, resource-group and vnet-name are required") | |
| os.Exit(1) | |
| } | |
| var missing []string | |
| if subscriptionID == "" { | |
| missing = append(missing, "subscription-id") | |
| } | |
| if resourceGroup == "" { | |
| missing = append(missing, "resource-group") | |
| } | |
| if vnetName == "" { | |
| missing = append(missing, "vnet-name") | |
| } | |
| if len(missing) > 0 { | |
| setupLog.Error(fmt.Errorf("missing required Azure configuration: %v", missing), "config validation failed") | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
In services/dis-pgsql-operator/cmd/main.go around lines 236 to 239, the
validation error message is generic; change the logic to collect which of
subscriptionID, resourceGroup, and vnetName are empty, build a concise "missing
config" error that includes the names of the missing parameters (e.g., join them
into a comma-separated string), and pass that detailed message to setupLog.Error
(or create an error with fmt.Errorf including the missing list) before
os.Exit(1) so the log clearly identifies exactly which parameter(s) are missing.
There was a problem hiding this comment.
this suggestion sounds too verbose to me, I'll leave it up to you to decide @tjololo
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
1 participant
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.