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.

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

Alternative to GRPC_XDS_BOOSTRAP env variable #7605

Closed
erikjoh opened this issue Nov 9, 2020 · 11 comments
Closed

Alternative to GRPC_XDS_BOOSTRAP env variable #7605

erikjoh opened this issue Nov 9, 2020 · 11 comments

Comments

@erikjoh
Copy link
Contributor

erikjoh commented Nov 9, 2020

Goal

I’m looking into creating a utility function that can dynamically create and assign the xDS bootstrap.
This could be hooked into the application startup so that xDS can be configured across different environments (IDE, GCE, GKE) without the need of startup shell scripts etc.

While almost every programming language supports dynamically setting environment variables, Java does not.
Would it be possible to have an alternative way in java to provide the location of the bootstrap file?

Possible solution

One possible solution that doesn’t break the existing API and gRPC spec would be to also try reading e.g. a java system property (System.getProperty(“grpc.xds.bootstrap”);) at

String filePath = System.getenv(BOOTSTRAP_PATH_SYS_ENV_VAR);
if (filePath == null) {
throw new XdsInitializationException(
"Environment variable " + BOOTSTRAP_PATH_SYS_ENV_VAR + " not defined.");
}

In the case that the GRPC_XDS_BOOSTRAP environment variable isn’t found.

Maybe something as simple as:

private static final String BOOTSTRAP_PATH_SYS_PROPERTY_VAR = "grpc.xds.bootstrap";
.
.
.
String filePath = Optional.ofNullable(System.getenv(BOOTSTRAP_PATH_SYS_ENV_VAR))
                          .orElse(System.getProperty(BOOTSTRAP_PATH_SYS_PROPERTY_VAR)); 

Alternatives

Alternative solutions would most likely require larger changes to the existing classes in order to pass the config down.

@sanjaypujare
Copy link
Contributor

cc @ejona86 . I think this request makes sense. WDYT?

@ejona86
Copy link
Member

ejona86 commented Nov 9, 2020

A system property is probably the best way of doing this, if we were going to do it. We might want to consider the system property overriding the environment variable (such that we'd check the system property first), but it's not obvious to me which ordering is better.

It is weird that this would be provided by the binary itself though. @erikjoh, how are you generating the bootstrap file? You have a java library that auto-detects the current environment and creates it? Normally it needs deployment-specific information...

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 9, 2020

@ejona86 I have not implemented anything yet but the plan is to have an internal Java utility that can detect environment and generate the boostrap file accordingly (for example different xDS server and credentials for testing/local).
The currently recommended solution is to run an external binary as an initContainer or init script https://cloud.google.com/traffic-director/docs/set-up-proxyless-gke#setting_up_the_environment_variable_and_bootstrap_file in GKE/GCE but when running the application locally in an IDE or Docker container there are manual steps involved that we are trying to automate.

By running this utility function in the service framework startup flow we will be able to solve bootstrapping GKE, GCE, IDE and local Docker all at once without the need of manual developer intervention. Also, by doing the bootstrapping in the service framework we can utilize existing service configuration in order to expose optional config overrides to the xDS bootstrapper.

I agree that it is a bit "weird" but from a developers perspective we think a solution like this might be the most convenient to adopt (only a bump in service framework version and change of gRPC channel target will suffice for most use-cases).

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 9, 2020

I guess it would be even nicer to be able to provide the bootstrap config directly to gRPC without the need of a temporary json file, but that's maybe stretching it a bit too far, since no other grpc implementation would have such a feature and it may require exposing some internals that might change later on.
For us now, I think getting around the problem of env variables in Java would be enough.

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 9, 2020

As for the priority, my spontaneous thought is that the env variable should have higher prio so that all existing gRPC proxyless documentation still would apply without edge cases.
I imagine that a developer might want to learn about proxyless gRPC and follow the official documentation. If the java property took precedence and they used a service framework that internally sets that property and creates the file then they might waste some thought cycles trying to figure out "why it didn't work as they thought".
But if there are other scenarios that can motivate why the property should take precedence I think that's fine as well since we could just ignore setting the property if the env variable is set 🙂

@sanjaypujare
Copy link
Contributor

Env variable having precedence over Java system property sounds good to me.

@srini100
Copy link
Contributor

I guess it would be even nicer to be able to provide the bootstrap config directly to gRPC without the need of a temporary json file, but that's maybe stretching it a bit too far, since no other grpc implementation would have such a feature and it may require exposing some internals that might change later on.

We should consider this. Istio uses PROXY_CONFIG env variable for bootstrap config without using a file. It is good to have an option that doesn't require a file to be mounted.

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 10, 2020

So would it be okay if I made a PR to add the extra check for an "grpc.xds.bootstrap" property (naming based on what seems to be the standard format in java properties)?

@sanjaypujare
Copy link
Contributor

So would it be okay if I made a PR to add the extra check for an "grpc.xds.bootstrap" property (naming based on what seems to be the standard format in java properties)?

Sounds good to me

@ejona86
Copy link
Member

ejona86 commented Nov 10, 2020

They tend to be like package names, so io.grpc.xds.bootstrap

@ejona86
Copy link
Member

ejona86 commented Nov 16, 2020

Fixed by #7620

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants