Closed DaMandal0rian closed 1 month ago
PR Description updated to latest commit (https://github.com/subspace/infra/commit/303efb3dff79d243a3b97bd91fbd0a295912b547)
⏱️ Estimated effort to review [1-5] | 3, because the PR introduces a comprehensive set of Helm chart configurations for Kubernetes, which includes multiple resources such as ConfigMaps, Persistent Volumes, Service Accounts, and RBAC settings. The complexity and variety of Kubernetes objects involved require careful review to ensure they are correctly configured and do not introduce security or operational issues. |
🧪 Relevant tests | No |
🔍 Possible issues | Possible Bug: The server URL in the ClusterIssuer resource for ACME is set to the staging environment of Let's Encrypt (`https://acme-staging-v02.api.letsencrypt.org/directory`). This should be changed to the production URL for actual deployments to ensure valid SSL certificates are issued. |
🔒 Security concerns | No |
relevant file | explorer/k8s/helm/account-squid/misc/acme-certificate.yaml |
suggestion | Change the ACME server URL to the production endpoint to ensure that the SSL certificates issued are valid for production use. Replace the staging URL with 'https://acme-v02.api.letsencrypt.org/directory'. [important] |
relevant line | server: https://acme-staging-v02.api.letsencrypt.org/directory |
Category | Suggestions |
Best practice |
Ensure output values are properly quoted in YAML.___ **It is recommended to use thequote function for the output of the account-squid.fullname template to ensure that the values are properly quoted in YAML outputs, which can prevent issues with special characters and improve readability.** [explorer/k8s/helm/account-squid/templates/_helpers.tpl [9]](https://github.com/subspace/infra/pull/305/files#diff-0eed6d5237799290429451c73854a195acd7626b9df8591354bf1014cfa92652R9-R9) ```diff -{{ include "account-squid.fullname" . }} +{{ include "account-squid.fullname" . | quote }} ``` |
Add safety checks for autoscaling configuration values.___ **For the HorizontalPodAutoscaler, it is recommended to add safety checks around the.Values.autoscaling.targetCPUUtilizationPercentage and .Values.autoscaling.targetMemoryUtilizationPercentage to ensure they are defined before usage to prevent runtime errors.** [explorer/k8s/helm/account-squid/templates/hpa.yaml [20]](https://github.com/subspace/infra/pull/305/files#diff-90d37624ba0c0a524496a262c550e5a78d5d365e511f30b81fe1cbf6d38b433eR20-R20) ```diff -targetAverageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} +targetAverageUtilization: {{ required "A valid .Values.autoscaling.targetCPUUtilizationPercentage is required!" .Values.autoscaling.targetCPUUtilizationPercentage }} ``` | |
Simplify the service selector to ensure correct pod targeting.___ **The service definition uses a selector that might not correctly target the intended pods.The name key in the selector is redundant and could lead to confusion or misconfiguration, as labels should be sufficient for targeting. Consider removing the name key from the selector to simplify and ensure correct targeting.** [explorer/k8s/helm/account-squid/templates/service.yaml [13-15]](https://github.com/subspace/infra/pull/305/files#diff-fe23bb00fb25f8f9da25e86659241788de0e9d034d7f8e3397a1027459a42167R13-R15) ```diff selector: - name: {{ include "account-squid.fullname" . }}-app - app: {{ include "account-squid.fullname" . }}-app + app: {{ include "account-squid.fullname" . }}-app # Simplified and correct targeting ``` | |
Security |
Specify the
___
**It is a security best practice to specify the |
Use a specific ingress class name to avoid conflicts and enhance security.___ **To improve the security of your Helm chart, consider using a more specific ingress classname than the default 'nginx', especially if multiple ingress controllers are used within the cluster.** [explorer/k8s/helm/account-squid/templates/ingress.yaml [16]](https://github.com/subspace/infra/pull/305/files#diff-cddf7dc215e9d3ed168347636f8ac25fa9020885cca60511b156192127c5f6bbR16-R16) ```diff -ingressClassName: nginx +ingressClassName: custom-nginx ``` | |
Reduce the scope of permissions granted to the
___
**It is recommended to avoid using overly broad permissions for Kubernetes roles. The | |
Ensure secure handling and storage of sensitive data in secrets.___ **The base64 encoding used forPOSTGRES_PASSWORD and POSTGRES_USER in the secrets file does not encrypt or secure the data, it merely encodes it. It's important to ensure that the secrets are stored securely using Kubernetes secrets management practices or consider integrating a secrets management tool like HashiCorp Vault for enhanced security.** [explorer/k8s/helm/account-squid/templates/secrets.yaml [8-9]](https://github.com/subspace/infra/pull/305/files#diff-89a6d566f34729f3ff551286085e1075c9ec7cbda4f94a8ac14583d4c3508febR8-R9) ```diff -POSTGRES_PASSWORD: {{ .Values.postgres.postgresPassword | b64enc}} -POSTGRES_USER: {{ .Values.postgres.postgresUser | b64enc}} +POSTGRES_PASSWORD: {{ .Values.postgres.postgresPassword | b64enc}} # Ensure secure management of secrets +POSTGRES_USER: {{ .Values.postgres.postgresUser | b64enc}} # Consider using a secrets management tool ``` | |
Maintainability |
Ensure consistent formatting in ConfigMap key-value pairs.___ **To avoid potential configuration errors, ensure that spaces are consistent around colonsin key-value pairs in the ConfigMap data section.** [explorer/k8s/helm/account-squid/templates/configmap.yaml [11]](https://github.com/subspace/infra/pull/305/files#diff-0ab766e6d3e5d1dfd8c4502cb506a0cfc21bc2848b6609bea4342d728ee9b3e8R11-R11) ```diff -POSTGRES_HOST : {{ .Values.postgres.postgresHost }} +POSTGRES_HOST: {{ .Values.postgres.postgresHost }} ``` |
Use configurable environment variables for probe commands to enhance flexibility and maintainability.___ **ThelivenessProbe and readinessProbe for the postgres container use hardcoded IP addresses and ports. It's recommended to use environment variables or configuration values to make these settings more flexible and maintainable, especially if there might be changes in the network configuration or service ports.** [explorer/k8s/helm/account-squid/templates/statefulset.yaml [64-73]](https://github.com/subspace/infra/pull/305/files#diff-986ba76ff871e9ce1c927bf01b53b4de48eb7ee70a33e23cc67e1b0e235feff1R64-R73) ```diff livenessProbe: exec: command: - pg_isready - -U - postgres - -h - - 127.0.0.1 + - {{ .Values.postgres.host | default "127.0.0.1" }} - -p - - "5432" + - {{ .Values.postgres.port | default "5432" }} ``` | |
Performance |
Optimize image pulling policy to reduce registry load and improve deployment speed.___ **TheimagePullPolicy is set to Always , which can lead to unnecessary load on the image registry and increased deployment times, especially in production environments where image updates are less frequent. Consider changing the imagePullPolicy to IfNotPresent to use the cached image if it exists, reducing load and speeding up deployments.** [explorer/k8s/helm/account-squid/templates/statefulset.yaml [27]](https://github.com/subspace/infra/pull/305/files#diff-986ba76ff871e9ce1c927bf01b53b4de48eb7ee70a33e23cc67e1b0e235feff1R27-R27) ```diff -imagePullPolicy: {{ .Values.image.pullPolicy | quote }} +imagePullPolicy: IfNotPresent # Use cached image if available ``` |
closing as making #304 more reusable for all micro-squids
Type
enhancement
Description
account-squid
application.Changes walkthrough
1 files
_helpers.tpl
Add Helper Templates for Helm Chart Configuration
explorer/k8s/helm/account-squid/templates/_helpers.tpl
naming in Helm chart.
13 files
.helmignore
Create .helmignore for Helm Packaging
explorer/k8s/helm/account-squid/.helmignore
packaging.
Chart.yaml
Define Metadata and Versioning for Helm Chart
explorer/k8s/helm/account-squid/Chart.yaml - Defined basic metadata and versioning for the Helm chart.
explorer-env-file
Configure Environment Variables for Database and Network
explorer/k8s/helm/account-squid/config/explorer-env-file
acme-certificate.yaml
Setup ACME Certificate Configuration
explorer/k8s/helm/account-squid/misc/acme-certificate.yaml
cert-manager.
clusterroles.yaml
Define Cluster Roles and Bindings for Kubernetes
explorer/k8s/helm/account-squid/templates/clusterroles.yaml
privileges.
configmap.yaml
Create ConfigMap for PostgreSQL Configuration
explorer/k8s/helm/account-squid/templates/configmap.yaml - Created a ConfigMap for PostgreSQL configuration.
hpa.yaml
Configure Horizontal Pod Autoscaler
explorer/k8s/helm/account-squid/templates/hpa.yaml
ingress.yaml
Setup Ingress and TLS Configuration
explorer/k8s/helm/account-squid/templates/ingress.yaml - Setup Ingress for routing with TLS configuration.
loadbal-svc.yaml
Configure LoadBalancer Service
explorer/k8s/helm/account-squid/templates/loadbal-svc.yaml - Configured a LoadBalancer service for the application.
namespace.yaml
Create Namespace for Application
explorer/k8s/helm/account-squid/templates/namespace.yaml - Created a Kubernetes namespace for the application.
postgres-configmap.yaml
Define PostgreSQL Configurations in ConfigMap
explorer/k8s/helm/account-squid/templates/postgres-configmap.yaml - Defined PostgreSQL specific configurations in a ConfigMap.
pv.yaml
Setup Persistent Volume for Data Storage
explorer/k8s/helm/account-squid/templates/pv.yaml - Setup a persistent volume for data storage.
pvc.yaml
Create Persistent Volume Claim
explorer/k8s/helm/account-squid/templates/pvc.yaml - Created a persistent volume claim for the application.
1 files
NOTES.txt
Provide Access Instructions for Various Service Types
explorer/k8s/helm/account-squid/templates/NOTES.txt
different Kubernetes service types.