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

update gee authentication process #905

Closed
wants to merge 17 commits into from
Closed

update gee authentication process #905

wants to merge 17 commits into from

Conversation

dfguerrerom
Copy link
Collaborator

WIP

@dfguerrerom
Copy link
Collaborator Author

hi @12rambau , would you mind telling me what is the format of the EARTHENGINE_TOKEN that is stored in secrets? I have tested with my credentials on my local actions and I get a different error, the format of my credentials is:

{"client_id": "", "client_secret": "", "refresh_token": ""}

In any case I think we will have to add the project_id key, do you think you can check this?

@dfguerrerom dfguerrerom marked this pull request as ready for review February 27, 2024 15:26
@dfguerrerom
Copy link
Collaborator Author

I have copied the authentication process from the proposal on gee-community/pytest-gee#24.
I also have updated the tests here, everything should work if we add $EARTHENGINE_PROJECT var to the secrets.

@12rambau
Copy link
Member

12rambau commented Mar 4, 2024

Ok that's much more complicated than that. What they did should be forbidden and I already told them on multiple occasion.
The refactor they make with this compulsory "project" parameter has made all existing pipelines to fail.
This is very shortsighted but we cannot do anything about it.

2 main issues to overcome are:

  • set back the parameter to connect from a CI: keys in documentation and keys in gihub
  • update the init_ee decorator (to be honest I'm not sure it can be kept)
  • overcome the problem when the connection is performed from a service account, i.e. no project is available from the client: https://issuetracker.google.com/issues/325020447
  • use pytest-gee for testing

Let me report here when I move forward with one of the items

@12rambau
Copy link
Member

12rambau commented Mar 4, 2024

@dfguerrerom can you check a credential file from a user in SEPAL a copy/paste it here (anonymised) I want to check if the project is saved in the credentials or not with modern users.
If it's not the case the whole SEPAL structure will be broken if yes we need to be clever on the credential discovery.

@dfguerrerom
Copy link
Collaborator Author

dfguerrerom commented Mar 5, 2024

actually, yes, when an user, old or new, logs in using the SEPAL google authentication process, it will automatically save the project id selected on the UI in the credentials file, this file has the following structure:

{
    "access_token":"....",
    "access_token_expiry_date":"...", 
    "project_id":"something"
}

Note that SEPAL stores the project with the key project_id, while a local user that uses earthengine set_project "project_name" will add the key project to the credentials.

@12rambau
Copy link
Member

12rambau commented Mar 5, 2024

ok perfect so that is exactly the same as in a service account token. I think we should rely on that.
What if we start relying on geetools for this kind of stuff ?
I'm going to revamp the authentication/Initto match this new structure and it can then be called in the init_ee decorator.

Without parameter it will try to reach the default credentials and read the project_id from the token

@12rambau
Copy link
Member

12rambau commented Mar 8, 2024

I made a PR in this direction in geetools: gee-community/geetools#240
I will use it here once it's released

@dfguerrerom
Copy link
Collaborator Author

@12rambau, note that as I mentioned before, SEPAL uses project_id, while the default earthengine set_project projectname will store the project in the project key.

@12rambau
Copy link
Member

@cdanielw any reason why you are not relying on the default key generated by GEE ?

@cdanielw
Copy link

To my understanding, the default project id used when ee.initialize() is called, without explicitly specifying project, will be the legacy project. That one will stop working (if it hasn't already).

@dfguerrerom
Copy link
Collaborator Author

What I meant to say is that when an local user authenticates in earthengine, they have the possibility to earthengine set_project project_name. This project will be stored in the .config/earthengine/credentials file under the project key, SEPAL also stores this value in the credentials but in project_id key, not project. @12rambau I don't think there's a specific reason for that.

@cdanielw
Copy link

I didn't know there was a way to set that up. We can look into conforming to that.

@12rambau
Copy link
Member

I think it's pretty recent, at start it was only in service accounts but now it's available for all account type as you need to set up the project when Initializing GEE.

@12rambau
Copy link
Member

As this lib need to remain compatible with vanilla environments, I could you align on GEE behaviour ? I would like not to code custom behaviour that would only work on SEPAL.

@dfguerrerom
Copy link
Collaborator Author

dfguerrerom commented Mar 14, 2024

As this lib need to remain compatible with vanilla environments, I could you align on GEE behaviour ? I would like not to code custom behaviour that would only work on SEPAL.

I think that's something we (sepal) can change at some point.
@12rambau In the meantime I think we can rely on geetools to do the authentication, however I see that they (you) have implemented an extra auth method: from_user... I assume we'll have to implement that extra auth step in our init_ee but also keep supporting the current authentication method (for regular users) and also add from service account?

@12rambau
Copy link
Member

from_user without argument is only relying on the default credentials so nothing to do from your side, as long as the project is setup as in a normal credential.

@dfguerrerom
Copy link
Collaborator Author

actually, the credentials stored in sepal are quite different to the ones you're expecting in https://github.com/gee-community/geetools/blob/47cb326c1a6bf4757756b3f10a89aece91a365ce/geetools/Initialize/__init__.py#L59, we have:

{"access_token":"","access_token_expiry_date":,"project_id":""}

but that's fine because we install our own fork that will authenticate using our custom credentials file.

question, Is there a reason why are you instantiating the Credentials class with the values from the credentials instead of just letting earthengine-api do that? I also saw the project arg is not passed to the Initialize method.

@12rambau
Copy link
Member

  1. by doing that I'm agnostic to the account type (personnal or service account)
  2. As I try to provide support for multi-account I need to be able to use any file as source. Default will only use the one stored in .credentals

@12rambau
Copy link
Member

Could you add the 2 "authentication" path we talked about when I was in Rome ? I think PR should be validated so you can work on main again

@dfguerrerom
Copy link
Collaborator Author

Could you add the 2 "authentication" path we talked about when I was in Rome ? I think PR should be validated so you can work on main again

@12rambau This PR contains what we discussed in Rome... if I'm remember correctly, it will work with both service account or individual gee accounts since the project is saved in the credentials file (?).

@12rambau
Copy link
Member

Perfect, can you lint it and let's see how the tests behave from the CI/CD side

@dfguerrerom
Copy link
Collaborator Author

dfguerrerom commented Apr 24, 2024

@12rambau, I think it fails because there is no a $secrets.EARTHENGINE_TOKEN in this project?
to make it work we should have a EARTHENGINE_TOKEN with a project or EARTHENGINE_TOKEN with a EARTHENGINE_PROJECT venv.

@12rambau
Copy link
Member

I added the appropriate parameters. Before checking "project" you need to check "poject_id" as it's the one set up in the service account profile keys. "project" is only existing in SEPAL.

@dfguerrerom
Copy link
Collaborator Author

project

Actually that's how it is written, but first, it will try with the EARTHENGINE_PROJECT which you introduced in pytest-gee.

It fails because with the google service account you cannot authenticate using that private key, we'll have to use the method described here: https://developers.google.com/earth-engine/guides/service_account#use-a-service-account-with-a-private-key (which is the one you use in pytest-gee).

The init_ee method should determine whether the credentials file is a service account or not (?).

@12rambau
Copy link
Member

I guess yes but then it becomes more and more convoluted ;-)
They could have come up with something unified....

@dfguerrerom
Copy link
Collaborator Author

haha it failed again. I don't really know what is the structure of the EARTHENGINE_TOKEN you have, but the one that I created has this type key...

@12rambau
Copy link
Member

12rambau commented May 7, 2024

that is the one I store which a very default token from the latest GEE API version:

{
  "type": "service_account",
  "project_id": "sepal-ui",
  "private_key_id": "key_id",
  "private_key": "a key",
  "client_email": "a.fake@mail",
  "client_id": "id",
  "auth_uri": "a.fake.url",
  "token_uri": "a.fake.url",
  "auth_provider_x509_cert_url": "a.fake.url",
  "client_x509_cert_url": "a.fake.url",
  "universe_domain": "a.fake.domain"
}

@dfguerrerom
Copy link
Collaborator Author

closed in favor of #915

@dfguerrerom dfguerrerom closed this May 8, 2024
@12rambau
Copy link
Member

can you remove the branch if it's not needed anymore ?

@dfguerrerom dfguerrerom deleted the gee_auth branch May 10, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update to the new GEE authentication process
3 participants