Skip to content

fix: Implement UnixTime support for ECS timestamp fields - #767

Merged
stormcat24 merged 1 commit into
mainfrom
fix/ecs-unix-timestamp
Oct 16, 2025
Merged

fix: Implement UnixTime support for ECS timestamp fields#767
stormcat24 merged 1 commit into
mainfrom
fix/ecs-unix-timestamp

Conversation

@stormcat24

Copy link
Copy Markdown
Member

Summary

This PR fixes ECS timestamp field handling to resolve Terraform AWS provider deserialization errors. The issue was that KECS was returning RFC3339 format strings instead of Unix timestamps (JSON numbers) for timestamp fields, causing Terraform to reject the responses.

Changes

  • Code Generation: Update codegen to detect timestamp fields by "Date", "Time", and "At" suffixes, enabling proper common.UnixTime type generation for ECS service
  • Type Generation: Convert 35+ ECS timestamp fields from *time.Time to *common.UnixTime
  • API Handlers: Add UnixTime helper to ptr package and update all API handlers to use ptr.UnixTime() for timestamp responses
  • Code Regeneration: Regenerate ECS API code with corrected timestamp handling
  • Test Fixes: Fix enum test to use correct ClusterSettingName value and fix ListAttributes test to use correct TargetType enum value

Error Fixed

  • Terraform deserialization error: "expected Timestamp to be a JSON Number, got string instead"

Test Plan

  • ✅ All unit tests pass
  • ✅ All integration tests pass
  • ✅ Full test suite with coverage analysis passes
  • ✅ Enum values are correctly generated with proper case
  • ✅ Attribute filtering works correctly with proper enum values

- Update codegen to detect timestamp fields by Date, Time, and At suffix patterns
- Convert 35+ ECS timestamp fields from time.Time to common.UnixTime
- Add UnixTime helper to ptr package for timestamp conversion
- Update API handlers to use ptr.UnixTime() for all timestamp responses
- Regenerate ECS API code with corrected timestamp handling
- Fix enum test to use correct ClusterSettingName value (containerInsights)
- Fix ListAttributes test to use correct TargetType enum value (container-instance)

Fixes Terraform deserialization error: expected Timestamp to be a JSON Number

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Greptile Review Summary- Critical Issues: 0- Logic Issues: 0- Style Suggestions: 0✅ **No blocking issues found

See Greptile Workflow Guide for handling reviews.

@github-actions

Copy link
Copy Markdown

Code Metrics Report

#766 (bb2f287) #767 (a2793cd) +/-
Coverage 12.8% 12.8% +0.0%
Code to Test Ratio 1:0.2 1:0.2 -0.1
Test Execution Time 3m9s 3m23s +14s
Details
  |                     | #766 (bb2f287) | #767 (a2793cd) |  +/-  |
  |---------------------|----------------|----------------|-------|
+ | Coverage            |          12.8% |          12.8% | +0.0% |
  |   Files             |            269 |            269 |     0 |
  |   Lines             |          42061 |          42056 |    -5 |
  |   Covered           |           5397 |           5397 |     0 |
- | Code to Test Ratio  |          1:0.2 |          1:0.2 |  -0.1 |
  |   Code              |          71287 |          71292 |    +5 |
  |   Test              |          16352 |          16352 |     0 |
- | Test Execution Time |           3m9s |          3m23s |  +14s |

Code coverage of files in pull request scope (24.8% → 24.8%)

Files Coverage +/- Status
controlplane/cmd/codegen/generator/types.go 0.0% 0.0% modified
controlplane/internal/controllers/sync/mappers/service_mapper.go 0.0% 0.0% modified
controlplane/internal/controlplane/api/container_instance_ecs_api.go 55.1% 0.0% modified
controlplane/internal/controlplane/api/generated/ptr/ptr.go 64.1% -1.3% modified
controlplane/internal/controlplane/api/generated/routing.go 0.0% 0.0% modified
controlplane/internal/controlplane/api/generated/types.go 0.0% 0.0% modified
controlplane/internal/controlplane/api/service_ecs_api.go 20.0% 0.0% modified
controlplane/internal/controlplane/api/task_definition_ecs_api.go 56.8% 0.0% modified
controlplane/internal/controlplane/api/task_ecs_api.go 49.5% 0.0% modified
controlplane/internal/controlplane/api/task_set_ecs_api.go 66.3% 0.0% modified

Reported by octocov

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Greptile Overview

Summary

This PR successfully fixes Terraform AWS provider deserialization errors by implementing proper Unix timestamp handling for ECS API responses. The root cause was that KECS was returning RFC3339 formatted strings for timestamp fields, but Terraform expects JSON numbers (Unix timestamps).

