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.

Sign up for GitHub

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

Jump to bottom

xds: add null reference checks in SslContextProviderSupplier #8169

Merged
merged 2 commits into from May 12, 2021
Merged

xds: add null reference checks in SslContextProviderSupplier #8169

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9717d5f
xds: add null reference checks in SslContextProviderSupplier
sanjaypujare May 12, 2021
f62629d
address review comment
sanjaypujare May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions xds/src/main/java/io/grpc/xds/internal/sds/SslContextProviderSupplier.java
View file
Open in desktop
Expand Up @@ -41,8 +41,8 @@ public final class SslContextProviderSupplier implements Closeable {

public SslContextProviderSupplier(
BaseTlsContext tlsContext, TlsContextManager tlsContextManager) {
this.tlsContext = tlsContext;
this.tlsContextManager = tlsContextManager;
this.tlsContext = checkNotNull(tlsContext, "tlsContext");
this.tlsContextManager = checkNotNull(tlsContextManager, "tlsContextManager");
}

public BaseTlsContext getTlsContext() {
Expand Down Expand Up @@ -92,10 +92,12 @@ private SslContextProvider getSslContextProvider() {
/** Called by consumer when tlsContext changes. */
@Override
public synchronized void close() {
if (tlsContext instanceof UpstreamTlsContext) {
tlsContextManager.releaseClientSslContextProvider(sslContextProvider);
} else {
tlsContextManager.releaseServerSslContextProvider(sslContextProvider);
if (sslContextProvider != null) {
if (tlsContext instanceof UpstreamTlsContext) {
tlsContextManager.releaseClientSslContextProvider(sslContextProvider);
} else {
tlsContextManager.releaseServerSslContextProvider(sslContextProvider);
}
}
shutdown = true;
}
Expand Down
37 changes: 29 additions & 8 deletions xds/src/test/java/io/grpc/xds/internal/sds/SslContextProviderSupplierTest.java
View file
Open in desktop
Expand Up @@ -23,7 +23,9 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

Expand Down Expand Up @@ -57,24 +59,26 @@ public void setUp() {
MockitoAnnotations.initMocks(this);
}

private void prepareSupplier() {
private void prepareSupplier(boolean callUpdateSslContext) {
upstreamTlsContext =
CommonTlsContextTestsUtil.buildUpstreamTlsContextFromFilenames(
CLIENT_KEY_FILE, CLIENT_PEM_FILE, CA_PEM_FILE);
mockSslContextProvider = mock(SslContextProvider.class);
doReturn(mockSslContextProvider)
.when(mockTlsContextManager)
.findOrCreateClientSslContextProvider(eq(upstreamTlsContext));
mockCallback = mock(SslContextProvider.Callback.class);
Executor mockExecutor = mock(Executor.class);
doReturn(mockExecutor).when(mockCallback).getExecutor();
supplier = new SslContextProviderSupplier(upstreamTlsContext, mockTlsContextManager);
supplier.updateSslContext(mockCallback);
if (callUpdateSslContext) {
mockCallback = mock(SslContextProvider.Callback.class);
Executor mockExecutor = mock(Executor.class);
doReturn(mockExecutor).when(mockCallback).getExecutor();
supplier.updateSslContext(mockCallback);
}
}

@Test
public void get_updateSecret() {
prepareSupplier();
prepareSupplier(true);
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest use two separate methods:

prepareSupplier();
updateSslContext();

The boolean arg true looks less readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

verify(mockTlsContextManager, times(2))
.findOrCreateClientSslContextProvider(eq(upstreamTlsContext));
verify(mockTlsContextManager, times(0))
Expand All @@ -96,7 +100,7 @@ public void get_updateSecret() {

@Test
public void get_onException() {
prepareSupplier();
prepareSupplier(true);
ArgumentCaptor<SslContextProvider.Callback> callbackCaptor = ArgumentCaptor.forClass(null);
verify(mockSslContextProvider, times(1)).addCallback(callbackCaptor.capture());
SslContextProvider.Callback capturedCallback = callbackCaptor.getValue();
Expand All @@ -108,7 +112,7 @@ public void get_onException() {

@Test
public void testClose() {
prepareSupplier();
prepareSupplier(true);
supplier.close();
verify(mockTlsContextManager, times(1))
.releaseClientSslContextProvider(eq(mockSslContextProvider));
Expand All @@ -120,4 +124,21 @@ public void testClose() {
assertThat(expected).hasMessageThat().isEqualTo("Supplier is shutdown!");
}
}

@Test
public void testClose_nullSslContextProvider() {
prepareSupplier(false);
doThrow(new NullPointerException()).when(mockTlsContextManager)
.releaseClientSslContextProvider(null);
supplier.close();
verify(mockTlsContextManager, never())
.releaseClientSslContextProvider(eq(mockSslContextProvider));
SslContextProvider.Callback mockCallback = mock(SslContextProvider.Callback.class);
try {
supplier.updateSslContext(mockCallback);
Assert.fail("no exception thrown");
} catch (IllegalStateException expected) {
assertThat(expected).hasMessageThat().isEqualTo("Supplier is shutdown!");
}
}
}