Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lowkey Vault Docker does not support dynamic host ports #1319

Closed
Xor-el opened this issue Jan 16, 2025 · 13 comments
Closed

Lowkey Vault Docker does not support dynamic host ports #1319

Xor-el opened this issue Jan 16, 2025 · 13 comments
Assignees
Labels
community requested Issues requested by our community enhancement New feature or request released Released, waiting for feedback

Comments

@Xor-el
Copy link

Xor-el commented Jan 16, 2025

Describe the bug

I encountered a blocker while working on this issue. According to the .NET TestContainers best practices, it is recommended to avoid static host port bindings for containers. Instead, the guidelines suggest using randomly assigned host ports via _builder.WithPortBinding(ushort, true) and retrieving the mapped port using _container.GetMappedPublicPort(ushort).

However, it seems that Lowkey Vault Docker does not support the use of random host ports. Instead, it requires the host, container, and server ports to match, which conflicts with the best practices outlined by .NET TestContainers.

To reproduce

Steps to reproduce the behavior:

  1. Clone this repo and build

  2. run this docker command, note that the host port is different from the container port.
    docker run --rm --name lowkey -d -p 8550:8443 nagyesta/lowkey-vault:2.6.15

  3. run the demo built in Step 1

Expected behavior

Dynamic host ports should be supported as it is not guaranteed that a port would always be available on the host.

Actual behavior

When I run the demo

Image

Image

A minimal project that can be used to reproduce the issue

LowKeyVaultDockerDemo

Environment

  • OS: [e.g. Windows]
  • Version: [e.g. 2.6.15]
  • Java version [e.g. 17.0.13]

Additional context

Using a static port for the host might not always be possible as that port might be unavailable on the host.

@Xor-el Xor-el added the bug Something isn't working label Jan 16, 2025
@nagyesta nagyesta added enhancement New feature or request community requested Issues requested by our community and removed bug Something isn't working labels Jan 17, 2025
@nagyesta nagyesta self-assigned this Jan 17, 2025
@nagyesta
Copy link
Owner

Hi @Xor-el ,
thanks for reporting this! I have reclassified it as a feature request because it was a known limitation due to how the vault identification works. Nonetheless, I see value in solving this, because as you have mentioned, we cannot guarantee that the same port is always open.

The very hacky workaround which exists is to find a free port and configure Lowkey Vault to use that port.

I will take a look during the weekend and see how I can make this work in a convenient way with the random port assigned by Testcontainers.

Thank you again!

@nagyesta nagyesta moved this from Ideas to Todo for next increment in Lowkey Vault Roadmap Jan 17, 2025
@Xor-el
Copy link
Author

Xor-el commented Jan 17, 2025

@nagyesta Thanks for looking into this. very much appreciated.

@nagyesta nagyesta moved this from Todo for next increment to In progress in Lowkey Vault Roadmap Jan 17, 2025
nagyesta added a commit that referenced this issue Jan 17, 2025
- Adds new configuration option to allow relaxed matching of vault URI ports
- Updates tests
- Deprecates Testcontainers method setting a fixed host port
- Enhances Testcontainers module to always use the relaxed matching feature
- Adds more verification steps to Testcontainers tests
- Updates documentation

Updates #1319

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
nagyesta added a commit that referenced this issue Jan 18, 2025
- Adds new configuration option to allow relaxed matching of vault URI ports
- Updates tests
- Deprecates Testcontainers method setting a fixed host port
- Enhances Testcontainers module to always use the relaxed matching feature
- Adds more verification steps to Testcontainers tests
- Updates documentation

Updates #1319
{minor}

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
nagyesta added a commit that referenced this issue Jan 18, 2025
- Adds new configuration option to allow relaxed matching of vault URI ports
- Updates tests
- Deprecates Testcontainers method setting a fixed host port
- Enhances Testcontainers module to always use the relaxed matching feature
- Adds more verification steps to Testcontainers tests
- Updates documentation

Updates #1319
{minor}

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
@nagyesta nagyesta moved this from In progress to Released in Lowkey Vault Roadmap Jan 18, 2025
@nagyesta
Copy link
Owner

