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
47 changes: 47 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
name: Build

on:
workflow_dispatch:
push:
branches:
- master

Comment on lines +3 to +8
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Consider adding a PR build (no push) trigger, or explicitly document “master-only push” behavior.

Right now this only runs on workflow_dispatch and push to master, so you won’t get CI signal on PRs unless someone manually dispatches. If that’s intentional, a short comment helps future maintainers.

🤖 Prompt for AI Agents
In @.github/workflows/build.yml around lines 6 - 11, The workflow currently
triggers only on workflow_dispatch and push to master (the on: workflow_dispatch
and on: push: branches: - master block), which means PRs won't run CI; either
add a pull_request trigger (add on: pull_request with desired branches or types)
to run CI for PRs, or add a clear inline YAML comment above the on: section
documenting that push is intentionally restricted to master and PRs must be
manually dispatched—make one change (prefer adding pull_request for automatic PR
builds) and ensure the YAML remains valid.

jobs:
build:
name: Build & deploy
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 #v6.0.1

- name: Login to Docker Hub
uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef #v3.6.0
with:
username: ${{ secrets.ACR_USERNAME }}
password: ${{ secrets.ACR_PASSWORD }}
registry: cratecache.azurecr.io/new-black
Comment on lines +16 to +21
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix the registry configuration.

The registry field should contain only the registry hostname (e.g., cratecache.azurecr.io), not include the repository path /new-black. The namespace belongs in the image tags, not the registry URL.

🔧 Proposed fix
       - name: Login to Docker Hub
         uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef #v3.6.0
         with:
           username: ${{ secrets.ACR_USERNAME }}
           password: ${{ secrets.ACR_PASSWORD }}
-          registry: cratecache.azurecr.io/new-black
+          registry: cratecache.azurecr.io
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Login to Docker Hub
uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef #v3.6.0
with:
username: ${{ secrets.ACR_USERNAME }}
password: ${{ secrets.ACR_PASSWORD }}
registry: cratecache.azurecr.io/new-black
- name: Login to Docker Hub
uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef #v3.6.0
with:
username: ${{ secrets.ACR_USERNAME }}
password: ${{ secrets.ACR_PASSWORD }}
registry: cratecache.azurecr.io
🤖 Prompt for AI Agents
In @.github/build.yml around lines 19 - 24, The Docker login step using
docker/login-action (step name "Login to Docker Hub") incorrectly sets registry
to include the repository path; change the registry value from
"cratecache.azurecr.io/new-black" to just "cratecache.azurecr.io" and ensure the
"new-black" namespace is applied when tagging/pushing images (not in the
registry field) so image references become e.g.
cratecache.azurecr.io/new-black:tag at push time.


Comment on lines +16 to +22
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Step name is misleading (this logs into ACR, not Docker Hub).

Rename the step to avoid confusion during incident/debugging.

Proposed diff
-      - name: Login to Docker Hub
+      - name: Login to Azure Container Registry (ACR)
         uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef #v3.6.0
         with:
           username: ${{ secrets.ACR_USERNAME }}
           password: ${{ secrets.ACR_PASSWORD }}
           registry: cratecache.azurecr.io/new-black
🤖 Prompt for AI Agents
In @.github/workflows/build.yml around lines 19 - 25, The workflow step named
"Login to Docker Hub" is misleading because it logs into Azure Container
Registry using docker/login-action; rename the step (the "name" field for the
step using docker/login-action@5e57cd1) to something like "Login to ACR" or
"Login to Azure Container Registry" so the step accurately reflects the action
and registry (cratecache.azurecr.io/new-black); no other behavior changes
required.

# Switch to a different builder
- name: Set up Docker Buildx
id: buildx
uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f #3.12.0

# Determine the run number
- name: Set the run number
id: commit-id
run: |
echo "build_number=$GITHUB_RUN_NUMBER" >> $GITHUB_ENV

Comment on lines +29 to +33
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Minor: quote $GITHUB_ENV and align the step id with what it sets.

id: commit-id is a bit confusing since you’re exporting a run/build number.

Proposed diff
-      - name: Set the run number
-        id: commit-id
+      - name: Set the run number
+        id: build-number
         run: |
