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

Setup update and API Token Authentication #137

Closed
wants to merge 14 commits into from
Closed

Conversation

gideonmaina
Copy link
Collaborator

@gideonmaina gideonmaina commented Aug 16, 2024

Description

The main aim of this PR is to simplify local development and implement authentication for key endpoints prone to bot crawling or misuse by clients whilst fixing a few issues related to dependencies and migrations. The affected endpoints/views in API v1 are data, now and node. This will attempt to reduce the lag observed when making calls to the API.

Fixes issues #134, #135, #136

Setup changes

  • The feinstaub sensors app was removed from the working tree as pip install takes care of that.
  • The migration file *04 in sensorsafrica app was removed as it is applied before a dependency

Tests for these changes were both done using local and staging databases.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots

Screenshot 2024-08-16 at 11 08 02 Screenshot 2024-08-16 at 16 04 36

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have successfully build the the project and performed necessary tests

Attempting to apply migration before its dependency
openstuttgart's feinstaub sensors app is taken care of by pip git repo install
Affects data and now endpoints
Prevents error with SSL certificate
Limit URLLIB3 to version 1 to resolve conflict with Sentry SDK
Read replica db no longer in use
@gideonmaina gideonmaina added bug Something isn't working enhancement New feature or request dependencies Pull requests that update a dependency file labels Aug 16, 2024
The migrations files in the sensorsafrica directory will resolve the migration issues and prompt removal of pip installing opendata-stuttgart master feinstaub-api
@thepsalmist thepsalmist changed the title Setup update Setup update and API Token Authentication Sep 16, 2024
@thepsalmist thepsalmist requested a review from a team September 16, 2024 13:50
Copy link

@DavidTheProgrammer DavidTheProgrammer left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

👍🏽


If I understand what we're trying to do, then I see this as breaking change i.e. the new secure endpoints are no longer v1 but v2 (or whatever v we're currently on).

My approach would be:

  1. Create new v2 endpoints that work exactly like the equivalent v1 endpoints but secure.
  2. Test and deploy backend to PROD.
  3. Upgrade the front-end up to point to v2 endpoints.
  4. Test and deploy front-end to PROD.
  5. Remove or redirect unsecure v1 endpoints.
  6. Test and deploy backend to PROD.
  7. Profit

Comment on lines +16 to 20
# Upgrade pip from trusted hosts
RUN python -m pip install --trusted-host pypi.python.org --trusted-host files.pythonhosted.org --trusted-host pypi.org --upgrade pip

# Upgrade pip and setuptools
RUN pip install -q -U pip setuptools
Copy link
Member

Choose a reason for hiding this comment

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

Why 2 ways of upgrading pip?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍🏽 the upgrade flag will be removed to just install the setup tools

Comment on lines +31 to +33
username: `sensorsafrica`
email: blank
password: `sensorsafrica`
Copy link
Member

Choose a reason for hiding this comment

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

Must username/password be sensorsafrica?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. Let's say it a convention. Any thoughts?

@@ -2,15 +2,20 @@ version: '3.3'

services:
rabbitmq:
image: rabbitmq:3.5.1
image: rabbitmq:3.12.7-management
Copy link
Member

Choose a reason for hiding this comment

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

Is 3.12 the version used in PROD as well?

Comment on lines +34 to +37
SENSORSAFRICA_DATABASE_URL: ${SENSORSAFRICA_DATABASE_URL}
SENSORSAFRICA_RABBITMQ_URL: ${SENSORSAFRICA_RABBITMQ_URL}
SENSORSAFRICA_FLOWER_ADMIN_USERNAME: ${SENSORSAFRICA_FLOWER_ADMIN_USERNAME}
SENSORSAFRICA_FLOWER_ADMIN_PASSWORD: ${SENSORSAFRICA_FLOWER_ADMIN_PASSWORD}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a default for any of these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was previously.

  SENSORSAFRICA_DATABASE_URL: postgres://sensorsafrica:sensorsafrica@postgres:5432/sensorsafrica
  SENSORSAFRICA_RABBITMQ_URL: amqp://sensorsafrica:sensorsafrica@rabbitmq:5672
  SENSORSAFRICA_FLOWER_ADMIN_USERNAME: admin
  SENSORSAFRICA_FLOWER_ADMIN_PASSWORD: password```

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be updated in the README for local setup.

for index, read_database_url in enumerate(READ_DATABASE_URLS,start=1):
DATABASES[f"read_replica_{index}"] = dj_database_url.parse(read_database_url)

DATABASE_ROUTERS = ["sensorsafrica.router.ReplicaRouter", ]
Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

@thepsalmist
Copy link
Contributor

👍🏽

If I understand what we're trying to do, then I see this as breaking change i.e. the new secure endpoints are no longer v1 but v2 (or whatever v we're currently on).

My approach would be:

  1. Create new v2 endpoints that work exactly like the equivalent v1 endpoints but secure.
  2. Test and deploy backend to PROD.
  3. Upgrade the front-end up to point to v2 endpoints.
  4. Test and deploy front-end to PROD.
  5. Remove or redirect unsecure v1 endpoints.
  6. Test and deploy backend to PROD.
  7. Profit

Cool, makes sense @gideonmaina I'll setup a review to plan for this implementation approach

@kilemensi
Copy link
Member

PS: Not sure what SENSORSAFRICA_CELERY_SLACK_WEBHOOK_FAILURES_ONLY is for. If we need to send errors notifications to Slack, the best approach it to integrate with Sentry and Sentry will send notifications over email, Slack, etc.

@thepsalmist
Copy link
Contributor

thepsalmist commented Oct 2, 2024

Changes should be merged in #138 , Tracked in #134

@thepsalmist thepsalmist closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants