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

Avoid depending on appengine-api-1.0-sdk #50

Closed
ajkannan opened this issue Nov 18, 2015 · 11 comments
Closed

Avoid depending on appengine-api-1.0-sdk #50

ajkannan opened this issue Nov 18, 2015 · 11 comments
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@ajkannan
Copy link

As @aozarov mentioned in #45, it'd be nice to avoid depending on appengine-api-1.0-sdk (17Mb jar) when not necessary.

Alternate options include:

  • Use "provided" or a like as a maven dependency
  • Use reflection to invoke methods on AppIdentityService. gcloud-java used to do the latter.
@anthmgoogle
Copy link
Contributor

I'd like to understand this better.

The main http-oauth2 assembly does not have a hard dependency on this. It makes a late bound call to the SDK method that tells you definitively if you are running in a GAE process. Only if true does it make a second late-bound call to the appengine-specific auth component. That in turn has a hard dependency on the appengine-api-i1.0-SDK. So this should avoid needing to take any app-engine related .jar file along for non GAE scenarios, either the SDK or the auth component.

For the case where you are running in GAE, the app-engine specific component does have a hard depedency. But any app-engine app will also have this dependency. Is there a case on App engine where you want to dependent on the app-engine specific auth component, but not the app-engine SDK?

@ajkannan
Copy link
Author

The gcloud-java library supports users running on App Engine, but it can also be used from outside of App Engine. As a result, gcloud-java has to take the appengine module as a dependency (and thus pull in the SDK jar), even if a user happens to not be running on App Engine.

@anthmgoogle
Copy link
Contributor

It is also the case that the Http-OAuth2 component is used inside and outside app engine. In both cases there is a late bound call for good reasons. The OAuth2 libraries also don't take a direct dependency on the App-engine SDK. Only the App-Egine-specific component does and that is itself already invoked late bound by the Oauth2 component.

@aozarov
Copy link

aozarov commented Nov 20, 2015

@anthmgoogle gcloud-java has one artifact for all platforms.

As I see it, in the current state, that means we have 2 options:

  1. Add to our pom a dependency on google-auth-library-appengine and pay the price of depending and pulling 17Mb even when not needed.

  2. Direct people that use gcloud-java on AE to also add a dependency on google-auth-library-appengine

I would like to avoid (2) in all cost. I think we can also avoid (1) by using the suggestions mentioned above.

Do you think we can avoid pulling appengine-api-1.0-sdk without requiring gcloud-java users on AE to add the dependency themselves?

@anthmgoogle
Copy link
Contributor

Having gcloud-java dependend on google-auth-library-appengine seems wrong, because this will also pull that binary around for non GAE scenarios. It is currently small, but when 3LO logic is added, it will be bigger and have more of its own dependencies.

I'll follow up on the tradeoffs related to (2) shortly.

@ajkannan
Copy link
Author

Any updates on your take on this issue @anthmgoogle?

@anthmgoogle
Copy link
Contributor

Thoughts on the tradeoffs of using reflection for the GAE identity.

Without reflection a GAE app needs to know about this indirect dependency. Without it, they will fail when they try to get the application working for the first time. It would be better if this "just worked".

The GAE credential could be implemented completely in reflection to address the getting started dependency above. The biggest cost I can see is that this will add a per-request reflection call to get the access token. In common synchronous cases, this is probably dwarfed by the network round-trip. But in async or CPU-bound cases you may want to avoid it. A secondary cost is that reflection code is more costly to write and maintain.

For the code in gcloud-java this has the additional benefit of saving a whole compilation unit. This is not the case for the auth libraries, because App-Engine specific surface is needed for explicit use of App Identity and 3LO.

One way to solve both the performance and getting started issues is to implement a second fall back component in reflection and only use it if the app-engine auth library is not present. Cost is in duplicated logic. A performance-sensitive customer may prefer initial failure to silently falling back to a slower mode of operation.

It may be worth understanding ways of mitigating the the getting started problem:

  1. Ensuring all GAE samples have this dependency.
  2. Adding this dependency by default to GAE templates.
  3. Ensuring the error message when the assembly is missing is very specific. (It already should be, but we can test whether people are getting confused by it).
  4. Documentation in appropriate locations.

I think there is a nuanced tradeoff. I can see the appeal of eliminating one more piece of getting started friction. However, steps that have very clear failure modes don't cause much friction, and there is long term benefit in having a single, fast implementation.

@aozarov
Copy link

aozarov commented Nov 23, 2015

To be clear, as of today we are only talking about this simple class.
Therefore, I don't think changing this class to use reflection is a big deal (maintenance wise).

Reflection should happen only when calling appIdentityService.getAccessToken(scopes), which
is using an RPC call, therefore the cost of the reflection part should be negligible.

If you insist of not using reflection, did you consider the maven proivded scope option?

One of the nice things about gcloud-java is the easy setup and get started. I would really want to avoid
this extra documentation and setup step. Therefore, if you choose to keep it as is, we will keep our existing reflection based AE credentials (though we can clean it up to match AppEngineCredentials.java) and use it as a fallback.

@ejona86
Copy link
Contributor

ejona86 commented Nov 23, 2015

Going through the steps of setting up reflection is slow, but calling invoke() isn't all that slow if on a hot-path.

I'd find it hard to believe that calling invoke() each getAccessToken() call could be prohibitive.

@anthmgoogle
Copy link
Contributor

OK. If the method can be cached this feature should be OK.

@mziccard
Copy link
Contributor

This was fixed in #71

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

6 participants