Add Inventory service IaC: Dockerfile, Helm chart, ArgoCD manifests#18
Add Inventory service IaC: Dockerfile, Helm chart, ArgoCD manifests#18devin-ai-integration[bot] wants to merge 2 commits into
Conversation
- Add docker/inventory/Dockerfile: .NET 8 multi-stage build for Inventory.Api - Add helm/inventory/ chart with Deployment, Service, Ingress, NetworkPolicy, ServiceMonitor, HPA templates (mirrors ordermanager chart patterns) - NetworkPolicy allows traffic from ordermanager pods and ingress-nginx, plus Prometheus scraping from monitoring namespace - Health check probes point to /health endpoint - Add ArgoCD Application manifests for dev and staging environments - Add values-dev.yaml and values-staging.yaml environment overrides
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 No CI/CD pipeline exists for building the inventory Docker image
The existing ci/build-push.yaml only builds and pushes the ordermanager image (ECR_REPOSITORY is hardcoded to workshop/ordermanager and it checks out app_dotnet-angular-monolith). There is no equivalent pipeline for the new inventory service. The docker/inventory/Dockerfile is added but nothing will build it. The Helm values reference 599083837640.dkr.ecr.us-east-1.amazonaws.com/workshop/inventory as the image repository (helm/inventory/values.yaml:4), but no automation exists to push images there. This means the inventory deployment will fail with an image pull error until a CI pipeline is added or images are pushed manually.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — out of scope for this PR. The task was to add the Dockerfile/Helm/ArgoCD manifests modeled on the existing monolith patterns; the CI build-push pipeline for the inventory image is a separate follow-up. Flagging to the repo owner rather than adding a pipeline here, since the ECR repo/permissions setup for workshop/inventory needs to be provisioned first.
There was a problem hiding this comment.
🔍 Ingress template has cert-manager annotation but no TLS section
The base values.yaml includes the annotation cert-manager.io/cluster-issuer: letsencrypt-staging (helm/inventory/values.yaml:17), which tells cert-manager to provision a TLS certificate. However, the ingress template (helm/inventory/templates/ingress.yaml) has no tls section in the spec. Without a tls block specifying the secret name and hosts, cert-manager won't know which certificate to create and the ingress won't terminate TLS. This is consistent with the existing ordermanager ingress template (helm/ordermanager/templates/ingress.yaml), so it may be intentional for the workshop context, but it means HTTPS won't actually work.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and intentional — as noted, this matches the existing helm/ordermanager/templates/ingress.yaml which also has no tls block. Kept consistent with the monolith chart per the task ("modeled on the existing monolith chart"). Adding a tls section would be part of a broader TLS-enablement change that should be applied to both charts together.
| value: "Data Source=/data/inventory.db" | ||
|
|
||
| persistence: | ||
| enabled: true | ||
| size: 1Gi | ||
| storageClass: gp2 |
There was a problem hiding this comment.
🔴 Production database path references a non-existent directory, causing the service to fail on startup
The production connection string is set to write the database at /data/inventory.db (helm/inventory/values.yaml:42), but no persistent volume or volume mount is defined in the deployment template, so the /data directory does not exist in the container and the database cannot be created.
Impact: The inventory service will crash or error on first database access in production because the target directory is missing.
Missing PVC template and volume mount despite persistence configuration
The values.yaml configures persistence.enabled: true with size: 1Gi and storageClass: gp2 (helm/inventory/values.yaml:44-47), clearly intending for a PersistentVolumeClaim to be created and mounted at /data. However:
- There is no PVC template in
helm/inventory/templates/— nopvc.yamlor similar file exists. - The deployment template (
helm/inventory/templates/deployment.yaml) has novolumesorvolumeMountssections. - The runtime container image is based on
mcr.microsoft.com/dotnet/aspnet:8.0-alpine(docker/inventory/Dockerfile:10), which does not have a/datadirectory.
When the .NET application tries to open Data Source=/data/inventory.db, SQLite will fail with "unable to open database file" because the directory doesn't exist. The dev environment works around this by using a relative path Data Source=inventory.db (helm/inventory/values-dev.yaml:25), which writes to the working directory /app.
Note: The existing ordermanager chart has the same pattern (helm/ordermanager/values.yaml:41-47), suggesting this may be a known gap, but it's still a bug that would prevent the production deployment from functioning correctly.
Prompt for agents
The helm/inventory/values.yaml configures persistence (enabled: true, size: 1Gi, storageClass: gp2) and the connection string references /data/inventory.db, but there is no PVC template or volume mount in the deployment. Two things need to happen:
1. Create a new template file helm/inventory/templates/pvc.yaml that creates a PersistentVolumeClaim when .Values.persistence.enabled is true, using .Values.persistence.size and .Values.persistence.storageClass.
2. Update helm/inventory/templates/deployment.yaml to add a volume referencing the PVC (when persistence is enabled) and a volumeMount at /data in the container spec.
Alternatively, if persistence is not yet needed, change the connection string in values.yaml from Data Source=/data/inventory.db to Data Source=inventory.db (relative to /app working directory) and remove or comment out the persistence values to avoid confusion. Note that the same issue exists in the ordermanager chart (helm/ordermanager/) and should be fixed there too.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in cba8dfb's follow-up commit. Added helm/inventory/templates/pvc.yaml (creates a PersistentVolumeClaim named <fullname>-data when persistence.enabled), and wired a volume + volumeMount at /data into deployment.yaml. helm template now renders the PVC (1Gi, gp2, RWO) and the mount, so Data Source=/data/inventory.db resolves to a real directory in production.
| autoscaling: | ||
| enabled: true | ||
| minReplicas: 2 | ||
| maxReplicas: 4 | ||
| targetCPUUtilizationPercentage: 75 |
There was a problem hiding this comment.
🔍 Staging enables autoscaling with multiple replicas but uses SQLite, which doesn't support concurrent writes
The staging values (helm/inventory/values-staging.yaml:21-24) enable autoscaling with 2-4 replicas. The base connection string uses SQLite (Data Source=/data/inventory.db at helm/inventory/values.yaml:42), and staging doesn't override it. SQLite is a file-based database that does not handle concurrent writes from multiple processes well. With 2+ replicas, each pod would either have its own ephemeral copy of the database (data inconsistency) or, if a shared volume were mounted, would encounter frequent database locking errors. This configuration is likely fine for a workshop/demo context but would be problematic in a real staging environment.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid concern for a real environment. As you note, this is a workshop/demo config and mirrors the ordermanager staging setup. A proper fix would swap SQLite for a networked DB (Postgres/RDS) before scaling past 1 replica, or pin the service to replicaCount: 1/autoscaling.enabled: false with a RWO volume. Leaving as-is to stay consistent with the monolith chart, but flagging that this shouldn't be carried into a real multi-replica deployment.
| spec: | ||
| project: default | ||
| source: | ||
| repoURL: https://github.com/Cognition-Partner-Workshops/ordermanager-iac.git |
There was a problem hiding this comment.
🔍 ArgoCD inventory apps reference a different repo URL than the existing ordermanager apps
The new inventory ArgoCD manifests use repoURL: https://github.com/Cognition-Partner-Workshops/ordermanager-iac.git (argocd/inventory-dev.yaml:9), which matches the actual repository name. However, the existing ordermanager ArgoCD apps use repoURL: https://github.com/Cognition-Partner-Workshops/app_dotnet-angular-monolith-iac.git (argocd/application-dev.yaml:9). This discrepancy suggests the repository was renamed at some point and the old ArgoCD manifests were never updated. The new inventory URLs appear correct, but the inconsistency may cause confusion and the old ordermanager ArgoCD apps may need updating to the current repo name.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The new inventory manifests deliberately use the correct current repo name (ordermanager-iac). The existing ordermanager apps still point at the old app_dotnet-angular-monolith-iac.git name — that's a pre-existing inconsistency from a repo rename, outside this PR's scope. Flagging to the repo owner that those older ArgoCD apps should be updated separately.
| spec: | ||
| containers: | ||
| - name: {{ .Chart.Name }} | ||
| image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | ||
| ports: | ||
| - name: http | ||
| containerPort: {{ .Values.service.targetPort }} | ||
| protocol: TCP | ||
| {{- if .Values.env }} | ||
| env: | ||
| {{- toYaml .Values.env | nindent 12 }} | ||
| {{- end }} | ||
| resources: | ||
| {{- toYaml .Values.resources | nindent 12 }} | ||
| livenessProbe: | ||
| httpGet: | ||
| path: /health | ||
| port: http | ||
| initialDelaySeconds: 15 | ||
| periodSeconds: 20 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /health | ||
| port: http | ||
| initialDelaySeconds: 5 | ||
| periodSeconds: 10 |
There was a problem hiding this comment.
🟨 Missing pod security context allows containers to run with elevated privileges
The deployment template (helm/inventory/templates/deployment.yaml) does not define a securityContext at either the pod or container level. Without explicit settings like runAsNonRoot: true, readOnlyRootFilesystem: true, or allowPrivilegeEscalation: false, the container may run as root and have unnecessary filesystem write access, increasing the blast radius if the application is compromised.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Noted. The chart mirrors the existing helm/ordermanager deployment, which also omits a securityContext, so I've kept it consistent per the task. A hardening pass adding runAsNonRoot: true / allowPrivilegeEscalation: false / readOnlyRootFilesystem: true would be a good follow-up applied to both charts — note readOnlyRootFilesystem needs care here since the app writes the SQLite DB to the mounted /data volume (that mount would remain writable). Flagging rather than diverging from the monolith pattern in this PR.
| enabled: true | ||
| className: nginx | ||
| annotations: | ||
| cert-manager.io/cluster-issuer: letsencrypt-staging |
There was a problem hiding this comment.
🟨 Production TLS uses Let's Encrypt staging issuer, producing untrusted certificates
The base values.yaml (which serves as the production configuration) specifies cert-manager.io/cluster-issuer: letsencrypt-staging (helm/inventory/values.yaml:17). Let's Encrypt staging certificates are signed by an untrusted CA and will cause TLS validation failures for all clients connecting to the production inventory service.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional — this mirrors the existing helm/ordermanager chart exactly (same letsencrypt-staging issuer), and the hostnames are *.workshop.local (non-public, demo/workshop context) so a trusted CA cert isn't obtainable anyway. Switching to letsencrypt-prod would be the change for a real public deployment; deferring to keep consistency with the monolith chart per the task requirements.
1 participant
Summary
IaC for the new Inventory microservice extracted from the OrderManager monolith. Follows existing monolith patterns and the platform-engineering-shared-services standard.
Dockerfile (
docker/inventory/Dockerfile):dotnet/sdk:8.0→dotnet/aspnet:8.0-alpinesrc/Inventory.Api/Inventory.Api.csproj, publishes to/app/publishASPNETCORE_URLS=http://+:8080Helm chart (
helm/inventory/):/health, env-configurable DB connection stringinventory.workshop.local), HPAingress-nginx, fromordermanagerpods (for inter-service HTTP), and frommonitoringnamespace; DNS + HTTPS egress/metricsvalues-dev.yaml(1 replica, no persistence) andvalues-staging.yaml(2 replicas, HPA enabled)ArgoCD (
argocd/inventory-{dev,staging}.yaml):decomposition-dev/decomposition-stagingnamespacesCompanion app PR:
ordermanager-monolithbranchdemo-CBA(PR #287)Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/5e036e7cc7b24c85af46761838b0d590
Requested by: @VedantKh