Use local GCP credentials, if any, in local docker environment. #5827
Conversation
| @@ -168,4 +173,21 @@ public RemoteEnvironment createEnvironment(Environment environment) throws Excep | |||
|
|
|||
| return DockerContainerEnvironment.create(docker, environment, containerId, instructionHandler); | |||
| } | |||
|
|
|||
| private List<String> gcsCredentialArgs() { | |||
| String dockerGcloudConfig = "/root/lconfig/gcloud"; | |||
There was a problem hiding this comment.
What happens if .config/gcloud already exists?
There was a problem hiding this comment.
I thought lconfig is a typo - it's not a folder recognized by gcloud, but .config is. I'm not sure what happens if it already exists, but lconfig doesn't look right.
There was a problem hiding this comment.
I'm not sure what happens if .config/gcloud already exists, but it seems unlikely to exist in the base image, at the very least for security reasons. I think we can be relatively confident this will not collide.
In any case, if we wanted to use lconfig, we would at the very least have to set a special environment variable. For example, CLOUDSDK_CONFIG=$HOME/lconfig/gcloud would work for gcloud. I'm not even sure that would be picked up by the Java code that actually requires authentication.
There was a problem hiding this comment.
Yes, the environment variable is set below.
On the other hand, if we're pretty confident about non-collision, maybe we should just hook it up into the "extra mount points" code where it would just be a "~/.config/gcloud" -> "/root/.config/gcloud" entry.
There was a problem hiding this comment.
OK, looks like this may actually be respected by the Java auth library: googleapis/google-auth-library-java#63. If we've tested this and are pretty confident, this should work. It would be good to at least include an inline comment about why we're not using the standard .config directory.
There was a problem hiding this comment.
Given this discussion, I've simplified to just use .config/gcloud.
| ImmutableList.of( | ||
| // NOTE: Host networking does not work on Mac, but the command line flag is accepted. | ||
| "--network=host"); | ||
| List<String> volArg = new ArrayList<>(); |
There was a problem hiding this comment.
Any reason not to use ImmutableList.Builder?
There was a problem hiding this comment.
Yes, much nicer. Done.
| // NOTE: Host networking does not work on Mac, but the command line flag is accepted. | ||
| "--network=host"); | ||
| List<String> volArg = new ArrayList<>(); | ||
| volArg.addAll(gcsCredentialArgs()); |
There was a problem hiding this comment.
I don't thinnk we always want to mount this. Can you make doing so a job server command line arg and plumb it through?
robertwb
force-pushed
the
docker-gcs
branch
2 times, most recently
from
d15e2d0
to
8aa25f7
Compare
|
It was unclear exactly how this should be plumbed through--should it be a pipeline option or a job service option? Also, this to be desirable if (and only if) docker is being run manually (which is not really a Flink vs. non-Flink issue). I filed BEAM-4729 to track figuring out the configuration, but think it's better to have this on unconditionally than never in the meantime to make running locally usable. PTAL. |
|
The plumbing may be somewhat painful, so it's probably fine to leave this as is for now. However, when we do plumb it, it should be a job server option because it's fundamentally a runner option. For example, it only makes sense to do this mount if the job server is running locally. |
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.It will help us expedite review of your Pull Request if you tag someone (e.g.
@username) to look at it.Post-Commit Tests Status (on master branch)