[BEAM-895] Allow empty GCP credential for pipelines that only access to public data.

[peihe]

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

• Make sure the PR title is formatted like:
  [BEAM-<Jira issue #>] Description of pull request
• Make sure tests pass via mvn clean verify. (Even better, enable
  Travis-CI on your fork and ensure the whole test matrix passes).
• Replace <Jira issue #> in the title with the actual Jira issue
  number, if there is one.
• If this contribution is large, please file an Apache
  Individual Contributor License Agreement.

---

[peihe]

R: @davorbonaci

[lukecwik]

For cases when the user doesn't have credentials and is trying to run a pipeline, how does there error message now change for an unauthorized user. Before we would fail on creation of the client, but now we might try to stage files or contact BQ to see if datasets exist. Is the message as to why it failed same, better or worse off for various GCP service use cases.

[peihe]

1. Users will see the same error message: NULL_CREDENTIAL_REASON.
2. There will be few lines differences about where the exception is thrown.
   We typically do:
   someClient = Transport.newSomeClient(); // previous throws in here
   // few lines between
   someClient.list/insert(); // Now throws in here.

I think this changes should apply to GCP data services: BigQuery, PubSub, Storage, but not CloudResourceManager. Updated to get around this.

[lukecwik]

Does it make sense to leave the contract of this method the same but migrate GcpOptions to ignore failures in getting the credential?

[peihe]

I think we want to fail immediate if users provide a bad key file (line 112).
So, ignoring in here could differentiate the two cases.

[davorbonaci]

Can you update Javadoc of this method to document this new contact, and possibly Javadoc of any calling method that has changed as a consequence?

[davorbonaci]

better name? SpecificExceptionAfterNonauthorizedResponse? something better?

[peihe]

I prefer to keep NullCredential.

SpecificExceptionAfterNonauthorizedResponse is describing the implementation. If we need to do more for a "NullCredential", we will have to change the name. And, NullCredential follows the current pattern, and reads better in Transport as following:
if (credential == null) {
return new ChainingHttpRequestInitializer(new NullCredential(), httpRequestInitializer);
} else {
return new ChainingHttpRequestInitializer(credential, httpRequestInitializer);
}

[davorbonaci]

NullCredentialInitializer?

[davorbonaci]

Also, per Luke's comment -- we should do a manual inspection could any callers be negatively impacted.

[peihe]

Updated and merged with the gcp credentials change.

@lukecwik could you take another look?
Looks like we no longer accept key files and secrets files, right?

[lukecwik]

@peihe, thats correct. The user can call GoogleCredentials.fromStream(InputStream) to load any location they want if they want to use custom credential locations. The credentials library supports many more ways to load/create credentials beyond the few that were supported by Dataflow.

[peihe]

One issue I found is that gcs writes could be buffered in the client, and the exception could be deferred until the file is closed. The result is one exception per bundle in TextIO.Write and BigQueryIO.Write.

I have verified following cases with DirectRunner:
(There is no public folder in gcp, so only one test case for Write.)

1. TextIO.Read from public gcp input files works.
2. TextIO.Read from private gcp input files failed with expected exception.
3. BigQueryIO.Read from public/private dataset failed. Looks to me, BigQuery required authenticated users. Exceptions are expected.
   4.TextIO.Write (and, BigQuery.Write which write to gcs) failed. There is one exception for each write bundle. The exception itself is expected.

[peihe]

Verified TextIO.Write works with a public buckets.

[davorbonaci]

Where does this PR stand? Can we move forward?

[lukecwik]

@davorbonaci My understanding is that your the primary reviewer. Has that changed?

[davorbonaci]

LGTM from a review standpoint.

I know @peihe was investigating a few corner cases, but that seems to be done now. So, we are good to go, I think.

[peihe]

PTAL

done rename

[lukecwik]

I don't think you need the NoopCredentialFactory anymore since the default is to return null which implies to do the service calls with no credentials which is effectively what NoopCredentials is already doing.

[peihe]

Creating a DataflowClient with null credential is considered as a error, since nothing is public with dataflow requests.

NoopCredentialFactory is used by DataflowRunnerTest to create a non-null fake Credentials.