Skip to content
This repository was archived by the owner on Jul 12, 2023. It is now read-only.

Handle GCP project not found #1092

Merged
merged 6 commits into from
Jul 6, 2023
Merged

Handle GCP project not found #1092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d74041
Handle GCP project not found
honnix Jul 5, 2023
57af368
Update test case
honnix Jul 5, 2023
a2cbc40
Minor tweak to logs
honnix Jul 6, 2023
18fd07c
Fix wrong unit test
honnix Jul 6, 2023
bf43caf
Update styx-service-common/src/main/java/com/spotify/styx/api/Authent…
honnix Jul 6, 2023
bb70f26
Update styx-service-common/src/main/java/com/spotify/styx/api/Service…
honnix Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions styx-service-common/src/main/java/com/spotify/styx/api/Authenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import static com.github.rholder.retry.StopStrategies.stopAfterDelay;
import static com.github.rholder.retry.WaitStrategies.exponentialWait;
import static com.spotify.apollo.Status.FORBIDDEN;
import static com.spotify.apollo.Status.NOT_FOUND;
import static com.spotify.styx.util.GoogleApiClientUtil.executeWithRetries;
import static java.util.concurrent.TimeUnit.SECONDS;

Expand Down Expand Up @@ -279,7 +281,7 @@ private String getProjectIdOfServiceAccount(String email) throws IOException {
var serviceAccount = executeWithRetries(request, retryWaitStrategy, retryStopStrategy);
return serviceAccount.getProjectId();
} catch (GoogleJsonResponseException e) {
if (e.getStatusCode() == 404) {
if (e.getStatusCode() == NOT_FOUND.code()) {
logger.debug("Service account {} doesn't exist", email, e);
return null;
}
Expand All @@ -295,12 +297,14 @@ private boolean resolveProjectAccess(String projectId) throws IOException {
try {
ancestry = executeWithRetries(request, retryWaitStrategy, retryStopStrategy);
} catch (GoogleJsonResponseException e) {
if (e.getStatusCode() == 404) {
logger.debug("Project {} doesn't exist", projectId, e);
return false;
// GCP returns 403 in case of project not found for security reason, but that makes it impossible
// to differentiate that from missing permission; we take the risk here assuming Styx service account does have
// proper permissions
if (e.getStatusCode() == FORBIDDEN.code()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Real change.

logger.debug("Project {} doesn't exist or Styx lacks permissions", projectId, e);
} else {
logger.info("Cannot get project with id {}", projectId, e);
}

logger.info("Cannot get project with id {}", projectId, e);
return false;
}
if (ancestry.getAncestor() == null) {
Expand Down
19 changes: 12 additions & 7 deletions styx-service-common/src/main/java/com/spotify/styx/api/ServiceAccountUsageAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static com.google.api.services.directory.DirectoryScopes.ADMIN_DIRECTORY_GROUP_MEMBER_READONLY;
import static com.spotify.apollo.Status.BAD_REQUEST;
import static com.spotify.apollo.Status.FORBIDDEN;
import static com.spotify.apollo.Status.NOT_FOUND;
import static com.spotify.styx.util.ConfigUtil.get;
import static java.lang.Boolean.TRUE;
import static java.util.concurrent.TimeUnit.SECONDS;
Expand Down Expand Up @@ -63,6 +64,7 @@
import com.google.common.io.Closer;
import com.spotify.apollo.Environment;
import com.spotify.apollo.Response;
import com.spotify.apollo.StatusType.Family;
import com.spotify.styx.model.WorkflowId;
import com.typesafe.config.Config;
import io.norberg.automatter.AutoMatter;
Expand Down Expand Up @@ -348,7 +350,7 @@ private String lookupServiceAccountProjectId(String email) {
} catch (ExecutionException e) {
final Throwable cause = e.getCause();
if (cause instanceof GoogleJsonResponseException
&& ((GoogleJsonResponseException) cause).getStatusCode() == 404) {
&& ((GoogleJsonResponseException) cause).getStatusCode() == NOT_FOUND.code()) {
log.debug("Service account {} doesn't exist", email, e);
throw new ResponseException(Response.forStatus(
BAD_REQUEST.withReasonPhrase("Service account does not exist: " + email)));
Expand Down Expand Up @@ -413,11 +415,11 @@ private boolean isMemberOfGroup(String principalEmail, String group) {
var statusCode = ((GoogleJsonResponseException) cause).getStatusCode();
// hasMember API returns 404 if the group does not exist, while returning 400 if the principal
// email does not exist or does not have the same domain as the group if it is enforced
if (statusCode == 400) {
if (statusCode == BAD_REQUEST.code()) {
log.info("Principal {} does not exist or belongs to different domain than group {}",
principalEmail, group);
return false;
} else if (statusCode == 404) {
} else if (statusCode == NOT_FOUND.code()) {
log.info("Group {} does not exist", group);
return false;
}
Expand All @@ -436,9 +438,12 @@ private Optional<com.google.api.services.cloudresourcemanager.model.Policy> getP
.execute()));
} catch (ExecutionException e) {
final Throwable cause = e.getCause();
// GCP returns 403 in case of project not found for security reason, but that makes it impossible
// to differentiate that from missing permission; we take the risk here assuming Styx service account does have
// proper permissions
if (cause instanceof GoogleJsonResponseException
&& ((GoogleJsonResponseException) cause).getStatusCode() == 404) {
log.info("Project {} does not exist", projectId, cause);
&& ((GoogleJsonResponseException) cause).getStatusCode() == FORBIDDEN.code()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Real change.

log.info("Project {} does not exist or Styx lacks permissions", projectId, cause);
return Optional.empty();
}
throw new RuntimeException(e);
Expand All @@ -455,7 +460,7 @@ private Optional<com.google.api.services.iam.v1.model.Policy> getServiceAccountP
} catch (ExecutionException e) {
final Throwable cause = e.getCause();
if (cause instanceof GoogleJsonResponseException
&& ((GoogleJsonResponseException) cause).getStatusCode() == 404) {
&& ((GoogleJsonResponseException) cause).getStatusCode() == NOT_FOUND.code()) {
log.info("Service account {} does not exist", serviceAccount, cause);
return Optional.empty();
}
Expand Down Expand Up @@ -485,7 +490,7 @@ private static <T> void onRequestAttempt(Attempt<T> attempt) {
private static boolean isRetryableException(Throwable t) {
if (t instanceof IOException) {
if (t instanceof GoogleJsonResponseException) {
return ((GoogleJsonResponseException) t).getStatusCode() / 100 == 5;
return Family.familyOf(((GoogleJsonResponseException) t).getStatusCode()) == Family.SERVER_ERROR;
}
return true;
}
Expand Down
78 changes: 43 additions & 35 deletions styx-service-common/src/test/java/com/spotify/styx/api/AuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no test we need to change for the 404->403 code change in styx-service-common/src/main/java/com/spotify/styx/api/Authenticator.java ?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/spotify/styx/pull/1092/files#diff-6d62dc7647527de82216a154436bab32cb014d6a27fc005028168031ea1a95f2R313-R332f those are the changes. There is no externally observable difference though because we treat exceptions the same except the logging.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah true

package com.spotify.styx.api;

import static com.spotify.apollo.Status.FORBIDDEN;
import static com.spotify.apollo.Status.TOO_MANY_REQUESTS;
import static com.spotify.styx.api.Authenticator.resourceId;
import static java.util.stream.Collectors.toList;
import static org.hamcrest.MatcherAssert.assertThat;
Expand All @@ -37,7 +39,7 @@
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is deprecated.

import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import com.github.rholder.retry.StopStrategies;
Expand All @@ -58,6 +60,7 @@
import com.google.api.services.cloudresourcemanager.model.ResourceId;
import com.google.api.services.iam.v1.Iam;
import com.google.api.services.iam.v1.model.ServiceAccount;
import com.spotify.apollo.Status;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.util.List;
Expand Down Expand Up @@ -113,19 +116,24 @@ public class AuthenticatorTest {

private static final GoogleJsonResponseException PERMISSION_DENIED =
new GoogleJsonResponseException(
new HttpResponseException.Builder(403, "Forbidden", new HttpHeaders()),
new HttpResponseException.Builder(FORBIDDEN.code(), "Forbidden", new HttpHeaders()),
new GoogleJsonError().set("status", "PERMISSION_DENIED"));

private static final GoogleJsonResponseException NOT_FOUND =
new GoogleJsonResponseException(
new HttpResponseException.Builder(404, "Not found", new HttpHeaders()),
new HttpResponseException.Builder(Status.NOT_FOUND.code(), "Not found", new HttpHeaders()),
new GoogleJsonError().set("status", "NOT_FOUND"));

private static final GoogleJsonResponseException QUOTA_EXHAUSTED =
new GoogleJsonResponseException(
new HttpResponseException.Builder(429, "per-user search quota temporarily exhausted", new HttpHeaders()),
new HttpResponseException.Builder(TOO_MANY_REQUESTS.code(), "per-user search quota temporarily exhausted", new HttpHeaders()),
new GoogleJsonError().set("status", "RESOURCE_EXHAUSTED"));

private static final GoogleJsonResponseException IM_A_TEAPOT =
new GoogleJsonResponseException(
new HttpResponseException.Builder(Status.IM_A_TEAPOT.code(), "I'm a Teapot", new HttpHeaders()),
new GoogleJsonError().set("status", "IM_A_TEAPOT"));

private static final WaitStrategy RETRY_WAIT_STRATEGY = WaitStrategies.noWait();
private static final StopStrategy RETRY_STOP_STRATEGY = StopStrategies.stopAfterAttempt(3);

Expand Down Expand Up @@ -220,37 +228,37 @@ public void shouldFailIfInvalidToken() throws GeneralSecurityException, IOExcept
when(verifier.verify(anyString())).thenThrow(new GeneralSecurityException());
assertThat(validator.authenticate("token"), is(nullValue()));

verifyZeroInteractions(idToken);
verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(idToken);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
public void shouldFailToVerifyToken() throws GeneralSecurityException, IOException {
when(verifier.verify(anyString())).thenThrow(new IOException());
assertThat(validator.authenticate("token"), is(nullValue()));

verifyZeroInteractions(idToken);
verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(idToken);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
public void shouldBeWhitelisted() {
idTokenPayload.setEmail("[email protected]");
assertThat(validator.authenticate("token"), is(idToken));

verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
public void shouldFailIfInvalidEmailAddress() {
idTokenPayload.setEmail("example.com");
assertThat(validator.authenticate("token"), is(nullValue()));

verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
Expand All @@ -266,8 +274,8 @@ public void shouldHitProjectCache() throws IOException {
assertThat(validator.authenticate("token"), is(idToken));

verify(cloudResourceManager.projects(), never()).getAncestry(eq("foo"), any());
verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
Expand All @@ -280,7 +288,7 @@ public void shouldMissProjectCache() throws IOException {

verify(cloudResourceManager.projects()).getAncestry(eq(UNCACHED_PROJECT.getProjectId()), any());
verify(projectsGetAncestry).execute();
verifyZeroInteractions(iam);
verifyNoMoreInteractions(iam);
}

private static GetAncestryResponse ancestryResponse(ResourceId... ancestors) {
Expand All @@ -297,29 +305,29 @@ public void shouldFailToGetProject() throws IOException {
idTokenPayload.setEmail("[email protected]");
assertThat(validator.authenticate("token"), is(nullValue()));

verifyZeroInteractions(iam);
verifyNoMoreInteractions(iam);
verify(projectsGetAncestry, times(3)).execute();
}

@Test
public void shouldFailForNonExistProject() throws IOException {
doThrow(NOT_FOUND).when(projectsGetAncestry).execute();
doThrow(PERMISSION_DENIED).when(projectsGetAncestry).execute();

idTokenPayload.setEmail("[email protected]");
assertThat(validator.authenticate("token"), is(nullValue()));

verify(projectsGetAncestry).execute();
verifyZeroInteractions(iam);
verifyNoMoreInteractions(iam);
}

@Test
public void shouldFailIfNoPermissionGettingProject() throws IOException {
doThrow(PERMISSION_DENIED).when(projectsGetAncestry).execute();
public void shouldFailIfOtherErrorGettingProject() throws IOException {
doThrow(IM_A_TEAPOT).when(projectsGetAncestry).execute();

idTokenPayload.setEmail("[email protected]");
assertThat(validator.authenticate("token"), is(nullValue()));

verifyZeroInteractions(iam);
verifyNoMoreInteractions(iam);
verify(projectsGetAncestry).execute();
}

Expand All @@ -336,8 +344,8 @@ public void shouldHitValidatedEmailCache() throws IOException {
reset(projectsGetAncestry);
assertThat(validator.authenticate("token"), is(idToken));

verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
Expand All @@ -348,7 +356,7 @@ public void shouldGetProjectFromIAMAndThenHitProjectCache() throws IOException {
assertThat(validator.authenticate("token"), is(idToken));

verify(serviceAccountsGet).execute();
verifyZeroInteractions(projectsGetAncestry);
verifyNoMoreInteractions(projectsGetAncestry);
}

@Test
Expand All @@ -358,7 +366,7 @@ public void shouldFailToGetProjectFromIAM() throws IOException {
assertThat(validator.authenticate("token"), is(nullValue()));

verify(serviceAccountsGet, times(3)).execute();
verifyZeroInteractions(projectsGetAncestry);
verifyNoMoreInteractions(projectsGetAncestry);
}

@Test
Expand All @@ -368,7 +376,7 @@ public void shouldFailForNonExistServiceAccount() throws IOException {
assertThat(validator.authenticate("token"), is(nullValue()));

verify(serviceAccountsGet).execute();
verifyZeroInteractions(projectsGetAncestry);
verifyNoMoreInteractions(projectsGetAncestry);
}

@Test
Expand All @@ -378,32 +386,32 @@ public void shouldFailIfNoPermissionGettingServiceAccountFromIAM() throws IOExce
assertThat(validator.authenticate("token"), is(nullValue()));

verify(serviceAccountsGet).execute();
verifyZeroInteractions(projectsGetAncestry);
verifyNoMoreInteractions(projectsGetAncestry);
}

@Test
public void shouldFailTokenWithNoEmail() {
idTokenPayload.setEmail(null);
assertThat(validator.authenticate("token"), is(nullValue()));
verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
public void shouldDenyWhitelistedEmailNonSA() {
idTokenPayload.setEmail("[email protected]");
assertThat(validator.authenticate("token"), is(nullValue()));
verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
public void shouldDenyIncorrectAudience() {
idTokenPayload.setAudience("https://foo.example.net");
idTokenPayload.setEmail("[email protected]");
assertThat(validator.authenticate("token"), is(nullValue()));
verifyZeroInteractions(projectsGetAncestry);
verifyZeroInteractions(iam);
verifyNoMoreInteractions(projectsGetAncestry);
verifyNoMoreInteractions(iam);
}

@Test
Expand Down
Loading