Skip to content

feat: regenerate module for networksecurity#13311

Closed
blakeli0 wants to merge 1 commit into
googleapis:mainfrom
blakeli0:new-library/networksecurity
Closed

feat: regenerate module for networksecurity#13311
blakeli0 wants to merge 1 commit into
googleapis:mainfrom
blakeli0:new-library/networksecurity

Conversation

@blakeli0
Copy link
Copy Markdown
Contributor

Command used:

python3 generation/new_client_hermetic_build/add-new-client-config.py add-new-library
--api-shortname="networksecurity"
--name-pretty="Network Security API"
--proto-path="google/cloud/networksecurity/v1"
--product-docs="https://cloud.google.com/products/networking"
--api-description="The Network Security API provides resources for configuring and managing network security."
--googleapis-committish="29c6482ccb75b302c33665d8c7d7a38d3698be53"
--library-name="network-security"
--release-level="stable"

@blakeli0 blakeli0 requested review from a team as code owners May 29, 2026 20:59
@blakeli0
Copy link
Copy Markdown
Contributor Author

PiperOrigin-RevId: 919268561

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a mechanism to refresh transport channels upon encountering an UnauthenticatedException when running in a specific environment (isMwlidEnvironment), along with rate-limiting for channel pool refreshes. Feedback on the changes highlights a critical bug in GrpcCallContext where a renamed parameter was not updated, leading to a potential NullPointerException. Additionally, the rate-limiting logic in ChannelPool could fail if System.nanoTime() returns a negative value, and querying the environment variable on every retry check in ApiResultRetryAlgorithm should be cached to avoid performance overhead.

Comment on lines +246 to +247
transportChannel.isDirectPath(),
inputChannel);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There is a critical bug here due to the parameter renaming. The parameter was renamed from transportChannel to inputChannel, but transportChannel.isDirectPath() was not updated to inputChannel.isDirectPath().

As a result:

  1. transportChannel now refers to the instance field this.transportChannel, which is null by default, leading to a NullPointerException when withTransportChannel is called.
  2. Even if this.transportChannel is not null, it incorrectly uses the isDirectPath property of the old channel instead of the new inputChannel.

It should be updated to inputChannel.isDirectPath().

Suggested change
transportChannel.isDirectPath(),
inputChannel);
inputChannel.isDirectPath(),
inputChannel);

Comment on lines +445 to +450
long now = System.nanoTime();
if (now - lastRefreshTimeNanos < TimeUnit.SECONDS.toNanos(5)) {
LOG.fine("Channel pool was refreshed recently, skipping duplicate refresh");
return;
}
lastRefreshTimeNanos = now;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since System.nanoTime() can return negative values (as its origin is arbitrary), initializing lastRefreshTimeNanos to 0 can cause the first refresh (and all subsequent refreshes until System.nanoTime() becomes positive) to be skipped. This is because now - 0 will be negative, which is always less than TimeUnit.SECONDS.toNanos(5).

To fix this, you can treat 0 as a sentinel value indicating that no refresh has occurred yet.

Suggested change
long now = System.nanoTime();
if (now - lastRefreshTimeNanos < TimeUnit.SECONDS.toNanos(5)) {
LOG.fine("Channel pool was refreshed recently, skipping duplicate refresh");
return;
}
lastRefreshTimeNanos = now;
long now = System.nanoTime();
if (lastRefreshTimeNanos != 0 && now - lastRefreshTimeNanos < TimeUnit.SECONDS.toNanos(5)) {
LOG.fine("Channel pool was refreshed recently, skipping duplicate refresh");
return;
}
lastRefreshTimeNanos = now == 0 ? 1 : now;

/** Returns true if previousThrowable is an {@link ApiException} that is retryable. */
@Override
public boolean shouldRetry(Throwable previousThrowable, ResponseT previousResponse) {
if ("true".equalsIgnoreCase(System.getenv("isMwlidEnvironment"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Querying environment variables via System.getenv() on every retry check can introduce significant performance overhead in the hot path. Since environment variables do not change during the application's execution, it is highly recommended to cache this value in a private static final boolean field.

@blakeli0 blakeli0 force-pushed the new-library/networksecurity branch from fb34b2d to 4e0c744 Compare May 29, 2026 21:03
@blakeli0 blakeli0 closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant