Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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 }}
55 changes: 55 additions & 0 deletions helm/inventory/templates/deployment.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
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 }}
{{- if .Values.persistence.enabled }}
volumeMounts:
- name: data
mountPath: /data
{{- end }}
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.

{{- if .Values.persistence.enabled }}
volumes:
- name: data
persistentVolumeClaim:
claimName: {{ include "inventory.fullname" . }}-data
{{- end }}
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
17 changes: 17 additions & 0 deletions helm/inventory/templates/pvc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{{- if .Values.persistence.enabled }}
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: {{ include "inventory.fullname" . }}-data
labels:
{{- include "inventory.labels" . | nindent 4 }}
spec:
accessModes:
- ReadWriteOnce
{{- if .Values.persistence.storageClass }}
storageClassName: {{ .Values.persistence.storageClass }}
{{- end }}
resources:
requests:
storage: {{ .Values.persistence.size }}
{{- end }}
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.

Loading