feat(provider): add devolutions server provider support#5712
feat(provider): add devolutions server provider support#5712Skarlso merged 3 commits intoexternal-secrets:mainfrom
Conversation
|
Please update the golang.org/x/crypto library to 0.46.0 :) 🙇 |
1e4c6b4 to
4accffa
Compare
|
I had forgotten to run |
|
@Skarlso is anything required on our end? |
|
No. We will, eventually, get to this. :) Please be patient. :) |
✅ Actions performedReview triggered.
|
WalkthroughAdds Devolutions Server (DVLS) support across the project: new API types and deepcopy logic, CRD/schema and bundle updates, DVLS provider implementation and registration, auth/client logic (get/push/delete/exists), module changes, tests, docs, and example snippets. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
config/crds/bases/external-secrets.io_clustersecretstores.yaml (1)
1958-2038: DVLS provider schema looks consistent and ready to shipThe
dvlsprovider block is structurally in line with other providers:
auth.secretRef.appId/appSecretuse the standardSecretKeySelectorshape.auth.secretRefandserverUrlare correctly marked as required.- Naming (
serverUrl, descriptions usingServerURL) matches existing patterns likechef.serverUrl.No changes are required here. If, in future, DVLS needs custom CA handling for on‑prem/self‑signed deployments, you can extend this schema with
caBundle/caProviderfields similar to other TLS‑enabled providers, but that can be a follow‑up and isn’t a blocker for this PR.apis/externalsecrets/v1/secretstore_dvls_types.go (1)
23-32: Consider adding URL format validation to ServerURL.The ServerURL field is marked as required, which is good. Consider adding a kubebuilder format validation to ensure it's a valid URL:
// ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). // +kubebuilder:validation:Required // +kubebuilder:validation:Format=uri ServerURL string `json:"serverUrl"`This would provide early validation feedback to users if they provide an invalid URL format.
deploy/crds/bundle.yaml (1)
3981-3983: Consider adding basic URL validation forserverUrl.The
serverUrlfield accepts any string without format constraints. While CRD schema validation doesn't support native URL format validation, consider adding:
- A
maxLengthconstraint to prevent excessively long values (consistent with other string fields in this schema)- Optionally, a pattern to enforce a basic URL structure (e.g.,
^https?://)This is a minor hardening suggestion; the provider code likely validates this at runtime anyway.
🔎 Suggested improvement
serverUrl: description: ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). + maxLength: 1024 + pattern: ^https?:// type: stringproviders/v1/dvls/client.go (1)
158-166: DeleteSecret lacks timeout protection and ignores context.Other DVLS operations (
GetSecret,PushSecret,SecretExists) use timeout wrappers, butDeleteSecretcallsDeleteByIDdirectly without timeout protection or context propagation. This inconsistency could cause the operation to hang indefinitely if the DVLS server is unresponsive.🔎 Proposed fix to add timeout protection
// DeleteSecret deletes a secret from DVLS. -func (c *Client) DeleteSecret(_ context.Context, ref esv1.PushSecretRemoteRef) error { +func (c *Client) DeleteSecret(ctx context.Context, ref esv1.PushSecretRemoteRef) error { vaultID, entryID, err := c.parseSecretRef(ref.GetRemoteKey()) if err != nil { return err } - return c.dvls.DeleteByID(vaultID, entryID) + return runWithTimeoutErr(ctx, c.timeout, "delete entry", func() error { + return c.dvls.DeleteByID(vaultID, entryID) + }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧬 Code graph analysis (6)
apis/externalsecrets/v1/secretstore_types.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-32)
providers/v1/dvls/client_test.go (4)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)apis/generators/v1alpha1/types_password.go (1)
Password(77-82)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)
providers/v1/dvls/auth.go (2)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-32)
pkg/register/dvls.go (1)
providers/v1/dvls/provider.go (3)
NewProvider(127-129)ProviderSpec(132-136)MaintenanceStatus(139-141)
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSAuth(35-39)DVLSAuthSecretRef(42-50)DVLSProvider(24-32)
providers/v1/dvls/client.go (3)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/keepersecurity/client.go (1)
Secret(105-111)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)
🪛 Gitleaks (8.30.0)
providers/v1/dvls/client_test.go
[high] 266-278: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 YAMLlint (1.37.1)
docs/snippets/dvls-external-secret.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (55)
go.mod (3)
35-35: LGTM!The replace directive for the DVLS provider is correctly placed and follows the established pattern for provider modules in this monorepo.
101-101: LGTM!The golang.org/x/* dependency updates are consistent with the maintainer's request to update golang.org/x/crypto to v0.46.0. These indirect dependency bumps align with the module graph requirements.
Also applies to: 381-381, 479-486
198-198: No action required. The go-dvls library at v0.14.0 is the latest stable release and has no known security vulnerabilities.providers/v1/dvls/go.mod (1)
91-94: LGTM!The replace directives correctly map the APIs and runtime modules to their local paths, following the standard pattern for provider modules in this monorepo.
docs/introduction/stability-support.md (2)
98-98: LGTM!The Devolutions Server entry in the Provider Stability table is correctly formatted, appropriately marked as alpha stability, and positioned alphabetically after Barbican.
140-140: LGTM!The Devolutions Server entry in the Provider Feature Support table correctly indicates support for store validation and push secret, which is appropriate for an alpha-stage provider.
pkg/register/dvls.go (1)
1-30: LGTM!The DVLS provider registration file correctly follows the established pattern with appropriate build tags and uses the standard registration mechanism via the init function. The code properly references the provider's NewProvider(), ProviderSpec(), and MaintenanceStatus() functions.
docs/snippets/dvls-secret-store.yaml (1)
1-17: LGTM!The DVLS SecretStore example provides a clear and valid configuration demonstrating the serverUrl and authentication setup using secretRef. The use of HTTPS for serverUrl is a good security practice.
hack/api-docs/mkdocs.yml (1)
163-163: LGTM!The Devolutions Server documentation entry is correctly added to the navigation with the proper formatting and documentation path.
apis/externalsecrets/v1/secretstore_types.go (1)
203-205: LGTM! DVLS provider field follows established patterns.The addition of the DVLS provider field is consistent with other providers in the struct. The comment, JSON tag, and optional marker are all correctly applied.
docs/snippets/dvls-push-secret.yaml (1)
16-17: The remoteKey format'vault-uuid/entry-uuid'is correct and matches the DVLS provider implementation. According to the official documentation (docs/provider/devolutions-server.md), secrets are referenced using the format<vault-id>/<entry-id>, where vault-id is the UUID of the vault and entry-id is the UUID of the credential entry. The PushSecret type definition confirmsremoteKeyis the correct field name for this reference.tests/__snapshot__/secretstore-v1.yaml (1)
300-311: LGTM! Snapshot structure aligns with DVLS provider types.The new DVLS configuration block follows the established pattern for provider snapshots and correctly mirrors the DVLSProvider, DVLSAuth, and DVLSAuthSecretRef types defined in the API.
docs/provider/devolutions-server.md (1)
1-117: Excellent documentation with clear guidance and limitations.The documentation is comprehensive and well-structured, covering authentication, setup, examples, and troubleshooting. The limitations section at lines 93-98 is particularly valuable for users.
tests/__snapshot__/clustersecretstore-v1.yaml (1)
300-311: LGTM! ClusterSecretStore snapshot correctly mirrors SecretStore structure.The DVLS configuration is consistent with the SecretStore snapshot and appropriately includes namespace fields for cluster-scoped secret references.
docs/snippets/dvls-external-secret.yaml (1)
1-61: Good example coverage; YAMLlint error is expected.The three ExternalSecret examples effectively demonstrate different DVLS usage patterns. The YAMLlint syntax error on line 1 is expected since
{% raw %}is template syntax for documentation generation, not standard YAML.apis/externalsecrets/v1/zz_generated.deepcopy.go (2)
1263-1310: LGTM! Auto-generated DeepCopy methods follow standard patterns.The generated DeepCopy implementations for DVLSAuth, DVLSAuthSecretRef, and DVLSProvider correctly handle nested structures and nil checks as expected.
3546-3550: LGTM! DVLS field properly integrated into SecretStoreProvider.The DVLS field is correctly deep-copied within SecretStoreProvider.DeepCopyInto, following the same pattern as other provider types.
providers/v1/dvls/auth_test.go (4)
32-57: LGTM! Success case properly validates secret retrieval.The test correctly sets up a fake client with a secret and validates that getSecretValue returns the expected app ID.
59-73: LGTM! Error handling validated for missing secrets.The test correctly verifies that getSecretValue returns an appropriate error when the secret doesn't exist.
75-99: LGTM! Key not found scenario properly tested.The test validates that an error is returned with a descriptive message when the requested key doesn't exist in the secret.
101-127: LGTM! Namespace override behavior verified.The test correctly validates that when a selector specifies a namespace, the secret is retrieved from that namespace rather than the default, which is critical for ClusterSecretStore functionality.
providers/v1/dvls/provider_test.go (10)
40-71: LGTM! Valid configuration test case.The test correctly validates a properly configured SecretStore with all required DVLS fields.
73-103: LGTM! ClusterSecretStore namespace validation works correctly.The test properly validates that ClusterSecretStore requires namespace in secret references, which is a critical security boundary.
105-137: LGTM! ClusterSecretStore succeeds with explicit namespace.The test confirms that when namespace is provided in secret references, ClusterSecretStore validation passes.
139-150: LGTM! Nil provider check validated.The test ensures appropriate error handling when DVLS provider configuration is missing.
152-181: LGTM! Required serverUrl field validated.The test confirms that an empty serverUrl is properly caught during validation.
183-212: LGTM! AppID key validation works correctly.The test ensures that missing appId key is caught with a descriptive error message.
214-243: LGTM! AppSecret name validation works correctly.The test validates that missing appSecret name is properly detected.
245-276: LGTM! Cross-namespace reference validation for AppID.The test correctly validates that SecretStore (namespace-scoped) cannot reference secrets in other namespaces without being a ClusterSecretStore.
278-309: LGTM! Cross-namespace reference validation for AppSecret.The test validates the same cross-namespace constraint for AppSecret, ensuring consistency with AppID validation.
312-333: LGTM! Provider metadata tests validated.The tests confirm that Capabilities returns ReadWrite, NewProvider returns the correct type, ProviderSpec is populated, and MaintenanceStatus indicates maintained status.
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
34-50: LGTM! Authentication types properly structured.The DVLSAuth and DVLSAuthSecretRef types are well-designed with appropriate validation annotations. Using SecretKeySelector from esmeta provides consistent secret reference handling across providers.
config/crds/bases/external-secrets.io_secretstores.yaml (1)
1958-2038: DVLS provider schema looks consistent and correct
dvlsis wired intospec.providerin line with other providers, withauth.secretRef.appId/appSecretandserverUrlcorrectly modeled and marked as required. The nestedSecretKeySelectorshapes (key/name/namespace) match the rest of this CRD, so the schema should serialize/deserialize cleanly against the new DVLS Go types.providers/v1/dvls/auth.go (2)
33-51: LGTM! Clean authentication flow with proper error handling.The function correctly retrieves credentials from Kubernetes secrets and constructs the DVLS client. Error messages are context-rich and helpful.
53-74: LGTM! Proper secret retrieval with namespace override support.The function correctly handles the optional namespace override (lines 60-62) and provides clear error messages for both secret retrieval and missing key scenarios.
Note: The unused
storeKindparameter (line 53) is marked with_, which is appropriate for maintaining interface consistency with other providers in the ESO codebase.providers/v1/dvls/client_test.go (5)
34-98: LGTM! Well-designed test infrastructure.The mock credential client provides flexible error injection capabilities, and the test stubs properly implement the required interfaces for comprehensive testing.
100-150: LGTM! Comprehensive parsing and validation tests.The test suite thoroughly covers valid inputs, invalid formats, and edge cases like spaces and empty components. The validation and initialization tests are appropriate.
152-319: LGTM! Excellent coverage of credential type mappings.The tests comprehensively cover all supported credential subtypes (Default, AccessCode, ApiKey, AzureServicePrincipal, ConnectionString, PrivateKey), including edge cases like unsupported types and partial data. The assertions correctly verify field mappings and the omission of empty fields.
257-281: Static analysis false positive: test data, not a security issue.The Gitleaks tool flagged lines 266-278 as containing a private key. This is test data (a truncated private key string literal used in the PrivateKey credential type test) and does not represent a security concern. Test fixtures commonly include sample cryptographic material.
321-447: LGTM! Thorough CRUD operation tests.The test suite provides excellent coverage of:
- GetSecret and GetSecretMap with not-found and success scenarios
- SecretExists with present/absent cases and error propagation
- DeleteSecret verification
- PushSecret for multiple subtypes, not-found errors, and unsupported subtype handling
All test logic is correct and assertions are appropriate.
deploy/crds/bundle.yaml (2)
3913-3987: Schema structure looks correct and follows project conventions.The DVLS provider schema definition:
- Properly defines required fields (
auth,serverUrl,secretRef,appId,appSecret)- Uses consistent validation patterns for Kubernetes resource references (name, namespace, key constraints)
- Aligns with the standard
SecretKeySelectorpattern used by other providers in the project
15612-15686: Consistent schema definition across CRD types.This schema block is identical to the one added earlier in the bundle, which is correct—both SecretStore and ClusterSecretStore should share the same DVLS provider configuration structure.
providers/v1/dvls/provider.go (4)
1-36: LGTM on the license, imports, and Provider struct.The structure follows ESO provider conventions with proper interface compliance check via
var _ esv1.Provider = &Provider{}.
37-52: LGTM on NewClient implementation.The method correctly extracts the provider configuration, gets the store kind for namespace resolution, creates the DVLS client, and wraps it in the SecretsClient interface.
54-80: LGTM on the rest of ValidateStore.The validation correctly checks for empty ServerURL, validates the URL scheme, and validates auth secret references.
82-141: LGTM on Capabilities and helper/factory functions.
Capabilitiescorrectly returnsSecretStoreReadWrite.validateAuthSecretRefandrequireSecretSelectorprovide clear validation with descriptive error messages.getDVLSProvidersafely extracts the provider configuration.- Factory functions follow the ESO registration pattern.
providers/v1/dvls/client.go (9)
1-51: LGTM on the package setup, constants, and interface definitions.The
credentialClientinterface provides a clean abstraction over the DVLS library, enabling easier testing and mocking.
53-75: LGTM on realCredentialClient adapter and NewClient constructor.The adapter correctly delegates to the DVLS library methods, and the constructor initializes with a sensible 15-second default timeout.
77-133: LGTM on GetSecret, GetSecretMap, and GetAllSecrets.
- Proper key parsing and timeout handling.
- Correct use of
esv1.NoSecretErrfor not-found cases.- Property extraction logic handles both specific property requests and full JSON marshaling.
GetAllSecretsexplicitly returns not implemented, which is appropriate for DVLS limitations.
135-156: PushSecret only updates existing entries.The implementation returns an error if the entry is not found (lines 148-150), meaning it cannot create new entries. This may be intentional given DVLS API constraints, but it's worth confirming this aligns with the expected behavior documented in the provider docs.
168-197: LGTM on SecretExists, Validate, and Close.
SecretExistscorrectly interprets not-found errors asfalsewithout error.Validatechecks for nil client initialization.Closeis appropriately a no-op since there's no persistent connection to clean up.
199-233: LGTM on parseSecretRef and entryToSecretMap helpers.
parseSecretRefproperly validates the key format and trims whitespace.entryToSecretMapcorrectly converts string values to byte slices.
235-250: LGTM on extractPushValue.Clear validation with descriptive error messages for missing key and empty values.
270-299: Goroutine may leak on timeout, but this is acceptable.The
runWithTimeoutpattern spawns a goroutine that continues executing even after the context times out. The result is discarded but the goroutine eventually completes. This is a common trade-off when the underlying library doesn't support context cancellation natively. Just be aware that under sustained timeouts, goroutines may accumulate temporarily.
301-316: Error detection fallback is fragile but pragmatic.The string-based checks for "404" and "not found" (lines 311-315) are brittle, but they serve as a reasonable fallback when the DVLS library doesn't expose a typed not-found error. The primary check via
dvls.IsNotFound(err)should handle most cases.
|
My apologies, I just wanted to validate if there was a blocker on our end :) I have fixed the 2 issues from CodeRabbit. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
config/crds/bases/external-secrets.io_clustersecretstores.yaml (1)
1958-2038: DVLS provider schema looks consistent and correctThe
dvlsblock follows the existing provider patterns: auth is properly nested,appId/appSecretuse the standard SecretKeySelector-style schema (key/name/namespace + validations), andrequiredchains (secretRef→auth→dvls) are wired correctly. I don’t see any schema or validation issues here.If you later find users need custom CA handling for DVLS (similar to
caBundle/caProvideron other providers), that could be added as optional fields without breaking this schema. For this PR, this looks good to ship.providers/v1/dvls/client.go (1)
301-315: String-based fallback may match unrelated errors.The primary check using
dvls.IsNotFound(err)is correct. However, the fallback string matching on line 315 ("404"or"not found") could potentially match unrelated errors. Consider limiting the fallback to just theRequestErrortype check, or verify this broader matching is needed based on actual DVLS error behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- hack/api-docs/mkdocs.yml
- providers/v1/dvls/provider_test.go
- docs/introduction/stability-support.md
- apis/externalsecrets/v1/zz_generated.deepcopy.go
- tests/snapshot/clustersecretstore-v1.yaml
- providers/v1/dvls/auth_test.go
- config/crds/bases/external-secrets.io_secretstores.yaml
- docs/provider/devolutions-server.md
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧬 Code graph analysis (5)
apis/externalsecrets/v1/secretstore_types.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-32)
pkg/register/dvls.go (1)
providers/v1/dvls/provider.go (3)
NewProvider(130-132)ProviderSpec(135-139)MaintenanceStatus(142-144)
providers/v1/dvls/auth.go (2)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-32)
providers/v1/dvls/client_test.go (4)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)apis/generators/v1alpha1/types_password.go (1)
Password(77-82)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)
providers/v1/dvls/client.go (1)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)
🪛 Gitleaks (8.30.0)
providers/v1/dvls/client_test.go
[high] 266-278: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 YAMLlint (1.37.1)
docs/snippets/dvls-external-secret.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (31)
apis/externalsecrets/v1/secretstore_types.go (1)
203-206: LGTM! Clean provider integration.The DVLS provider field follows the established pattern used by all other providers in this file. The comment, optional marker, and JSON tag are consistent with conventions.
docs/snippets/dvls-secret-store.yaml (1)
1-17: LGTM! Clear and correct example.The SecretStore configuration correctly demonstrates DVLS provider setup with proper field names matching the Go struct JSON tags and appropriate secret references.
pkg/register/dvls.go (2)
1-1: LGTM! Correct build tag for conditional compilation.The build tag allows the DVLS provider to be conditionally compiled, enabling flexible binary configurations.
27-30: LGTM! Provider registration follows established pattern.The registration correctly wires the DVLS provider into the External Secrets operator using the standard three-function registration pattern (NewProvider, ProviderSpec, MaintenanceStatus).
docs/snippets/dvls-push-secret.yaml (1)
1-17: LGTM! Clear PushSecret example.The PushSecret configuration correctly demonstrates writing secrets to DVLS with appropriate API version (v1alpha1), secret mapping, and the hierarchical remoteKey format using vault and entry UUIDs.
tests/__snapshot__/secretstore-v1.yaml (1)
300-311: LGTM! DVLS schema correctly integrated.The DVLS provider schema is properly structured and consistent with the type definitions introduced in the PR.
docs/snippets/dvls-external-secret.yaml (1)
1-61: LGTM! Well-structured documentation examples.The three examples effectively demonstrate different DVLS usage patterns (single property fetch, dataFrom extract, and secret entry access). The YAMLlint syntax error is a false positive—the
{% raw %}and{% endraw %}tags are intentional Liquid/Jekyll template markers for documentation generation.providers/v1/dvls/auth.go (1)
34-51: LGTM! Clean authentication flow.The function properly retrieves credentials from Kubernetes secrets, initializes the DVLS client, and wraps the credential client interface. Error handling is appropriate with context-enriched messages.
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
23-50: LGTM! Well-structured type definitions.The DVLS provider types are cleanly defined with proper kubebuilder validation annotations and JSON tags. The use of the existing
esmeta.SecretKeySelectortype for credentials follows established patterns in the codebase.providers/v1/dvls/go.mod (1)
1-94: LGTM! Module configuration is correct.The Go version (1.25.5) now matches the root module, addressing the previous review concern. Dependencies and replace directives are properly configured for the DVLS provider module.
go.mod (1)
35-35: LGTM! DVLS provider properly integrated.The DVLS provider is correctly wired into the root module with appropriate replace directives and dependency declarations. The go-dvls library (v0.14.0) is properly included as an indirect dependency.
Also applies to: 149-149, 198-198
providers/v1/dvls/client_test.go (6)
1-32: LGTM!License header and imports are properly structured. The test file correctly imports the necessary testing utilities and DVLS dependencies.
34-79: Well-designed mock for testing DVLS client behavior.The mock correctly simulates DVLS API responses including 404 errors via
dvls.RequestError, and tracks operations for assertion purposes.
100-134: Good coverage of secret reference parsing.Tests cover the key edge cases: valid format, missing separator, empty vault/entry IDs, and whitespace handling.
257-281: Test data for private key mapping is appropriate.The static analysis tool flagged this as a potential secret leak, but this is a false positive — the key is clearly truncated test data (
MIIE...) used solely to verify that theentryToSecretMapfunction correctly maps private key fields. Using representative test data here is necessary for comprehensive test coverage.
321-350: Good test coverage for GetSecret operations.Tests properly validate the not-found error mapping to
esv1.NoSecretErrand successful secret retrieval with property extraction.
380-447: Comprehensive PushSecret test coverage.Tests cover the key scenarios: updating Default and AccessCode subtypes, handling not-found entries, and rejecting unsupported subtypes. The assertions verify both the error conditions and the actual data mutations.
providers/v1/dvls/provider.go (5)
32-35: LGTM!Good use of compile-time interface compliance check with
var _ esv1.Provider = &Provider{}.
37-52: Clean client creation with proper error handling.The method correctly extracts the provider configuration, creates the underlying DVLS client, and wraps it in the ESO client abstraction.
54-83: Thorough URL and auth validation.The URL validation correctly handles all cases:
- Empty URL check
- Parse error with proper wrapping
- Missing scheme/host without nil-error wrapping (addressing previous review feedback)
- Scheme type validation (http/https only)
Auth validation ensures both
appIdandappSecretare properly configured.
90-118: Well-structured validation helpers.The
validateAuthSecretReffunction properly validates both credentials with clear error messages, andrequireSecretSelectorprovides reusable validation for secret references.
129-144: Factory functions follow ESO provider registration patterns.The
NewProvider,ProviderSpec, andMaintenanceStatusfunctions provide the expected interface for provider registration in the External Secrets ecosystem.deploy/crds/bundle.yaml (2)
3913-3987: LGTM! DVLS provider schema is well-structured.The schema follows Kubernetes CRD conventions and is consistent with other provider configurations in this bundle. Required fields are properly marked, validation patterns are appropriate, and descriptions are clear.
15654-15728: LGTM! Schema is consistent across CRD definitions.This DVLS provider schema is identical to the one at lines 3913-3987, which is expected and correct for maintaining consistency across different CRD types (e.g., SecretStore and ClusterSecretStore).
providers/v1/dvls/client.go (7)
39-75: Well-structured client design with interface abstraction.Good use of the
credentialClientinterface for testability and the compile-time interface assertion. The separation of concerns is clean.
77-110: LGTM!Proper handling of not-found errors with
esv1.NoSecretErr, clean property extraction, and sensible JSON fallback when no property is specified.
112-133: LGTM!
GetSecretMapis consistent withGetSecret. The "not implemented" return forGetAllSecretsis acceptable given DVLS API constraints.
168-197: LGTM!
SecretExistscorrectly returns(false, nil)for not-found cases.ValidateandCloseimplementations are appropriate for this client.
199-233: LGTM!
parseSecretRefcorrectly usesSplitNwith limit 2 to allow entry IDs containing slashes. The whitespace trimming and empty-value validation are good defensive measures.
235-250: LGTM!Thorough validation with clear, contextual error messages.
270-292: Timeout implementation is acceptable given library constraints.The goroutine continues running after context timeout because the
go-dvlslibrary'sGetByIdandUpdatemethods do not accept context parameters for cancellation. The buffered channel (size 1) properly prevents goroutine leaks by allowing the result to be written even when the caller has already returned. This is the correct pattern for wrapping non-context-aware operations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
providers/v1/dvls/client.go (1)
158-168: Timeout handling correctly implemented.The previous concern about missing timeout protection has been addressed. DeleteSecret now properly wraps the DVLS API call with timeout handling consistent with other methods.
🧹 Nitpick comments (2)
providers/v1/dvls/auth_test.go (1)
33-122: Test coverage for cross-namespace security is good, with a minor assertion improvement opportunity.The test properly validates the security boundary between SecretStore and ClusterSecretStore regarding cross-namespace secret access.
🔎 Optional: Simplify success case assertions
Lines 116-119 check if an error (when present) doesn't contain a specific message, even in success scenarios. Consider using a direct assertion instead:
} else { - if err != nil { - assert.NotContains(t, err.Error(), "cannot get Kubernetes secret") - } + assert.NoError(t, err) }This makes the test's intent clearer: when
expectErroris false, there should be no error at all.deploy/crds/bundle.yaml (1)
3981-3983: Consider adding URL format validation to serverUrl.The
serverUrlfield accepts any string without format validation. Addingformat: uriwould provide client-side validation and improve the user experience by catching configuration errors earlier.🔎 Suggested enhancement
serverUrl: description: ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). type: string + format: uri
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- docs/introduction/stability-support.md
- apis/externalsecrets/v1/secretstore_types.go
- providers/v1/dvls/go.mod
- tests/snapshot/clustersecretstore-v1.yaml
- providers/v1/dvls/provider_test.go
- apis/externalsecrets/v1/zz_generated.deepcopy.go
- docs/snippets/dvls-secret-store.yaml
- tests/snapshot/secretstore-v1.yaml
- docs/snippets/dvls-push-secret.yaml
- config/crds/bases/external-secrets.io_secretstores.yaml
- providers/v1/dvls/auth.go
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/register/dvls.go (1)
providers/v1/dvls/provider.go (3)
NewProvider(130-132)ProviderSpec(135-139)MaintenanceStatus(142-144)
providers/v1/dvls/auth_test.go (3)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSProvider(24-33)DVLSAuth(36-40)DVLSAuthSecretRef(43-51)providers/v1/vault/util/vault.go (1)
Auth(31-33)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-49)
providers/v1/dvls/provider.go (3)
providers/v1/dvls/auth.go (1)
NewDVLSClient(32-49)apis/externalsecrets/v1/secretstore_dvls_types.go (2)
DVLSAuthSecretRef(43-51)DVLSProvider(24-33)apis/externalsecrets/v1/provider_schema_maintenance.go (1)
MaintenanceStatusMaintained(29-29)
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
providers/v1/vault/util/vault.go (1)
Auth(31-33)
providers/v1/dvls/client.go (2)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/keepersecurity/client.go (1)
Secret(105-111)
providers/v1/dvls/client_test.go (1)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)
🪛 Gitleaks (8.30.0)
providers/v1/dvls/client_test.go
[high] 266-278: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 YAMLlint (1.37.1)
docs/snippets/dvls-external-secret.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (39)
hack/api-docs/mkdocs.yml (1)
163-163: LGTM! Documentation navigation entry added correctly.The Devolutions Server provider documentation link is properly positioned in alphabetical order within the Provider section.
providers/v1/dvls/client.go (13)
33-37: LGTM! Constants are well-defined.The default timeout of 15 seconds is appropriate for DVLS API operations, and error message constants improve consistency.
39-75: LGTM! Clean abstraction design.The
credentialClientinterface andrealCredentialClientadapter provide good separation for testing, and the constructor properly initializes the default timeout.
77-110: LGTM! GetSecret implementation is robust.The method correctly handles secret retrieval with timeout protection, proper not-found error handling, and supports both property-specific and full secret extraction.
112-128: LGTM! GetSecretMap follows the same robust pattern.Consistent implementation with proper timeout and error handling.
130-133: GetAllSecrets appropriately marked as not implemented.This aligns with the documented limitations in the provider documentation. The error message is clear.
170-186: LGTM! SecretExists implementation is correct.The method properly distinguishes between "not found" (returns false, nil) and actual errors (returns false, err).
188-199: LGTM! Lifecycle methods are appropriate.Validate performs a simple but necessary initialization check, and Close being a no-op is acceptable if the underlying DVLS client doesn't require explicit cleanup.
201-220: LGTM! parseSecretRef is robust.The parsing logic properly validates the key format, trims whitespace, and provides clear error messages for invalid inputs.
222-235: LGTM! entryToSecretMap conversion is straightforward.The method properly converts DVLS credential map strings to byte arrays expected by Kubernetes secrets.
237-252: LGTM! extractPushValue validation is thorough.The function properly validates that the secret key is specified, exists in the Kubernetes secret, and contains a non-empty value.
254-270: LGTM! Entry operation helpers properly use timeouts.Both methods correctly wrap DVLS API calls with timeout protection. The value copy at line 261 is appropriate to avoid mutating the input entry.
272-301: LGTM! Generic timeout helpers are well-designed.The implementation properly uses goroutines with buffered channels to prevent leaks, handles context cancellation, and provides type-safe wrappers for both value-returning and error-only functions.
303-314: LGTM! isNotFoundError provides robust detection.The dual check using both
dvls.IsNotFound()and HTTP 404 status ensures reliable not-found error detection across different DVLS client responses.go.mod (2)
35-35: LGTM! DVLS provider module correctly integrated.The provider replacement, requirement declaration, and go-dvls dependency are properly added. The module structure follows the existing pattern for other providers.
Also applies to: 149-149, 198-198
101-101: LGTM! Dependency updates align with maintainer feedback.The golang.org/x dependencies have been updated to newer versions, including golang.org/x/crypto v0.46.0 as requested by the maintainer in PR comments.
Also applies to: 381-381, 479-483, 485-485
pkg/register/dvls.go (1)
1-30: LGTM! Provider registration follows established patterns.The build tag, registration call, and structure match the existing provider registration approach. The provider is properly initialized during package init.
docs/provider/devolutions-server.md (1)
1-117: LGTM! Comprehensive provider documentation.The documentation thoroughly covers authentication setup, configuration, credential types, usage examples, and limitations. Well-structured and informative.
Note: A potential inconsistency between the documentation's claim that "Push secret creates or updates" (line 91) and the actual implementation behavior was flagged in the client.go review.
docs/snippets/dvls-external-secret.yaml (1)
1-61: LGTM! Example snippets are clear and demonstrate key use cases.The three examples effectively illustrate:
- Fetching specific properties
- Extracting all fields with dataFrom
- Working with Secret entry types
The YAMLlint syntax error on line 1 is a false positive;
{% raw %}is valid mkdocs template syntax for including literal YAML in documentation.apis/externalsecrets/v1/secretstore_dvls_types.go (4)
17-21: LGTM! Clean package structure.The package declaration and imports are minimal and appropriate. Using only
esmeta.SecretKeySelectorfor credential references follows the secure pattern used across other ESO providers.
23-33: LGTM! Well-structured provider configuration.The
DVLSProvidertype follows Kubernetes API conventions:
- Clear documentation with example
Format=urivalidation on ServerURL prevents invalid URLs- Required annotations on critical fields
- JSON tags follow camelCase convention
35-40: LGTM! Auth structure supports future extensibility.The
DVLSAuthtype provides a clear grouping for authentication configuration and allows for future addition of alternative auth methods without breaking the API.
42-51: LGTM! Secure credential handling.The
DVLSAuthSecretReftype correctly usesSecretKeySelectorfor both credentials, ensuring secrets are referenced rather than embedded. This follows ESO security best practices and prevents credential exposure in the API specifications.deploy/crds/bundle.yaml (2)
15654-15728: Schema duplication is appropriate.This second
dvlsschema block is identical to the first (lines 3913-3987), which is expected for a CRD bundle that defines the schema across multiple resource types. The same validation and enhancement suggestions from the first block apply here.
3922-3947: The schema structure is correct as designed. TheappIdandappSecretfields at thesecretReflevel are properly marked as required, which ensures users must provide both secret references. Within those objects, thekey,name, andnamespaceproperties intentionally do not have a required array because this pattern is consistent across 426+ SecretKeySelector definitions throughout the bundle—allowing flexibility where some fields may be defaulted by the provider while others require explicit configuration. This is documented in the field description: "Some instances of this field may be defaulted, in others it may be required."Likely an incorrect or invalid review comment.
providers/v1/dvls/client_test.go (5)
34-79: LGTM! Clean mock implementation.The mock credential client is well-structured with proper error injection, operation tracking, and defensive nil handling in the constructor.
81-98: LGTM! Appropriate test stubs.The push secret stubs correctly implement the required interfaces with minimal viable implementations suitable for testing.
100-134: LGTM! Comprehensive parsing tests.The test cases thoroughly cover valid formats, invalid formats, empty components, and whitespace handling.
152-319: LGTM! Thorough credential mapping tests.The tests comprehensively cover all supported credential subtypes (Default, AccessCode, ApiKey, AzureServicePrincipal, ConnectionString, PrivateKey) including error cases (unsupported types) and edge cases (partial data). The verification that empty fields are excluded from the secret map (lines 314-317) is particularly good.
Note: The static analysis tool flagged line 266 as containing a private key, but this is a false positive—it's a test fixture string literal, not an actual secret.
321-447: LGTM! Solid CRUD operation tests.The tests effectively validate:
- Secret retrieval with not-found handling and successful mapping
- Existence checks with error propagation
- Delete operations with tracking verification
- Push operations across multiple subtypes with proper error handling for not-found and unsupported scenarios
providers/v1/dvls/provider.go (8)
32-52: LGTM! Standard provider implementation.The
NewClientmethod follows the External Secrets provider pattern correctly: extracts configuration, obtains store metadata, creates the underlying client, and wraps it appropriately.
54-83: LGTM! Proper validation logic.The validation correctly handles:
- Empty ServerURL check
- URL parsing errors (separate from scheme/host validation)
- Scheme/host presence validation
- Scheme restriction to http/https
- Authentication reference validation
The previous issue with error wrapping when parsing succeeds but scheme/host are empty has been properly addressed.
85-88: LGTM! Correct capability declaration.The provider correctly declares
SecretStoreReadWritecapability, which aligns with the implementation of both read operations (GetSecret, GetSecretMap) and write operations (PushSecret, DeleteSecret) in the client.
90-106: LGTM! Thorough authentication validation.The function properly validates both
appIdandappSecretreferences by checking required fields and delegating to the standardesutils.ValidateSecretSelectorfor consistency with other providers.
108-118: LGTM! Clear validation helper.The helper ensures both
NameandKeyare present in the secret selector with contextual error messages.
120-127: LGTM! Defensive configuration extraction.The function correctly validates the configuration hierarchy with appropriate nil checks at each level.
129-132: LGTM! Standard factory function.
134-144: LGTM! Proper registration metadata.Both functions correctly provide the metadata needed for provider registration:
ProviderSpecreturns the schema structure for CRD generationMaintenanceStatusappropriately declares the provider as maintainedconfig/crds/bases/external-secrets.io_clustersecretstores.yaml (1)
1958-2038: DVLS provider schema is consistent and correctly wired into ClusterSecretStore v1.The dvls block mirrors existing provider patterns: nested
auth.secretRefusing standard SecretKeySelectors forappIdandappSecret, andserverUrlrequired alongsideauth. Required lists and field shapes look correct, and descriptions are clear and aligned with neighboring providers. I don’t see any schema issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
providers/v1/dvls/client.go (1)
135-156: Documentation and implementation mismatch for PushSecret.The implementation requires the entry to exist beforehand (line 149 returns an error if not found), but past review comments indicate the documentation states "creates or updates."
Please align the documentation and implementation:
- Option 1 (simpler): Update the documentation to clarify that entries must exist before pushing secrets
- Option 2: Extend the implementation to create new entries when missing
Based on past review comments suggesting this issue.
🧹 Nitpick comments (3)
providers/v1/dvls/auth_test.go (1)
86-88: Error fromAddToSchemeis silently ignored.The blank identifier suppresses any error from
corev1.AddToScheme(scheme). While this typically succeeds, silently ignoring errors in tests can mask unexpected issues.🔎 Consider using require to fail fast on setup errors
+import "github.com/stretchr/testify/require" + scheme := runtime.NewScheme() -_ = corev1.AddToScheme(scheme) +require.NoError(t, corev1.AddToScheme(scheme)) kube := fake.NewClientBuilder().WithScheme(scheme).WithObjects(secret).Build()apis/externalsecrets/v1/secretstore_dvls_types.go (1)
23-32: Consider adding URL format validation to ServerURL.The
DVLSProviderstruct is well-defined and follows Kubernetes API conventions. However, adding URL format validation to theServerURLfield would help catch configuration errors earlier.🔎 Proposed addition of URL format validation
// DVLSProvider configures a store to sync secrets using Devolutions Server. type DVLSProvider struct { // ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). + // +kubebuilder:validation:Pattern=`^https?://[a-zA-Z0-9\-\.]+[a-zA-Z0-9]+(:[0-9]+)?(/.*)?$` // +kubebuilder:validation:Required ServerURL string `json:"serverUrl"` // Auth defines the authentication method to use. // +kubebuilder:validation:Required Auth DVLSAuth `json:"auth"` }Alternatively, you can use the simpler format validator:
// DVLSProvider configures a store to sync secrets using Devolutions Server. type DVLSProvider struct { // ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). + // +kubebuilder:validation:Format=uri // +kubebuilder:validation:Required ServerURL string `json:"serverUrl"` // Auth defines the authentication method to use. // +kubebuilder:validation:Required Auth DVLSAuth `json:"auth"` }deploy/crds/bundle.yaml (1)
3981-3983: Consider adding format validation for serverUrl.The
serverUrlfield currently accepts any string without format validation. Adding a URL format constraint or pattern validation would catch configuration errors earlier at apply time rather than at runtime.🔎 Example: Add format validation
serverUrl: description: ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). type: string + format: uri + pattern: ^https?://Alternatively, if
format: uriis too strict, a simple pattern ensuring the URL starts withhttp://orhttps://would be helpful.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- providers/v1/dvls/auth.go
- providers/v1/dvls/go.mod
- tests/snapshot/secretstore-v1.yaml
- apis/externalsecrets/v1/secretstore_types.go
- tests/snapshot/clustersecretstore-v1.yaml
- docs/introduction/stability-support.md
- providers/v1/dvls/provider_test.go
- docs/snippets/dvls-secret-store.yaml
- docs/snippets/dvls-push-secret.yaml
- hack/api-docs/mkdocs.yml
- docs/api/spec.md
🧰 Additional context used
🧬 Code graph analysis (4)
providers/v1/dvls/auth_test.go (2)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSProvider(24-32)DVLSAuth(35-39)DVLSAuthSecretRef(42-50)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-49)
providers/v1/dvls/client_test.go (5)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)apis/generators/v1alpha1/types_password.go (1)
Password(77-82)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)providers/v1/keepersecurity/client.go (1)
Secret(105-111)apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)
apis/externalsecrets/v1/zz_generated.deepcopy.go (2)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSAuth(35-39)DVLSAuthSecretRef(42-50)DVLSProvider(24-32)providers/v1/vault/util/vault.go (1)
Auth(31-33)
providers/v1/dvls/provider.go (3)
providers/v1/dvls/client.go (2)
NewClient(70-75)Client(42-45)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-49)apis/externalsecrets/v1/secretstore_dvls_types.go (2)
DVLSAuthSecretRef(42-50)DVLSProvider(24-32)
🪛 Gitleaks (8.30.0)
providers/v1/dvls/client_test.go
[high] 266-278: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 YAMLlint (1.37.1)
docs/snippets/dvls-external-secret.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (31)
pkg/register/dvls.go (1)
1-30: LGTM! Clean provider registration.The DVLS provider registration follows the established pattern with correct build tags and clean initialization. The
init()function properly registers the provider with all required parameters.go.mod (5)
35-35: LGTM! Correct replace directive.The DVLS provider replace directive is properly positioned alphabetically and follows the established pattern for provider modules in this monorepo.
101-101: LGTM! Addresses maintainer feedback.The golang.org/x/crypto update to v0.46.0 addresses the specific request from Skarlso mentioned in the PR comments.
149-149: LGTM! Proper provider dependency declaration.The DVLS provider is correctly added as an indirect dependency with the standard placeholder version, consistent with other local provider modules.
381-381: LGTM! Transitive dependency updates.The golang.org/x/* module updates are expected when adding a new provider and running
make reviewable, as mentioned in the PR comments. These transitive dependencies are properly managed.Also applies to: 479-485
198-198: No action required. Thegithub.com/Devolutions/go-dvls v0.14.0dependency is actively used in the DVLS provider implementation (providers/v1/dvls/) and no known security vulnerabilities were found for this version.config/crds/bases/external-secrets.io_clustersecretstores.yaml (1)
1958-2038: DVLS provider CRD schema looks consistent and completeThe
dvlsblock follows existing provider patterns (requiredauth.secretRefwithappId/appSecretselectors and a requiredserverUrl), and the SecretKeySelector shapes and constraints match the rest of the CRD. I don’t see schema or validation gaps here.apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
1263-1310: Auto-generated DeepCopy code looks correct.The generated DeepCopy implementations for
DVLSAuth,DVLSAuthSecretRef, andDVLSProviderfollow the established pattern used by other providers in this file. TheSecretStoreProvider.DeepCopyIntomethod correctly handles the optionalDVLSfield with nil-check and allocation.Also applies to: 3556-3560
docs/snippets/dvls-external-secret.yaml (1)
1-61: Documentation snippets are well-structured.The three ExternalSecret examples demonstrate key use cases for the DVLS provider. The static analysis error about
{% raw %}is a false positive—these are Jinja2 template directives required by MkDocs for proper rendering and are consistent with other snippet files in the project.docs/provider/devolutions-server.md (2)
1-117: Comprehensive provider documentation.The documentation covers all essential aspects: authentication, configuration, usage patterns, limitations, and troubleshooting. The credential type mapping table (lines 56-67) is particularly helpful for users.
14-14: Verify menu item name in DVLS UI."Applications identities" may be the exact menu label in DVLS. If not, consider correcting to "Application Identities" for grammatical consistency.
config/crds/bases/external-secrets.io_secretstores.yaml (1)
1958-2038: DVLS provider schema is consistent and well‑structuredThe dvls block mirrors existing provider patterns (auth via SecretRefs with appId/appSecret and a required serverUrl), field names and descriptions are clear, and required lists enforce the minimal config correctly. I don't see any schema or compatibility issues here.
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
1-21: LGTM! Clean structure and minimal imports.The license header, package declaration, and imports are properly structured. The single import of
esmetais necessary for theSecretKeySelectortype used in secret references.
34-39: LGTM! Good design for future extensibility.The
DVLSAuthstruct is well-designed. While it currently contains onlySecretRef, this structure provides a clean path for adding alternative authentication methods in the future (e.g., OAuth, token-based auth) without breaking changes.
41-50: LGTM! Secure credential handling using Kubernetes standards.The
DVLSAuthSecretRefstruct correctly usesesmeta.SecretKeySelectorfor bothAppIDandAppSecretfields, following Kubernetes best practices for referencing sensitive data stored in secrets. The required annotations and clear naming make the API intuitive.deploy/crds/bundle.yaml (1)
15654-15728: LGTM! Duplicate schema is expected.This hunk adds the identical DVLS provider schema as hunk 1, which is the correct approach for CRD bundle files where the same provider configuration needs to be available across multiple CRD types (e.g.,
SecretStoreandClusterSecretStore).The same suggestion about
serverUrlformat validation from the previous hunk applies here as well.providers/v1/dvls/client_test.go (3)
257-281: LGTM! Comprehensive test coverage for PrivateKey credential type.The test correctly validates all fields (username, password, private-key, public-key, passphrase) are properly mapped from the DVLS entry to the secret map.
Note: The static analysis warning about the private key on lines 266-278 is a false positive - this is test data, not an actual secret.
296-318: LGTM! Good edge case coverage for partial credentials.The test correctly validates that empty fields are not included in the secret map, which is the expected behavior.
380-447: LGTM! Good coverage of PushSecret scenarios.The tests validate:
- Successful updates for Default and AccessCode credential types
- Error handling when entry is not found
- Error handling for unsupported credential subtypes
The mock tracking correctly verifies that updates are applied.
providers/v1/dvls/provider.go (5)
37-52: LGTM! Clean client creation flow.The method correctly:
- Extracts and validates the DVLS provider configuration
- Obtains the store kind for proper secret resolution
- Creates the DVLS client with appropriate error wrapping
- Returns a wrapped SecretsClient
54-83: LGTM! ValidateStore correctly validates all required configuration.The validation logic properly:
- Checks for required serverUrl
- Separates URL parsing errors from empty scheme/host validation
- Validates allowed schemes (http/https)
- Validates authentication secret references
The previous issue with wrapping nil errors has been properly addressed.
85-88: LGTM! Capabilities correctly reflect read/write support.
90-118: LGTM! Validation helpers provide thorough secret reference validation.The functions correctly:
- Validate required fields (name, key) are present
- Use framework utilities for namespace/scope validation
- Return clear, specific error messages
120-144: LGTM! Factory and registration functions follow the provider pattern.These functions correctly implement the provider registration interface, enabling the DVLS provider to be discovered and initialized by the External Secrets operator.
providers/v1/dvls/client.go (7)
33-75: LGTM! Well-structured client with good separation of concerns.The design correctly:
- Uses interface abstraction for testability (credentialClient)
- Provides an adapter for the real DVLS client
- Sets a reasonable default timeout (15 seconds)
- Follows the External Secrets SecretsClient interface
77-128: LGTM! GetSecret and GetSecretMap correctly implement secret retrieval.Both methods properly:
- Parse the vault-id/entry-id format
- Use timeout-wrapped DVLS calls
- Map not-found errors to esv1.NoSecretErr
- Support property-specific or full secret retrieval
158-168: LGTM! DeleteSecret now correctly includes timeout handling.The method properly uses runWithTimeoutErr to ensure the delete operation cannot hang indefinitely. The previous timeout handling issue has been resolved.
170-199: LGTM! SecretExists, Validate, and Close are correctly implemented.
- SecretExists properly interprets not-found errors as false existence
- Validate ensures the client is initialized
- Close is appropriately a no-op for the DVLS client
201-220: LGTM! Robust key parsing with good validation.The method correctly:
- Splits the key into vault and entry IDs
- Trims whitespace for flexibility
- Validates both components are present
- Returns clear, descriptive error messages
222-252: LGTM! Helper functions provide robust data extraction and validation.
- entryToSecretMap correctly delegates to the library's ToCredentialMap method
- extractPushValue thoroughly validates the secret key exists and is non-empty
254-314: LGTM! Well-implemented timeout handling and error detection.The timeout wrappers correctly:
- Use context.WithTimeout for deadline enforcement
- Employ goroutine+select pattern with buffered channels to prevent leaks
- Provide generic implementations via type parameters
- Return descriptive timeout errors
The isNotFoundError function properly checks both the DVLS library's IsNotFound helper and explicit 404 status codes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/crds/bundle.yaml (1)
3981-3983: Consider adding URI format validation to serverUrl.The
serverUrlfield is defined as a plain string without format validation. Consider addingformat: urito ensure users provide valid URLs.🔎 Proposed enhancement
serverUrl: description: ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). type: string + format: uriThis would provide client-side validation and better user feedback when an invalid URL is provided.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- hack/api-docs/mkdocs.yml
- providers/v1/dvls/go.mod
- docs/snippets/dvls-secret-store.yaml
- pkg/register/dvls.go
- docs/snippets/dvls-push-secret.yaml
- apis/externalsecrets/v1/secretstore_dvls_types.go
- docs/provider/devolutions-server.md
- providers/v1/dvls/provider.go
- providers/v1/dvls/provider_test.go
- providers/v1/dvls/auth_test.go
- config/crds/bases/external-secrets.io_secretstores.yaml
- tests/snapshot/secretstore-v1.yaml
- docs/introduction/stability-support.md
- config/crds/bases/external-secrets.io_clustersecretstores.yaml
- docs/api/spec.md
- tests/snapshot/clustersecretstore-v1.yaml
🧰 Additional context used
🧬 Code graph analysis (5)
providers/v1/dvls/auth.go (3)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-32)providers/v1/vault/util/vault.go (1)
Auth(31-33)
apis/externalsecrets/v1/secretstore_types.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-32)
providers/v1/dvls/client.go (3)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/keepersecurity/client.go (1)
Secret(105-111)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSAuth(35-39)DVLSAuthSecretRef(42-50)DVLSProvider(24-32)
providers/v1/dvls/client_test.go (1)
providers/v1/dvls/client.go (2)
Client(42-45)NewClient(70-75)
🪛 Gitleaks (8.30.0)
providers/v1/dvls/client_test.go
[high] 266-278: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 YAMLlint (1.37.1)
docs/snippets/dvls-external-secret.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (19)
go.mod (2)
101-101: LGTM!The golang.org/x/* module updates are consistent and properly coordinated across the dependency graph.
Also applies to: 381-381, 479-485
198-198: No action needed — v0.14.0 is a valid and secure release.The go-dvls v0.14.0 release exists and is free from known security vulnerabilities. The version was released on December 10, 2025 with legitimate feature additions (methods to flatten credential entries and set secret values).
providers/v1/dvls/client.go (8)
33-51: LGTM!The interface abstraction and constants are well-defined. The 15-second default timeout is reasonable for API operations.
42-75: LGTM!The adapter pattern cleanly wraps the DVLS credential service, and the constructor properly initializes the client with sensible defaults.
77-110: LGTM!GetSecret properly handles timeouts, not-found scenarios, and supports both property extraction and full secret retrieval.
112-133: LGTM!GetSecretMap correctly delegates to the entry conversion logic, and GetAllSecrets appropriately returns an unimplemented error.
158-186: LGTM!DeleteSecret now includes proper timeout handling, and SecretExists correctly infers existence from API responses.
188-199: LGTM!The validation logic is appropriate, and the no-op Close implementation is acceptable for the DVLS client.
201-314: LGTM!The helper functions are well-structured with proper validation, error handling, and timeout management using goroutines and context cancellation.
135-156: The documentation atdocs/provider/devolutions-server.md(line 91) accurately states: "Push secret updates an existing entry's password field. The entry must already exist in DVLS." This aligns correctly with the implementation, which enforces this requirement by returning an error if the entry is not found. No documentation update is needed.apis/externalsecrets/v1/secretstore_types.go (1)
203-206: LGTM!The DVLS provider field is properly integrated into SecretStoreProvider, following the established pattern for other providers.
docs/snippets/dvls-external-secret.yaml (1)
1-61: LGTM!The YAML snippets provide comprehensive examples for different DVLS usage patterns. The static analysis syntax error is a false positive—the
{% raw %}tags are intentional Liquid template syntax for documentation rendering.providers/v1/dvls/auth.go (1)
31-49: LGTM!The authentication logic correctly uses
resolvers.SecretKeyRef, which properly enforces ClusterSecretStore-only cross-namespace secret access. Error handling is appropriate with clear context in error messages.providers/v1/dvls/client_test.go (3)
34-79: LGTM!The mock credential client is well-designed with proper error injection capabilities and operation tracking for test verification.
100-319: LGTM!Excellent test coverage for parseSecretRef, Validate, entryToSecretMap, and various credential subtypes. The tests handle edge cases like whitespace, empty fields, and unsupported types.
321-446: LGTM!Comprehensive integration tests covering GetSecret, GetSecretMap, SecretExists, DeleteSecret, and PushSecret operations for various scenarios including not-found cases and different credential subtypes. The static analysis warning about a private key on line 266 is a false positive—it's a truncated example key in test data.
deploy/crds/bundle.yaml (2)
3916-3980: LGTM! Authentication schema is well-designed.The authentication schema properly uses Kubernetes secret references for both Application ID and Application Secret, which is secure and follows external-secrets conventions. The required fields, validation patterns, and descriptions are all appropriate.
15654-15728: Schema duplication is expected in bundle.yaml.This hunk contains an identical DVLS provider schema definition. This duplication is normal for
bundle.yamlfiles that consolidate multiple CRD definitions (e.g., SecretStore and ClusterSecretStore).The same observations from the first hunk apply here:
- Consider adding
format: urivalidation to theserverUrlfield (line 15723).- The authentication schema is well-designed and secure.
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
1263-1277: LGTM! DVLS deepcopy implementations are correctly generated.The auto-generated deepcopy methods for DVLS types follow the established patterns consistently:
- DVLSAuth, DVLSAuthSecretRef, DVLSProvider: Each implements the standard deepcopy pattern with shallow copy followed by recursive
DeepCopyIntocalls on nested fields- SecretStoreProvider DVLS handling: Correctly implements the nil-check, allocation, and recursive deepcopy pattern used by all optional providers
- Alphabetical ordering: DVLS is properly placed between Device42 and Infisical
The implementations match the type definitions provided in the relevant code snippets and are consistent with similar providers (Device42, Barbican, etc.) throughout the file.
Also applies to: 1279-1294, 1296-1310, 3556-3560
Skarlso
left a comment
There was a problem hiding this comment.
I have a couple of questions here and there. :) Nothing big all in all. Well done.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @docs/snippets/dvls-external-secret.yaml:
- Around line 1-61: Remove the Jinja raw markers `{% raw %}` and `{% endraw %}`
that wrap the YAML document; they are invalid YAML tokens causing yamllint
failures. Edit the dvls-external-secret.yaml snippet to keep the three
Kubernetes ExternalSecret documents (the three 'apiVersion:
external-secrets.io/v1' / 'kind: ExternalSecret' blocks with their metadata
names database-credentials, api-credentials, and app-secret) and delete only the
`{% raw %}` and `{% endraw %}` lines so the file begins with '---' and ends with
the final resource without any Jinja raw tags.
In @providers/v1/dvls/auth.go:
- Around line 33-41: The code dereferences provider.Auth.SecretRef when calling
resolvers.SecretKeyRef; add explicit nil checks for provider, provider.Auth, and
provider.Auth.SecretRef at the start of the function (before using
provider.Auth.SecretRef) and return clear wrapped errors (e.g., "missing
provider auth", "missing provider auth secretRef") to avoid panics; then call
resolvers.SecretKeyRef with the validated pointers as before and update the
returned fmt.Errorf messages to use consistent, user-facing wording like "failed
to get app ID" and "failed to get app secret" for the appID/appSecret error
cases.
In @providers/v1/dvls/client.go:
- Around line 230-245: The function extractPushValue should explicitly check for
a nil secret.Data before indexing it; add an early guard in extractPushValue
that returns a clear error (including secret.Name and the expected secret key
from data.GetSecretKey()) when secret.Data == nil so callers receive a
descriptive "no data in secret" style error rather than the generic "not found"
error when attempting to access secret.Data[data.GetSecretKey()].
- Around line 126-153: PushSecret currently assumes the incoming *corev1.Secret
and its Data map are non-nil and can panic; add guards at the start of
Client.PushSecret to return a clear error if secret == nil or secret.Data == nil
(or missing the key used by extractPushValue) before calling extractPushValue,
and adjust extractPushValue callers (or the function itself) to handle absent
keys gracefully; keep existing logic for parsing remote key (parseSecretRef),
fetching entry (c.dvls.GetByID), and updating via
existingEntry.SetCredentialSecret and c.dvls.Update.
- Around line 247-258: Replace fragile string matching in isNotFoundError with a
status-code check: when casting to dvls.RequestError (reqErr), check
reqErr.StatusCode == http.StatusNotFound instead of
strings.Contains(reqErr.Error(), "404"); also ensure the net/http package is
imported so http.StatusNotFound is available. This updates the fallback path in
isNotFoundError to use the RequestError.StatusCode field for reliable not-found
detection.
🧹 Nitpick comments (6)
config/crds/bases/external-secrets.io_secretstores.yaml (1)
2032-2044: Consider tighteningserverUrlschema (URI format + optional CEL validation for https-by-default).
Right nowserverUrlis justtype: string, which is consistent with many providers but loses some cheap validation. At minimum, considerformat: uri. Ifinsecureis intended to allow plain HTTP (as the description says), consider a CEL rule to requirehttps://unlessinsecure: true.Proposed diff (URI format)
serverUrl: description: ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). + format: uri type: stringproviders/v1/dvls/auth_test.go (1)
117-122: Test approach for non-error case could be clearer.The logic here validates that Kubernetes secret retrieval succeeded by checking that specific error messages are absent, even when
err != nil. This works because the DVLS client creation will fail with the test server URL, but the goal is to verify the secret access phase passed.Consider mocking DVLS client creation for test clarity
While the current approach is functional, consider using a test interface or dependency injection to mock
dvls.NewClientso the test can:
- Explicitly verify successful secret retrieval
- Return success instead of relying on a fake server error
- Make the test's intent more obvious to future maintainers
Example pattern:
// Define interface in production code type dvlsClientFactory interface { NewClient(appID, appSecret, serverURL string) (Client, error) } // Use mock in tests to return successThis would eliminate the need to parse error messages to infer which phase succeeded.
deploy/crds/bundle.yaml (1)
3987-3989: Add validation constraints to the serverUrl field.The
serverUrlfield lacks schema-level validation (no pattern, format, or length constraints). This could allow invalid URLs to be accepted at admission time, leading to runtime errors when the provider attempts to connect.Consider adding validation to the source Go type (likely in
apis/externalsecrets/v1/secretstore_dvls_types.go) using kubebuilder markers:
- URL format validation (e.g.,
+kubebuilder:validation:Patternfor http/https URLs)- Minimum length constraint
- Consider using
+kubebuilder:validation:Format=uriif supportedExample kubebuilder markers for the source Go type
// ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:Pattern=`^https?://` ServerURL string `json:"serverUrl"`providers/v1/dvls/client.go (3)
194-213:parseSecretRef: good validation; consider rejecting keys with leading/trailing/early for clearer errors. (Optional.)
103-124: Consider using a shared error sentinel forGetAllSecretsinstead of creating a new error each call. The Senhasegura provider defineserrNotImplemented = errors.New("not implemented")and returns it directly, which is more efficient than wrapping a string constant witherrors.New()on each invocation. This pattern improves code consistency across providers.
155-179: Consider treating "not found" on delete as success for idempotency. Vault provider already implements this pattern—when a secret doesn't exist during deletion, it returns success rather than propagating the error. This aligns with standard delete semantics and makes DeleteSecret idempotent. TheisNotFoundErrorhelper already exists in this file and is used in other methods. Verify whether controllers expect this behavior and adjust accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- providers/v1/dvls/go.mod
- apis/externalsecrets/v1/secretstore_types.go
- tests/snapshot/secretstore-v1.yaml
- docs/snippets/dvls-secret-store.yaml
- hack/api-docs/mkdocs.yml
- providers/v1/dvls/provider_test.go
- providers/v1/dvls/provider.go
- docs/snippets/dvls-push-secret.yaml
- providers/v1/dvls/client_test.go
- config/crds/bases/external-secrets.io_clustersecretstores.yaml
- docs/provider/devolutions-server.md
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧬 Code graph analysis (3)
providers/v1/dvls/auth_test.go (2)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSProvider(24-38)DVLSAuth(41-45)DVLSAuthSecretRef(48-56)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-49)
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSAuth(41-45)DVLSAuthSecretRef(48-56)DVLSProvider(24-38)
providers/v1/dvls/client.go (2)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/keepersecurity/client.go (1)
Secret(105-111)
🪛 YAMLlint (1.37.1)
docs/snippets/dvls-external-secret.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (16)
config/crds/bases/external-secrets.io_secretstores.yaml (1)
1958-2044: No duplicate YAML keys detected—dvls:appears exactly once at line 1958 under the provider properties block with the correct structure. No action needed.pkg/register/dvls.go (1)
1-30: LGTM! Clean provider registration.The DVLS provider registration follows the established pattern for External Secrets providers, with appropriate build tags and standard initialization.
apis/externalsecrets/v1/secretstore_dvls_types.go (2)
23-56: Well-structured API types with proper validation.The DVLS provider types follow External Secrets conventions, with appropriate kubebuilder validation markers and clear field documentation. The use of
esmeta.SecretKeySelectorfor credentials is consistent with other providers.
29-33: Verify the Insecure field is required for legitimate use cases.The
Insecurefield enables plain HTTP connections, which introduces security risks. While the comment appropriately warns against production use, please confirm:
- Is this needed for specific development/testing scenarios or on-premise deployments without TLS?
- Are there additional safeguards or documentation to prevent accidental misuse in production?
The warning comment is helpful, but consider whether this capability should be feature-gated or accompanied by additional runtime warnings when enabled.
providers/v1/dvls/auth_test.go (1)
34-67: Well-structured cross-namespace security test.The table-driven test correctly validates that:
ClusterSecretStorecan access secrets in any namespaceSecretStoreis restricted to its own namespaceThis aligns with External Secrets security model.
go.mod (3)
35-35: LGTM! DVLS provider module properly integrated.The replace directive and require statement follow the established pattern for External Secrets provider modules.
Also applies to: 149-149
101-101: LGTM! Dependency updates applied as requested.The golang.org/x/* module updates (including crypto v0.46.0 as requested in PR comments) are appropriate.
Also applies to: 381-381, 479-485
198-198: No security vulnerabilities found forgithub.com/Devolutions/go-dvls v0.14.0in public databases. The dependency is safe to use at this version.apis/externalsecrets/v1/zz_generated.deepcopy.go (2)
1263-1310: DVLS deepcopy implementations look correct for the new API structs.
3556-3560: DVLS added toSecretStoreProvider.DeepCopyIntocorrectly (nil-check + deep copy).providers/v1/dvls/client.go (2)
31-68: Nice seam for testing viacredentialClient+realCredentialClientadapter.
70-101:GetSecret: good not-found mapping toesv1.NoSecretErr+ sensible defaultproperty=password.tests/__snapshot__/clustersecretstore-v1.yaml (1)
300-312: Snapshot addition forspec.provider.dvlsmatches expected schema shape (serverUrl + auth.secretRef).docs/introduction/stability-support.md (1)
100-100: DVLS provider documentation and table entries verified. The provider doc exists atdocs/provider/devolutions-server.mdwith complete authentication setup instructions. Both table entries (lines 100 and 142) are properly formatted and consistent with other providers.providers/v1/dvls/auth.go (2)
17-29: Clean package structure and imports.The package declaration, imports, and overall structure are well-organized. The function correctly wraps the DVLS client's credential entry in a
realCredentialClientfor the provider interface.Note: The
credentialClientinterface andrealCredentialClientstruct are not defined in this file, but based on the AI summary, they are expected to be defined inprovider.go.Also applies to: 48-48
43-46: The review comment's primary concern about ServerURL validation is not applicable. Theprovider.ServerURLis already comprehensively validated inValidateStore()(lines 61–79 ofproviders/v1/dvls/provider.go) beforeNewDVLSClient()is invoked. The validation checks for empty values, valid URL parsing, required scheme and host, whitelisted schemes (http/https), and enforces the insecure flag for HTTP. Adding duplicate validation inauth.gois redundant.The library (
go-dvls v0.14.0) is actively maintained by Devolutions with a recent v0.15.0 release (Jan 9, 2026). The API usage is correct and error handling is appropriate. No security issues were identified with the current version.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @config/crds/bases/external-secrets.io_secretstores.yaml:
- Around line 1958-2044: The v1beta1 CRD is missing the DVLS provider schema;
add the same "dvls" object from the v1 schema into the v1beta1 providers section
so DVLS support is parity restored (copy the "dvls" block including properties:
auth -> secretRef -> appId/appSecret, insecure, serverUrl, and required: [auth,
serverUrl]); in that block strengthen serverUrl by adding format: uri and
optionally add a CEL validation rule that enforces serverUrl starts with
"https://" unless insecure is true (use the dvls.insecure and dvls.serverUrl
fields in the CEL expression), ensuring field names like
dvls.auth.secretRef.appId, dvls.auth.secretRef.appSecret, dvls.insecure and
dvls.serverUrl match the v1 definitions exactly.
🧹 Nitpick comments (5)
providers/v1/dvls/client.go (2)
32-35: Consider using sentinel errors for better error handling.The error constants are defined as strings but used with
fmt.Errorf. ForerrNotImplemented, consider using a sentinel error for consistent error checking.♻️ Optional improvement
const ( - errNotImplemented = "not implemented" errFailedToGetEntry = "failed to get entry: %w" ) + +var errNotImplemented = errors.New("not implemented")Then update line 124:
- return nil, errors.New(errNotImplemented) + return nil, errNotImplemented
188-199: Validate could perform a lightweight connectivity check.The current
Validateonly checks if the client is initialized but doesn't verify connectivity to DVLS. Consider adding a lightweight API call to confirm the client can reach DVLS.Other providers often perform a simple API call during validation to ensure credentials are working. This would catch configuration issues earlier. However, the current implementation is acceptable for alpha stability.
providers/v1/dvls/provider_test.go (1)
29-38: Unused test constants.
testAppIDandtestAppSecretconstants are defined but never used in the tests. Consider removing them to avoid dead code, or use them in place of string literals if they're intended for future use.Suggested fix
const ( testNamespace = "default" testServerURL = "https://dvls.example.com" secretName = "dvls-secret" appIDKey = "app-id" appSecretKey = "app-secret" otherNS = "other" - testAppID = "test-app-id" - testAppSecret = "test-app-secret" )providers/v1/dvls/client_test.go (1)
136-150: Consider adding a positive validation test.The
Validatetest only covers the nil client case. Consider adding a test case for successful validation with a properly initialized client to ensure complete coverage.deploy/crds/bundle.yaml (1)
3913-3993: Well-structured DVLS provider schema.The CRD schema for the DVLS provider is well-designed and follows Kubernetes conventions. The required fields are properly marked, validation patterns are appropriate, and the security warning on the
insecureflag is clear.Optional enhancements for better validation
Consider adding format validation to
serverUrlto catch malformed URLs at admission time:serverUrl: description: ServerURL is the DVLS instance URL (e.g., https://dvls.example.com). type: string + format: uriOptionally, you could also make the
insecuredefault explicit for clarity (though Kubernetes defaults booleans tofalse):insecure: description: |- Insecure allows connecting to DVLS over plain HTTP. This is NOT RECOMMENDED for production use. Set to true only if you understand the security implications. type: boolean + default: false
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- hack/api-docs/mkdocs.yml
- apis/externalsecrets/v1/secretstore_dvls_types.go
- docs/snippets/dvls-push-secret.yaml
- docs/snippets/dvls-external-secret.yaml
- tests/snapshot/secretstore-v1.yaml
- providers/v1/dvls/go.mod
- tests/snapshot/clustersecretstore-v1.yaml
- docs/snippets/dvls-secret-store.yaml
- config/crds/bases/external-secrets.io_clustersecretstores.yaml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧬 Code graph analysis (8)
apis/externalsecrets/v1/secretstore_types.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-38)
pkg/register/dvls.go (1)
providers/v1/dvls/provider.go (3)
NewProvider(134-136)ProviderSpec(139-143)MaintenanceStatus(146-148)
providers/v1/dvls/auth_test.go (2)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSProvider(24-38)DVLSAuth(41-45)DVLSAuthSecretRef(48-56)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-53)
providers/v1/dvls/client_test.go (1)
providers/v1/dvls/client.go (2)
Client(40-42)NewClient(67-69)
providers/v1/dvls/auth.go (2)
providers/v1/dvls/client.go (2)
Client(40-42)NewClient(67-69)apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-38)
providers/v1/dvls/provider.go (4)
providers/v1/dvls/client.go (2)
NewClient(67-69)Client(40-42)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-53)apis/externalsecrets/v1/secretstore_dvls_types.go (2)
DVLSAuthSecretRef(48-56)DVLSProvider(24-38)apis/externalsecrets/v1/provider_schema_maintenance.go (1)
MaintenanceStatusMaintained(29-29)
providers/v1/dvls/client.go (3)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/keepersecurity/client.go (1)
Secret(105-111)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSAuth(41-45)DVLSAuthSecretRef(48-56)DVLSProvider(24-38)
🪛 Gitleaks (8.30.0)
providers/v1/dvls/client_test.go
[high] 266-278: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (33)
apis/externalsecrets/v1/secretstore_types.go (1)
203-205: LGTM! Clean provider integration.The DVLS provider field follows the established pattern perfectly—proper documentation, kubebuilder markers, JSON tags, and alphabetical placement.
go.mod (3)
35-35: LGTM! Standard provider module integration.The replace directive and require entry follow the exact pattern used for all other providers in the monorepo.
Also applies to: 149-149
101-101: golang.org/x/crypto update to v0.46.0 is valid.The version exists and is available in the Go module registry.
198-198: go-dvls v0.15.0 is valid and no security vulnerabilities found.v0.15.0 was released on January 9, 2026, with context support improvements to the client library. No known security vulnerabilities affect this version of the go-dvls package.
apis/externalsecrets/v1/zz_generated.deepcopy.go (2)
1263-1310: LGTM - Auto-generated deepcopy implementations follow established patterns.The deepcopy methods for
DVLSAuth,DVLSAuthSecretRef, andDVLSProvidercorrectly handle their respective fields:
- Value types copied directly
- Nested structs use
DeepCopyIntoappropriately
3556-3560: LGTM - DVLS field handling in SecretStoreProvider follows the standard pattern.The nil-check and allocation pattern is consistent with other provider fields in this function.
providers/v1/dvls/client.go (4)
71-102: LGTM - GetSecret implementation with sensible defaults.Good design choices:
- Proper not-found error handling with
esv1.NoSecretErr- Default to "password" property when none specified (consistent with 1Password provider as noted)
- Clear error messages for missing properties
127-160: LGTM - PushSecret correctly requires pre-existing entry.The implementation appropriately:
- Validates the secret parameter
- Requires the entry to exist before updating (documented behavior)
- Only updates the credential secret field
This is a reasonable constraint for DVLS integration.
201-220: LGTM - Robust key parsing with clear error messages.The
parseSecretReffunction:
- Uses
SplitNcorrectly to handle entry IDs containing/- Trims whitespace for user convenience
- Provides clear error messages for invalid formats
237-256: LGTM - Thorough validation in extractPushValue.Good defensive programming:
- Checks for empty secretKey
- Validates secret.Data is not nil
- Ensures value is not empty
docs/introduction/stability-support.md (2)
100-100: LGTM - Provider stability entry is accurate.Alpha stability is appropriate for a new provider. The maintainer matches the PR author.
142-142: LGTM - Feature support accurately reflects implementation.The feature matrix correctly shows:
- ✓ Store validation (implemented in
Validate)- ✓ Push secret (implemented in
PushSecret)- No find by name/tags (GetAllSecrets returns not implemented)
pkg/register/dvls.go (1)
1-29: LGTM - Provider registration follows established patterns.The build tag, imports, and registration call are consistent with other providers in the codebase (e.g.,
barbican.go,ngrok.go).docs/provider/devolutions-server.md (3)
1-6: LGTM - Clear and informative introduction.Good context about what DVLS is and its capabilities.
53-70: LGTM - Helpful credential type mapping table.The table clearly documents which properties are available for each credential type, which will help users configure their ExternalSecrets correctly.
96-101: LGTM - Limitations are clearly documented.Good transparency about current limitations:
- GetAllSecrets not supported
- Custom CA certificates workaround
- UUID-only references
- Certificate entries not supported
This helps users understand what to expect.
providers/v1/dvls/auth_test.go (1)
111-122: Good handling of expected DVLS connection failure in tests.The test correctly handles the case where Kubernetes secret access succeeds but the DVLS connection fails (expected with fake server). The assertions verify that the error is not from secret access, which is the behavior being tested.
providers/v1/dvls/provider_test.go (2)
40-372: LGTM!The
ValidateStoretests provide comprehensive coverage of validation scenarios including valid configurations, cluster store namespace requirements, HTTP/HTTPS URL handling, and auth secret reference validation. The assertions on specific error messages ensure validation logic is precise.
374-395: LGTM!The utility function tests (
Capabilities,NewProvider,ProviderSpec,MaintenanceStatus) appropriately verify the public API surface of the DVLS provider.providers/v1/dvls/client_test.go (7)
34-48: LGTM!The mock credential client is well-structured with configurable error injection and operation tracking (
lastUpdated,lastDeleted), enabling thorough testing of client behavior under various conditions.
50-79: LGTM!The mock methods correctly simulate DVLS API behavior, including proper 404 error responses for missing entries and tracking of mutation operations.
81-99: LGTM!The stub types provide minimal but sufficient implementations for testing
PushSecret,SecretExists, andDeleteSecretoperations.
100-134: LGTM!The
parseSecretReftests thoroughly cover the key format parsing including edge cases for invalid formats, empty components, and whitespace handling.
257-281: LGTM!The PrivateKey credential type test uses placeholder/dummy key content for testing purposes. The static analysis warning about a private key is a false positive since "-----BEGIN RSA PRIVATE KEY-----\nMIIE..." is clearly truncated test data, not a real secret.
321-350: LGTM!The
GetSecretandGetSecretMaptests properly verify not-found error handling and successful secret retrieval with property extraction.
352-447: LGTM!The
SecretExists,DeleteSecret, andPushSecrettests provide solid coverage of the write operations, including error propagation and handling of unsupported credential subtypes.providers/v1/dvls/provider.go (6)
32-35: LGTM!The interface compliance check
var _ esv1.Provider = &Provider{}ensures compile-time verification thatProviderimplements the required interface.
37-52: LGTM!The
NewClientfunction properly extracts provider configuration, resolves the store kind for secret resolution, and delegates toNewDVLSClientfor authentication handling. Error messages provide appropriate context.
54-87: LGTM!The
ValidateStorefunction provides comprehensive validation:
- Server URL presence and format verification
- Scheme validation (http/https only)
- Security-conscious requirement for explicit
insecure: truewhen using HTTP- Proper delegation to
validateAuthSecretReffor authentication configuration
94-122: LGTM!The
validateAuthSecretRefandrequireSecretSelectorfunctions provide layered validation: first ensuring required fields are present, then delegating toesutils.ValidateSecretSelectorfor store-level validation (e.g., namespace requirements for ClusterSecretStore).
124-131: LGTM!The
getDVLSProviderfunction properly validates the configuration chain (spec → provider → DVLS) and returns a clear error message when configuration is missing.
133-148: LGTM!The registration functions (
NewProvider,ProviderSpec,MaintenanceStatus) follow the standard External Secrets Operator provider registration pattern. TheMaintenanceStatusMaintainedstatus correctly indicates this is an actively supported provider.deploy/crds/bundle.yaml (1)
15660-15740: Expected schema duplication in CRD bundle.This hunk contains the same DVLS provider schema as the first hunk, which is expected for
bundle.yaml(typically one definition forSecretStoreand one forClusterSecretStore). The schema structure and validation are identical and correct.
Signed-off-by: Richard Boisvert <rboisvert@devolutions.net>
|
for the nitpicks |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/snippets/dvls-external-secret.yaml (2)
1-23: Clarify “Vault entry” wording to avoid confusion with HashiCorp Vault (and optionally document key format).
Right now “Vault entry” can be misread as the Vault provider rather than DVLS vault; this snippet is otherwise well-formed and appropriately usesexternal-secrets.io/v1. (Based on learnings, v1-only is correct.)Proposed doc-only tweak
--- -# Fetch a single property from a credential entry +# Fetch a single property from a DVLS credential entry (stored in a DVLS vault) apiVersion: external-secrets.io/v1 kind: ExternalSecret @@ data: - secretKey: username remoteRef: - key: 'vault-uuid/entry-uuid' + key: 'dvls-vault-uuid/credential-entry-uuid' property: username - secretKey: password remoteRef: - key: 'vault-uuid/entry-uuid' + key: 'dvls-vault-uuid/credential-entry-uuid' property: password
24-40:dataFrom.extractexample looks correct; consider aligning placeholder naming with DVLS terminology.Optional placeholder alignment
--- -# Fetch all fields from a credential entry +# Fetch all fields from a DVLS credential entry apiVersion: external-secrets.io/v1 kind: ExternalSecret @@ dataFrom: - extract: - key: 'vault-uuid/entry-uuid' + key: 'dvls-vault-uuid/credential-entry-uuid'config/crds/bases/external-secrets.io_secretstores.yaml (1)
2032-2037: Clarifyinsecuresemantics (HTTP vs TLS verify).
The description says “plain HTTP”, but many users readinsecureas “skip TLS verification”. Consider tightening the wording to explicitly say it permitshttp://inserverUrl(or, if the implementation actually skips TLS verify, update the description accordingly).providers/v1/dvls/client.go (1)
161-168: Consider idempotent delete behavior.If the entry doesn't exist, the DVLS SDK error will propagate to the caller. Some providers handle this case by returning
nilfor idempotent delete semantics (i.e., deleting a non-existent secret is not an error).Verify if PushSecret deletion reconciliation expects idempotent behavior. If so, consider handling not-found errors:
♻️ Optional: Handle not-found for idempotent delete
func (c *Client) DeleteSecret(ctx context.Context, ref esv1.PushSecretRemoteRef) error { vaultID, entryID, err := c.parseSecretRef(ref.GetRemoteKey()) if err != nil { return err } - return c.dvls.DeleteByID(ctx, vaultID, entryID) + err = c.dvls.DeleteByID(ctx, vaultID, entryID) + if isNotFoundError(err) { + return nil + } + return err }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproviders/v1/dvls/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
apis/externalsecrets/v1/secretstore_dvls_types.goapis/externalsecrets/v1/secretstore_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/introduction/stability-support.mddocs/provider/devolutions-server.mddocs/snippets/dvls-external-secret.yamldocs/snippets/dvls-push-secret.yamldocs/snippets/dvls-secret-store.yamlgo.modhack/api-docs/mkdocs.ymlpkg/register/dvls.goproviders/v1/dvls/auth.goproviders/v1/dvls/auth_test.goproviders/v1/dvls/client.goproviders/v1/dvls/client_test.goproviders/v1/dvls/go.modproviders/v1/dvls/provider.goproviders/v1/dvls/provider_test.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
✅ Files skipped from review due to trivial changes (1)
- docs/provider/devolutions-server.md
🚧 Files skipped from review as they are similar to previous changes (10)
- apis/externalsecrets/v1/secretstore_types.go
- docs/introduction/stability-support.md
- docs/snippets/dvls-push-secret.yaml
- providers/v1/dvls/auth_test.go
- tests/snapshot/clustersecretstore-v1.yaml
- apis/externalsecrets/v1/secretstore_dvls_types.go
- docs/snippets/dvls-secret-store.yaml
- providers/v1/dvls/provider_test.go
- hack/api-docs/mkdocs.yml
- tests/snapshot/secretstore-v1.yaml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
providers/v1/dvls/go.modpkg/register/dvls.goproviders/v1/dvls/auth.godeploy/crds/bundle.yamlproviders/v1/dvls/provider.gogo.modproviders/v1/dvls/client_test.goapis/externalsecrets/v1/zz_generated.deepcopy.godocs/api/spec.mddocs/snippets/dvls-external-secret.yamlproviders/v1/dvls/client.go
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In this repo’s CRDs, avoid adding provider-specific schema validations (e.g., `format: uri` or CEL rules) when other providers don’t use them; prioritize consistency across providers.
Applied to files:
providers/v1/dvls/auth.godeploy/crds/bundle.yaml
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, add new providers only to the GA v1 schema. Do not introduce new features to v1beta1, which exists only for backward compatibility. Ensure that any provider-related changes (e.g., DVLS) are implemented in v1-only definitions to avoid breaking older installations. This guideline applies to YAML files under config/crds/bases that define CRD schemas for external-secrets providers.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In CRD YAML files under config/crds, avoid provider-specific schema validations (e.g., format: uri or CEL rules) when other providers don’t use them. Aim for consistent, provider-agnostic validations across all CRDs to ensure uniform behavior and compatibility across providers. If a field requires provider-specific constraints, document and implement them in a separate, clearly scoped area or add conditional validation only where it’s universally applicable.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
🧬 Code graph analysis (6)
pkg/register/dvls.go (1)
providers/v1/dvls/provider.go (3)
NewProvider(134-136)ProviderSpec(139-143)MaintenanceStatus(146-148)
providers/v1/dvls/auth.go (3)
providers/v1/dvls/client.go (2)
Client(39-41)NewClient(66-68)apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-38)providers/v1/vault/util/vault.go (1)
Auth(31-33)
providers/v1/dvls/provider.go (4)
providers/v1/dvls/client.go (2)
NewClient(66-68)Client(39-41)providers/v1/dvls/auth.go (1)
NewDVLSClient(32-53)apis/externalsecrets/v1/secretstore_dvls_types.go (2)
DVLSAuthSecretRef(48-56)DVLSProvider(24-38)apis/externalsecrets/v1/provider_schema_maintenance.go (1)
MaintenanceStatusMaintained(29-29)
providers/v1/dvls/client_test.go (3)
providers/v1/dvls/client.go (2)
Client(39-41)NewClient(66-68)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (3)
DVLSAuth(41-45)DVLSAuthSecretRef(48-56)DVLSProvider(24-38)
providers/v1/dvls/client.go (3)
apis/externalsecrets/v1alpha1/pushsecret_types.go (1)
PushSecret(261-267)providers/v1/keepersecurity/client.go (1)
Secret(105-111)providers/v1/oracle/oracle.go (1)
SecretExists(110-110)
🔇 Additional comments (40)
docs/snippets/dvls-external-secret.yaml (1)
41-59: Verify the “Access Code type” →property: passwordmapping matches DVLS provider semantics.
If DVLS uses a different field name (e.g.,accessCode), this example will silently mislead users.providers/v1/dvls/go.mod (4)
67-67: LGTM: golang.org/x/crypto updated as requested.The dependency includes golang.org/x/crypto v0.46.0 as requested in PR feedback.
5-14: All direct dependencies are properly aligned with the base module.Verification confirms that the Kubernetes modules (k8s.io/api, k8s.io/apiextensions-apiserver, k8s.io/apimachinery) all use v0.34.1, controller-runtime uses v0.22.3, and Devolutions/go-dvls v0.15.0 is the correct version. No version conflicts or compatibility issues detected.
3-3: Go version 1.25.5 is intentional and properly aligned.The provider module correctly inherits the base go.mod's Go version directive (1.25.5). Both are aligned with no mismatches or conflicts detected. No action needed.
91-94: No action required—replace directives in non-root modules will not escape to consumers.Go module resolution explicitly ignores
replacedirectives in non-root modules when that module is consumed as a dependency. Only the root module's replaces are applied. In this monorepo, the rootgo.modalready correctly maps all provider paths, includinggithub.com/external-secrets/external-secrets/providers/v1/dvls => ./providers/v1/dvls. The dvls provider is not published independently and is only used internally through the root module. The replaces inproviders/v1/dvls/go.modare safe and follow the same pattern as all other providers in the repository—they won't cause downstream resolution issues.config/crds/bases/external-secrets.io_clustersecretstores.yaml (2)
2032-2037: Thedvls.insecuredescription is accurate and matches the implementation.The
insecureflag is used solely to permit plain HTTP URLs (as confirmed inproviders/v1/dvls/provider.go:77). It is validated at store initialization and does not affect TLS certificate verification—the go-dvls client is initialized without any TLS configuration overrides regardless of the flag's value. The current description correctly reflects this behavior.Regarding the optional
default: falsesuggestion: Kubernetes boolean fields default tofalseimplicitly, so adding an explicit default is not necessary but would serve as documentation.
1958-2044: No action needed—DVLS is correctly implemented as v1-only.Verification confirms DVLS is properly added only to the v1 schema (line 1958) and does not appear in v1beta1 (which starts at line 6172). The schema follows project conventions: no provider-specific validators like
format: uriare added, and the secret references use standard patterns consistent with other providers.apis/externalsecrets/v1/zz_generated.deepcopy.go (2)
1263-1310: LGTM - Autogenerated deepcopy functions follow established patterns.The DVLS type deepcopy implementations (
DVLSAuth,DVLSAuthSecretRef,DVLSProvider) are correctly generated by controller-gen and follow the same patterns used by other providers in this file.
3556-3560: LGTM - DVLS field handling in SecretStoreProvider is consistent.The DVLS nil-check and deepcopy pattern matches all other provider fields in
SecretStoreProvider.DeepCopyInto.pkg/register/dvls.go (1)
1-30: LGTM - Provider registration follows established patterns.The build tag (
dvls || all_providers), imports, and registration call ininit()are consistent with other provider registration files in this codebase.providers/v1/dvls/client_test.go (4)
34-79: LGTM - Well-structured mock with proper error simulation.The
mockCredentialClientprovides good test isolation with configurable error injection and operation tracking (lastUpdated,lastDeleted). The 404 handling correctly simulates DVLS API behavior.
100-134: LGTM - Thorough parseSecretRef testing.Good coverage of edge cases: valid format, missing separator, empty vault ID, empty entry ID, and whitespace handling.
159-326: LGTM - Comprehensive credential subtype testing.Tests cover all supported DVLS credential subtypes (Default, AccessCode, ApiKey, AzureServicePrincipal, ConnectionString, PrivateKey), plus unsupported subtype error handling and partial data scenarios. The assertions verify both the presence of expected fields and the absence of empty fields.
387-454: LGTM - PushSecret tests cover key scenarios.Tests validate updating Default and AccessCode subtypes, not-found handling, and unsupported subtype rejection. Good verification of the actual data written to the mock.
providers/v1/dvls/auth.go (2)
37-45: LGTM - Secret resolution follows established patterns.The use of
resolvers.SecretKeyReffor fetchingappIdandappSecretis consistent with how other providers resolve authentication credentials. Error wrapping provides good context for debugging.
31-53: TheInsecurefield is properly validated and being used; it does not need to be passed todvls.NewClient.The
Insecurefield is validated inprovider.go'sValidateStoremethod (line 77–78), which enforces that HTTP URLs requireinsecure: trueto be set explicitly. Thedvls.NewClientcall inauth.gocorrectly uses only the scheme fromprovider.ServerURLto determine the protocol (HTTP or HTTPS); the go-dvls library'sNewClientsignature accepts only username, password, and URL and has no parameter for insecure mode. The current implementation is correct.go.mod (3)
35-35: LGTM - Replace directive follows established pattern.The local replace directive for
providers/v1/dvlsis consistent with how other provider modules are structured in this monorepo.
101-101: LGTM - Crypto dependency update as requested.The
golang.org/x/cryptoupdate to v0.46.0 addresses the reviewer request from Skarlso in the PR comments.
198-198: No action needed: go-dvls v0.15.0 is the latest stable version (released January 9, 2026).config/crds/bases/external-secrets.io_secretstores.yaml (1)
1958-2044: DVLS provider schema addition looks correct and follows repo conventions.DVLS is properly added only to the v1 schema (line 1958), keeping v1beta1 untouched for backward compatibility. The schema uses standard SecretKeySelector patterns without custom format validations (consistent with other providers like Chef), and the required fields (
auth,serverUrl) align with typical provider configurations.providers/v1/dvls/provider.go (5)
37-52: LGTM! Clean client instantiation flow.The NewClient method correctly extracts the provider configuration, retrieves the store kind for cross-namespace validation, and properly delegates to NewDVLSClient and NewClient. Error handling and context propagation are appropriate.
54-87: Excellent validation with strong security posture.The ValidateStore method provides comprehensive validation:
- ServerURL presence and format checks
- URL scheme/host validation
- Security enforcement requiring explicit
insecure: truefor http URLs (lines 77-79) prevents accidental plaintext connections- Proper authentication reference validation
The validation logic is thorough and follows security best practices.
89-92: LGTM! Correct capability declaration.SecretStoreReadWrite is appropriate for a provider that supports get, push, delete, and exists operations as described in the PR objectives.
94-122: Well-structured validation helpers.The two-layer validation approach is well-designed:
requireSecretSelectorprovides clear, immediate feedback for missing required fieldsesutils.ValidateSecretSelectorhandles store-specific validation rules (e.g., cross-namespace restrictions)This pattern ensures both basic requirements and store-specific constraints are validated with appropriate error messages.
124-148: LGTM! Standard provider registration pattern.The factory and helper functions follow the established patterns for provider registration:
- getDVLSProvider performs proper nil checks at each level
- NewProvider, ProviderSpec, and MaintenanceStatus align with external-secrets provider conventions
deploy/crds/bundle.yaml (2)
3913-3993: LGTM - DVLS provider schema is well-structured.The schema follows existing provider patterns with proper required field declarations and clear descriptions. The
insecureflag appropriately warns users about security implications. Based on learnings, this correctly avoids provider-specific validations (likeformat: urion serverUrl) to maintain consistency with other providers.
15660-15740: LGTM - Schema is consistent with the SecretStore definition.The ClusterSecretStore DVLS schema mirrors the SecretStore definition exactly, ensuring consistent behavior across namespaced and cluster-scoped stores.
providers/v1/dvls/client.go (13)
32-41: LGTM!Good use of compile-time interface compliance check and clean struct design with dependency injection for the credential client.
49-63: LGTM!Clean adapter pattern wrapping the DVLS SDK's
EntryCredentialService, enabling testability through thecredentialClientinterface.
70-101: LGTM!The implementation correctly handles the not-found case by returning
esv1.NoSecretErr, and the default property fallback to "password" is well-documented.
103-119: LGTM!Consistent implementation pattern with
GetSecret, properly handling not-found errors.
121-124: LGTM!Returning
errNotImplementedis acceptable when the provider doesn't support find/list operations.
126-159: LGTM!Good implementation with clear requirements: entry must exist before pushing, and only the credential/password field is updated. Error messages are informative.
170-185: LGTM!Clean existence check implementation with proper not-found handling.
187-193: LGTM!Basic nil-check validation is acceptable. Connectivity will be verified on first operation.
195-198: LGTM!No-op close is appropriate for HTTP-based clients that don't hold persistent connections.
200-219: LGTM!Good parsing logic with
SplitN(key, "/", 2)allowing entry IDs to contain "/" while vault IDs cannot. Proper whitespace trimming and empty validation.
221-234: LGTM!Efficient conversion with pre-allocated map capacity.
236-255: LGTM!Comprehensive validation with clear, contextual error messages.
257-268: LGTM!Robust not-found detection with dual-layer check: SDK-specific
IsNotFoundand HTTP 404 status fallback.
Thanks @Skarlso! I should have addressed all the points and the new ones from code rabbit. |
|
/ok-to-test sha=a6dc256ca0c60ceba96b96ed4daa875d2732e451 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/crds/bases/external-secrets.io_secretstores.yaml (1)
1958-2044: DVLS provider schema is sound; optional doc clarification forinsecurefield aligns with patterns in other providersThe DVLS schema correctly follows existing provider conventions (SecretKeySelector-style refs, required
auth+serverUrl), avoids provider-specific validations, and is properly added to v1 only without appearing in v1beta1. No duplicate entries exist.The suggested clarification for
insecureis valid and consistent with how other providers document conditional HTTPS behavior. Adding "If serverUrl uses HTTPS, this field has no effect." would help users understand when this setting applies.Optional doc-only tweak
insecure: description: |- Insecure allows connecting to DVLS over plain HTTP. + If serverUrl uses HTTPS, this field has no effect. This is NOT RECOMMENDED for production use. Set to true only if you understand the security implications. type: boolean
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apis/externalsecrets/v1/secretstore_types.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mdhack/api-docs/mkdocs.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/api-docs/mkdocs.yml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
apis/externalsecrets/v1/secretstore_types.godeploy/crds/bundle.yamldocs/api/spec.md
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In this repo’s CRDs, avoid adding provider-specific schema validations (e.g., `format: uri` or CEL rules) when other providers don’t use them; prioritize consistency across providers.
Applied to files:
deploy/crds/bundle.yaml
📚 Learning: 2026-01-10T10:58:07.223Z
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: deploy/crds/bundle.yaml:5686-5707
Timestamp: 2026-01-10T10:58:07.223Z
Learning: In Kubernetes CRD YAML (e.g., deploy/crds/bundle.yaml), do not apply minLength constraints to duration fields (strings like "5m", "1h", "30s"). Duration values are semantic time representations; rely on proper duration patterns or server-side validation (e.g., OpenAPI schema with a proper format or a custom validator) rather than generic string length constraints.
Applied to files:
deploy/crds/bundle.yaml
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, add new providers only to the GA v1 schema. Do not introduce new features to v1beta1, which exists only for backward compatibility. Ensure that any provider-related changes (e.g., DVLS) are implemented in v1-only definitions to avoid breaking older installations. This guideline applies to YAML files under config/crds/bases that define CRD schemas for external-secrets providers.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In CRD YAML files under config/crds, avoid provider-specific schema validations (e.g., format: uri or CEL rules) when other providers don’t use them. Aim for consistent, provider-agnostic validations across all CRDs to ensure uniform behavior and compatibility across providers. If a field requires provider-specific constraints, document and implement them in a separate, clearly scoped area or add conditional validation only where it’s universally applicable.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
📚 Learning: 2026-01-10T10:41:30.240Z
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: config/crds/bases/external-secrets.io_clustersecretstores.yaml:3868-3887
Timestamp: 2026-01-10T10:41:30.240Z
Learning: In config/crds/bases/external-secrets.io_clustersecretstores.yaml: Make the CacheConfig block optional; allow cache: {} to enable caching with defaults. Do not require ttl. Use +kubebuilder:validation:MinLength=1 for ttl when provided and +kubebuilder:default="5m". For maxSize, use +kubebuilder:validation:Minimum=1 and default 100. Update docs wording to: "If omitted, caching is disabled (default)."
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
🧬 Code graph analysis (1)
apis/externalsecrets/v1/secretstore_types.go (1)
apis/externalsecrets/v1/secretstore_dvls_types.go (1)
DVLSProvider(24-38)
🔇 Additional comments (4)
apis/externalsecrets/v1/secretstore_types.go (1)
203-205: LGTM! Provider addition follows established patterns.The DVLS provider field is correctly integrated into the SecretStoreProvider struct:
- Properly placed in alphabetical order between Device42 and Infisical
- Follows the consistent pattern used by all other providers (pointer type, json tag with omitempty, +optional marker)
- Comment clearly describes the provider's purpose
config/crds/bases/external-secrets.io_clustersecretstores.yaml (1)
1958-2044: No changes required. DVLS is correctly scoped to the GAv1schema only and does not appear in the deprecatedv1beta1schema, following repo conventions. Theinsecurefield description accurately reflects its implementation: it controls whether plain HTTP connections are allowed to the DVLS instance.deploy/crds/bundle.yaml (2)
3913-3993: LGTM! Well-structured DVLS provider schema.The schema follows existing provider patterns in this repository:
- Proper validation constraints on secret references (key, name, namespace) using standard Kubernetes patterns
- Required fields are correctly marked at each nesting level
- The
insecurefield includes an appropriate security warning- No provider-specific validations like
format: urionserverUrl, which is consistent with how other providers handle URLsBased on learnings, this v1-only addition aligns with the project convention that new providers are added to the GA v1 schema only.
15660-15740: LGTM! ClusterSecretStore schema matches SecretStore.This DVLS provider schema is identical to the SecretStore definition, which is the expected pattern for maintaining consistency between SecretStore and ClusterSecretStore CRDs.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/__snapshot__/clustersecretstore-v1.yaml:
- Around line 300-312: The snapshot YAML sets dvls.insecure: true which disables
TLS verification; change the example/default to insecure: false to make the
configuration secure-by-default and align with other providers (e.g.,
senhasegura.ignoreSslCertificate: false); update the dvls → auth → secretRef
block in the cluster secret store snapshot so the insecure field is false and
regenerate the snapshot.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mdtests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/snapshot/secretstore-v1.yaml
- apis/externalsecrets/v1/zz_generated.deepcopy.go
- config/crds/bases/external-secrets.io_secretstores.yaml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
tests/__snapshot__/clustersecretstore-v1.yamldocs/api/spec.mddeploy/crds/bundle.yaml
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In this repo’s CRDs, avoid adding provider-specific schema validations (e.g., `format: uri` or CEL rules) when other providers don’t use them; prioritize consistency across providers.
Applied to files:
deploy/crds/bundle.yaml
📚 Learning: 2026-01-10T10:58:07.223Z
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: deploy/crds/bundle.yaml:5686-5707
Timestamp: 2026-01-10T10:58:07.223Z
Learning: In Kubernetes CRD YAML (e.g., deploy/crds/bundle.yaml), do not apply minLength constraints to duration fields (strings like "5m", "1h", "30s"). Duration values are semantic time representations; rely on proper duration patterns or server-side validation (e.g., OpenAPI schema with a proper format or a custom validator) rather than generic string length constraints.
Applied to files:
deploy/crds/bundle.yaml
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, add new providers only to the GA v1 schema. Do not introduce new features to v1beta1, which exists only for backward compatibility. Ensure that any provider-related changes (e.g., DVLS) are implemented in v1-only definitions to avoid breaking older installations. This guideline applies to YAML files under config/crds/bases that define CRD schemas for external-secrets providers.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yaml
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In CRD YAML files under config/crds, avoid provider-specific schema validations (e.g., format: uri or CEL rules) when other providers don’t use them. Aim for consistent, provider-agnostic validations across all CRDs to ensure uniform behavior and compatibility across providers. If a field requires provider-specific constraints, document and implement them in a separate, clearly scoped area or add conditional validation only where it’s universally applicable.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yaml
📚 Learning: 2026-01-10T10:41:30.240Z
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: config/crds/bases/external-secrets.io_clustersecretstores.yaml:3868-3887
Timestamp: 2026-01-10T10:41:30.240Z
Learning: In config/crds/bases/external-secrets.io_clustersecretstores.yaml: Make the CacheConfig block optional; allow cache: {} to enable caching with defaults. Do not require ttl. Use +kubebuilder:validation:MinLength=1 for ttl when provided and +kubebuilder:default="5m". For maxSize, use +kubebuilder:validation:Minimum=1 and default 100. Update docs wording to: "If omitted, caching is disabled (default)."
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yaml
🔇 Additional comments (4)
config/crds/bases/external-secrets.io_clustersecretstores.yaml (2)
1958-2044: No issues found. DVLS provider is correctly added only to the GA v1 schema and does not appear in the deprecated v1beta1 schema (which begins at line 6195). The schema follows repo conventions by avoiding provider-specific validations and maintaining consistency with other providers.
2032-2041: Remove - field semantics are unambiguous.The
Insecurefield is specifically for allowing plain HTTP scheme URLs (http:// protocol), not for skipping TLS verification. The implementation validates the URL scheme in provider.go with the checkif parsedURL.Scheme == "http" && !dvlsProvider.Insecure, and the error message confirms: "http URLs require 'insecure: true'". The description "connecting to DVLS over plain HTTP" accurately reflects this behavior. No TLS verification skip logic is present in the DVLS provider code.Likely an incorrect or invalid review comment.
deploy/crds/bundle.yaml (2)
3913-3993: LGTM: DVLS provider schema follows Kubernetes conventions.The DVLS provider schema is well-structured with:
- Appropriate required fields (
auth,serverUrl)- Standard Kubernetes secret reference patterns for authentication
- Secure defaults (insecure boolean defaults to false)
- Clear security warnings for the
insecurefieldAlso applies to: 15682-15762
3913-3993: DVLS is correctly scoped to v1 schemas only.Verification confirms that DVLS is added to v1 schemas only (lines 35–6194 in both
external-secrets.io_secretstores.yamlandexternal-secrets.io_clustersecretstores.yaml), with no presence in v1beta1 schemas (lines 6195+), which aligns with the requirement that new providers are GA v1-only for backward compatibility.
| dvls: | ||
| auth: | ||
| secretRef: | ||
| appId: | ||
| key: string | ||
| name: string | ||
| namespace: string | ||
| appSecret: | ||
| key: string | ||
| name: string | ||
| namespace: string | ||
| insecure: true | ||
| serverUrl: string |
There was a problem hiding this comment.
Set insecure to false to promote secure-by-default configuration.
The snapshot shows insecure: true (Line 311), which disables TLS certificate verification. Since snapshot files often serve as templates or references for users, this example could lead to insecure deployments.
Compare with similar fields in other providers: senhasegura.ignoreSslCertificate: false (Line 709).
🔒 Proposed fix
dvls:
auth:
secretRef:
appId:
key: string
name: string
namespace: string
appSecret:
key: string
name: string
namespace: string
- insecure: true
+ insecure: false
serverUrl: string📝 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.
| dvls: | |
| auth: | |
| secretRef: | |
| appId: | |
| key: string | |
| name: string | |
| namespace: string | |
| appSecret: | |
| key: string | |
| name: string | |
| namespace: string | |
| insecure: true | |
| serverUrl: string | |
| dvls: | |
| auth: | |
| secretRef: | |
| appId: | |
| key: string | |
| name: string | |
| namespace: string | |
| appSecret: | |
| key: string | |
| name: string | |
| namespace: string | |
| insecure: false | |
| serverUrl: string |
🤖 Prompt for AI Agents
In @tests/__snapshot__/clustersecretstore-v1.yaml around lines 300 - 312, The
snapshot YAML sets dvls.insecure: true which disables TLS verification; change
the example/default to insecure: false to make the configuration
secure-by-default and align with other providers (e.g.,
senhasegura.ignoreSslCertificate: false); update the dvls → auth → secretRef
block in the cluster secret store snapshot so the insecure field is false and
regenerate the snapshot.
There was a problem hiding this comment.
That's not what that field is for buddy. It's just an extra measure if people define http making sure they don't leave the 's' off by accident.
There was a problem hiding this comment.
@Skarlso, thank you for the clarification! I misunderstood the purpose of the insecure field. You're absolutely right—it's a safety mechanism that requires explicit acknowledgment when using HTTP instead of HTTPS, preventing accidental omission of the 's'. I appreciate you explaining the design intent.
✏️ Learnings added
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5712
File: tests/__snapshot__/clustersecretstore-v1.yaml:300-312
Timestamp: 2026-01-10T16:35:03.175Z
Learning: In the external-secrets DVLS provider (providers/v1/dvls), the `insecure` field is a safety mechanism that requires users to explicitly set `insecure: true` when using HTTP URLs (without TLS). This prevents accidentally using an insecure connection when HTTPS was intended. The validation enforces that HTTP URLs must have the insecure flag set to true; otherwise, an error is returned.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Oh sorry, I haven't seen this question. That's interesting. :D I think in case of Barbican it was missed? Normally we should modify it, but we also need to create the team for it in GitHub. :D So I guess we'll create a followup PR to add everyone who's missing. :) |
…rets#5712) Co-authored-by: Gergely Bräutigam <skarlso777@gmail.com>
Problem Statement
We want to add a provider for Devolutions Server that uses our go-dvls library.
Related Issue
Fixes #5682
Proposed Changes
Added the provider, based on the following PRs:
Checklist
git commit --signoffmake testmake reviewableQuestion
I did not modify the
CODEOWNERS.mdfile but I am uncertain if it is needed. It was modified in the ngrok PR, and not in the Barbican one.Summary
Adds Devolutions Server (DVLS) as a new provider to External Secrets Operator.
Key Changes
API & CRDs
Provider implementation
Tests
Modules & dependencies
Documentation & examples
Misc
Estimated review effort: High.