Skip to content
This repository has been archived by the owner on Nov 11, 2022. It is now read-only.

Try to find the active gcloud config #348

Closed
wants to merge 2 commits into from
Closed

Try to find the active gcloud config #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62db4af
Try to find the active gcloud config
Jul 28, 2016
4261b7b
Add tests for active gcloud config
Jul 28, 2016
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
74 changes: 59 additions & 15 deletions sdk/src/main/java/com/google/cloud/dataflow/sdk/options/GcpOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.security.GeneralSecurityException;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -183,24 +184,67 @@ void setCredentialFactoryClass(
public static class DefaultProjectFactory implements DefaultValueFactory<String> {
private static final Logger LOG = LoggerFactory.getLogger(DefaultProjectFactory.class);

private static String getUserHome() {
return System.getProperty("user.home");
}

private String getActiveConfig() throws IOException {
final String defaultConfigName = "default";

// get active configuration
File activeConfFile = new File(
getUserHome(),
".config/gcloud/active_config");
Copy link
Contributor

@lukecwik lukecwik Jul 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Do the paths specified in these locations work on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - there is if branch for windows before, also previous version of the code assumed the same hierarchy for all platforms (here). What would you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems as though Google is vending a library that already does this and instead of Dataflow owning this logic, we can rely on this library but wanted to make sure that this meets your needs:

https://github.com/GoogleCloudPlatform/gcloud-java/blob/master/gcloud-java-core/src/main/java/com/google/cloud/ServiceOptions.java
Look at the logic for defaultProject(), specifically activeGoogleCloudConfig() and googleCloudProjectId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @lukecwik - was on vacation. I see, thanks for the gcloud-java link, I looked at the code, and it's pretty much the same. afaiu current DF does not check for:

    String projectId = System.getProperty(PROJECT_ENV_NAME, System.getenv(PROJECT_ENV_NAME));
    if (projectId == null) {
      projectId = appEngineProjectId();
    }
    if (projectId == null) {
      projectId = serviceAccountProjectId();
    }

Specifically serviceAccountProjectId could be interesting, afaiu it would discover if user is using a service account through GOOGLE_APPLICATION_CREDENTIALS and then if available, fetch the project from the credentials file. One could argue if that is good or bad idea, probably good for defaultProject.

It seems like we could push this PR, I could submit PR to gcloud-java to abstract the project discover logic away from ServiceOptions, then I could submit another PR to DF to start using gcloud-java? Does it sound like a good plan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping @lukecwik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping 2x @lukecwik - do you think we should wait for google-cloud-java or move forward?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would wait for google-cloud-java, it seems as though they are working towards the change as evidenced on googleapis/google-cloud-java#1207


if (!activeConfFile.exists()) {
LOG.debug("No active configuration file - use default");
return defaultConfigName;
}

List<String> config = Files.readLines(activeConfFile, StandardCharsets.UTF_8);

if (config.size() != 1) {
LOG.debug("Active config file {} corrupted - use default",
activeConfFile.getCanonicalPath());
return defaultConfigName;
}

String configName = config.get(0);

if (configName.isEmpty()) {
LOG.debug("Active config in {} is empty - use default", activeConfFile);
return defaultConfigName;
}

return configName;
}

private File getConfigFile() throws IOException {
File configFile;
if (getEnvironment().containsKey("CLOUDSDK_CONFIG")) {
configFile = new File(getEnvironment().get("CLOUDSDK_CONFIG"), "properties");
} else if (isWindows() && getEnvironment().containsKey("APPDATA")) {
configFile = new File(getEnvironment().get("APPDATA"), "gcloud/properties");
} else {
final String configName = getActiveConfig();

// New versions of gcloud use this file
configFile = new File(
getUserHome(),
".config/gcloud/configurations/config_" + configName);

if (!configFile.exists()) {
// Old versions of gcloud use this file
configFile = new File(getUserHome(), ".config/gcloud/properties");
}
}
return configFile;
}

@Override
public String create(PipelineOptions options) {
try {
File configFile;
if (getEnvironment().containsKey("CLOUDSDK_CONFIG")) {
configFile = new File(getEnvironment().get("CLOUDSDK_CONFIG"), "properties");
} else if (isWindows() && getEnvironment().containsKey("APPDATA")) {
configFile = new File(getEnvironment().get("APPDATA"), "gcloud/properties");
} else {
// New versions of gcloud use this file
configFile = new File(
System.getProperty("user.home"),
".config/gcloud/configurations/config_default");
if (!configFile.exists()) {
// Old versions of gcloud use this file
configFile = new File(System.getProperty("user.home"), ".config/gcloud/properties");
}
}
File configFile = getConfigFile();
String section = null;
Pattern projectPattern = Pattern.compile("^project\\s*=\\s*(.*)$");
Pattern sectionPattern = Pattern.compile("^\\[(.*)\\]$");
Expand Down
75 changes: 75 additions & 0 deletions sdk/src/test/java/com/google/cloud/dataflow/sdk/options/GcpOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,76 @@ public void testGetProjectFromUserHomeOldAndNewPrefersNew() throws Exception {
environment));
}

@Test
public void testGetProjectFromUserHomeDefaultAndActivePrefersActive() throws Exception {
Map<String, String> environment = ImmutableMap.of();
System.setProperty("user.home", tmpFolder.getRoot().getAbsolutePath());

makeActiveConfigFile(new File(tmpFolder.newFolder(".config", "gcloud"), "active_config"),
"testactive"); // active configuration name

File configurations = tmpFolder.newFolder(".config", "gcloud", "configurations");
makePropertiesFileWithProject(
new File(configurations, "config_testactive"),
"active-project"); // name of the active project

assertEquals("active-project",
runGetProjectTest(
new File(configurations, "config_default"),
environment));
}

@Test
public void testGetProjectFromUserHomeDefaultAndEmptyActivePrefersDefault() throws Exception {
Map<String, String> environment = ImmutableMap.of();
System.setProperty("user.home", tmpFolder.getRoot().getAbsolutePath());

makeActiveConfigFile(new File(tmpFolder.newFolder(".config", "gcloud"), "active_config"),
""); // active configuration name

File configurations = tmpFolder.newFolder(".config", "gcloud", "configurations");
makePropertiesFileWithProject(
new File(configurations, "config_testactive"),
"active-project"); // name of the active project

assertEquals("test-project",
runGetProjectTest(
new File(configurations, "config_default"),
environment));
}

@Test
public void testGetProjectFromUserHomeDefaultAndInvalidActivePrefersDefault() throws Exception {
Map<String, String> environment = ImmutableMap.of();
System.setProperty("user.home", tmpFolder.getRoot().getAbsolutePath());

makeActiveConfigFile(new File(tmpFolder.newFolder(".config", "gcloud"), "active_config"),
"testactive\ntestactive2"); // active configuration name

File configurations = tmpFolder.newFolder(".config", "gcloud", "configurations");
makePropertiesFileWithProject(
new File(configurations, "config_testactive"),
"active-project"); // name of the active project

assertEquals("test-project",
runGetProjectTest(
new File(configurations, "config_default"),
environment));
}

@Test
public void testGetProjectFromUserHomeOldAndNoActiveFilePrefersOld() throws Exception {
Map<String, String> environment = ImmutableMap.of();
System.setProperty("user.home", tmpFolder.getRoot().getAbsolutePath());

File gcloudFolder = tmpFolder.newFolder(".config", "gcloud");
makeActiveConfigFile(new File(gcloudFolder, "active_config"), "testactive");
assertEquals("test-project",
runGetProjectTest(
new File(gcloudFolder, "properties"),
environment));
}

@Test
public void testUnableToGetDefaultProject() throws Exception {
System.setProperty("user.home", tmpFolder.getRoot().getAbsolutePath());
Expand All @@ -101,6 +171,11 @@ public void testUnableToGetDefaultProject() throws Exception {
assertNull(projectFactory.create(PipelineOptionsFactory.create()));
}

private static void makeActiveConfigFile(File path, String activeConfigName)
throws IOException {
Files.write(activeConfigName, path, StandardCharsets.UTF_8);
}

private static void makePropertiesFileWithProject(File path, String projectId)
throws IOException {
String properties = String.format("[core]%n"
Expand Down