-          echo "build_number=$GITHUB_RUN_NUMBER" >> $GITHUB_ENV
+          echo "build_number=$GITHUB_RUN_NUMBER" >> "$GITHUB_ENV"
🤖 Prompt for AI Agents
In @.github/workflows/build.yml around lines 32 - 36, The workflow step uses id:
commit-id but actually exports the run/build number and writes to $GITHUB_ENV
unquoted; rename the step id to something descriptive like build-number or
set-build-number (e.g., id: build-number) and update the run command to quote
the environment variable file path (echo "build_number=$GITHUB_RUN_NUMBER" >>
"$GITHUB_ENV") so the id matches intent and the $GITHUB_ENV is safely quoted.

# Build the production image
- name: Build production image
uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 #v6.18.0
with:
context: .
platforms: linux/amd64,linux/arm64
push: true
target: release
build-args: |
MAXMIND_LICENSE_KEY=${{ secrets.MAXMIND_LICENSE_KEY }}
tags: |
cratecache.azurecr.io/new-black/eva-whoami:latest
cratecache.azurecr.io/new-black/eva-whoami:${{ env.build_number }}
builder: ${{ steps.buildx.outputs.name }}
6 changes: 6 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ RUN sed 's/GeoLite2-Country_[0-9]*.tar.gz/GeoLite2-Country.tar.gz/g' -i GeoLite2
RUN sha256sum -c GeoLite2-Country.tar.gz.sha256
RUN tar xvf GeoLite2-Country.tar.gz --strip 1

RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz
RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256
RUN sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256
RUN sha256sum -c GeoLite2-City.tar.gz.sha256
RUN tar xvf GeoLite2-City.tar.gz --strip 1
Comment on lines +22 to +26
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The steps for handling the GeoLite2-City database are correctly implemented, ensuring secure download, verification, and extraction. To optimize the Dockerfile and reduce the number of layers, consider combining these RUN commands into a single RUN command using && to chain the commands together. This can improve the build performance and reduce the image size.

-RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz
-RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256
-RUN sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256
-RUN sha256sum -c GeoLite2-City.tar.gz.sha256
-RUN tar xvf GeoLite2-City.tar.gz --strip 1
+RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz && \
+    wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256 && \
+    sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256 && \
+    sha256sum -c GeoLite2-City.tar.gz.sha256 && \
+    tar xvf GeoLite2-City.tar.gz --strip 1

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz
RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256
RUN sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256
RUN sha256sum -c GeoLite2-City.tar.gz.sha256
RUN tar xvf GeoLite2-City.tar.gz --strip 1
RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz && \
wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256 && \
sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256 && \
sha256sum -c GeoLite2-City.tar.gz.sha256 && \
tar xvf GeoLite2-City.tar.gz --strip 1


FROM alpine:3.23 as release
LABEL name="ipinfo.tw"
RUN mkdir -p /run/nginx/ /usr/share/GeoIP/
Expand Down
8 changes: 6 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,12 @@ Use any http(s) client to explore the server, e.g. https://ipinfo.tw,
- `wget -qO- https://ipinfo.tw`
- `curl https://ipinfo.tw`

Without any specified URI, the server will return IP address, country, AS, and user agent.
Without any specified URI, the server will return IP address, country, timezone, AS, and user agent.

If you prefer to receive a machine-readable result, use path `/json` (without trailing slash), e.g. `https://ipinfo.tw/json`, the result will look like:

```json
{"ip":"3.115.123.234","country_code":"JP","country_name":"Japan","asn":"16509","as_desc":"Amazon.com, Inc.","user_agent":"curl/7.58.0"}
{"ip":"3.115.123.234","country_code":"JP","country_name":"Japan","timezone":"Asia/Tokyo","asn":"16509","as_desc":"Amazon.com, Inc.","user_agent":"curl/7.58.0"}
```

#### Endpoints
Comment on lines 55 to 66
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]

The description of the demo setup mentions "an reverse proxy" which should be corrected to "a reverse proxy" for grammatical accuracy.

- this demo is behind an reverse proxy with https enabled
+ this demo is behind a reverse proxy with https enabled

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]

There's a minor typographical error with "http traffic" which should be capitalized as "HTTP traffic" for consistency with standard terminology.

