feat: Route53 record mutations and change status polling#70
Conversation
- Add create, update, delete operations for A/CNAME DNS records - Type-to-confirm for record deletion (matches RDS/SG pattern) - Multi-step create form: Name → Type (A/CNAME) → Value → TTL - Edit form for value and TTL of existing A/CNAME records - Change status polling (PENDING → INSYNC) after mutations - Add ChangeResourceRecordSets and GetChange to Route53ClientAPI - Add 9 new tests for mutation operations and change status Closes #40
- Add /senior-review skill for codebase quality review with LoC breakdown, severity-rated findings, and refactoring priorities - Add Phase 0 auto-suggest to /implement-feature that cross-references PLAN.md and open GitHub issues to recommend next feature
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds Route53 DNS record mutation operations (create, edit, delete) with change status polling. The implementation is generally solid with comprehensive test coverage (9 new tests) and a well-structured TUI workflow.
Critical Finding
Security Vulnerability Found: The DeleteRecord function contains a critical bug in alias record deletion logic that uses the wrong HostedZoneId parameter. This will cause deletion failures and could potentially corrupt DNS records across zones. This must be fixed before merge.
Review Complete
1 blocking issue identified that requires immediate attention.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
internal/services/aws/route53.go
Outdated
| rrs.ResourceRecords = nil | ||
| rrs.AliasTarget = &r53types.AliasTarget{ | ||
| DNSName: awssdk.String(record.AliasTarget), | ||
| HostedZoneId: awssdk.String(zoneID), |
There was a problem hiding this comment.
🛑 Security Vulnerability: Alias record deletion uses incorrect HostedZoneId parameter causing potential cross-zone data corruption. The code sets HostedZoneId: awssdk.String(zoneID) for alias target, but this should be the target resource's hosted zone ID (from the original record), not the source zone ID. This will cause deletion failures for cross-zone aliases and could corrupt DNS records if AWS doesn't validate properly.
HostedZoneId: awssdk.String(record.AliasTarget),- DNSRecord now stores AliasHostedZoneId from the alias target - DeleteRecord uses record.AliasHostedZoneId instead of the source zone ID, preventing cross-zone data corruption - Add test verifying alias deletion uses target zone ID, not source
|
Yes, I'll perform another review of the pull request. Let me analyze the changes in this PR which adds Route53 record mutation operations (create, edit, delete) with change status polling, along with new skills for feature implementation. |
There was a problem hiding this comment.
This PR successfully implements Route53 DNS record mutation operations with comprehensive test coverage. The implementation follows the established patterns for RDS and Security Group mutations, providing a consistent user experience.
Key Strengths:
- Complete CRUD operations (Create, Update, Delete) for A and CNAME records
- Proper change status polling with PENDING → INSYNC state tracking
- Well-structured test coverage with 9 new tests covering mutations and change status
- Alias record support with correct hosted zone ID handling in delete operations
- Type-to-confirm deletion safety matching existing patterns
Critical Issue:
- The parseTTL function needs error handling to prevent silent failures on invalid input
The code is well-organized and production-ready once the TTL parsing issue is addressed.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| func parseTTL(s string) int64 { | ||
| var ttl int64 = 300 | ||
| if s != "" { | ||
| fmt.Sscanf(s, "%d", &ttl) | ||
| } | ||
| return ttl | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: The parseTTL function ignores Sscanf errors, which means invalid TTL inputs (non-numeric strings) silently fall back to the default value of 300. This could mask user input errors. Add error handling to properly validate TTL input before attempting API calls.
| func parseTTL(s string) int64 { | |
| var ttl int64 = 300 | |
| if s != "" { | |
| fmt.Sscanf(s, "%d", &ttl) | |
| } | |
| return ttl | |
| } | |
| func parseTTL(s string) int64 { | |
| var ttl int64 = 300 | |
| if s != "" { | |
| if _, err := fmt.Sscanf(s, "%d", &ttl); err != nil { | |
| // Invalid TTL format, use default | |
| ttl = 300 | |
| } | |
| } | |
| return ttl | |
| } |
- Check Sscanf return error and negative values - Use named defaultTTL constant for clarity - Return default (300) on any invalid input instead of silently ignoring
|
/q review |
There was a problem hiding this comment.
Code Review Summary
This PR successfully implements Route53 DNS record mutation operations (create, edit, delete) with change status polling, completing the M3.6 milestone. The implementation is well-structured, thoroughly tested, and follows AWS best practices.
Key Strengths
Service Layer (route53.go)
- Proper use of UPSERT action for create and update operations follows AWS best practices
- Correct handling of alias records in delete operations, properly setting alias target and zone ID
- Comprehensive error handling with wrapped errors for better debugging
- Clean separation of concerns with helper functions like
extractChangeInfoandcleanZoneID
TUI Layer (screen_route53.go)
- Multi-step form workflow provides good user experience
- Type-to-confirm deletion pattern matches existing RDS/SG patterns for consistency
- Change status polling with 2-second intervals provides real-time feedback
- Proper input validation and field prefilling
Testing (route53_test.go)
- 9 new comprehensive tests covering success, error, and edge cases
- Excellent test coverage including alias record deletion with correct hosted zone ID verification
- Mock client implementation allows isolated unit testing
The code is production-ready and meets all requirements for the Route53 mutation milestone.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
2 participants
Summary
Complete the Route53 M3.6 milestone by adding DNS record mutation operations:
c(create),e(edit),d(delete) on record detail/list screensAlso includes:
/senior-reviewskill for codebase quality review/implement-featurewhen invoked without argumentsRelated Issues
Closes #40
Changes
Service Layer
CreateRecord,UpdateRecord,DeleteRecordmethods usingChangeResourceRecordSetsAPIGetChangeStatusmethod usingGetChangeAPIChangeInfomodel structChangeResourceRecordSetsandGetChangeadded toRoute53ClientAPIinterfaceTUI Layer
screenRoute53RecordCreate,screenRoute53RecordEdit,screenRoute53RecordDeleteConfirmSkills
/senior-review— LoC inventory, 5-category code review, severity-rated findings/implement-featurePhase 0 — auto-suggest next feature from PLAN.md + open issuesValidation
make test— all tests pass (9 new tests for mutations + change status)make build— compiles cleanly