gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 17:11:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: harden route table error handling (silent-failure-hunter review)

Browse Source 
- getRouteTable: distinguish ErrKeyNotFound (→ not found) from other
  errors (→ log + internal error) instead of mapping all to "not found"
- putRouteTable: log underlying error on marshal/write failures
- KV scan loops (allRouteTablesForVPC, DescribeRouteTables,
  DisassociateRouteTable, ReplaceRouteTableAssociation): fail on
  transient NATS errors instead of silently skipping entries. Only
  skip ErrKeyNotFound (deleted between Keys() and Get())
- ReplaceRouteTableAssociation: compensating write if second KV put
  fails — restores association to old table to prevent data loss
- createMainRouteTable: returns error instead of swallowing it
- ensureDefaultIGWRoute: log actual errors at Warn level, not Debug
- E2E test: fail on missing default VPC instead of skipping
This commit is contained in:
Ben Duncan
2026-03-26 13:32:05 +10:00
parent 1c194da915
commit d025cab50e
4 changed files with 99 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/daemon/daemon_handlers_vpc.go
+6 -1
View File
@@ -214,7 +214,11 @@ func (d *Daemon) ensureDefaultIGWRoute(accountID, vpcID string) {
{Name: &mainFilter, Values: []*string{&trueVal}},
},
}, accountID)
if err != nil || len(descOut.RouteTables) == 0 {
if err != nil {
slog.Warn("Failed to query main route table for default IGW route", "vpcId", vpcID, "err", err)
return
}
if len(descOut.RouteTables) == 0 {
slog.Debug("No main route table found for default IGW route", "vpcId", vpcID, "accountID", accountID)
return
}
@@ -231,6 +235,7 @@ func (d *Daemon) ensureDefaultIGWRoute(accountID, vpcID string) {
// Find the attached IGW for this VPC
igwOut, err := d.igwService.DescribeInternetGateways(&ec2.DescribeInternetGatewaysInput{}, accountID)
if err != nil {
slog.Warn("Failed to query IGWs for default route", "vpcId", vpcID, "err", err)
return
}
var igwID string
spinifex/handlers/ec2/routetable/service_impl.go
+53 -11
View File
@@ -78,10 +78,15 @@ func NewRouteTableServiceImplWithNATS(cfg *config.Config, natsConn *nats.Conn) (
func (s *RouteTableServiceImpl) getRouteTable(accountID, rtbID string) (*RouteTableRecord, error) {
entry, err := s.rtbKV.Get(utils.AccountKey(accountID, rtbID))
if err != nil {
return nil, errors.New(awserrors.ErrorInvalidRouteTableIDNotFound)
if errors.Is(err, nats.ErrKeyNotFound) {
return nil, errors.New(awserrors.ErrorInvalidRouteTableIDNotFound)
}
slog.Error("Failed to read route table from KV", "routeTableId", rtbID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
var record RouteTableRecord
if err := json.Unmarshal(entry.Value(), &record); err != nil {
slog.Error("Corrupt route table record in KV", "routeTableId", rtbID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
return &record, nil
@@ -91,9 +96,11 @@ func (s *RouteTableServiceImpl) getRouteTable(accountID, rtbID string) (*RouteTa
func (s *RouteTableServiceImpl) putRouteTable(accountID string, record *RouteTableRecord) error {
data, err := json.Marshal(record)
if err != nil {
slog.Error("Failed to marshal route table record", "routeTableId", record.RouteTableId, "err", err)
return errors.New(awserrors.ErrorServerInternal)
}
if _, err := s.rtbKV.Put(utils.AccountKey(accountID, record.RouteTableId), data); err != nil {
slog.Error("Failed to write route table to KV", "routeTableId", record.RouteTableId, "err", err)
return errors.New(awserrors.ErrorServerInternal)
}
return nil
@@ -130,11 +137,16 @@ func (s *RouteTableServiceImpl) allRouteTablesForVPC(accountID, vpcID string) ([
}
entry, err := s.rtbKV.Get(key)
if err != nil {
continue
if errors.Is(err, nats.ErrKeyNotFound) {
continue // deleted between Keys() and Get()
}
slog.Error("Failed to read route table during VPC scan", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
var record RouteTableRecord
if err := json.Unmarshal(entry.Value(), &record); err != nil {
continue
slog.Error("Corrupt route table record", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
if record.VpcId == vpcID {
results = append(results, record)
@@ -274,12 +286,17 @@ func (s *RouteTableServiceImpl) DescribeRouteTables(input *ec2.DescribeRouteTabl
entry, err := s.rtbKV.Get(key)
if err != nil {
continue
if errors.Is(err, nats.ErrKeyNotFound) {
continue
}
slog.Error("Failed to read route table during describe", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
var record RouteTableRecord
if err := json.Unmarshal(entry.Value(), &record); err != nil {
continue
slog.Error("Corrupt route table record", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
if len(rtbIDs) > 0 && !rtbIDs[record.RouteTableId] {
@@ -554,7 +571,11 @@ func (s *RouteTableServiceImpl) DisassociateRouteTable(input *ec2.DisassociateRo
prefix := accountID + "."
keys, err := s.rtbKV.Keys()
if err != nil {
return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound)
if errors.Is(err, nats.ErrNoKeysFound) {
return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound)
}
slog.Error("Failed to list route table keys", "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
for _, key := range keys {
@@ -564,12 +585,17 @@ func (s *RouteTableServiceImpl) DisassociateRouteTable(input *ec2.DisassociateRo
entry, err := s.rtbKV.Get(key)
if err != nil {
continue
if errors.Is(err, nats.ErrKeyNotFound) {
continue
}
slog.Error("Failed to read route table during disassociate scan", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
var record RouteTableRecord
if err := json.Unmarshal(entry.Value(), &record); err != nil {
continue
slog.Error("Corrupt route table record", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
for i, assoc := range record.Associations {
@@ -613,7 +639,11 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(input *ec2.ReplaceR
prefix := accountID + "."
keys, err := s.rtbKV.Keys()
if err != nil {
return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound)
if errors.Is(err, nats.ErrNoKeysFound) {
return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound)
}
slog.Error("Failed to list route table keys", "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
for _, key := range keys {
@@ -623,12 +653,17 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(input *ec2.ReplaceR
entry, err := s.rtbKV.Get(key)
if err != nil {
continue
if errors.Is(err, nats.ErrKeyNotFound) {
continue
}
slog.Error("Failed to read route table during replace scan", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
var oldRecord RouteTableRecord
if err := json.Unmarshal(entry.Value(), &oldRecord); err != nil {
continue
slog.Error("Corrupt route table record", "key", key, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
for i, assoc := range oldRecord.Associations {
@@ -657,6 +692,13 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(input *ec2.ReplaceR
Main: assoc.Main,
})
if err := s.putRouteTable(accountID, newRecord); err != nil {
// Compensate: restore association to old table to avoid data loss
oldRecord.Associations = append(oldRecord.Associations, assoc)
if restoreErr := s.putRouteTable(accountID, &oldRecord); restoreErr != nil {
slog.Error("CRITICAL: ReplaceRouteTableAssociation partial failure, association lost",
"associationId", assocID, "oldRouteTableId", oldRecord.RouteTableId,
"newRouteTableId", newRtbID, "restoreErr", restoreErr, "originalErr", err)
}
return nil, err
}
spinifex/handlers/ec2/vpc/service_impl.go
+10 -7
View File
@@ -248,7 +248,9 @@ func (s *VPCServiceImpl) CreateVpc(input *ec2.CreateVpcInput, accountID string)
// Auto-create main route table with local route (matches AWS behavior)
if s.rtbKV != nil {
s.createMainRouteTable(accountID, vpcID, record.CidrBlock)
if err := s.createMainRouteTable(accountID, vpcID, record.CidrBlock); err != nil {
slog.Error("Failed to create main route table for VPC", "vpcId", vpcID, "err", err)
}
}
return &ec2.CreateVpcOutput{
@@ -933,7 +935,9 @@ func (s *VPCServiceImpl) EnsureDefaultVPC(accountID string, bootstrap ...Bootstr
// Create main route table with local route (written directly to KV to avoid circular import)
if s.rtbKV != nil {
s.createMainRouteTable(accountID, vpcID, DefaultVPCCidr)
if err := s.createMainRouteTable(accountID, vpcID, DefaultVPCCidr); err != nil {
slog.Error("Failed to create main route table for VPC", "vpcId", vpcID, "err", err)
}
}
slog.Info("Created default VPC and subnet",
@@ -954,7 +958,7 @@ func (s *VPCServiceImpl) EnsureDefaultVPC(accountID string, bootstrap ...Bootstr
// createMainRouteTable writes a main route table record directly to the route table KV bucket.
// This avoids a circular import (routetable package imports vpc package for validation).
func (s *VPCServiceImpl) createMainRouteTable(accountID, vpcID, vpcCidr string) {
func (s *VPCServiceImpl) createMainRouteTable(accountID, vpcID, vpcCidr string) error {
type rtbRecord struct {
RouteTableId string `json:"route_table_id"`
VpcId string `json:"vpc_id"`
@@ -1000,15 +1004,14 @@ func (s *VPCServiceImpl) createMainRouteTable(accountID, vpcID, vpcCidr string)
data, err := json.Marshal(record)
if err != nil {
slog.Error("Failed to marshal main route table", "vpcId", vpcID, "err", err)
return
return fmt.Errorf("marshal main route table: %w", err)
}
if _, err := s.rtbKV.Put(utils.AccountKey(accountID, rtbID), data); err != nil {
slog.Error("Failed to store main route table", "vpcId", vpcID, "err", err)
return
return fmt.Errorf("store main route table: %w", err)
}
slog.Info("Created main route table for VPC", "routeTableId", rtbID, "vpcId", vpcID, "accountID", accountID)
return nil
}
// GetDefaultSubnet returns the default subnet for RunInstances when no SubnetId is specified.
tests/e2e/run-e2e.sh
+30 -29
View File
@@ -2604,37 +2604,38 @@ echo "========================================"
echo "Step 1: Verify default VPC main route table"
RTB_DEFAULT_VPC=$(aws ec2 describe-vpcs --query 'Vpcs[?IsDefault==`true`].VpcId | [0]' --output text)
if [ -z "$RTB_DEFAULT_VPC" ] || [ "$RTB_DEFAULT_VPC" = "None" ]; then
echo "SKIP: No default VPC found"
else
MAIN_RTB=$(aws ec2 describe-route-tables \
--filters "Name=vpc-id,Values=$RTB_DEFAULT_VPC" "Name=association.main,Values=true" \
--query 'RouteTables[0].RouteTableId' --output text)
if [ -z "$MAIN_RTB" ] || [ "$MAIN_RTB" = "None" ]; then
echo "FAIL: No main route table found for default VPC $RTB_DEFAULT_VPC"
exit 1
fi
echo " Main route table: $MAIN_RTB"
# Verify local route exists
LOCAL_ROUTE=$(aws ec2 describe-route-tables --route-table-ids "$MAIN_RTB" \
--query 'RouteTables[0].Routes[?GatewayId==`local`].DestinationCidrBlock | [0]' --output text)
if [ -z "$LOCAL_ROUTE" ] || [ "$LOCAL_ROUTE" = "None" ]; then
echo "FAIL: Main route table missing local route"
exit 1
fi
echo " Local route: $LOCAL_ROUTE → local"
# Verify IGW route exists
IGW_ROUTE=$(aws ec2 describe-route-tables --route-table-ids "$MAIN_RTB" \
--query 'RouteTables[0].Routes[?DestinationCidrBlock==`0.0.0.0/0`].GatewayId | [0]' --output text)
if [ -z "$IGW_ROUTE" ] || [ "$IGW_ROUTE" = "None" ]; then
echo "FAIL: Main route table missing 0.0.0.0/0 → IGW route"
exit 1
fi
echo " Default route: 0.0.0.0/0 → $IGW_ROUTE"
echo " PASS: Default VPC main route table verified"
echo "FAIL: No default VPC found — route table tests require a default VPC"
exit 1
fi
MAIN_RTB=$(aws ec2 describe-route-tables \
--filters "Name=vpc-id,Values=$RTB_DEFAULT_VPC" "Name=association.main,Values=true" \
--query 'RouteTables[0].RouteTableId' --output text)
if [ -z "$MAIN_RTB" ] || [ "$MAIN_RTB" = "None" ]; then
echo "FAIL: No main route table found for default VPC $RTB_DEFAULT_VPC"
exit 1
fi
echo " Main route table: $MAIN_RTB"
# Verify local route exists
LOCAL_ROUTE=$(aws ec2 describe-route-tables --route-table-ids "$MAIN_RTB" \
--query 'RouteTables[0].Routes[?GatewayId==`local`].DestinationCidrBlock | [0]' --output text)
if [ -z "$LOCAL_ROUTE" ] || [ "$LOCAL_ROUTE" = "None" ]; then
echo "FAIL: Main route table missing local route"
exit 1
fi
echo " Local route: $LOCAL_ROUTE → local"
# Verify IGW route exists
IGW_ROUTE=$(aws ec2 describe-route-tables --route-table-ids "$MAIN_RTB" \
--query 'RouteTables[0].Routes[?DestinationCidrBlock==`0.0.0.0/0`].GatewayId | [0]' --output text)
if [ -z "$IGW_ROUTE" ] || [ "$IGW_ROUTE" = "None" ]; then
echo "FAIL: Main route table missing 0.0.0.0/0 → IGW route"
exit 1
fi
echo " Default route: 0.0.0.0/0 → $IGW_ROUTE"
echo " PASS: Default VPC main route table verified"
# Step 2: Custom route table lifecycle
echo ""
echo "Step 2: Custom route table CRUD lifecycle"