- http traffic will be redirected to use https
+ HTTP traffic will be redirected to use https

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [49-49]

The phrase "pass the it to the container" seems to contain an extra word. It should be corrected for clarity.

- set up an `X-Real-IP` header and pass the it to the container
+ set up an `X-Real-IP` header and pass it to the container

Expand All @@ -75,6 +75,7 @@ You can also specify the following URI to retrieve certain info:
- `asn`: AS number
- `as_desc`: AS description
- `user_agent`: User agent string
- `timezone`: Timezone based on the city (e.g Europe/Amsterdam)

Examples:

Expand All @@ -97,6 +98,9 @@ HK
$ curl https://ipinfo.tw/country_name
South Korea

$ curl https://ipinfo.tw/timezone
Europe/Amsterdam

$ curl https://ipinfo.tw/as
AS16509 / Amazon.com, Inc.

Expand Down
39 changes: 39 additions & 0 deletions k8s/deployment.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: eva-whoami
name: eva-whoami
namespace: platform-tools
spec:
progressDeadlineSeconds: 600
replicas: 2
revisionHistoryLimit: 10
selector:
matchLabels:
app: eva-whoami
strategy:
rollingUpdate:
maxSurge: 25%
maxUnavailable: 25%
type: RollingUpdate
template:
metadata:
labels:
app: eva-whoami
spec:
containers:
- image: cratecache.azurecr.io/new-black/eva-whoami
imagePullPolicy: Always
name: whoami
ports:
- containerPort: 8080
protocol: TCP
resources: {}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Define resource limits and requests.

The deployment has no resource constraints, which can lead to resource contention, unpredictable performance, and potential node exhaustion. Production deployments should always specify CPU and memory limits/requests.

⚙️ Proposed resource configuration
-        resources: {}
+        resources:
+          requests:
+            cpu: 100m
+            memory: 128Mi
+          limits:
+            cpu: 500m
+            memory: 256Mi

Adjust values based on actual application requirements.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resources: {}
resources:
requests:
cpu: 100m
memory: 128Mi
limits:
cpu: 500m
memory: 256Mi
🤖 Prompt for AI Agents
In @k8s/deployment.yaml at line 32, The deployment currently has an empty
resources block ("resources: {}"); replace it with explicit resource requests
and limits by adding a resources section containing requests (cpu and memory)
and limits (cpu and memory) for the container (e.g., under the container spec
where "resources" appears), choosing appropriate values for your app (start with
conservative defaults like requests: cpu: "100m", memory: "128Mi" and limits:
cpu: "500m", memory: "256Mi" and adjust based on real usage); ensure the new
resources block is added to the same container spec that originally had
"resources: {}".

terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
dnsPolicy: ClusterFirst
restartPolicy: Always
schedulerName: default-scheduler
securityContext: {}
Comment on lines +25 to +38
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add security context to prevent privilege escalation.

The deployment lacks security context constraints, leaving it vulnerable to privilege escalation and running as root. This violates Kubernetes security best practices and Pod Security Standards.

🔒 Proposed security hardening
       containers:
       - image: cratecache.azurecr.io/new-black/eva-whoami
         imagePullPolicy: Always
         name: whoami
         ports:
         - containerPort: 8080
           protocol: TCP
         resources: {}
+        securityContext:
+          allowPrivilegeEscalation: false
+          runAsNonRoot: true
+          runAsUser: 10001
+          capabilities:
+            drop:
+              - ALL
+          readOnlyRootFilesystem: true
         terminationMessagePath: /dev/termination-log
         terminationMessagePolicy: File
       dnsPolicy: ClusterFirst
       restartPolicy: Always
       schedulerName: default-scheduler
-      securityContext: {}
+      securityContext:
+        runAsNonRoot: true
+        seccompProfile:
+          type: RuntimeDefault
       terminationGracePeriodSeconds: 30

Note: If the application requires write access, mount an emptyDir volume for temporary storage instead of using a writable root filesystem.

Based on static analysis hints.

terminationGracePeriodSeconds: 30
Comment on lines +1 to +39
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical mismatch: Files don't match PR objectives.

