Switch to building tar1090 from source with lighttpd

[Sol455]

Summary

This PR replaces the pre-built SDR Enthusiasts tar1090 Docker image with a from-scratch build using lighttpd. This resolves aircraft not appearing when using the adsb.lol fallback and gives us full control over the image configuration.

Issues Resolved

1. Aircraft not displaying with adsb.lol fallback

• The SDR Enthusiasts image uses nginx, but our proxy configs were written for lighttpd
• The lighttpd proxy config (lighttpd-proxy.conf) was never being applied
• Building from source with lighttpd ensures our proxy configuration works correctly

2. binCraft/zstd format incompatibility

• adsb.lol returns JSON, but the default tar1090 config expected binCraft format
• Added data/receiver.json with binCraft: false and zstd: false

3. Fallback logic never triggering

• Previous logic only fell back to adsb.lol on fetch errors
• When no ADS-B receiver is connected, readsb returns 0 aircraft (not an error)
• Fixed: Now falls back if error or 0 aircraft detected

4. Proxy port conflict with blah2-api

• Default proxy port changed from 3000 to 3005 to avoid conflicts with blah2-api

close #11
close #12

[claude]

PR Review: Switch to building tar1090 from source with lighttpd

Summary

This PR successfully addresses multiple issues by migrating from the pre-built SDR Enthusiasts Docker image to a from-scratch build. The changes are well-motivated and the implementation is generally solid.

---

Positive Aspects

Problem Solving

• Correctly identifies and fixes the root cause of aircraft not displaying with adsb.lol fallback
• The improved fallback logic (checking for 0 aircraft, not just errors) is a significant improvement
• Port change to 3005 prevents conflicts with blah2-api

Code Quality

• The proxy server logic in server.js:91-126 is well-structured with clear fallback flow
• Good separation of concerns: fetchAdsbLol() extracted as a separate function
• Improved logging provides better visibility into data source selection

---

Issues & Recommendations

1. Critical: Signal Handling Race Condition (Dockerfile:44-50)

The current signal trap implementation has a potential issue:

cleanup() {
    echo "Shutting down..."
    kill $PROXY_PID 2>/dev/null || true
    exit 0
}
trap cleanup SIGTERM SIGINT

Problem: lighttpd -D runs in foreground and blocks. If it exits unexpectedly, the proxy server will continue running as an orphan process.

Recommendation: Use wait to properly handle both processes:

#!/bin/bash
set -e

cleanup() {
    echo "Shutting down..."
    kill $PROXY_PID 2>/dev/null || true
    kill $LIGHTTPD_PID 2>/dev/null || true
    exit 0
}
trap cleanup SIGTERM SIGINT

echo "Starting aircraft data proxy on port ${PROXY_PORT:-3005}..."
node /opt/proxy/server.js &
PROXY_PID=$!

sleep 1
echo "Starting tar1090 web interface on port 80..."
lighttpd -D -f /etc/lighttpd/lighttpd.conf &
LIGHTTPD_PID=$!

wait -n
exit_code=$?
cleanup
exit $exit_code

2. Moderate: Port Hardcoding (docker/lighttpd-proxy.conf:5)

( "host" => "127.0.0.1", "port" => 3005 )

Issue: The proxy port is hardcoded but can be changed via PROXY_PORT environment variable. This creates inconsistency.

Recommendation: Either:

• Document that changing PROXY_PORT requires rebuilding the image
• Generate the lighttpd config at runtime using environment variable substitution

3. Minor: Missing Error Handling (proxy/server.js:4)

The READSB_URL defaults to http://127.0.0.1:80/data/aircraft.json, which will create a circular dependency since the proxy itself is handling /data/aircraft.json.

Recommendation: Update the default or add validation:

const READSB_URL = process.env.READSB_URL || 'http://readsb:80/data/aircraft.json';

4. Minor: Incomplete Cleanup (Dockerfile)

The removed Dockerfile.tar1090 is still referenced in .github/workflows/docker_build.yml (though this is fixed in the PR).

Recommendation: Consider removing docker/entrypoint.sh if it's no longer used (verify it's not referenced elsewhere).

5. Test Coverage Gap

No tests exist for the proxy server logic or the new fallback behavior.

Recommendations:

• Add unit tests for getAircraftData() fallback logic
• Add integration tests to verify the proxy correctly handles:
  • Local readsb returning 0 aircraft
  • Local readsb timeout/error
  • adsb.lol fallback success/failure
