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

feat: update runtimes and add pgstac customization options #100

Merged

Conversation

vincentsarago
Copy link
Member

@vincentsarago vincentsarago commented Feb 26, 2024

This PR closes #88 and add customizations option for PgSTAC bootstrap to:

  • enable/disable the context extension
  • enable/disable the mosaic table index

⚠️ Checklist if your PR is changing anything else than documentation

  • Posted the link to a successful manually triggered deployment workflow (successful including the resources destruction)

Merge request description

sql.SQL(
"CREATE INDEX IF NOT EXISTS searches_mosaic ON searches ((true)) WHERE metadata->>'type'='mosaic';"
)
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is taken from eoapi repo

@vincentsarago vincentsarago force-pushed the feature/update-runtimes-and-add-pgstac-customization branch from 30bf562 to 1bbd801 Compare February 26, 2024 07:55
@vincentsarago vincentsarago changed the title feature: update runtimes and add pgstac customization options feat: update runtimes and add pgstac customization options Feb 26, 2024
@emileten
Copy link
Contributor

Co-authored-by: Emile Tenezakis  <e.tenezakis@gmail.com>
@emileten
Copy link
Contributor

Questions for my understanding

enable/disable the context extension
enable/disable the mosaic table index

What were we doing before, was it disabled by default?

@vincentsarago
Copy link
Member Author

@emileten we were adding the mosaic index by default but not the context extension, maybe we can switch back to false 👍

@emileten
Copy link
Contributor

Maybe we can switch back to false

I'll defer to you on this decision, I don't know much about these options. Doesn't sound like it hurts to have the context on by default ?

lib/database/index.ts Outdated Show resolved Hide resolved
@emileten
Copy link
Contributor

A live example of the bootstrapper logs

The workflow failed again, there's something we need to fix in the bootstrapper changes https://github.com/developmentseed/eoapi-cdk/actions/runs/8045602231/job/21971192880#step:9:2478

The lambda logs https://us-east-1.console.aws.amazon.com/cloudwatch/home?region=us-east-1#logsV2:log-groups/log-group/$252Faws$252Flambda$252Fpgstac60fe14-pgstacdblambda91737152-ELDP6T9rb8jI/log-events/2024$252F02$252F26$252F$255B$2524LATEST$255Df9a0fff7fa2042bb84e34c9220cb7071

Unable to bootstrap database with exception=must be member of role "pgstac_ingest" 

Also httpx failed responding with the error

send(..) failed executing httpx.put(..): can only concatenate str (not "int") to str 

@emileten
Copy link
Contributor

emileten commented Mar 7, 2024

@vincentsarago just checking, were you expecting the last commit to fix the bug ? or is there more to do

@vincentsarago
Copy link
Member Author

I was hopping yes 😅 but couldn't see where I had to click to run the deployment workflow

@emileten
Copy link
Contributor

emileten commented Mar 7, 2024

ugh sorry, just started one. Will show you later

@emileten
Copy link
Contributor

emileten commented Mar 7, 2024

@vincentsarago looks like we still have the problem

[ERROR] InsufficientPrivilege: must be member of role "pgstac_ingest"
Traceback (most recent call last):
  File "/var/task/handler.py", line 280, in handler
    raise e
  File "/var/task/handler.py", line 243, in handler
    Migrate(pgdb).run_migration(params["pgstac_version"])
  File "/var/task/pypgstac/migrate.py", line 149, in run_migration
    cur.execute(migration_sql)
  File "/var/task/psycopg/cursor.py", line 732, in execute
    raise ex.with_traceback(None)

I can take a look tomorrow as well.

FYI to run a deployment:

  • click on the Actions tab
  • On the left click on the Deployment workflow
  • On the right click on Run Workflow, choose your branch and click on Run Workflow

@vincentsarago
Copy link
Member Author

@emileten let's fall back to pgstac 0.7.10 to validate the changes and then we can debug why it doesn't work for pgstac 0.8.4

@emileten
Copy link
Contributor

emileten commented Mar 8, 2024

@vincentsarago like this 0d675e2?

@vincentsarago
Copy link
Member Author

@emileten just pgstac, we can keep titiler-pgstac and tipg to the latest version

@emileten
Copy link
Contributor

emileten commented Mar 8, 2024

Ok seems to work https://github.com/developmentseed/eoapi-cdk/actions/runs/8204255860/job/22438560336#step:10:798.

Now we need to

debug why it doesn't work for pgstac 0.8.4

😄

@emileten
Copy link
Contributor

emileten commented Mar 8, 2024

there seems to be a 0.8.3 change that sounds like it could have triggered this https://github.com/stac-utils/pgstac/blob/e0b7de4c9c8130c55de9fe6eb0a87324cd705135/CHANGELOG.md?plain=1#L37 we can try with the previous version

@vincentsarago
Copy link
Member Author

👁️ stac-utils/pgstac#239

@vincentsarago
Copy link
Member Author

@emileten if I understand well

looking at #100 (comment) it seems the migration step fails with

[ERROR] InsufficientPrivilege: must be member of role "pgstac_ingest"

in

Migrate(pgdb).run_migration(params["pgstac_version"])

when we use

user=connection_params["username"],
password=connection_params["password"],

username/password (must be postgres by default)

After the migration we then grant the username/password (pgstac_user by default)

"GRANT pgstac_read TO {username};"
"GRANT pgstac_ingest TO {username};"
"GRANT pgstac_admin TO {username};"

so to me it seems that we should GRANT pgstac_ingest TO postgres; but to me it seems weird because pgstac_ingest role doesn't exist before the migration starts...

cc @bitner

@bitner
Copy link

bitner commented Mar 12, 2024

@emileten @vincentsarago
The pypgstac migrate should be run as superuser (or a role with sufficient root-like privileges in the case of systems like RDS that do not allow true superusers -- this is typically the postgres role). pypgstac migrate on a fresh database will create the pgstac_read, pgstac_ingest, and pgstac_admin roles.

Once pgstac has been installed, you can assign these roles to other database login roles. Ideally, you would use least-access necessary roles for different tasks. Best practice would be to have three roles to interact with pgstac.

  • An admin role that is used to manage migrations. Whatever role that is used to run pypgstac migrate will automatically be granted the pgstac_admin role.
  • A loader role that is used with the transactions extension or when using pypgstac load. GRANT pgstac_ingest to dataloader;
  • A user role that is used with the API for search or other read operations GRANT pgstac_read to apiuser;. Do note that this role is not truly read only as it will still be able to (indirectly via functions that run as the pgstac_admin role) update the searches and search_wheres table.

* (fix): fix database bootstrap

* fix handler

* update to pgstac 0.8.5
@emileten
Copy link
Contributor

@vincentsarago vincentsarago merged commit 9e49e7e into main Mar 13, 2024
4 checks passed
@vincentsarago vincentsarago deleted the feature/update-runtimes-and-add-pgstac-customization branch March 13, 2024 15:12
github-actions bot pushed a commit that referenced this pull request Mar 13, 2024
# [7.1.0](v7.0.1...v7.1.0) (2024-03-13)

### Features

* update runtimes and add pgstac customization options ([#100](#100)) ([9e49e7e](9e49e7e)), closes [#102](#102)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update core packages in default runtimes
3 participants