Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions argocd/inventory-dev.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
name: inventory-dev
namespace: argocd
spec:
project: default
source:
repoURL: https://github.com/Cognition-Partner-Workshops/ordermanager-iac.git

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

targetRevision: main
path: helm/inventory
helm:
valueFiles:
- values.yaml
- values-dev.yaml
destination:
server: https://kubernetes.default.svc
namespace: decomposition-dev
syncPolicy:
automated:
prune: true
selfHeal: true
syncOptions:
- CreateNamespace=true
24 changes: 24 additions & 0 deletions argocd/inventory-staging.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
name: inventory-staging
namespace: argocd
spec:
project: default
source:
repoURL: https://github.com/Cognition-Partner-Workshops/ordermanager-iac.git
targetRevision: main
path: helm/inventory
helm:
valueFiles:
- values.yaml
- values-staging.yaml
destination:
server: https://kubernetes.default.svc
namespace: decomposition-staging
syncPolicy:
automated:
prune: true
selfHeal: true
syncOptions:
- CreateNamespace=true
15 changes: 15 additions & 0 deletions docker/inventory/Dockerfile

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Stage 1: Build .NET Inventory API
FROM mcr.microsoft.com/dotnet/sdk:8.0 AS build
WORKDIR /src
COPY src/Inventory.Api/Inventory.Api.csproj src/Inventory.Api/
RUN dotnet restore src/Inventory.Api/Inventory.Api.csproj
COPY src/Inventory.Api/ src/Inventory.Api/
RUN dotnet publish src/Inventory.Api/Inventory.Api.csproj -c Release -o /app/publish