The PR summary states this adds "timezone information to the available fields" for the ipinfo.tw project with nginx configuration changes, but this file deploys an entirely different service (eva-whoami) in Kubernetes. This appears to be either:

  • Files from a different PR mistakenly included
  • A completely unrelated change bundled in the timezone PR

Please verify that the correct files are included in this PR.

🧰 Tools
🪛 Checkov (3.2.334)

[medium] 1-39: Containers should not run with allowPrivilegeEscalation

(CKV_K8S_20)


[medium] 1-39: Minimize the admission of root containers

(CKV_K8S_23)

29 changes: 29 additions & 0 deletions k8s/ingress.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
cert-manager.io/acme-challenge-type: dns01
cert-manager.io/cluster-issuer: letsencrypt-prod-dns
kubernetes.io/ingress.class: nginx
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove deprecated ingress class annotation.

The deprecated annotation kubernetes.io/ingress.class: nginx is redundant with the newer ingressClassName: nginx field (line 14). Modern Kubernetes versions (1.18+) should use only the ingressClassName field.

🧹 Proposed cleanup
   annotations:
     cert-manager.io/acme-challenge-type: dns01
     cert-manager.io/cluster-issuer: letsencrypt-prod-dns
-    kubernetes.io/ingress.class: nginx
   generation: 2

Also applies to: 14-14

🤖 Prompt for AI Agents
In @k8s/ingress.yaml at line 7, The manifest still uses the deprecated
annotation key "kubernetes.io/ingress.class: nginx" while also specifying the
newer field ingressClassName: nginx; remove the deprecated annotation entry so
the Ingress relies solely on the ingressClassName field. Locate the Ingress
metadata annotations block and delete the "kubernetes.io/ingress.class"
annotation, leaving ingressClassName: nginx intact to avoid duplication and
deprecation warnings.

generation: 2
labels:
name: eva-whoami
name: eva-whoami
namespace: platform-tools
spec:
ingressClassName: nginx
rules:
- host: whoami.on-eva.io
http:
paths:
- backend:
service:
name: eva-whoami
port:
number: 8080
path: /
pathType: ImplementationSpecific
tls:
- hosts:
- whoami.on-eva.io
secretName: eva-whoami-tls
23 changes: 23 additions & 0 deletions k8s/service.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
apiVersion: v1
kind: Service
metadata:
annotations:
service.beta.kubernetes.io/azure-load-balancer-resource-group: rg-prod-eva
labels:
component: eva-whoami
name: eva-whoami
namespace: platform-tools
spec:
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack
ports:
- name: whoami
nodePort: 31514
port: 8080
protocol: TCP
targetPort: 8080
selector:
app: eva-whoami
sessionAffinity: None
type: LoadBalancer
5 changes: 5 additions & 0 deletions nginx/conf.d/geoip2.conf
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,8 @@ geoip2 /usr/share/GeoIP/GeoLite2-ASN.mmdb {
$ip_aso source=$remote_addr autonomous_system_organization;
$ip_as_build_epoch metadata build_epoch;
}

geoip2 /usr/share/GeoIP/GeoLite2-City.mmdb {
auto_reload 1d;
$ip_time_zone source=$remote_addr location time_zone;
}
5 changes: 4 additions & 1 deletion nginx/conf.d/ipinfo.conf
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ server {
location = /user_agent {
return 200 "$http_user_agent\n";
}
location = /timezone {
return 200 "$ip_time_zone\n";
}
location ~* ^/index.htm(l)?$ {
rewrite ^(.*)$ /;
}
Expand All @@ -58,7 +61,7 @@ server {
}
location = /json {
default_type application/json;
return 200 "{\"ip\":\"$remote_addr\",\"country_code\":\"$ip_country_code\",\"country_name\":\"$ip_country_name\",\"asn\":\"$ip_asn\",\"as_desc\":\"$ip_aso\",\"user_agent\":\"$http_user_agent\"}\n";
return 200 "{\"ip\":\"$remote_addr\",\"country_code\":\"$ip_country_code\",\"country_name\":\"$ip_country_name\",\"timezone\":\"$ip_time_zone\",\"asn\":\"$ip_asn\",\"as_desc\":\"$ip_aso\",\"user_agent\":\"$http_user_agent\"}\n";
}
location = /build_epoch {
default_type application/json;
Expand Down