Hi @Xor-el ,
I think I have managed to solve this. Please try with 2.7.0 using the new relaxed port matching feature.

Please feel free to reach out on this ticket in case it is still not working!

@nagyesta nagyesta added the released Released, waiting for feedback label Jan 18, 2025
@Xor-el
Copy link
Author

Xor-el commented Jan 18, 2025

Hello @nagyesta , thanks for the fix.
I can confirm that this works as expected.
one more thing, could you please update the multi-platform image (-ubi9-minimal) to v2.7.0?

@Xor-el
Copy link
Author

Xor-el commented Jan 18, 2025

so this seems to have broken vault aliases.
will post a demo to reproduce it soon.

@nagyesta
Copy link
Owner

Thanks @Xor-el! Sure, I will, just waiting for Renovate to raise the PR when the artifact becomes usable.

@Xor-el
Copy link
Author

Xor-el commented Jan 18, 2025

so this seems to have broken vault aliases. will post a demo to reproduce it soon.

so here is how to reproduce this

set any of the below as the env

LOWKEY_ARGS=--LOWKEY_VAULT_NAMES=demo --LOWKEY_VAULT_ALIASES="demo.localhost=demo.127.0.0.1 --LOWKEY_VAULT_RELAXED_PORTS=true

or

LOWKEY_ARGS=--LOWKEY_VAULT_NAMES=demo --LOWKEY_VAULT_ALIASES="demo.localhost=demo.127.0.0.1:8443 --LOWKEY_VAULT_RELAXED_PORTS=true

then run

docker run --rm --name lowkey -d -p 8550:8443 nagyesta/lowkey-vault:2.7.0

Lowkey Vault then crashes with an exception.

@nagyesta
Copy link
Owner

Thanks @Xor-el ! I am sorry to hear this! I will take a look tonight and see what is causing this.

@nagyesta
Copy link
Owner

Thanks @Xor-el! Sure, I will, just waiting for Renovate to raise the PR when the artifact becomes usable.

This is complete in the meantime.

@Xor-el
Copy link
Author

Xor-el commented Jan 18, 2025

@nagyesta here is a screenshot of the exception

Image

nagyesta added a commit that referenced this issue Jan 18, 2025
- Adds better URI validation to filter out invalid host names
- Updates tests
- Updates documentation

Updates #1319
{patch}

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
nagyesta added a commit that referenced this issue Jan 18, 2025
- Adds better URI validation to filter out invalid host names
- Updates tests
- Updates documentation

Updates #1319
{patch}

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
@nagyesta
Copy link
Owner

I have found the root cause @Xor-el ! The Java URI parser implementation, which is required for the dynamic ports to work, is not able to find the "host" part of the https://demo.127.0.0.1:8443 URI. Probably because it should either contain an IP address or DNS name labels, but we cannot mix the two. As soon as I changed the alias to a valid name, it worked.

Based on this, I have only made it slightly nicer to produce a more informative log. Hopefully this will help the users find the issues in the input parameters:

Caused by: java.lang.IllegalArgumentException: URI couldn't be parsed: https://demo.127.0.0.1:8443
	at com.github.nagyesta.lowkeyvault.context.util.VaultUriUtil.vaultUri(VaultUriUtil.java:29)
	at com.github.nagyesta.lowkeyvault.context.util.VaultUriUtil.aliasUri(VaultUriUtil.java:44)
	at com.github.nagyesta.lowkeyvault.AppConfiguration.lambda$doAddVaultAliases$4(AppConfiguration.java:98)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)

This small fix will be available in v2.7.1 when the release process completes.

@Xor-el
Copy link
Author

Xor-el commented Jan 19, 2025

Hello @nagyesta thanks for the explanation, well understood.
I think we can close this issue now.

@nagyesta
Copy link
Owner

Okay, thank you for your understanding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community requested Issues requested by our community enhancement New feature or request released Released, waiting for feedback
Projects
Status: Released
Development

No branches or pull requests

2 participants