# Stage 2: Runtime
FROM mcr.microsoft.com/dotnet/aspnet:8.0-alpine AS runtime
WORKDIR /app
COPY --from=build /app/publish .
EXPOSE 8080
ENV ASPNETCORE_URLS=http://+:8080
ENTRYPOINT ["dotnet", "Inventory.Api.dll"]
6 changes: 6 additions & 0 deletions helm/inventory/Chart.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: v2
name: inventory
description: Helm chart for the Inventory microservice (extracted from OrderManager monolith)
type: application
version: 0.1.0
appVersion: "1.0.0"
27 changes: 27 additions & 0 deletions helm/inventory/templates/_helpers.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{{/*
Expand the name of the chart.
*/}}
{{- define "inventory.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

{{- define "inventory.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}

{{- define "inventory.labels" -}}
helm.sh/chart: {{ .Chart.Name }}-{{ .Chart.Version }}
app.kubernetes.io/name: {{ include "inventory.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

{{- define "inventory.selectorLabels" -}}
app.kubernetes.io/name: {{ include "inventory.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}
44 changes: 44 additions & 0 deletions helm/inventory/templates/deployment.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: {{ include "inventory.fullname" . }}
labels:
{{- include "inventory.labels" . | nindent 4 }}
spec:
{{- if not .Values.autoscaling.enabled }}
replicas: {{ .Values.replicaCount }}
{{- end }}
selector:
matchLabels:
{{- include "inventory.selectorLabels" . | nindent 6 }}
template:
metadata:
labels:
{{- include "inventory.selectorLabels" . | nindent 8 }}
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
Comment on lines +18 to +49

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟨 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

22 changes: 22 additions & 0 deletions helm/inventory/templates/hpa.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{{- if .Values.autoscaling.enabled }}
apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
name: {{ include "inventory.fullname" . }}
labels:
{{- include "inventory.labels" . | nindent 4 }}
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "inventory.fullname" . }}
minReplicas: {{ .Values.autoscaling.minReplicas }}
maxReplicas: {{ .Values.autoscaling.maxReplicas }}
metrics:
- type: Resource
resource:
name: cpu
target:
type: Utilization
averageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }}
{{- end }}
29 changes: 29 additions & 0 deletions helm/inventory/templates/ingress.yaml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{{- if .Values.ingress.enabled -}}
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: {{ include "inventory.fullname" . }}
labels:
{{- include "inventory.labels" . | nindent 4 }}
{{- with .Values.ingress.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
spec:
ingressClassName: {{ .Values.ingress.className }}
rules:
{{- range .Values.ingress.hosts }}
- host: {{ .host | quote }}
http:
paths:
{{- range .paths }}
- path: {{ .path }}
pathType: {{ .pathType }}
backend:
service:
name: {{ include "inventory.fullname" $ }}
port:
number: {{ $.Values.service.port }}
{{- end }}
{{- end }}
{{- end }}
48 changes: 48 additions & 0 deletions helm/inventory/templates/networkpolicy.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: {{ include "inventory.fullname" . }}
labels:
{{- include "inventory.labels" . | nindent 4 }}
spec:
podSelector:
matchLabels:
{{- include "inventory.selectorLabels" . | nindent 6 }}
policyTypes:
- Ingress
- Egress
ingress:
- from:
- namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: ingress-nginx
ports:
- protocol: TCP
port: {{ .Values.service.targetPort }}
- from:
- podSelector:
matchLabels:
app.kubernetes.io/name: ordermanager
ports:
- protocol: TCP
port: {{ .Values.service.targetPort }}
{{- if .Values.monitoring.enabled }}
- from:
- namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: monitoring
ports:
- protocol: TCP
port: {{ .Values.monitoring.port }}
{{- end }}
egress:
- to: []
ports:
- protocol: UDP
port: 53
- protocol: TCP
port: 53
- to: []
ports:
- protocol: TCP
port: 443
15 changes: 15 additions & 0 deletions helm/inventory/templates/service.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
apiVersion: v1
kind: Service
metadata:
name: {{ include "inventory.fullname" . }}
labels:
{{- include "inventory.labels" . | nindent 4 }}
spec:
type: {{ .Values.service.type }}
ports:
- port: {{ .Values.service.port }}
targetPort: {{ .Values.service.targetPort }}
protocol: TCP
name: http
selector:
{{- include "inventory.selectorLabels" . | nindent 4 }}
16 changes: 16 additions & 0 deletions helm/inventory/templates/servicemonitor.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{{- if .Values.monitoring.enabled }}
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
name: {{ include "inventory.fullname" . }}
labels:
{{- include "inventory.labels" . | nindent 4 }}
spec:
selector:
matchLabels:
{{- include "inventory.selectorLabels" . | nindent 6 }}
endpoints:
- port: http
path: {{ .Values.monitoring.path }}
interval: 30s
{{- end }}
28 changes: 28 additions & 0 deletions helm/inventory/values-dev.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
replicaCount: 1

image:
tag: dev-latest

ingress:
hosts:
- host: inventory-dev.workshop.local
paths:
- path: /
pathType: Prefix

resources:
requests:
cpu: 50m
memory: 128Mi
limits:
cpu: 250m
memory: 256Mi

env:
- name: ASPNETCORE_ENVIRONMENT
value: Development
- name: ConnectionStrings__DefaultConnection
value: "Data Source=inventory.db"

persistence:
enabled: false
25 changes: 25 additions & 0 deletions helm/inventory/values-staging.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
replicaCount: 2

image:
tag: staging-latest

ingress:
hosts:
- host: inventory-staging.workshop.local
paths:
- path: /
pathType: Prefix

resources:
requests:
cpu: 100m
memory: 128Mi
limits:
cpu: 250m
memory: 256Mi

autoscaling:
enabled: true
minReplicas: 2
maxReplicas: 4
targetCPUUtilizationPercentage: 75
Comment on lines +21 to +25

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

52 changes: 52 additions & 0 deletions helm/inventory/values.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
replicaCount: 1

image:
repository: 599083837640.dkr.ecr.us-east-1.amazonaws.com/workshop/inventory
tag: latest
pullPolicy: IfNotPresent

service:
type: ClusterIP
port: 80
targetPort: 8080

ingress:
enabled: true
className: nginx
annotations:
cert-manager.io/cluster-issuer: letsencrypt-staging

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟨 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

hosts:
- host: inventory.workshop.local
paths:
- path: /
pathType: Prefix

resources:
requests:
cpu: 50m
memory: 128Mi
limits:
cpu: 250m
memory: 256Mi

autoscaling:
enabled: false
minReplicas: 1
maxReplicas: 3
targetCPUUtilizationPercentage: 80

env:
- name: ASPNETCORE_ENVIRONMENT
value: Production
- name: ConnectionStrings__DefaultConnection
value: "Data Source=/data/inventory.db"

persistence:
enabled: true
size: 1Gi
storageClass: gp2
Comment on lines +42 to +47

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔴 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:

  1. There is no PVC template in helm/inventory/templates/ — no pvc.yaml or similar file exists.
  2. The deployment template (helm/inventory/templates/deployment.yaml) has no volumes or volumeMounts sections.
  3. The runtime container image is based on mcr.microsoft.com/dotnet/aspnet:8.0-alpine (docker/inventory/Dockerfile:10), which does not have a /data directory.

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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


monitoring:
enabled: true
path: /metrics
port: 8080