feat(connectivity): add Connectivity module bound to the stack ledger - #492
feat(connectivity): add Connectivity module bound to the stack ledger#492Dav-14 wants to merge 17 commits into
Conversation
WalkthroughThis change adds the v1beta1 Connectivity resource, delegated reconciliation through Ledger v3, Gateway routing, capability detection, credential and monitoring handling, network policies, tests, and module documentation. ChangesConnectivity module
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectivityResource
participant ConnectivityReconciler
participant Ledger
participant DelegatedConnectivity
participant Gateway
ConnectivityResource->>ConnectivityReconciler: Provide module configuration
ConnectivityReconciler->>Ledger: Check readiness and credentials
Ledger-->>ConnectivityReconciler: Return v3 backend and credential references
ConnectivityReconciler->>DelegatedConnectivity: Apply delegated Connectivity resource
DelegatedConnectivity-->>ConnectivityReconciler: Return phase and readiness
ConnectivityReconciler->>Gateway: Apply secured API route
ConnectivityReconciler-->>ConnectivityResource: Set status conditions
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
🛑 Changes requested — automated reviewThe Connectivity module introduces several correctness and reliability gaps that must be addressed before merging. The most critical are: (1) the reconciler registers no watch on the ledger Credentials object it creates, so it can stall indefinitely in a pending state once Credentials are created but not yet ready; (2) when network policies are enabled, Connectivity pods dial the Ledger gRPC port (8888) but are not covered by any ingress rule in the Ledger v3 NetworkPolicy, causing silent connection drops at runtime; (3) deleting the Connectivity module leaves the privileged Credentials object and distributed secret alive because the object is owned by the Stack with no finalizer or owner reference on the Connectivity CR driving cleanup; (4) if the Credentials CRD or RBAC is absent, the reconciler hard-fails instead of reporting pending; and (5) for stacks using Findings outside the diff🟠 [major] Connectivity pods blocked by Ledger v3 NetworkPolicy when network policies are enabled — When Suggestion: Add an ingress rule to the Ledger v3 NetworkPolicy (or a dedicated NetworkPolicy) that allows pods matching the Connectivity workload pod selector to reach the Ledger gRPC port (8888). |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #492 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 2 stale NumaryBot review threads (1 fixed, 1 outdated).
Summary: #492 (comment)
f5bf061 to
1f6c39e
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #492 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #492 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).
Summary: #492 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #492 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #492 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #492 (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: 9
🧹 Nitpick comments (5)
internal/tests/ledger_v3_controller_test.go (1)
1084-1093: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegister an explicit cleanup for the re-created default configuration.
The
DeferCleanupat Line 946 closes over theconfigurationvariable, so reassigning it here is what keeps the second, cluster-scopedDefaultLedgerConfigurationNameobject from leaking. That's implicit and easy to break (e.g. by shadowing with:=), and a leaked defaultLedgerConfigurationwould affect other specs in thisSerialsuite.♻️ Proposed explicit cleanup
Expect(Create(configuration)).To(Succeed()) + DeferCleanup(func() { + Expect(client.IgnoreNotFound(Delete(configuration))).To(Succeed()) + })🤖 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 `@internal/tests/ledger_v3_controller_test.go` around lines 1084 - 1093, After reassigning the cluster-scoped configuration in this test, register an explicit DeferCleanup for that recreated DefaultLedgerConfigurationName object rather than relying on the earlier cleanup closure over configuration. Ensure the cleanup deletes this exact LedgerConfiguration and remains effective even if the variable is later shadowed.internal/tests/networkpolicy_controller_test.go (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the actual Raft/gRPC port values, not just the count.
allow-ledger-v2-from-v3below checks the concrete port (8080), but here onlyHaveLen(2)is checked, so a regression swapping 7777/8888 for other ports would pass.♻️ Proposed assertion
- g.Expect(np.Spec.Ingress[0].Ports).To(HaveLen(2)) + g.Expect(np.Spec.Ingress[0].Ports).To(HaveLen(2)) + g.Expect(np.Spec.Ingress[0].Ports[0].Port.IntValue()).To(Equal(7777)) + g.Expect(np.Spec.Ingress[0].Ports[1].Port.IntValue()).To(Equal(8888))🤖 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 `@internal/tests/networkpolicy_controller_test.go` around lines 93 - 98, Update the ingress assertions in the allow-ledger-v2-from-v3 test to verify the concrete Raft and gRPC port values 7777 and 8888, not only that np.Spec.Ingress[0].Ports has length two. Preserve the existing count assertion and assert both expected port entries using the same port-value assertion pattern as the nearby 8080 check.deployment/operator/helpers.go (2)
108-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStore registry credentials directly instead of re-extracting via an unchecked type assertion.
dc.RegistryAuth[0].(dockerbuild.RegistryArgs)panics ifRegistryAuthis ever empty or holds a different concrete type. SincenewDockerConfigalready has the username/password at construction time, storing them directly ondockerConfigavoids this indexing/type-assertion round-trip.♻️ Proposed refactor
type dockerConfig struct { Registry string PullRegistry string BuilderName string ImageTag string Platforms []string RegistryAuth dockerbuild.RegistryArray + Username pulumi.StringPtrInput + Password pulumi.StringPtrInput } ... + username := config.GetSecret(ctx, "registry-username") + password := config.GetSecret(ctx, "registry-password") return &dockerConfig{ ... + Username: username, + Password: password, RegistryAuth: dockerbuild.RegistryArray{ dockerbuild.RegistryArgs{ Address: pulumi.String(registry), - Username: config.GetSecret(ctx, "registry-username"), - Password: config.GetSecret(ctx, "registry-password"), + Username: username, + Password: password, }, }, } ... Registry: dockerbuild.RegistryArgs{ Address: pulumi.String(dc.Registry), - Username: dc.RegistryAuth[0].(dockerbuild.RegistryArgs).Username, - Password: dc.RegistryAuth[0].(dockerbuild.RegistryArgs).Password, + Username: dc.Username, + Password: dc.Password, },Also applies to: 185-197
🤖 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 `@deployment/operator/helpers.go` around lines 108 - 121, Update dockerConfig and newDockerConfig to store the registry username and password directly from config.GetSecret, rather than wrapping them only in RegistryAuth. Replace any dc.RegistryAuth[0].(dockerbuild.RegistryArgs) indexing and type assertions in the affected logic with the direct credential fields, preserving the existing credential values.
94-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueArch-to-platform mapping is fragile.
strings.HasSuffix(p, arch)againstallPlatformshas no case normalization and offers no explicit way to select multiple platforms (e.g., both amd64+arm64) other than relying on a coincidental shared suffix. Consider accepting a comma-separated list and matching exact platform names.🤖 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 `@deployment/operator/helpers.go` around lines 94 - 106, Update the platform-selection logic around cfg.Get("arch") to normalize architecture values and parse comma-separated entries, allowing multiple architectures such as amd64 and arm64. Match each requested architecture against explicit platform names in allPlatforms rather than relying on strings.HasSuffix, while preserving the linux-<arch> fallback when no valid platforms are selected.internal/resources/stacks/networkpolicies.go (1)
222-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
intstr.FromIntwithFromInt32.
intstr.FromIntis deprecated ink8s.io/apimachinery v0.34.2; this helper already converts 64-bitintvalues for a field backed by anint32, so passingint32values explicitly avoids the conversion/deprecation issue.♻️ Proposed refactor
-func networkPolicyTCPPorts(ports ...int) []networkingv1.NetworkPolicyPort { +func networkPolicyTCPPorts(ports ...int32) []networkingv1.NetworkPolicy Port { protocol := corev1.ProtocolTCP ret := make([]networkingv1.NetworkPolicyPort, 0, len(ports)) for _, port := range ports { - value := intstr.FromInt(port) + value := intstr.FromInt32(port) ret = append(ret, networkingv1.NetworkPolicyPort{Protocol: &protocol, Port: &value}) ret = append(ret, networkingv1.NetworkPolicyPort{Protocol: &protocol, Port: &value}) } return ret }🤖 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 `@internal/resources/stacks/networkpolicies.go` around lines 222 - 230, Update networkPolicyTCPPorts to convert each port to int32 and construct the IntOrString value with FromInt32 instead of the deprecated intstr.FromInt, preserving the existing NetworkPolicyPort construction and returned results.
🤖 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 `@deployment/operator/main.go`:
- Around line 96-98: Update the Helm image value in the operator deployment
configuration to use dc.ImageTag instead of the hardcoded "latest" prefix, while
preserving the existing digest suffix from operatorImage.Digest and repository
value.
- Around line 64-81: Update the licence configuration block to retrieve the
token with the Pulumi secret-aware getter config.GetSecret(ctx, "licence-token")
instead of cfg.Get("licence-token"), and pass that secret output through the
existing licenceValues token entry so the Helm release preserves the token as
masked.
In `@Dockerfile`:
- Around line 28-34: Add a non-root USER directive to the final Dockerfile stage
before the ENTRYPOINT, creating or selecting an unprivileged user that can
execute /usr/bin/operator. Ensure the operator runs under that user by default
while preserving the existing binary copy and entrypoint behavior.
In `@docs/04-Modules/03-Ledger.md`:
- Around line 7-8: Update the PostgreSQL and Broker Markdown links in the module
documentation to use descriptive link text naming each configuration guide
instead of “here,” while preserving the existing destinations and optional
Broker labeling.
In `@docs/09-Configuration` reference/02-Custom Resource Definitions.md:
- Around line 2477-2482: Remove the duplicate ready field from the
GatewayGRPCAPIStatus source schema or generator input, preserving the documented
ready description and the info field, then regenerate the Custom Resource
Definitions reference so the table contains ready only once.
- Around line 2650-2653: Update the `cluster` field description in the
configuration reference so `ClusterSpec` no longer links to the unresolved
`#clusterspec` anchor; link it to the authoritative ClusterSpec reference if
available, otherwise document the type locally or render it as plain text.
In `@internal/resources/connectivities/init_test.go`:
- Around line 199-259: Update the Connectivity controller setup to watch
resources with ledgerCredentialsGVK and map credential events to the affected
Connectivity resource using the credential’s stack selector/namespace data,
since the Credential is not owned by Connectivity. Add an event-driven test
alongside TestEnsureLedgerCredentialsCreatesGodCredentialAndReportsPending and
TestEnsureLedgerCredentialsReportsKeyAndSecretWhenReady that starts with a
pending credential, updates status.phase to Ready, and verifies the
corresponding Connectivity reconciliation is requeued.
In `@internal/resources/gateways/Caddyfile.gotpl`:
- Around line 61-63: Update the GRPCServices branch in the Caddyfile template to
preserve encrypted HTTP/2 by configuring the full protocol set, including h1,
h2, and h2c, instead of only h1 and h2c.
In `@internal/resources/gateways/deployment.go`:
- Around line 124-128: Update the Secret lookup in the gateway deployment
reconciliation loop over sortedSecretNames to detect a not-found error and
return NewPendingError() instead of wrapping it as a hard failure. Preserve the
existing wrapped error for other client.Get failures, and keep the successful
secret-processing path unchanged.
---
Nitpick comments:
In `@deployment/operator/helpers.go`:
- Around line 108-121: Update dockerConfig and newDockerConfig to store the
registry username and password directly from config.GetSecret, rather than
wrapping them only in RegistryAuth. Replace any
dc.RegistryAuth[0].(dockerbuild.RegistryArgs) indexing and type assertions in
the affected logic with the direct credential fields, preserving the existing
credential values.
- Around line 94-106: Update the platform-selection logic around cfg.Get("arch")
to normalize architecture values and parse comma-separated entries, allowing
multiple architectures such as amd64 and arm64. Match each requested
architecture against explicit platform names in allPlatforms rather than relying
on strings.HasSuffix, while preserving the linux-<arch> fallback when no valid
platforms are selected.
In `@internal/resources/stacks/networkpolicies.go`:
- Around line 222-230: Update networkPolicyTCPPorts to convert each port to
int32 and construct the IntOrString value with FromInt32 instead of the
deprecated intstr.FromInt, preserving the existing NetworkPolicyPort
construction and returned results.
In `@internal/tests/ledger_v3_controller_test.go`:
- Around line 1084-1093: After reassigning the cluster-scoped configuration in
this test, register an explicit DeferCleanup for that recreated
DefaultLedgerConfigurationName object rather than relying on the earlier cleanup
closure over configuration. Ensure the cleanup deletes this exact
LedgerConfiguration and remains effective even if the variable is later
shadowed.
In `@internal/tests/networkpolicy_controller_test.go`:
- Around line 93-98: Update the ingress assertions in the
allow-ledger-v2-from-v3 test to verify the concrete Raft and gRPC port values
7777 and 8888, not only that np.Spec.Ingress[0].Ports has length two. Preserve
the existing count assertion and assert both expected port entries using the
same port-value assertion pattern as the nearby 8080 check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28e6cc05-e158-4ed5-b091-d89ebd421e8e
⛔ Files ignored due to path filters (43)
config/crd/bases/formance.com_connectivities.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_gatewaygrpcapis.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_gatewayhttpapis.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_gateways.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_ledgerconfigurations.yamlis excluded by!**/*.yamlconfig/crd/kustomization.yamlis excluded by!**/*.yamlconfig/rbac/ledgerconfiguration_editor_role.yamlis excluded by!**/*.yamlconfig/rbac/ledgerconfiguration_viewer_role.yamlis excluded by!**/*.yamlconfig/rbac/role.yamlis excluded by!**/*.yamlconfig/samples/formance.com_v1beta1_ledgerconfiguration.yamlis excluded by!**/*.yamlconfig/samples/kustomization.yamlis excluded by!**/*.yamldeployment/operator/Pulumi.yamlis excluded by!**/*.yamldeployment/operator/go.modis excluded by!**/*.moddeployment/operator/go.sumis excluded by!**/*.sum,!**/*.sumdocs/09-Configuration reference/settings.catalog.jsonis excluded by!**/*.jsongo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_connectivities.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_gatewaygrpcapis.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_gatewayhttpapis.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_gateways.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_ledgerconfigurations.formance.com.yamlis excluded by!**/*.yamlhelm/operator/templates/gen/rbac.authorization.k8s.io_v1_clusterrole_formance-manager-role.yamlis excluded by!**/gen/**,!**/*.yaml,!**/gen/**internal/tests/crds/cert-manager.io_certificates.yamlis excluded by!**/*.yamlinternal/tests/crds/cert-manager.io_issuers.yamlis excluded by!**/*.yamlinternal/tests/crds/ledger.formance.com_clusters.yamlis excluded by!**/*.yamlinternal/tests/testdata/resources/gateway-controller/configmap-with-audit.yamlis excluded by!**/*.yamlinternal/tests/testdata/resources/gateway-controller/configmap-with-ledger-and-another-service.yamlis excluded by!**/*.yamlinternal/tests/testdata/resources/gateway-controller/configmap-with-ledger-and-grpc.yamlis excluded by!**/*.yamlinternal/tests/testdata/resources/gateway-controller/configmap-with-ledger-only.yamlis excluded by!**/*.yamlinternal/tests/testdata/resources/gateway-controller/configmap-with-opentelemetry.yamlis excluded by!**/*.yamltests/e2e/chainsaw/02-stack-lifecycle/asserts/networkpolicies.yamlis excluded by!**/*.yamltests/e2e/chainsaw/14-ledger-module/chainsaw-test.yamlis excluded by!**/*.yamltests/e2e/chainsaw/14-ledger-module/resources/database.yamlis excluded by!**/*.yamltests/e2e/chainsaw/14-ledger-module/resources/stack.yamlis excluded by!**/*.yamltests/e2e/chainsaw/26-gatewaygrpcapi-sync/chainsaw-test.yamlis excluded by!**/*.yamltests/e2e/chainsaw/26-gatewaygrpcapi-sync/resources/gateway.yamlis excluded by!**/*.yamltests/e2e/chainsaw/26-gatewaygrpcapi-sync/resources/grpcapi-updated.yamlis excluded by!**/*.yamltests/e2e/chainsaw/26-gatewaygrpcapi-sync/resources/grpcapi.yamlis excluded by!**/*.yamltests/e2e/chainsaw/26-gatewaygrpcapi-sync/resources/httpapi-ledger.yamlis excluded by!**/*.yamltests/e2e/chainsaw/26-gatewaygrpcapi-sync/resources/stack.yamlis excluded by!**/*.yamltools/kubectl-stacks/go.modis excluded by!**/*.modtools/kubectl-stacks/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (55)
.gitignoreDockerfileEarthfilePROJECTapi/formance.com/v1beta1/connectivity_types.goapi/formance.com/v1beta1/gateway_types.goapi/formance.com/v1beta1/gatewaybackend_types.goapi/formance.com/v1beta1/gatewaygrpcapi_types.goapi/formance.com/v1beta1/gatewayhttpapi_types.goapi/formance.com/v1beta1/ledger_types.goapi/formance.com/v1beta1/ledgerconfiguration_types.goapi/formance.com/v1beta1/zz_generated.deepcopy.godeployment/operator/.gitignoredeployment/operator/helpers.godeployment/operator/main.godocs/04-Modules/03-Ledger.mddocs/09-Configuration reference/01-Settings.mddocs/09-Configuration reference/02-Custom Resource Definitions.mddocs/10-Development/01-Adding a module.mdinternal/core/setup.gointernal/resources/all.gointernal/resources/auths/env.gointernal/resources/connectivities/init.gointernal/resources/connectivities/init_test.gointernal/resources/gatewaygrpcapis/create.gointernal/resources/gatewaygrpcapis/init.gointernal/resources/gatewayhttpapis/create.gointernal/resources/gateways/Caddyfile.gotplinternal/resources/gateways/caddyfile.gointernal/resources/gateways/caddyfile_test.gointernal/resources/gateways/configuration.gointernal/resources/gateways/deployment.gointernal/resources/gateways/init.gointernal/resources/ledgers/exports.gointernal/resources/ledgers/init.gointernal/resources/ledgers/v3.gointernal/resources/ledgers/v3_preview.gointernal/resources/ledgers/v3_spec.gointernal/resources/ledgers/v3_spec_test.gointernal/resources/ledgers/v3_test.gointernal/resources/ledgers/v3_tls.gointernal/resources/settings/opentelemetry.gointernal/resources/stacks/networkpolicies.gointernal/tests/application_test.gointernal/tests/auth_scopes_settings_test.gointernal/tests/gateway_controller_test.gointernal/tests/gatewaygrpcapi_controller_test.gointernal/tests/jobs_controller_test.gointernal/tests/ledger_controller_test.gointernal/tests/ledger_v3_controller_test.gointernal/tests/networkpolicy_controller_test.gointernal/tests/orchestration_controller_test.gointernal/tests/registries_test.gointernal/tests/transactionplane_controller_test.gointernal/tests/wallets_controller_test.go
| "repository": pulumi.Sprintf("%s/formancehq/operator", dc.PullRegistry), | ||
| "tag": pulumi.Sprintf("latest@%s", operatorImage.Digest), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Helm image tag hardcodes "latest" while the actual pushed tag is dc.ImageTag.
buildImage pushes the index as %s/%s:%s using dc.ImageTag (commit/timestamp-based), never as latest. The Helm value here uses the literal "latest@%s" instead, so the human-readable tag component doesn't correspond to any tag actually pushed to the registry — confusing even though the digest pin still resolves correctly on pull.
🐛 Proposed fix
"image": pulumi.Map{
"repository": pulumi.Sprintf("%s/formancehq/operator", dc.PullRegistry),
- "tag": pulumi.Sprintf("latest@%s", operatorImage.Digest),
+ "tag": pulumi.Sprintf("%s@%s", dc.ImageTag, operatorImage.Digest),
},📝 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.
| "repository": pulumi.Sprintf("%s/formancehq/operator", dc.PullRegistry), | |
| "tag": pulumi.Sprintf("latest@%s", operatorImage.Digest), | |
| }, | |
| "repository": pulumi.Sprintf("%s/formancehq/operator", dc.PullRegistry), | |
| "tag": pulumi.Sprintf("%s@%s", dc.ImageTag, operatorImage.Digest), | |
| }, |
🤖 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 `@deployment/operator/main.go` around lines 96 - 98, Update the Helm image
value in the operator deployment configuration to use dc.ImageTag instead of the
hardcoded "latest" prefix, while preserving the existing digest suffix from
operatorImage.Digest and repository value.
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `ready` _boolean_ | Ready indicates if the resource is seen as completely reconciled | | | | ||
| | `info` _string_ | Info can contain any additional like reconciliation errors | | | | ||
| | `ready` _boolean_ | | | | | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicate ready status field.
GatewayGRPCAPIStatus documents ready twice, with the second entry lacking a description. Fix the source schema/doc generator and regenerate this reference.
🤖 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 `@docs/09-Configuration` reference/02-Custom Resource Definitions.md around
lines 2477 - 2482, Remove the duplicate ready field from the
GatewayGRPCAPIStatus source schema or generator input, preserving the documented
ready description and the info field, then regenerate the Custom Resource
Definitions reference so the table contains ready only once.
| {{- if .GRPCServices }} | ||
| protocols h1 h2c | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Caddy servers protocols directive default h1 h2 h2c
💡 Result:
In Caddy, the protocols directive specifies which HTTP protocols are enabled for a server [1][2]. The default protocols are ["h1", "h2", "h3"] [1][3][4]. Supported values are: - h1: HTTP/1.1 - h2: HTTP/2 (requires TLS) - h2c: HTTP/2 cleartext (no TLS) - h3: HTTP/3 (requires QUIC/UDP) Key implementation details include: - When h2 or h2c is enabled, h1 must also be included because of Go standard library limitations [1][5][2]. - HTTP/2 operates over TLS, while HTTP/3 uses a separate UDP socket [1][5][3]. - H2C is not implemented by the Go standard library and is generally recommended against unless necessary, such as for specific gRPC proxying scenarios [1][2]. - If you leave the protocols directive empty or unset, Caddy defaults to ["h1", "h2", "h3"] [1][3][4]. The directive can be configured globally within a servers block in the Caddyfile [2]: servers:443 { protocols h1 h2 h3 } Or in JSON configuration: { "apps": { "http": { "servers": { "example": { "protocols": ["h1", "h2", "h3"] } } } } }
Citations:
- 1: https://github.com/caddyserver/caddy/blob/03e6e439/modules/caddyhttp/server.go
- 2: https://caddyserver.com/docs/caddyfile/options
- 3: https://deepwiki.com/caddyserver/caddy/3.1-application-and-server-lifecycle
- 4: https://github.com/caddyserver/caddy/blob/4fd8c87f/modules/caddyhttp/server.go
- 5: https://caddyserver-caddy.mintlify.app/modules/http/server
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'Caddyfile\.gotpl|internal/resources/gateways|resources/gateways' . | sed 's#^\./##' | head -100
echo
echo "Target file excerpt:"
if [ -f internal/resources/gateways/Caddyfile.gotpl ]; then
sed -n '1,240p' internal/resources/gateways/Caddyfile.gotpl | nl -ba | sed -n '1,240p'
else
echo "internal/resources/gateways/Caddyfile.gotpl not found"
fi
echo
echo "Protocol/server listener context searches:"
rg -n "ports|:{{ \.\Port }}|listen|tls|GRPCServices|transport http|protocols h1 h2c|protocols h1 h2 h2c" internal/resources/gateways -S || true
echo
echo "Template usages / Port value:"
rg -n "GRPCServices|Port|grpc|Gateway" --glob '*.go' --glob '*.ts' --glob '*.ts' internal -S | head -200Repository: formancehq/operator
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target file excerpt:"
sed -n '1,240p' internal/resources/gateways/Caddyfile.gotpl
echo
echo "Protocol/server listener context searches:"
rg -n "ports|:{{ \.\Port }}|listen|tls|GRPCServices|transport http|protocols h1 h2c|protocols h1 h2 h2c" internal/resources/gateways -S || true
echo
echo "Template usages / Port value:"
rg -n "GRPCServices|Port|grpc|Gateway" --glob '*.go' --glob '*.ts' internal -S | head -200Repository: formancehq/operator
Length of output: 26996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Gateway listener/deployment/service/ingress creation:"
sed -n '1,210p' internal/resources/gateways/deployment.go
echo
sed -n '1,170p' internal/resources/gateways/ingress.go
echo
sed -n '1,210p' internal/resources/gateways/configuration.go
echo
rg -n -F "ServicePort" internal/resources/gateways -SRepository: formancehq/operator
Length of output: 11290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Service creation:"
rg -n "createService|NewService|ServicePort|ServiceName|name: .*gateway|port: .*8080|name: http|protocol: TLS|tls" internal/resources/gateways internal/resources/services internal/resources/applications -S
echo
fd -a Caddyfile\* . | sed 's#^\./##'
echo
sed -n '1,120p' internal/resources/payments/Caddyfile.gotplRepository: formancehq/operator
Length of output: 3042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Gateway service creation:"
rg -n -C 8 "CreateOrUpdate\\[\\*corev1\\.Service\\]|\"http\"|Port:" internal/resources/gateways/gateways.go internal/resources/gateways -X '*.go' -S || true
echo
echo "Full services helper:"
sed -n '1,90p' internal/resources/services/services.go
echo
echo "Full Gateway reconcile/service code:"
sed -n '102,210p' internal/resources/gateways/gateways.goRepository: formancehq/operator
Length of output: 2507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Gateway reconcile/service create call:"
rg -n -C 12 'Create\(|WithDefault|core\.CreateOrUpdate\[\*corev1\.Service\]' internal/resources/gateways/gateways.go internal/resources/gateways -S
echo
echo "Read-only structural/data verifier:"
python3 - <<'PY'
from pathlib import Path
for p in ["internal/resources/gateways/Caddyfile.gotpl", "internal/resources/gateways/ingress.go", "internal/resources/services/services.go"]:
text = Path(p).read_text()
print(f"\n== {p} ==")
print("contains `protocols h1 h2c`:", "protocols h1 h2c" in text)
print("contains listen/targeting `:{{ .Port }}`:", ":{{ .Port }}" in text)
print("contains Ingress TLS secret name:", "tlsEnabled" in text and "withTls" in text)
print("contains ingress backend service port name \"http\":", 'Port: v1.ServiceBackendPort{\n\t\t\t\t\t\tName: "http",' in text)
print("creates service port named http with Port 8080:", 'Name: cfg.PortName,\n\t\t\t\t\tPort: cfg.Port,' in text and 'PortName: "http",' in text and 'Port: 8080,' in text)
print("contains h2 enablement:", "h2" in text)
PYRepository: formancehq/operator
Length of output: 2865
🌐 Web query:
Caddy servers option protocols directive default h1 h2 h3 h2c no explicit setting
💡 Result:
In the Caddy web server, the default setting for the protocols directive is [h1 h2 h3] [1][2][3][4]. This means that by default, Caddy enables HTTP/1.1 (h1), HTTP/2 (h2), and HTTP/3 (h3) [1][3]. When no explicit protocols directive is configured, Caddy automatically applies these defaults to all servers [1][3]. Important technical considerations regarding these protocols include: - H2 (HTTP/2) and H2C (HTTP/2 Cleartext) Requirements: Due to limitations in the Go standard library, if you explicitly enable h2 or h2c, you must also enable h1 [1][2][4]. - Transport Mechanisms: HTTP/2 operates exclusively over TLS (HTTPS), while HTTP/3 operates over UDP using the QUIC protocol [1][2]. - H2C (Cleartext HTTP/2): H2C is not enabled by default and is generally discouraged unless specifically required, such as for certain gRPC proxying scenarios where encryption is not feasible [1][4]. H2C operates over plain TCP [1][3]. - Configuration Scope: The protocols directive can be configured globally within the Caddyfile servers block or applied to specific server blocks by matching the listener address [5][4][6]. Caddy recommends that most users rely on the default settings to ensure optimal performance and compatibility [1].
Citations:
- 1: https://github.com/caddyserver/caddy/blob/03e6e439/modules/caddyhttp/server.go
- 2: https://caddyserver-caddy.mintlify.app/modules/http/server
- 3: https://deepwiki.com/caddyserver/caddy/3.1-application-and-server-lifecycle
- 4: https://caddyserver.com/docs/caddyfile/options
- 5: https://github.com/caddyserver/caddy/blob/03e6e439/caddyconfig/httpcaddyfile/serveroptions.go
- 6: https://caddy.community/t/how-can-i-set-protocols-for-one-app-in-caddyfile/21092
Preserve the default HTTPS/HTTP protocol set.
The gateway listener is used behind ingress TLS, so protocols h1 h2c disables h2 for encrypted clients. Use the full protocol set instead of h1 h2c alone.
🔧 Proposed fix
{{- if .GRPCServices }}
- protocols h1 h2c
+ protocols h1 h2 h2c h3
{{- end }}📝 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 .GRPCServices }} | |
| protocols h1 h2c | |
| {{- end }} | |
| {{- if .GRPCServices }} | |
| protocols h1 h2 h2c h3 | |
| {{- end }} |
🤖 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 `@internal/resources/gateways/Caddyfile.gotpl` around lines 61 - 63, Update the
GRPCServices branch in the Caddyfile template to preserve encrypted HTTP/2 by
configuring the full protocol set, including h1, h2, and h2c, instead of only h1
and h2c.
Introduce a new `formance.com/v1beta1 Connectivity` stack module, mirroring the Ledger v3 delegation pattern. The module does not run the workload itself. It: - detects, at controller start-up, whether the connectivity operator (`connectivity.formance.com` Connectivity CRD) is installed and reachable with the required RBAC — the same capability + API-group probe the Ledger v3 module uses (CRD served-version check + SelfSubjectAccessReview per verb). When absent, the module reports the capability as unavailable and stays pending instead of failing the controller. - gates on the stack's ledger being v3 and ready (connectivity ingests into the Ledger v3 gRPC endpoint). - provisions a `connectivity.formance.com/v1alpha1 Connectivity` resource bound to that ledger: `ledgerAddress` = the ledger v3 gRPC service and `ledgerTLS` = the ledger backend TLS secret. The connection details are taken from `ledgers.V3GRPCBackendRef`, the single source of truth already used to reach the ledger over gRPC, so connectivity and the gateway stay in sync. - reflects the delegated resource's readiness back onto the module status. Includes the module type, reconciler + capability detection, unit tests for the capability-gating paths, and the generated CRD/RBAC/deepcopy + helm CRD.
Address review feedback and the Dirty check: - Resolve the ledger version with core.ResolveModuleVersion so the v3 gate also works for stacks using spec.versionsFromFile (previously the version fell back to empty and Connectivity stayed stuck on LedgerNotV3). - Register bases/formance.com_connectivities.yaml in config/crd/kustomization so non-Helm (kustomize) installs create the Connectivity CRD. - Regenerate CRD reference docs + helm CRD (just pre-commit).
Document the current end-to-end process for adding a stack module: the module CR type (incl. the mandatory formance.com/kind=module label), the reconciler + init registration, all.go + config/crd/kustomization registration, capability detection for delegating modules, version resolution, codegen, and the deployment gotchas (reconcileStrategy: Revision, the startup-only capability probe, and versionsFromFile requirements).
The delegated connectivity.formance.com Connectivity was created without spec.image, so the connectivity operator fell back to its built-in ghcr.io/formancehq/connectivity-core:latest default — bypassing the stack's registry rewrite (e.g. ghcr.io -> registry.v2.formance.dev) and pull secrets, which makes it unpullable on rewritten registries. Resolve the connectivity-core image via registries.GetFormanceImage (using the Connectivity module version) so it honours the stack registry settings, and set spec.image + spec.imagePullSecrets on the delegated resource.
- Always enable the connectivity-api companion on the delegated Connectivity (spec.api.enabled=true), resolving the connectivity-api image through the registry translation so it honours the stack's registry rewrite + pull secrets (not the connectivity operator's ghcr.io/...:latest default). - Register a GatewayHTTPAPI for the module routing /api/connectivity to the connectivity-api Service (<stack>-api:8080) the connectivity operator provisions, and own it so changes reconcile.
…e auth The connectivity module deployed the delegated Connectivity CR but never wired connectivity-core's authentication to the stack's Ledger v3 gRPC endpoint, so connectivity-core sent no token and the ledger rejected every call with 'requires scope ledger:LedgerWrite'. Provision a god-mode ledger.formance.com/Credentials (cluster-scoped, owned by the Stack) selecting the stack's ledger Cluster: the ledger operator generates the Ed25519 keypair, registers the public key on the ledger, and distributes the private seed as a Secret in the stack namespace. Once Ready, wire the Connectivity CR's spec.auth (keyId + secretKeyRef->seed.hex) so the connectivity operator passes --auth-key-id/--auth-key-file and connectivity-core signs its gRPC tokens with the registered key. Regenerates RBAC for the new resource.
The connectivity repo's CI publishes formancehq/connectivity; the former connectivity-core repository no longer exists, so the module kept writing an unpullable image on the delegated resource.
The kubebuilder markers for ledger.formance.com credentials (added with the connectivity module) were present in config/rbac but the helm chart's generated ClusterRole was never refreshed, so the deployed operator was forbidden from listing Credentials -- its informer never synced and every Connectivity reconcile hung before reaching the delegated resource.
Main's ledgerV3GRPCBackendRef now takes the ledger Cluster's configured gRPC port; keep the connectivity-facing export on the default port.
3795ef8 to
a6746a0
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #492 (comment)
…ed resource (#500) * feat(connectivity): provision OpenTelemetry monitoring on the delegated resource Resolve the stack's OpenTelemetry configuration with settings.GetOpenTelemetryConfiguration (collector-aware: points at otel-collector.<stack>:4318 when the per-stack collector exists, else honours the opentelemetry.* Settings) and embed it inline in the delegated Connectivity's spec.monitoring, mirroring the Ledger v3 Cluster pattern. The connectivity operator turns spec.monitoring into OTEL_* env vars on the workload; there is no separate Monitoring object to reference. The whole spec.monitoring block is rebuilt on every reconcile and pruned when telemetry is disabled, so the reconcile stays idempotent. Not-tested: end-to-end env-var emission by the connectivity operator (covered by the connectivity repo); unit tests assert the inline spec mapping and idempotency. * fix(connectivity): drop unresolvable pod-name attribute from delegated monitoring GetOpenTelemetryConfiguration injects pod-name=$(POD_NAME), which only resolves when a downward-API POD_NAME env var is defined ahead of OTEL_RESOURCE_ATTRIBUTES. The connectivity operator emits OTEL_RESOURCE_ATTRIBUTES verbatim from spec.monitoring.attributes and defines no such env var, so the placeholder surfaced literally in the delegated workload's telemetry. Strip attributes whose value carries an unresolvable $(...) placeholder before forwarding, keeping literal resource attributes (stack, custom). Omit the attributes field entirely when nothing resolvable remains.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 4 new inline findings.
Summary: #492 (comment)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/10-Development/01-Adding a module.md (1)
43-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument watches for every readiness dependency.
Line 48 lists only
Ledger. The Connectivity reconciler also depends on the ledgerCredentialsresource. State that delegated modules must watch every readiness-gating resource. Otherwise, the module can remain pending afterCredentialsbecomes ready because no event triggers another reconcile.Proposed documentation update
WithWatchDependency[*v1beta1.<Module>](&v1beta1.Ledger{}), // re-reconcile on dependency change ), ) } + +For delegated modules, add a watch for every readiness-gating resource, +including the ledger `Credentials` resource when credential readiness controls +provisioning.🤖 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 `@docs/10-Development/01-Adding` a module.md around lines 43 - 49, Update the module setup documentation around WithWatchDependency to state that delegated modules must watch every readiness-gating resource, including both the Ledger and its Credentials resource. Clarify that each dependency required by the reconciler’s readiness checks needs a watch so readiness changes trigger reconciliation.internal/resources/connectivities/init_test.go (1)
407-428: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a Reconcile happy-path test.
The only
Reconciletest covers the capability-unavailable pending path. No test exercises the full success path: a ready Ledger v3, ready credentials, successful image resolution, and the resulting spec fields (ledgerAddress,ledgerTLS,auth.keyId,auth.secretKeyRef) on the created delegated Connectivity resource. Given this function is the central reconciliation path for the module, add a test with a fake client seeded with a readyLedger, a ready ledger Credentials object, and the required scheme registrations, then assert on the resulting unstructured object's spec.🤖 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 `@internal/resources/connectivities/init_test.go` around lines 407 - 428, Add a happy-path test alongside TestConnectivityReconcilePendingWhenCapabilityUnavailable that seeds a fake client with a ready Ledger v3 and ready ledger Credentials, registers the required schemes, and configures successful image resolution before calling Reconcile. Assert that the created delegated Connectivity resource contains the expected spec.ledgerAddress, spec.ledgerTLS, spec.auth.keyId, and spec.auth.secretKeyRef values.
🤖 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 `@internal/resources/connectivities/init.go`:
- Around line 500-509: In internal/resources/connectivities/init.go at lines
500-509, register a watch handler that maps status updates of the ledger
Credentials object (ledgerCredentialsGVK, named connectivity-<stack>) back to
the corresponding Connectivity reconcile request. This watch should be added to
the Init call, either directly via a Watches handler or integrated into the
withConnectivityClusterWatch options pattern, so that when the Credentials
object status changes, the Connectivity resource is requeued. In
internal/resources/connectivities/init_test.go at lines 201-261, add a test that
verifies this requeue path by starting with a pending Credentials object,
updating its status.phase to Ready, and asserting that the corresponding
Connectivity reconcile is requeued as a result of the watch firing.
- Around line 163-167: Update the Connectivity initialization around
V3GRPCBackendRef so it uses the stack’s configured Ledger v3 gRPC port instead
of the default-port helper behavior. Pass or propagate the relevant configured
port into the backend reference construction, ensuring ledgerAddress is built
from the same non-default port used by Gateway.
- Around line 345-395: Add a finalizer-based cleanup mechanism to the
Connectivity resource that ensures the associated Credentials object is deleted
during Connectivity deletion. In the reconciliation logic, add a finalizer
constant for Connectivity deletion and check if the Connectivity resource is
being deleted; when deletion is detected, delete the cluster-scoped Credentials
object named connectivity-<stack.Name> (using the same naming pattern as in
ensureLedgerCredentials) before removing the finalizer from the Connectivity
resource. This ties the privileged Credentials and its distributed Secret to the
Connectivity module lifecycle so they are revoked when the module is removed,
regardless of the Stack's lifecycle.
---
Nitpick comments:
In `@docs/10-Development/01-Adding` a module.md:
- Around line 43-49: Update the module setup documentation around
WithWatchDependency to state that delegated modules must watch every
readiness-gating resource, including both the Ledger and its Credentials
resource. Clarify that each dependency required by the reconciler’s readiness
checks needs a watch so readiness changes trigger reconciliation.
In `@internal/resources/connectivities/init_test.go`:
- Around line 407-428: Add a happy-path test alongside
TestConnectivityReconcilePendingWhenCapabilityUnavailable that seeds a fake
client with a ready Ledger v3 and ready ledger Credentials, registers the
required schemes, and configures successful image resolution before calling
Reconcile. Assert that the created delegated Connectivity resource contains the
expected spec.ledgerAddress, spec.ledgerTLS, spec.auth.keyId, and
spec.auth.secretKeyRef values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62aaed12-7a50-4b92-bcf3-128097420cf8
⛔ Files ignored due to path filters (5)
config/crd/bases/formance.com_connectivities.yamlis excluded by!**/*.yamlconfig/crd/kustomization.yamlis excluded by!**/*.yamlconfig/rbac/role.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_connectivities.formance.com.yamlis excluded by!**/*.yamlhelm/operator/templates/gen/rbac.authorization.k8s.io_v1_clusterrole_formance-manager-role.yamlis excluded by!**/gen/**,!**/*.yaml,!**/gen/**
📒 Files selected for processing (8)
api/formance.com/v1beta1/connectivity_types.goapi/formance.com/v1beta1/zz_generated.deepcopy.godocs/09-Configuration reference/02-Custom Resource Definitions.mddocs/10-Development/01-Adding a module.mdinternal/resources/all.gointernal/resources/connectivities/init.gointernal/resources/connectivities/init_test.gointernal/resources/ledgers/exports.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/resources/all.go
- internal/resources/ledgers/exports.go
- api/formance.com/v1beta1/connectivity_types.go
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Reviewed as the root of the linked stack. The current head still contains the credential watch/cleanup, closed-gate teardown, configurable gRPC port, gateway TLS pending-state, and NetworkPolicy issues tracked in the existing threads; those fixes live in descendant PRs #501–#507 rather than in this head. I am not duplicating the inline findings or approving this root in isolation. It is safe to reconsider once the stack is merged/restacked so those fixes are part of the effective change.
When a backend module has not yet provisioned its TLS Secret, the Gateway deployment reconciler fetched the Secret and returned any error as a hard failure. A NotFound during this race surfaced the Gateway as errored rather than pending. Return core.NewPendingError() on apierrors.IsNotFound so the framework treats it as pending (matching how other 'not ready yet' conditions are handled) and retries. All other Get errors remain hard errors.
…etriggered (#504) The Connectivity reconciler returns a PendingError while the cluster-scoped ledger Credentials it provisions (connectivity-<stack>) is not yet Ready (LedgerCredentialsPending). The reconcile loop treats a PendingError as a terminal ctrl.Result{} with no RequeueAfter, and the Credentials is owned by the Stack rather than the namespaced Connectivity, so nothing re-triggered the module when the ledger operator flipped the Credentials status.phase to Ready: the reconcile could stall indefinitely. Register a raw builder watch on the ledger.formance.com/v1alpha1 Credentials GVK (unstructured, mirroring withConnectivityClusterWatch) that maps a Credentials event back to the Connectivity in the matching stack, derived from the connectivity-<stack> name and listed via the stack field index. The watch is gated on the Credentials CRD being installed so controller setup never fails when the ledger operator is absent. RBAC already grants watch on credentials. Add unit tests for the mapping (enqueues the matching Connectivity, returns nothing for a stack without one, ignores foreign Credentials) and for the capability gate (disabled when the CRD is absent or discovery fails).
… closes (#505) * fix(connectivity): tear down delegated resources when the ledger gate closes When a stack had already provisioned the delegated Connectivity and its GatewayHTTPAPI, closing a hard Ledger gate on a later reconcile only set a pending condition and returned; the delegated workload kept running and stayed exposed through the gateway even though its prerequisite no longer held. Introduce teardownDelegated(ctx, stack, connectivity), which idempotently deletes both the delegated Connectivity and the GatewayHTTPAPI (client.IgnoreNotFound). Call it from the hard/persistent gates only: - LedgerNotFound (module removed) -> teardown - LedgerNotV3 (real downgrade) -> teardown Leave the transient gates untouched so a momentary blip does not flap the workload: - LedgerVersionUnresolved (resolution error, not a downgrade) -> keep - LedgerNotReady (v3 but momentarily not ready) -> keep Add unit tests covering both the teardown-on-not-v3 and keep-on-v3-not-ready paths. * fix(connectivity): tear down credentials and route independently on hard gate close teardownDelegated returned on the first delete error, so a failed delegated-Connectivity deletion left the public GatewayHTTPAPI route exposed; it also never deleted the cluster-scoped god-mode Credentials, whose distributed private-key Secret stack-namespace GC never reclaims (and whose public key stays registered on the v3 Cluster). Attempt all three deletions independently via errors.Join and delete the Credentials (which cascades the ledger operator's key deregistration and Secret cleanup). Tests: assert Credentials cleanup on hard teardown, its retention on a transient gate, and that every deletion is attempted when one fails (raised in review of #505). * fix(connectivity): tear down on a closed ledger gate even without the operator The !connectivityAvailable guard returned before the LedgerNotFound/ LedgerNotV3 teardown, so if the connectivity operator became unavailable after resources were provisioned and the ledger was then removed or downgraded, the gateway route and god-mode Credentials stayed behind. Evaluate the ledger hard gate (ledgerGateClosed) in the capability- unavailable branch and tear down when it is closed, guarded so a transient operator outage with a healthy v3 ledger does not flap the resources. teardownDelegated now tolerates the connectivity CRD being absent (ignoreAbsent) so the delete is a no-op when the API is gone. Tests cover teardown-on-closed-gate and retention-on-open-gate (raised in review of #505).
V3GRPCBackendRef, the single source of truth consumed by the connectivity module to build its ledgerAddress, always passed port 0 to ledgerV3GRPCBackendRef and therefore assumed the default gRPC port. The gateway backend (v3.go / v3_preview.go) instead resolves the port from the stack LedgerConfiguration (spec.cluster.service.grpcPort), so a stack overriding the ledger Cluster gRPC service port got a Connectivity pointed at the wrong port while the gateway stayed correct. Resolve the configured port from the LedgerConfiguration inside V3GRPCBackendRef (via ledgerV3BaseSpec, the same base the gateway derives its clusterSpec from) so both consumers honour the override and fall back to the default port when unset. Thread the reconciler Context through the export and its connectivity caller. Add a table-driven unit test covering default, stack-scoped, wildcard, and precedence cases.
…a NetworkPolicy (#507) * fix(connectivity): allow connectivity pods to reach Ledger v3 gRPC via NetworkPolicy When networkpolicies.enabled, the default-deny-ingress policy drops all ingress to Ledger v3 pods except the explicitly-allowed gateway and intra-cluster ledger peers. The delegated connectivity workload dials the Ledger v3 gRPC endpoint (ledgers.V3GRPCBackendRef, port 8888) directly, but connectivity pods are neither gateway nor ledger pods, so their gRPC connections were silently dropped on network-policy stacks. Add a dedicated allow-ledger-v3-from-connectivity NetworkPolicy granting the connectivity workload ingress to the Ledger v3 pods on the gRPC port (8888), following the existing dedicated-policy pattern (allow-from-gateway). The connectivity pod labels are owned by the connectivity operator (separate repo) and cannot be confirmed here; connectivitySelector matches the operator-wide convention app.kubernetes.io/name=connectivity (documented as an assumption in code). Add unit tests rendering the policy and extend the network-policy controller test. * fix(stacks): leave connectivity->ledger-v3 gRPC port unrestricted The allow-ledger-v3-from-connectivity policy hardcoded port 8888, so a stack overriding spec.cluster.service.grpcPort had its connectivity gRPC traffic silently denied by the default-ingress policy. Rather than resolving the port (which would go stale unless the Stack controller also watched LedgerConfiguration), leave the port unrestricted for this tightly scoped same-namespace connectivity->ledger-v3 pair, mirroring allow-ledger-v3-cluster which is already port-agnostic for exactly this reason. The rule can no longer be broken or left stale by a grpcPort override, and needs no config watch.
) The delegated connectivity.formance.com/Connectivity is namespaced (one per stack namespace), so it no longer needs a stack-scoped name. Give it the fixed name "connectivity"; the connectivity operator derives the API Service from it, so it becomes "connectivity-api" instead of "<stack>-api". Credentials (connectivity-<stack>) and the GatewayHTTPAPI (<stack>-connectivity) stay stack-scoped — they are cluster-scoped and need the stack for uniqueness. No migration: the module is not yet released.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #492 (comment)
| setCondition(connectivity, metav1.ConditionFalse, "LedgerCredentialsFailed", err.Error()) | ||
| return err | ||
| } | ||
| if !credReady { |
There was a problem hiding this comment.
🟠 [major] No watch on ledger Credentials — reconciler can stall indefinitely
reported by NumaryBot, coderabbitai[bot]
When the Connectivity reconciler creates the ledger Credentials object and returns a pending/application error waiting for status.phase=Ready, it will not be requeued automatically because the Credentials resource is not registered as a watched object. The reconciler only watches GatewayHTTPAPI, the delegated Connectivity, settings, and Ledger. On a normal install the controller can remain stuck in LedgerCredentialsPending until some unrelated watched object triggers a reconcile.
Suggestion: Add a Watch for the ledger Credentials resource in the controller setup so that a status update on that object (e.g. phase→Ready) triggers a reconcile of the owning Connectivity. Alternatively, use a periodic requeue (e.g. ctrl.Result{RequeueAfter: someInterval}) whenever the reconciler returns while waiting for Credentials readiness.
| cred.SetGroupVersionKind(ledgerCredentialsGVK) | ||
| cred.SetName("connectivity-" + stack.Name) | ||
| if _, err = controllerutil.CreateOrUpdate(ctx, ctx.GetClient(), cred, func() error { | ||
| if err := controllerutil.SetControllerReference(stack, cred, ctx.GetScheme()); err != nil { |
There was a problem hiding this comment.
🟠 [major] Ledger credentials not cleaned up when Connectivity module is deleted
When a Connectivity module is removed but the Stack remains, the cluster-scoped Credentials object (connectivity-<stack>) is kept alive because it is owned by the Stack rather than the Connectivity module. There is no Connectivity finalizer or cleanup path that deletes this object, so the privileged ledger credential and distributed secret remain registered even after the module is disabled or deleted, leaving access that was provisioned but never revoked.
Suggestion: Add a finalizer to the Connectivity resource that explicitly deletes the associated Credentials object (and any distributed secret) during deletion. Alternatively, set the Credentials object's owner reference to the Connectivity CR so Kubernetes garbage collection handles cleanup automatically.
| // authenticate its gRPC calls. The ledger operator registers the public key | ||
| // on the ledger and distributes the private seed as a Secret in the stack | ||
| // namespace; connectivity-core is wired to it via spec.auth below. | ||
| authKeyID, authSecretName, credReady, err := ensureLedgerCredentials(ctx, stack) |
There was a problem hiding this comment.
🟠 [major] Missing ledger Credentials CRD/RBAC causes hard failure instead of pending
When the ledger.formance.com Credentials CRD or its RBAC is absent (e.g. an older ledger-operator install), ensureLedgerCredentials returns a hard NoMatch/Forbidden error and the module is marked failed. Since Credentials is an external capability needed only for Connectivity, this scenario should be treated as a pending/unavailable condition rather than a reconciliation failure.
Suggestion: Detect NoMatch and Forbidden errors from the Credentials API call and convert them into a NewPendingError() (or an explicit unavailable condition) so the module reports pending rather than failed when the CRD is not yet installed.
| setCondition(connectivity, metav1.ConditionFalse, "LedgerBackendResolveFailed", err.Error()) | ||
| return err | ||
| } | ||
| ledgerAddress := fmt.Sprintf("%s:%d", backend.TLS.ServerName, backend.Port) |
There was a problem hiding this comment.
🟠 [major] ledgerAddress uses SNI server name instead of the backend Service endpoint
ledgerAddress is built from backend.TLS.ServerName, which is the SNI/certificate hostname. In clusters whose DNS domain is not cluster.local, ledger-<stack>.<stack>.svc.cluster.local may not resolve. The gateway upstream path uses backend.Name as the Service address and TLS.ServerName only for TLS verification; ledgerAddress should follow the same pattern.
Suggestion: Build ledgerAddress from the backend Service name/endpoint rather than backend.TLS.ServerName, and use TLS.ServerName only for the TLS SNI/verification field.
| // connectivity-core can authenticate its gRPC calls to the stack's Ledger v3: | ||
| // the ledger operator generates the Ed25519 keypair, registers the public key | ||
| // on the matched ledger Cluster, and distributes the private seed as a Secret. | ||
| ledgerCredentialsGVK = schema.GroupVersionKind{ |
There was a problem hiding this comment.
🟠 [major] Ledger version resolved only from spec.version, not from versionsFromFile
For stacks that use .spec.versionsFromFile instead of .spec.version, the fallback never reads the Ledger entry from the referenced Versions object, so ledgerVersion stays empty and Connectivity remains stuck with LedgerNotV3 even when the Ledger module itself reconciled as v3 and is ready.
Suggestion: Use the same module-version resolution path as the Ledger reconciler (reading from the referenced Versions object when versionsFromFile is set) before applying the v3 gate.
There was a problem hiding this comment.
C'est pas mon probleme
|
|
||
| func init() { | ||
| Init( | ||
| WithModuleReconciler(Reconcile, |
There was a problem hiding this comment.
🟡 [minor] Version resolution fails on versionsFromFile stacks without a 'connectivity' entry
For stacks that use spec.versionsFromFile, the module framework resolves the Connectivity version before entering Reconcile. If the referenced Versions object has no 'connectivity' key, reconciliation fails before reaching the capability/Ledger-v3 gate, even though the resolved version value is never actually used by the reconciler. This forces operators to add a meaningless version entry to their Versions file just to unblock reconciliation.
Suggestion: Make the version field optional or provide a sensible default (e.g. empty string) so the reconciler can proceed when no connectivity version is specified. If the version is genuinely unused, skip the version-resolution gate entirely for this module.
| return core.NewPendingError().WithMessage("waiting for backend TLS Secret %s/%s", namespace, secretName) | ||
| } | ||
| return fmt.Errorf("getting backend TLS Secret %s/%s: %w", namespace, secretName, err) | ||
| } |
There was a problem hiding this comment.
🟡 [minor] Missing backend TLS Secret treated as hard error instead of pending
If the referenced backend TLS Secret doesn't exist yet (e.g., race with the owning module provisioning it), the code returns a hard error instead of a pending state — inconsistent with the 'broker not ready' handling in internal/resources/gateways/init.go which uses NewPendingError(). The labeled-secret watch will eventually retrigger reconciliation, but until then the Gateway will surface as errored rather than pending.
Suggestion: Check for apierrors.IsNotFound(err) and return core.NewPendingError().WithMessage(...) instead of a hard error when the Secret is not yet available.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/resources/connectivities/init.go`:
- Around line 316-321: Update the doc comment for teardownDelegated to state
that the delegated Connectivity uses connectivityDelegatedName ("connectivity")
as its name and the stack name as its namespace; retain the existing
GatewayHTTPAPI naming and scoping description.
- Around line 85-91: Use ledgerCredentialsWatchAvailable in Reconcile when
handling LedgerCredentialsPending: if the watch is unavailable, return a delayed
requeue result so pending Connectivity resources retry without CRD events;
preserve the existing NewPendingError behavior and watch-driven path when the
flag is true.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0075e17-fb3c-46d9-b7b5-9e9bf747f064
📒 Files selected for processing (9)
internal/resources/connectivities/init.gointernal/resources/connectivities/init_test.gointernal/resources/gateways/deployment.gointernal/resources/gateways/deployment_test.gointernal/resources/ledgers/exports.gointernal/resources/ledgers/exports_test.gointernal/resources/stacks/networkpolicies.gointernal/resources/stacks/networkpolicies_test.gointernal/tests/networkpolicy_controller_test.go
|
|
||
| // ledgerCredentialsWatchAvailable records whether, at controller start-up, | ||
| // the ledger Credentials CRD was present so the reconciler could register a | ||
| // watch on it. When true, changes to the connectivity-<stack> Credentials | ||
| // (notably its status.phase flipping to Ready) re-trigger the owning | ||
| // Connectivity's reconcile. | ||
| ledgerCredentialsWatchAvailable bool |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all reads of ledgerCredentialsWatchAvailable and any requeue fallback.
rg -n -C4 'ledgerCredentialsWatchAvailable' --type=go
rg -n -C3 'RequeueAfter' internal/resources/connectivities internal/core --type=goRepository: formancehq/operator
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
echo "== git files matching init.go =="
git ls-files | rg 'internal/resources/connectivities/init.go|LedgerCredentials|credential' || true
echo "== search ledgerCredentialsWatchAvailable exact/name =="
rg -n -C3 'ledgerCredentialsWatchAvailable|LedgerCredentials|LedgerCredentialsPending|RequeueAfter' --glob '*.go' . || true
echo "== file line counts =="
for f in $(git ls-files | rg 'internal/resources/connectivities/init.go'); do
wc -l "$f"
done
echo "== outline init.go =="
f="$(git ls-files | rg 'internal/resources/connectivities/init.go' | head -1)"
if [ -n "${f:-}" ]; then
ast-grep outline "$f" || true
fiRepository: formancehq/operator
Length of output: 35824
🏁 Script executed:
#!/bin/bash
set -u
echo "== internal/core/reconciler.go around PendingError handling =="
rg -n 'PendingError|PendingErrors|reconcileObject|Terminal|Requeue' internal/core/reconciler.go --type=go
sed -n '140,310p' internal/core/reconciler.go
echo "== exact read grep =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.go'):
text = p.read_text(errors='ignore')
if 'ledgerCredentialsWatchAvailable' in text:
for i,line in enumerate(text.splitlines(),1):
if 'ledgerCredentialsWatchAvailable' in line:
print(f"{p}:{i}:{line}")
PY
echo "== programmatic invariant check =="
python3 - <<'PY'
import re
from pathlib import Path
text = Path('internal/resources/connectivities/init.go').read_text()
write_refs = [i for i,l in enumerate(text.splitlines(),1) if 'ledgerCredentialsWatchAvailable' in l]
read_refs = []
for m in re.finditer(r'\bledgerCredentialsWatchAvailable\b', text):
line_num = text[:m.start()].count('\n') + 1
line = text.splitlines()[line_num-1]
if 'ledgerCredentialsWatchAvailable =' in line or 'ledgerCredentialsWatchAvailable = ' in line or line.strip().startswith('ledgerCredentialsWatchAvailable ='):
continue
read_refs.append((line_num, line.strip()))
print("write_refs=", write_refs)
print("read_refs=", read_refs)
print("no_reads=", len(read_refs) == 0)
PYRepository: formancehq/operator
Length of output: 8177
🏁 Script executed:
#!/bin/bash
set -u
sed -n '310,380p' internal/core/reconciler.go
rg -n -C2 'IsPending|PendingError|PendingErrors' --glob '*.go' . || trueRepository: formancehq/operator
Length of output: 22747
Add a requeue fallback when the ledger Credentials watch is unavailable.
ledgerCredentialsWatchAvailable is only written by withLedgerCredentialsWatch; it is never read. When the Credentials CRD is absent, controller setup disables the watch, and Reconcile still returns NewPendingError() with LedgerCredentialsPending. Since the reconciler returns ctrl.Result{} for that failure, the Connectivity stays pending with no scheduled retry. Return a delayed Requeue here, or remove this flag if the watch is expected to always be available.
🤖 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 `@internal/resources/connectivities/init.go` around lines 85 - 91, Use
ledgerCredentialsWatchAvailable in Reconcile when handling
LedgerCredentialsPending: if the watch is unavailable, return a delayed requeue
result so pending Connectivity resources retry without CRD events; preserve the
existing NewPendingError behavior and watch-driven path when the flag is true.
| // Deletes use client.IgnoreNotFound so the helper is idempotent and safe to call | ||
| // on every reconcile while the gate stays closed (including when the resources | ||
| // were never created). The names/scopes mirror how they are provisioned in | ||
| // Reconcile: the delegated Connectivity is namespaced (name == namespace == | ||
| // stack name) and the GatewayHTTPAPI is cluster-scoped ("<stack>-connectivity"). | ||
| func teardownDelegated(ctx Context, stack *v1beta1.Stack, connectivity *v1beta1.Connectivity) error { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale name description in the teardown doc comment.
The comment states the delegated Connectivity has name == namespace == stack name. The code sets the name to connectivityDelegatedName ("connectivity") and only the namespace to the stack name.
📝 Proposed doc fix
-// were never created). The names/scopes mirror how they are provisioned in
-// Reconcile: the delegated Connectivity is namespaced (name == namespace ==
-// stack name) and the GatewayHTTPAPI is cluster-scoped ("<stack>-connectivity").
+// were never created). The names/scopes mirror how they are provisioned in
+// Reconcile: the delegated Connectivity is namespaced in the stack namespace
+// with the fixed name "connectivity", and the GatewayHTTPAPI is cluster-scoped
+// ("<stack>-connectivity").📝 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.
| // Deletes use client.IgnoreNotFound so the helper is idempotent and safe to call | |
| // on every reconcile while the gate stays closed (including when the resources | |
| // were never created). The names/scopes mirror how they are provisioned in | |
| // Reconcile: the delegated Connectivity is namespaced (name == namespace == | |
| // stack name) and the GatewayHTTPAPI is cluster-scoped ("<stack>-connectivity"). | |
| func teardownDelegated(ctx Context, stack *v1beta1.Stack, connectivity *v1beta1.Connectivity) error { | |
| // Deletes use client.IgnoreNotFound so the helper is idempotent and safe to call | |
| // on every reconcile while the gate stays closed (including when the resources | |
| // were never created). The names/scopes mirror how they are provisioned in | |
| // Reconcile: the delegated Connectivity is namespaced in the stack namespace | |
| // with the fixed name "connectivity", and the GatewayHTTPAPI is cluster-scoped | |
| // ("<stack>-connectivity"). | |
| func teardownDelegated(ctx Context, stack *v1beta1.Stack, connectivity *v1beta1.Connectivity) error { |
🤖 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 `@internal/resources/connectivities/init.go` around lines 316 - 321, Update the
doc comment for teardownDelegated to state that the delegated Connectivity uses
connectivityDelegatedName ("connectivity") as its name and the stack name as its
namespace; retain the existing GatewayHTTPAPI naming and scoping description.
3 participants
What
Adds a new
formance.com/v1beta1 Connectivitystack module that binds connectivity to the stack's ledger, mirroring the Ledger v3 delegation pattern.The module doesn't run the workload itself — it delegates to the connectivity operator (
connectivity.formance.com) and reflects its readiness, exactly like theLedgermodule delegates to the ledger operator (ledger.formance.com Cluster).Behaviour
Capability + API-group detection (same mechanism as Ledger v3): at controller start-up it lists CRDs, checks the
connectivity.formance.com/ConnectivityCRD is present with a served version, and runs aSelfSubjectAccessReviewfor each required verb. If the group/CRD/RBAC is missing it reports the capability as unavailable and stays pending — it never fails controller setup.Gates on Ledger v3: connectivity ingests into the Ledger v3 gRPC endpoint, so it only provisions once the stack's
Ledgermodule is v3 and ready.Binds to the stack ledger: provisions a
connectivity.formance.com/v1alpha1 ConnectivitywithledgerAddress= the ledger v3 gRPC service, andledgerTLS= the ledger backend TLS secret (CA + SNI).The connection details come from
ledgers.V3GRPCBackendRef— the single source of truth already used to reach the ledger over gRPC — so connectivity and the gateway stay in sync (no duplicated address/secret naming).Reflects the delegated resource's
status.phaseback onto the module's Ready condition.Files
api/formance.com/v1beta1/connectivity_types.go— new module CR (Connectivity, labelledformance.com/kind=module).internal/resources/connectivities/init.go— reconciler, capability detection, ledger-v3 gate, delegated-resource bind.internal/resources/connectivities/init_test.go— unit tests for the capability-gating paths (discovery failure, inaccessible resource, missing RBAC, capability-unavailable reconcile).internal/resources/ledgers/exports.go— exportsIsV3+V3GRPCBackendRefso the ledger v3 gRPC connection stays the single source of truth.internal/resources/all.go— registers the module.Test
go build ./...✅go vet ./internal/resources/connectivities/...✅go test ./internal/resources/connectivities/...✅helm template ./helm/crdsrenders ✅Follow-ups (out of scope)