Key Changes:

  • Code Generation Enhancement: Updated timestamp field detection in types.go to identify fields containing "date", "time", or "at" (case-insensitive) and generate them as *common.UnixTime instead of *time.Time
  • Helper Function: Added ptr.UnixTime() helper that converts time.Time to *common.UnixTime, ensuring consistent usage across handlers
  • Type Regeneration: Converted 35+ timestamp fields across Service, Task, ContainerInstance, TaskSet, and Deployment types
  • Handler Updates: Updated all API handlers (service, task, container instance, task definition, task set) to use ptr.UnixTime() instead of ptr.Time()
  • Test Fixes: Corrected enum value assertions to use proper AWS ECS format (e.g., containerInsights instead of CONTAINER_INSIGHTS, container-instance instead of CONTAINER_INSTANCE)

Implementation Quality:
The common.UnixTime type is well-designed with bidirectional JSON marshaling support (unmarshals from both Unix timestamps and RFC3339 strings, marshals to Unix timestamps), comprehensive test coverage, and proper null handling. The codegen changes are scoped to ECS, SSM, and Secrets Manager services where this behavior is needed.

Impact:
This resolves the Terraform integration issue and ensures KECS API responses conform to AWS ECS API specifications for timestamp formatting.

Confidence Score: 5/5

  • This PR is safe to merge with high confidence - it fixes a critical Terraform integration bug with a well-tested, systematic approach
  • Score reflects comprehensive implementation with proper codegen changes, thorough test coverage for UnixTime type, systematic updates across all API handlers, correct enum fixes, and successful test suite execution. The changes are focused, well-documented, and follow existing patterns in the codebase.
  • No files require special attention - all changes are consistent and follow the same pattern throughout

Important Files Changed

File Analysis

Filename Score Overview
controlplane/cmd/codegen/generator/types.go 5/5 Updated timestamp field detection logic to include ECS service and detect fields with 'date', 'time', or 'at' suffixes (case-insensitive), correctly generating common.UnixTime types
controlplane/internal/controlplane/api/generated/ptr/ptr.go 5/5 Added UnixTime() helper function to convert time.Time to *common.UnixTime, enabling consistent timestamp handling in API responses
controlplane/internal/controlplane/api/generated/types.go 5/5 Regenerated types with 35+ timestamp fields converted from *time.Time to *common.UnixTime, ensuring JSON serialization as Unix timestamps instead of RFC3339 strings
controlplane/internal/controlplane/api/service_ecs_api.go 5/5 Updated 13 service-related timestamp fields to use ptr.UnixTime() instead of ptr.Time() for correct JSON serialization
controlplane/internal/controlplane/api/task_ecs_api.go 5/5 Updated 9 task-related timestamp fields to use ptr.UnixTime() for lifecycle timestamps (CreatedAt, StartedAt, StoppedAt, etc.)

Sequence Diagram

sequenceDiagram
    participant TF as Terraform AWS Provider
    participant KECS as KECS API Server
    participant Codegen as Code Generator
    participant Handler as API Handler
    participant Storage as Storage Layer
    
    Note over Codegen: Code Generation Phase
    Codegen->>Codegen: Detect timestamp fields<br/>(contains "date", "time", "at")
    Codegen->>Codegen: Generate types with<br/>*common.UnixTime
    
    Note over TF,Storage: Runtime - API Request Flow
    TF->>KECS: CreateService/DescribeService<br/>(expects Unix timestamps)
    KECS->>Handler: Process request
    Handler->>Storage: Fetch/Store service data<br/>(time.Time internally)
    Storage-->>Handler: Return data
    Handler->>Handler: Convert to generated types<br/>using ptr.UnixTime(timeValue)
    Handler->>Handler: JSON Marshal<br/>(UnixTime.MarshalJSON)
    Note over Handler: Converts time.Time to<br/>Unix timestamp (float64)
    Handler-->>KECS: Response with Unix timestamps
    KECS-->>TF: JSON: {"createdAt": 1755005925.233}
    TF->>TF: Deserialize as JSON Number ✓
    
    Note over TF,Storage: Before This Fix
    Handler->>Handler: Used ptr.Time()<br/>(*time.Time type)
    Handler->>Handler: JSON Marshal<br/>(time.Time default)
    Note over Handler: Produces RFC3339 string
    Handler-->>TF: JSON: {"createdAt": "2025-01-10T15:30:00Z"}
    TF->>TF: ERROR: Expected Number,<br/>got String ✗
Loading

13 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@stormcat24
stormcat24 merged commit edda8c9 into main Oct 16, 2025
7 checks passed
@stormcat24
stormcat24 deleted the fix/ecs-unix-timestamp branch October 16, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant