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

Consider not using UUID.ramdomUUID() #5735

Closed
ikhoon opened this issue Jan 31, 2024 · 3 comments · Fixed by #5815
Closed

Consider not using UUID.ramdomUUID() #5735

ikhoon opened this issue Jan 31, 2024 · 3 comments · Fixed by #5815
Assignees
Milestone

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Jan 31, 2024

Describe the bug

Problem

Armeria uses https://github.com/reactor/BlockHound to detect blocking IO on non-blocking threads such as event loops.
BlockHound agent reported UUID.ramdomUUID() as an improper usage. Because SecureRandom used in UUID.ramdomUUID() triggers blocking operations.

java.lang.Exception: java.io.FileInputStream#readBytes
	at com.linecorp.armeria.internal.testing.InternalTestingBlockHoundIntegration.writeBlockingMethod(InternalTestingBlockHoundIntegration.java:84)
	at reactor.blockhound.BlockHound$Builder.lambda$install$8(BlockHound.java:472)
	at reactor.blockhound.BlockHoundRuntime.checkBlocking(BlockHoundRuntime.java:89)
	at java.base/java.io.FileInputStream.readBytes(FileInputStream.java)
	at java.base/java.io.FileInputStream.read(FileInputStream.java:293)
	at java.base/java.io.FilterInputStream.read(FilterInputStream.java:119)
	at java.base/sun.security.provider.NativePRNG$RandomIO.readFully(NativePRNG.java:425)
	at java.base/sun.security.provider.NativePRNG$RandomIO.ensureBufferValid(NativePRNG.java:528)
	at java.base/sun.security.provider.NativePRNG$RandomIO.implNextBytes(NativePRNG.java:547)
	at java.base/sun.security.provider.NativePRNG.engineNextBytes(NativePRNG.java:221)
	at java.base/java.security.SecureRandom.nextBytes(SecureRandom.java:758)
	at java.base/java.util.UUID.randomUUID(UUID.java:151)
	at io.fabric8.kubernetes.client.http.StandardHttpRequest.<init>(StandardHttpRequest.java:116)
	at io.fabric8.kubernetes.client.http.StandardHttpRequest$Builder.build(StandardHttpRequest.java:201)

If the UUID is used for logging and tracing, I don't think a SecureRandom is genuinely necessary.

Suggestion

I propose to directly create UUID with a random variable or an atomic value. For example:

Fabric8 Kubernetes Client version

SNAPSHOT

Steps to reproduce

Create StandardHttpRequest

Expected behavior

Not to use SecureRandom.

Runtime

minikube

Kubernetes API Server version

1.25.3@latest

Environment

Linux

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

ikhoon added a commit to ikhoon/armeria that referenced this issue Jan 31, 2024
Motivation:

BlockHound reported that
`io.fabric8.kubernetes.client.http.StandardHttpRequest` uses a blocking
call.
https://github.com/line/armeria/actions/runs/7721798741/job/21048920064?pr=5370#step:17:30
```
java.lang.Exception: java.io.FileInputStream#readBytes
	at com.linecorp.armeria.internal.testing.InternalTestingBlockHoundIntegration.writeBlockingMethod(InternalTestingBlockHoundIntegration.java:84)
	at reactor.blockhound.BlockHound$Builder.lambda$install$8(BlockHound.java:472)
	at reactor.blockhound.BlockHoundRuntime.checkBlocking(BlockHoundRuntime.java:89)
	at java.base/java.io.FileInputStream.readBytes(FileInputStream.java)
	at java.base/java.io.FileInputStream.read(FileInputStream.java:293)
	at java.base/java.io.FilterInputStream.read(FilterInputStream.java:119)
	at java.base/sun.security.provider.NativePRNG$RandomIO.readFully(NativePRNG.java:425)
	at java.base/sun.security.provider.NativePRNG$RandomIO.ensureBufferValid(NativePRNG.java:528)
	at java.base/sun.security.provider.NativePRNG$RandomIO.implNextBytes(NativePRNG.java:547)
	at java.base/sun.security.provider.NativePRNG.engineNextBytes(NativePRNG.java:221)
	at java.base/java.security.SecureRandom.nextBytes(SecureRandom.java:758)
	at java.base/java.util.UUID.randomUUID(UUID.java:151)
	at io.fabric8.kubernetes.client.http.StandardHttpRequest.<init>(StandardHttpRequest.java:116)
	at io.fabric8.kubernetes.client.http.StandardHttpRequest$Builder.build(StandardHttpRequest.java:201)
```