• Consider adding health check endpoint testing

6. Documentation: Missing Migration Notes

Users upgrading from the old setup might experience issues.

Recommendation: Add migration notes covering:

• Port change from 3000 → 3005 (update any external consumers)
• The fact that READSB_URL now needs to point to actual readsb, not the proxy
• How the new fallback logic differs (0 aircraft triggers fallback)

---

Security Considerations

✅ Good:

• No obvious injection vulnerabilities
• CORS headers properly configured
• No sensitive data logging

⚠️ Minor Concerns:

1. SSRF potential: The proxy fetches from READSB_URL without validation. If this env var is user-controlled, it could be exploited.

   • Mitigation: Document that READSB_URL should only point to trusted internal services
   • Consider adding URL validation to reject non-local/non-internal addresses

2. No rate limiting: The adsb.lol API calls have no rate limiting, which could lead to being blocked by the upstream service

   • Recommendation: Add simple in-memory rate limiting or caching

---

Performance Considerations

1. Sequential Requests: The proxy always tries local first, then fallback. This adds latency when local is down.

   • Consider: Circuit breaker pattern if local consistently fails
   • Current approach is acceptable for initial implementation

2. No Caching: Every request fetches fresh data. For a 1-second refresh interval, this is fine.

---

Overall Assessment

Quality: Good
Security: Acceptable with minor concerns noted
Completeness: Ready to merge with signal handling fix

Recommendation: Approve with request to fix the critical signal handling issue. Other items can be addressed in follow-up PRs.

---

Action Items

Must fix before merge:

• Fix signal handling in Dockerfile to properly manage both processes

Should fix (can be follow-up PR):

• Address port hardcoding inconsistency
• Add basic test coverage
• Document migration notes for users
• Validate READSB_URL to prevent SSRF

Nice to have:

• Add rate limiting for adsb.lol API
• Implement circuit breaker for improved resilience

[claude]

Pull Request Review: Switch to building tar1090 from source with lighttpd

Summary

This PR successfully addresses the root causes of aircraft not displaying when using the adsb.lol fallback by replacing the pre-built SDR Enthusiasts Docker image with a from-scratch build using lighttpd. The changes are well-motivated and resolve issues #11 and #12.

Strengths

1. Clear problem identification: The PR correctly identifies that the nginx/lighttpd mismatch was preventing the proxy configuration from working
2. Comprehensive solution: Addresses multiple related issues (binCraft format, fallback logic, port conflicts)
3. Better control: Building from source gives full control over the image configuration

Code Quality Issues

Critical Issues

1. Hardcoded IP in .env.example - Line 23 contains 192.168.8.183 which should be a placeholder
2. Location coordinates changed to San Francisco - Should use placeholder values or document clearly
3. Port hardcoding between lighttpd-proxy.conf and server.js - Comment says do not change PROXY_PORT which creates tight coupling

Major Issues

4. Signal handling uses basic kill - Should use SIGTERM before SIGKILL for graceful shutdown
5. Startup uses arbitrary 1-second sleep - Should check if proxy is actually listening
6. No health check defined despite running two processes
7. No input validation for environment variables

Positive Observations

• Good logging throughout proxy server
• Proper file permissions set
• Clean Docker layer optimization
• Fallback logic correctly fixed to check for 0 aircraft

Security Concerns

• No rate limiting on adsb.lol proxy requests
• Environment variables not validated
• Error messages could leak information
• CORS allows all origins (likely intentional but should document)

Test Coverage

No tests added for:

• Proxy fallback logic (the main fix)
• Data conversion from adsb.lol to readsb format
• Error handling paths
• Container startup/shutdown

Recommendations

Critical (fix before merge):

• Remove hardcoded IP from .env.example
• Document port synchronization requirement

High Priority:

• Add health check to docker-compose.yml
• Add input validation for env vars
• Add tests for fallback logic

Medium Priority:

• Improve signal handling
• Add caching for adsb.lol requests
• Add rate limiting

Conclusion

Overall Assessment: Approve with minor changes requested

This PR successfully solves the core issues and is a significant improvement. Main concerns are hardcoded config values and lack of tests. Great work identifying the root cause!

[Sol455]

Closing in favour of modifying: https://github.com/sdr-enthusiasts/docker-tar1090