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

Add docker scaffolding for map-packer and guardianconnector services #64

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rudokemper
Copy link
Member

n/t

@luandro
Copy link
Contributor

luandro commented May 26, 2024

/review

Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR introduces significant changes to the environment configuration and service definitions in Docker, which requires careful review to ensure compatibility and security. The addition of new services and environment variables increases the complexity of the review.

🧪 Relevant tests

No

⚡ Possible issues

Possible Security Issue: The PR introduces new environment variables for Azure storage and Mapbox without default values, which could potentially be misconfigured or expose sensitive data if not handled properly.

Possible Configuration Error: The DB_PORT for the map-packer service is set to 8083, which is typically used for HTTP traffic and might conflict with other services.

🔒 Security concerns

Sensitive information exposure: The PR includes API keys and database credentials in the environment variables, which could be exposed if the Docker configuration or the environment is not secured properly.

Code feedback:
relevant filebalena.yml
suggestion      

Ensure that sensitive information such as AZURE_STORAGE_CONNECTION_STORAGE_KEY and VUE_APP_PLANET_API_KEY are not hardcoded or exposed in the configuration files. Consider using environment variables or secrets management solutions to handle sensitive data securely. [important]

relevant line- AZURE_STORAGE_CONNECTION_STORAGE_KEY:

relevant filedocker-compose.yml
suggestion      

Modify the DB_PORT for the map-packer service to use a more appropriate port that is not typically used for other common services to avoid potential conflicts. [important]

relevant lineDB_PORT: 8083

relevant filedocker-compose.yml
suggestion      

Consider adding a volume for the map-packer service to persist data if necessary, similar to other services to ensure data durability across container restarts. [medium]

relevant linevolumes:

relevant filedocker-compose.yml
suggestion      

Review and ensure that the DATABASE_URL environment variable is updated across all services that depend on the database to reflect the new database service name change from terrastories-db to database. [important]

relevant line- database

@rudokemper rudokemper changed the title Add docker scaffolding for map-packer and mapgl-tile-renderer Add docker scaffolding for map-packer and guardianconnector services May 26, 2024
@luandro
Copy link
Contributor

luandro commented May 26, 2024

/improve

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Pin the version of balena-cli to avoid potential breaking changes

Consider pinning the version of balena-cli to a specific major version to avoid potential
breaking changes with future updates. This can help ensure the stability and
predictability of your deployment process.

.github/actions/deploy-balena-image/action.yml [58]

-npm install balena-cli --global --production --unsafe-perm
+npm install balena-cli@18 --global --production --unsafe-perm
 
Suggestion importance[1-10]: 9

Why: Pinning the version of balena-cli helps prevent potential breaking changes from future updates, ensuring stability and predictability in the deployment process. This is a best practice for maintaining consistent behavior.

9
Specify a more precise node version to enhance reliability

Use a more specific node version instead of the latest major release to avoid potential
incompatibilities with newer versions. This change enhances the reliability of the setup.

.github/actions/deploy-balena-image/action.yml [53]

-node-version: '20'
+node-version: '20.x'
 
Suggestion importance[1-10]: 8

Why: Using a more specific node version (e.g., '20.x') instead of just '20' can prevent potential incompatibilities with newer versions, enhancing the reliability of the setup.

8
Security
Provide default values or secure handling for environment variables

It's recommended to provide default values for environment variables or ensure they are
securely managed if they are sensitive. This prevents potential runtime errors and
security vulnerabilities.

balena.yml [60-61]

-- AZURE_STORAGE_CONNECTION_ACCOUNT_NAME: 
-- AZURE_STORAGE_CONNECTION_STORAGE_KEY:
+- AZURE_STORAGE_CONNECTION_ACCOUNT_NAME: default_account_name
+- AZURE_STORAGE_CONNECTION_STORAGE_KEY: ${AZURE_STORAGE_KEY}
 
Suggestion importance[1-10]: 7

Why: Providing default values or ensuring secure management of environment variables can prevent runtime errors and security vulnerabilities. However, the suggested default values should be carefully chosen to avoid introducing security risks.

7
Change the database port to avoid conflicts and enhance security

Ensure that the database port does not conflict with other services and consider using a
non-default port for enhanced security.

docker-compose.yml [108]

-- 5432:5432
+- 5433:5432
 
Suggestion importance[1-10]: 6

Why: Changing the database port can help avoid conflicts with other services and enhance security. However, this change should be carefully coordinated to ensure it does not disrupt existing configurations.

6

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

Successfully merging this pull request may close these issues.

2 participants