feat(azure): add lowkey vault container #3542
Conversation
- Add new container for Lowkey Vault - Document the usage of the new container Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a Lowkey Vault module to Azure: new container API and Run function, Local/Network access modes, connection/token URL helpers, managed-identity env helpers, TLS client handling, docs, examples, tests, testdata, and Azure Key Vault / pkcs12 dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Test Client
participant Container as Lowkey Vault Container
participant Identity as Identity Endpoint
participant SecretSvc as Secrets Service
Client->>Container: Start container (Run)
Container-->>Client: Exposed connection & token endpoints
Client->>Container: ConnectionURL(ctx, accessMode)
Container-->>Client: Connection URL
Client->>Identity: Request token via TokenURL
Identity-->>Client: Bearer token
Client->>SecretSvc: Create Secrets client (credential + custom HTTP client)
SecretSvc-->>Client: Client ready
Client->>SecretSvc: Set secret(name, value)
SecretSvc->>Container: Persist secret
Container-->>SecretSvc: ACK
SecretSvc-->>Client: Secret created
Client->>SecretSvc: Get secret(name, version)
SecretSvc->>Container: Retrieve secret
Container-->>SecretSvc: Secret value
SecretSvc-->>Client: Return secret value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docs/modules/azure.md`:
- Around line 484-485: Update the sentence under the "Network mode" section that
currently reads "In the following example, we are starting the Lowkey Vault
container in Local mode and set the parameters of a go client..." to correctly
state "Network mode" and capitalize "Go": change it to something like "In the
following example, we are starting the Lowkey Vault container in Network mode
and set the parameters of a Go client container which will be used to connect to
the Key Vault API of the Lowkey Vault container in Network mode." Locate this
text by searching for the phrase "In the following example" within the "Network
mode" section of docs/modules/azure.md and replace the incorrect mention of
"Local mode" and lowercase "go".
- Around line 398-403: Correct the grammatical phrase and the alias typo in the
Network mode section: change the sentence "To use prepare the container..." to
"To prepare the container..." and "In example:" to "For example:", and in the
code snippet replace the incorrect identifier lowkeyvalt with lowkeyvault so the
snippet reads Run(ctx, "nagyesta/lowkey-vault:7.0.9-ubi10-minimal",
lowkeyvault.WithNetworkAlias("lowkey-vault", aNetwork),) while keeping the
WithNetworkAlias and Run call intact.
In `@modules/azure/lowkeyvault/examples_test.go`:
- Around line 166-182: The examples call azidentity.NewDefaultAzureCredential
and azsecrets.NewClient (and equivalents in ExampleRun_keyOperations and
ExampleRun_certificateOperations) while discarding errors, which can yield nil
credentials/clients and panics; update each example (ExampleRun,
ExampleRun_keyOperations, ExampleRun_certificateOperations) to capture and
handle the returned errors from azidentity.NewDefaultAzureCredential and
azsecrets.NewClient (and any azkeys/azcertificates client constructors) — check
err != nil and fail the test/example gracefully (t.Fatal / log.Fatalf / return)
or propagate the error before using cred, secretClient, keyClient, or
certClient; keep using lowkeyVaultContainer.PrepareClientForSelfSignedCert and
httpClient as before but only after successful error checks.
In `@modules/azure/lowkeyvault/lowkeyvault_test.go`:
- Around line 102-116: The test currently discards errors when creating Azure
credentials and clients (calls to azidentity.NewDefaultAzureCredential and
azsecrets.NewClient, azkeys.NewClient, azcertificates.NewClient), which can hide
failures and cause nil derefs; update each creation site to capture the returned
error variable (e.g., cred, err := azidentity.NewDefaultAzureCredential(...),
secretClient, err := azsecrets.NewClient(...)) and add require.NoError(t, err)
(and require.NotNil(t, secretClient/cred) as appropriate) immediately after each
call for the following symbols: azidentity.NewDefaultAzureCredential,
azsecrets.NewClient, azkeys.NewClient, azcertificates.NewClient, and any
subsequent secretClient creations referenced in the test.
- Line 1: The package name at the top is misspelled as `lowkeyvalt_test`; change
the package declaration to `lowkeyvault_test` and then update every reference of
the incorrect identifier prefix `lowkeyvalt.*` to `lowkeyvault.*` (e.g., any
uses like lowkeyvalt.NewClient, lowkeyvalt.TestSecretName, etc.) so imports and
identifiers match; ensure the import of `lowkeyvault` on line ~20 aligns with
the corrected package name and run `go test` to confirm compilation.
In `@modules/azure/lowkeyvault/lowkeyvault.go`:
- Around line 123-145: In _MappedHostAuthority on LowkeyVaultContainer: validate
accessMode and return an error for unsupported values instead of falling
through, and guard all index/slicing into Networks() and NetworkAliases() to
avoid panics — check that networks slice is non-empty, that hosts map contains
networks[0], and that hosts[networks[0]] has at least one alias before indexing;
if any are missing return descriptive errors (e.g., "no networks attached" / "no
aliases for network X"); preserve existing Host() and MappedPort() logic for
Local accessMode and include their error wrapping (Host, MappedPort, Networks,
NetworkAliases).
- Line 1: The package is misspelled as "lowkeyvalt"; change the package
declaration in lowkeyvault.go from "package lowkeyvalt" to "package
lowkeyvault", and update the test packages and usages: rename "lowkeyvalt_test"
to "lowkeyvault_test" in lowkeyvault_test.go and examples_test.go and update
every import/identifier usage that references "lowkeyvalt" to "lowkeyvault"
(there are ~9 occurrences in lowkeyvault_test.go and ~13 in examples_test.go) so
package names match the module path and avoid import/name mismatches.
- Around line 34-42: In WithNetworkAlias, the LOWKEY_VAULT_ALIASES value
currently contains the literal "<port>" — replace that placeholder with the
actual API port by converting defaultApiPort.Int() to a string when constructing
envs (the map used with testcontainers.WithEnv(...).Customize(req)); update the
envs assignment so the RHS is "localhost="+alias+":"+<stringified
defaultApiPort.Int()> (use fmt.Sprintf or strconv.Itoa) before calling
testcontainers.WithEnv(envs).Customize(req).
In `@modules/azure/lowkeyvault/testdata/main.go`:
- Around line 41-55: The credential and secret client creations currently ignore
returned errors (azidentity.NewDefaultAzureCredential and azsecrets.NewClient),
risking nil dereference on cred/secretClient; update the code to check both
error return values (for cred and for secretClient), handle failures the same
way other operations do in this file (log the error and return/exit early), and
ensure you do not call methods on secretClient when its initialization failed —
reference the variables cred, secretClient and the call site using
DisableChallengeResourceVerification to locate where to add the checks.
♻️ Duplicate comments (2)
modules/azure/lowkeyvault/examples_test.go (2)
233-249: Same missing error handling for credential/client creation here.
328-342: Same missing error handling for credential/client creation here.
🧹 Nitpick comments (3)
modules/azure/lowkeyvault/testdata/main.go (1)
26-28: Pin a minimum TLS version even with self‑signed certs.This avoids accidental downgrade to legacy TLS versions while still allowing self-signed certificates.
♻️ Proposed fix
- customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + customTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }modules/azure/lowkeyvault/lowkeyvault.go (1)
79-83: Pin a minimum TLS version for the self‑signed client.Avoids legacy TLS negotiation while still permitting self-signed certs.
♻️ Proposed fix
- customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + customTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }modules/azure/lowkeyvault/lowkeyvault_test.go (1)
281-284: Guard the PKCS#12 key type assertion to avoid panics.If the decoder returns a non‑ECDSA key, the current assertion will panic. Prefer an explicit type check for clearer failures.
🔎 Suggested safety check
- ecKey := key.(*ecdsa.PrivateKey) + ecKey, ok := key.(*ecdsa.PrivateKey) + require.True(t, ok)
- Fix typo in package name - Handle errors in credential creations - Handle errors in client creations - Guard against misconfiguration - Fix documentation Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
- Fix grammatical issue in documentation Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/modules/azure.md`:
- Line 432: Update the sentence that reads
"azidentity.NewDefaultAzureCredential(nil) as credential" to include the
indefinite article "a" before "credential" so it reads
"azidentity.NewDefaultAzureCredential(nil) as a credential"; locate the string
containing azidentity.NewDefaultAzureCredential(nil) to make this small grammar
fix.
- Around line 398-399: Remove the comma that appears before "and making" in the
sentence starting "To prepare the container for running in a network, and making
it accessible from other containers in the network, you can use the
`WithNetworkAlias` option." — edit that sentence to remove the comma so it reads
with parallel structure: "To prepare the container for running in a network and
making it accessible from other containers in the network, you can use the
`WithNetworkAlias` option."
- Fix grammatical issue in documentation Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/modules/azure.md`:
- Around line 484-485: Change the mixed tenses in the sentence "In the following
example, we are starting the Lowkey Vault container in Network mode and set the
parameters of a Go client container..." to use parallel/simple present tense;
e.g., replace "are starting" with "start" so the sentence reads "In the
following example, we start the Lowkey Vault container in Network mode and set
the parameters of a Go client container..." to ensure tense consistency.
♻️ Duplicate comments (1)
docs/modules/azure.md (1)
398-399: Fix parallel verb structure.The phrase "for running in a network and making it accessible" mixes verb forms. Use parallel infinitives for clarity.
✍️ Proposed fix
-To prepare the container for running in a network and making it accessible from other containers in the network, you can use the +To prepare the container for running in a network and make it accessible from other containers, use the
- Fix grammatical issue in documentation Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
|
@coderabbitai Is anything blocking approval and merge based on your review? |
🧩 Analysis chain🏁 Script executed: Length of output: 1319
✅ All completed status checks are passing (SUCCESS/NEUTRAL) Would you like me to review the changes now? I can perform a comprehensive review of the Lowkey Vault container implementation. ✅ Actions performedReview triggered.
|
|
Hi @mdelapenya, you were recommended by the bot for review. As far as I can see, the automated review suggestions were addressed. Please let me know in case any further fixes are needed! Thank you! |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
@nagyesta thanks for working on this new module, it looks really promising!
I added a few comments that would like to discuss before kicking the CI. Please let us know what you think.
|
|
||
| // SetManagedIdentityEnvVariables sets the environment variables required for managed identity authentication. | ||
| // This works only with local access mode | ||
| func (c *LowkeyVaultContainer) SetManagedIdentityEnvVariables(ctx context.Context) error { |
There was a problem hiding this comment.
suggestion: instead of forcing users to alter their environment, I'd provide a function that returns each value: e.g. rename TokenURL to IdentityEndpoint; and a new IdentityHeader... Or simply a IdentifyCredentials returning the URL, the Header and an error at the same time.
The users decide what to do with them to build their clients.
Thoughts?
There was a problem hiding this comment.
Hi,
thanks for the suggestion!
AFAIK there is no other choice, if they want to use the simulated managed identity, then they must set the environment variables. I am okay with only returning the values as you have suggested, but in the end they will simply implement the os.Setenv calls on their own. My intention was to make this as convenient as possible...
Should I change it?
There was a problem hiding this comment.
@stevenh what are your thoughts on this? I like the library to be in some way opinionated but I don't like "black magic" happening behind the scenes... unless it's pretty well documented.
| } | ||
|
|
||
| // PrepareClientForSelfSignedCert prepares a client which will accept insecure (self-signed) certificates. | ||
| func (c *LowkeyVaultContainer) PrepareClientForSelfSignedCert() http.Client { |
There was a problem hiding this comment.
suggestion (for a follow-up): for adding TLS support, see how other modules do that, leveraging mdelapenya/tlscert.
In the meantime, I'd rename this method to Client, advertising that it's using a self-signed cert for communications.
There was a problem hiding this comment.
Thank you! I really ike the idea of using the self-signed cert to establish trust instead of ignoring everything. I am not sure whether I should use the orignal cert using the dedicated endpoints offered by the container as mentioned here:
https://github.com/nagyesta/lowkey-vault/blob/3eab14c19429357d6dd03b70cc5660f25e1ba167/README.md?plain=1#L226-L228
or I could also generate one during the container config and provide it as a custom certificate. I think the first option can work perfectly for local mode and the in certain cases of network mode (depending on the alias) while generating a custom cert should always work but it makes the config way more complex.
What do you think?
- Fix issues identified by the code review Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@modules/azure/lowkeyvault/examples_test.go`:
- Around line 397-399: The CreateCertificate call's error handling in
examples_test.go currently logs the error but continues execution; update the
block after the call to CreateCertificate (check the err variable returned by
CreateCertificate) to return early on error (e.g., return from the test or
t.Fatal/t.Fatalf if in a test) so subsequent operations don't run when
certificate creation failed; modify the error branch that logs "failed to create
a certificate: %v" to perform the early return.
- Line 440: The code directly asserts key.(*ecdsa.PrivateKey) into ecKey which
can panic if key has a different type; change this to the comma-ok form (ecKey,
ok := key.(*ecdsa.PrivateKey)) and handle the false case (e.g., return an error
or fail the test with t.Fatalf / log and stop) so the code fails gracefully
instead of panicking.
- Around line 197-206: The code calls secretClient.SetSecret and
secretClient.GetSecret but only logs errors and continues, which can lead to a
nil dereference when reading fetched.Value; update the error handling in the
SetSecret and GetSecret branches (around the variables created, err :=
secretClient.SetSecret(...) and fetched, err := secretClient.GetSecret(...)) to
return immediately on error (or fail the test with t.Fatalf if inside a test)
instead of just logging, so execution stops before dereferencing fetched.Value.
- Around line 281-309: When CreateKey/Encrypt/Decrypt fail, the code currently
logs the error but continues and then dereferences encrResp.Result or
decrResp.Result; update the error handling in the CreateKey call and in the
Encrypt and Decrypt calls (functions: keyClient.CreateKey, keyClient.Encrypt,
keyClient.Decrypt) to return early (or exit the test) after logging the error to
avoid nil pointer dereferences when accessing createdKey, encrResp, or decrResp,
and fix the decrypt error log message to say "failed to decrypt a message"
instead of "encrypt".
🧹 Nitpick comments (3)
modules/azure/lowkeyvault/examples_test.go (1)
57-63: Consider removing thereturnstatement inside the defer.The
returnon line 61 inside the deferred function is a no-op since the defer executes when the function is already returning. While harmless, removing it improves clarity.♻️ Suggested simplification
defer func() { err := aNetwork.Remove(ctx) if err != nil { log.Printf("failed to remove network: %s", err) - return } }()modules/azure/lowkeyvault/lowkeyvault.go (2)
82-87: Consider adding TLS MinVersion for defense-in-depth.While
InsecureSkipVerify: trueis intentional and documented for accepting the self-signed certificate, adding a minimum TLS version is a low-cost security improvement, even for test containers.♻️ Proposed enhancement
func (c *Container) Client() http.Client { customTransport := http.DefaultTransport.(*http.Transport).Clone() - customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + customTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + } return http.Client{Transport: customTransport} }Based on static analysis hint about missing TLS MinVersion.
108-124: Be aware of test isolation implications withos.Setenv.The
os.Setenvcalls modify the process-wide environment, which can affect parallel tests. This is likely acceptable for the intended use case (local mode where the client runs on the host), but consider documenting this behavior or usingt.Setenvin test contexts for automatic cleanup.For test code specifically, users could leverage
t.Setenv()(available since Go 1.17) which automatically restores the original value after the test:// In test code, prefer: t.Setenv("IDENTITY_ENDPOINT", tokenURL) t.Setenv("IDENTITY_HEADER", "header")
- Fix code review issues identified by AI Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
- Fix code review issues identified by AI Resolves testcontainers#3541 Signed-off-by: Esta Nagy <[email protected]>
|
Hi @mdelapenya , |
2 participants
What does this PR do?
Why is it important?
Related issues