There is nothing we can do except add an exception rule for that.
So the issue is reported to the upstream.
fabric8io/kubernetes-client#5735
The blocking call is temporarily allowed until the problem is resolved
in the upstream.

Modifications:

- Allow blocking calls inside `StandardHttpRequest$Builder.build()`.

Result:

Make CI build pass to identify meaningful problems.
@manusa
Copy link
Member

manusa commented Jan 31, 2024

I think it's only used to create simple IDs, a solution such as the one Spring Framework uses should definitely work.

@manusa manusa added this to the 6.11.0 milestone Jan 31, 2024
@shawkins
Copy link
Contributor

shawkins commented Jan 31, 2024

We can also consider just removing / deprecating from StandardHttpRequest - it's not currently used because we changed the way the logging logic worked.

@manusa
Copy link
Member

manusa commented Jan 31, 2024

It's used in multiple places (besides StandardHttpRequest) for similar purposes.

(PostHandler, PodUpload, and also on NamespaceExtension, WebSocketSession)

ikhoon added a commit to line/armeria that referenced this issue Feb 1, 2024
…ng (#5430)

Motivation:

BlockHound reported that
`io.fabric8.kubernetes.client.http.StandardHttpRequest` invoked blocking
calls.

https://github.com/line/armeria/actions/runs/7721798741/job/21048920064?pr=5370#step:17:30
```
java.lang.Exception: java.io.FileInputStream#readBytes
	at com.linecorp.armeria.internal.testing.InternalTestingBlockHoundIntegration.writeBlockingMethod(InternalTestingBlockHoundIntegration.java:84)
	at reactor.blockhound.BlockHound$Builder.lambda$install$8(BlockHound.java:472)
	at reactor.blockhound.BlockHoundRuntime.checkBlocking(BlockHoundRuntime.java:89)
	at java.base/java.io.FileInputStream.readBytes(FileInputStream.java)
	at java.base/java.io.FileInputStream.read(FileInputStream.java:293)
	at java.base/java.io.FilterInputStream.read(FilterInputStream.java:119)
	at java.base/sun.security.provider.NativePRNG$RandomIO.readFully(NativePRNG.java:425)
	at java.base/sun.security.provider.NativePRNG$RandomIO.ensureBufferValid(NativePRNG.java:528)
	at java.base/sun.security.provider.NativePRNG$RandomIO.implNextBytes(NativePRNG.java:547)
	at java.base/sun.security.provider.NativePRNG.engineNextBytes(NativePRNG.java:221)
	at java.base/java.security.SecureRandom.nextBytes(SecureRandom.java:758)
	at java.base/java.util.UUID.randomUUID(UUID.java:151)
	at io.fabric8.kubernetes.client.http.StandardHttpRequest.<init>(StandardHttpRequest.java:116)
	at io.fabric8.kubernetes.client.http.StandardHttpRequest$Builder.build(StandardHttpRequest.java:201)
```

There is nothing we can do except add an exceptional rule for that. So
the issue is reported to the upstream.
fabric8io/kubernetes-client#5735 
The blocking call is temporarily allowed until the problem is resolved
in the upstream.

Modifications:

- Allow blocking calls inside `StandardHttpRequest$Builder.build()`.

Result:

Make CI build pass to identify meaningful problems.
@rohanKanojia rohanKanojia self-assigned this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants