From a2ede7c47670fd62118cab020cd81d53cfb37a73 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Fri, 12 Jun 2026 13:54:11 +1000 Subject: [PATCH] EKS AWS-compat + nodegroup lifecycle hardening (env19) (#353) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(eks): surface CREATING block reason at Info, not Debug A cluster stuck in CREATING logged its per-tick bootstrap/health block reason at Debug, invisible at the default level, so a create that ran to the 15m timeout FAILED gave no diagnosable cause. Bump both reasons to Info; they fire only during the CREATING window. * feat(iam): round-trip AWS-managed policy ARNs for stock EKS tooling Stock terraform-aws-eks / eksctl attach AWS-managed policies (AmazonEKSClusterPolicy, AmazonEKSWorkerNodePolicy, AmazonEKS_CNI_Policy, ...) that Spinifex does not provision. AttachRolePolicy rejected them NoSuchEntity and ListAttachedRolePolicies dropped them, breaking the node/cluster role round-trip DescribeNodegroup relies on. Accept and store managed ARNs opaquely (no backing document), deriving the policy name from the ARN on read. * feat(eks): echo cluster tags + store-only Tag/Untag/ListTags DescribeCluster never returned create-time tags and ClusterMeta did not store them, so a stock terraform-aws provider reconciling default_tags saw perpetual drift and issued TagResource on every apply — which itself returned NotImplemented. Store tags on the cluster record, echo them from DescribeCluster, and back the three tag ops store-only against the record (cluster ARNs only) so the default_tags round-trip is clean. * fix(eks): persist nodegroup workers incrementally + boot reclaim sweep launchWorkers only wrote the worker IDs to the record after the whole launch loop, so a daemon crash mid-loop stranded already-launched workers under a record that still showed empty InstanceIDs — nothing could reclaim them. Persist each worker before launching the next, and add a boot-time reclaim sweep (SpawnRegisteredReconcilers) that terminates recorded workers for any nodegroup left CREATING by a crashed launcher, or CREATE_FAILED still holding workers, settling it to CREATE_FAILED with InstanceIDs cleared. * fix: plumb query params into EKS gateway dispatcher EKS_Request never passed the query string to per-action handlers, so UntagResource (DELETE /tags/{arn}?tagKeys=k1&tagKeys=k2) saw an empty TagKeys and silently no-op'd. Fold url.Values into the JSON body when the body is empty so the handler unmarshal sees the keys. * feat(iam): resolve AWS-managed EKS policy grants for authorization Assumed-role/user authorization (checkPolicy -> Get{Role,User}Policies -> EvaluateAccess) could not honour AWS-managed policy grants: Spinifex stores no document for managed ARNs, so stock EKS roles whose permissions come entirely from managed policies (node role: AmazonEKSWorkerNodePolicy + AmazonEC2ContainerRegistryReadOnly + AmazonEKS_CNI_Policy) evaluated to an empty grant set and were denied every call. Add a builtin registry of well-known AWS-managed policy documents, resolved via a shared resolveAttachedPolicy helper used by both GetRolePolicies and GetUserPolicies. Unmodeled managed ARNs resolve to no grant (fail-closed deny) rather than erroring the whole request; customer-managed ARNs still fail closed when unresolvable. --- spinifex/gateway/eks.go | 15 ++ spinifex/handlers/eks/cluster_reconciler.go | 8 +- spinifex/handlers/eks/cluster_state.go | 5 + spinifex/handlers/eks/nodegroup.go | 76 ++++++- spinifex/handlers/eks/nodegroup_test.go | 45 ++++ spinifex/handlers/eks/service_impl.go | 118 +++++++++- spinifex/handlers/eks/service_impl_test.go | 52 ++++- spinifex/handlers/iam/managed_policies.go | 237 ++++++++++++++++++++ spinifex/handlers/iam/roles.go | 47 +++- spinifex/handlers/iam/roles_test.go | 92 ++++++++ spinifex/handlers/iam/service_impl.go | 11 +- 11 files changed, 670 insertions(+), 36 deletions(-) create mode 100644 spinifex/handlers/iam/managed_policies.go diff --git a/spinifex/gateway/eks.go b/spinifex/gateway/eks.go index e76415b63..98493bea4 100644 --- a/spinifex/gateway/eks.go +++ b/spinifex/gateway/eks.go @@ -1,6 +1,7 @@ package gateway import ( + "encoding/json" "errors" "io" "log/slog" @@ -256,6 +257,20 @@ func (gw *GatewayConfig) EKS_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorInvalidParameterValue) } + // Some REST-JSON actions carry their non-path inputs as query params with + // an empty body (e.g. UntagResource's tagKeys arrive as + // DELETE /tags/{arn}?tagKeys=k1&tagKeys=k2). url.Values is map[string][]string, + // so it marshals straight to the JSON the per-action unmarshal expects + // ({"tagKeys":["k1","k2"]}). Only folds when the body is empty so it never + // shadows a real payload. + if len(body) == 0 { + if q := r.URL.Query(); len(q) > 0 { + if qb, err := json.Marshal(map[string][]string(q)); err == nil { + body = qb + } + } + } + // Best-effort caller ARN; only CreateCluster consumes it, degrades to "". callerARN := eksCallerPrincipalARN(r) diff --git a/spinifex/handlers/eks/cluster_reconciler.go b/spinifex/handlers/eks/cluster_reconciler.go index f49fbc3c9..0590cf069 100644 --- a/spinifex/handlers/eks/cluster_reconciler.go +++ b/spinifex/handlers/eks/cluster_reconciler.go @@ -268,13 +268,17 @@ func (r *ClusterReconciler) reconcileOnce(ctx context.Context) error { case ClusterStatusCreating: ready, reason := r.bootstrapReady(meta) if !ready { - slog.Debug("ClusterReconciler: bootstrap not ready", + // Info, not Debug: this is the per-tick reason a CREATING cluster has + // not yet flipped ACTIVE. At Debug it is invisible at the default + // level, so a cluster that stalls to the create-timeout FAILED gives + // no diagnosable cause. Logged only during the CREATING window. + slog.Info("ClusterReconciler: bootstrap not ready", "cluster", r.clusterName, "reason", reason) return r.failIfCreateTimedOut(meta, "bootstrap not ready: "+reason) } issue, nodeCount := r.observe(ctx) if issue != "" { - slog.Debug("ClusterReconciler: health not ready", + slog.Info("ClusterReconciler: health not ready", "cluster", r.clusterName, "issue", issue) return r.failIfCreateTimedOut(meta, "healthz not ready: "+issue) } diff --git a/spinifex/handlers/eks/cluster_state.go b/spinifex/handlers/eks/cluster_state.go index 8322cd8c1..72a7639b1 100644 --- a/spinifex/handlers/eks/cluster_state.go +++ b/spinifex/handlers/eks/cluster_state.go @@ -75,6 +75,11 @@ type ClusterMeta struct { // BuiltinIngress is true when the cluster opted into K3s' bundled traefik + servicelb. // Default false = AWS parity (ingress via the AWS Load Balancer Controller). BuiltinIngress bool `json:"builtinIngress,omitempty"` + // Tags are the create-time resource tags, stored verbatim so DescribeCluster + // echoes them back. Without the round-trip a stock terraform-aws provider + // reconciling default_tags sees perpetual drift and issues TagResource on + // every apply. Store-only; no enforcement. + Tags map[string]string `json:"tags,omitempty"` } // ControlPlaneNode identifies one placed CP server VM. NodeID is the Spinifex diff --git a/spinifex/handlers/eks/nodegroup.go b/spinifex/handlers/eks/nodegroup.go index 89201b3cc..95d36c44a 100644 --- a/spinifex/handlers/eks/nodegroup.go +++ b/spinifex/handlers/eks/nodegroup.go @@ -302,10 +302,9 @@ func (s *EKSServiceImpl) launchNodegroupInfra(lc nodegroupLaunchCtx) { return } - instanceIDs, err := s.launchWorkers(accountID, rec, meta, ngSGID, amiID, token, lc.desired) - rec.InstanceIDs = instanceIDs - if err != nil { - // Persist whatever launched so DeleteNodegroup can reclaim it, then fail. + if _, err := s.launchWorkers(acctKV, accountID, rec, meta, ngSGID, amiID, token, lc.desired); err != nil { + // launchWorkers persisted each worker it launched (incrementally), so the + // reclaim path can already tear them down; just record the terminal failure. rec.Status = eks.NodegroupStatusCreateFailed rec.StatusReason = "launch workers: " + err.Error() rec.ModifiedAt = time.Now().UTC() @@ -327,13 +326,17 @@ func (s *EKSServiceImpl) launchNodegroupInfra(lc nodegroupLaunchCtx) { // so each gets a distinct node name + node label in its user-data) and returns // the launched instance IDs. On a partial failure it returns the IDs that did // launch plus the error so the caller can persist them for teardown. -func (s *EKSServiceImpl) launchWorkers(accountID string, rec *NodegroupRecord, meta *ClusterMeta, ngSGID, amiID, token string, count int) ([]string, error) { +func (s *EKSServiceImpl) launchWorkers(acctKV nats.KeyValue, accountID string, rec *NodegroupRecord, meta *ClusterMeta, ngSGID, amiID, token string, count int) ([]string, error) { instanceType := defaultNodegroupInstanceType if len(rec.InstanceTypes) > 0 { instanceType = rec.InstanceTypes[0] } subnet := rec.Subnets[0] + // base is the worker set already on the record (non-empty on a scale-up). + // Each incremental persist below writes base+newly-launched so the durable + // record always reflects every live worker, never just this call's additions. + base := append([]string(nil), rec.InstanceIDs...) ids := make([]string, 0, count) for i := range count { shortID := uuid.NewString()[:8] @@ -375,6 +378,15 @@ func (s *EKSServiceImpl) launchWorkers(accountID string, rec *NodegroupRecord, m ids = append(ids, id) } } + // Persist the launched worker IDs before issuing the next RunInstances so + // a crash mid-loop leaves every live worker recorded and reclaimable (by + // DeleteNodegroup or the boot reclaim sweep). Without this the record keeps + // its claim-time InstanceIDs until the loop's terminal write, which strands + // the already-launched workers if the daemon dies in between. + rec.InstanceIDs = append(append([]string(nil), base...), ids...) + if perr := PutNodegroupRecord(acctKV, rec); perr != nil { + return ids, fmt.Errorf("persist launched workers %d/%d: %w", i+1, count, perr) + } } return ids, nil } @@ -503,9 +515,8 @@ func (s *EKSServiceImpl) reconcileWorkerCount(acctKV nats.KeyValue, accountID st if err != nil { return fmt.Errorf("ensure cluster SGs: %w", err) } - newIDs, err := s.launchWorkers(accountID, rec, meta, ngSGID, amiID, token, desired-current) - rec.InstanceIDs = append(rec.InstanceIDs, newIDs...) - if err != nil { + // launchWorkers appends to and persists rec.InstanceIDs incrementally. + if _, err := s.launchWorkers(acctKV, accountID, rec, meta, ngSGID, amiID, token, desired-current); err != nil { return fmt.Errorf("launch workers: %w", err) } case desired < current: @@ -579,6 +590,55 @@ func (s *EKSServiceImpl) decryptNodeToken(kv nats.KeyValue, cluster string) (str return token, nil } +// reclaimOrphanedNodegroups terminates workers stranded by a daemon restart +// that interrupted createNodegroup. It runs once on boot +// (SpawnRegisteredReconcilers), before any launch goroutine from this process +// exists, so a record still in CREATING is definitionally orphaned: its launcher +// died with the prior process and nothing will ever drive it to a terminal +// state. Such records — and any CREATE_FAILED record still holding worker IDs — +// have their recorded workers terminated and the record settled to CREATE_FAILED +// with InstanceIDs cleared, which is idempotent on the next boot. +func (s *EKSServiceImpl) reclaimOrphanedNodegroups(accountID string, acctKV nats.KeyValue, cluster string) { + recs, err := ListNodegroupRecords(acctKV, cluster) + if err != nil { + slog.Warn("reclaimOrphanedNodegroups: list records failed", "cluster", cluster, "err", err) + return + } + for _, rec := range recs { + stuckCreating := rec.Status == eks.NodegroupStatusCreating + failedWithWorkers := rec.Status == eks.NodegroupStatusCreateFailed && len(rec.InstanceIDs) > 0 + if !stuckCreating && !failedWithWorkers { + continue + } + if len(rec.InstanceIDs) > 0 { + if err := s.deps.Worker.TerminateWorkerInstances(rec.InstanceIDs, accountID); err != nil { + // Leave the record untouched so the next boot retries the reclaim + // rather than orphaning the workers by clearing their IDs. + slog.Error("reclaimOrphanedNodegroups: terminate workers failed", + "cluster", cluster, "nodegroup", rec.Name, "instances", rec.InstanceIDs, "err", err) + continue + } + } + reason := rec.StatusReason + if stuckCreating { + reason = "create interrupted by daemon restart; stranded workers reclaimed" + } + priorStatus := rec.Status + workerCount := len(rec.InstanceIDs) + rec.Status = eks.NodegroupStatusCreateFailed + rec.StatusReason = reason + rec.InstanceIDs = nil + rec.ModifiedAt = time.Now().UTC() + if err := PutNodegroupRecord(acctKV, rec); err != nil { + slog.Warn("reclaimOrphanedNodegroups: persist settled record failed", + "cluster", cluster, "nodegroup", rec.Name, "err", err) + continue + } + slog.Info("reclaimOrphanedNodegroups: reclaimed stranded nodegroup workers", + "cluster", cluster, "nodegroup", rec.Name, "priorStatus", priorStatus, "workers", workerCount) + } +} + func (s *EKSServiceImpl) markNodegroupFailed(kv nats.KeyValue, cluster, ng, reason string) { rec, err := GetNodegroupRecord(kv, cluster, ng) if err != nil { diff --git a/spinifex/handlers/eks/nodegroup_test.go b/spinifex/handlers/eks/nodegroup_test.go index 1e2648234..b2a72a08a 100644 --- a/spinifex/handlers/eks/nodegroup_test.go +++ b/spinifex/handlers/eks/nodegroup_test.go @@ -86,6 +86,51 @@ func createNGInput(cluster, ng string, desired int64) *eks.CreateNodegroupInput } } +func TestReclaimOrphanedNodegroups_TerminatesStrandedWorkers(t *testing.T) { + f := newEKSServiceFixture(t) + const cluster = "toc" + seedActiveClusterWithToken(t, f, cluster) + + // CREATING left behind by a crashed launcher, workers already recorded. + require.NoError(t, PutNodegroupRecord(f.kv, &NodegroupRecord{ + ClusterName: cluster, Name: "ng-stuck", + Status: eks.NodegroupStatusCreating, + InstanceIDs: []string{"i-worker1", "i-worker2"}, + })) + // CREATE_FAILED that still holds a partially-launched worker. + require.NoError(t, PutNodegroupRecord(f.kv, &NodegroupRecord{ + ClusterName: cluster, Name: "ng-failed", + Status: eks.NodegroupStatusCreateFailed, + InstanceIDs: []string{"i-worker3"}, + })) + // ACTIVE must be left untouched. + require.NoError(t, PutNodegroupRecord(f.kv, &NodegroupRecord{ + ClusterName: cluster, Name: "ng-active", + Status: eks.NodegroupStatusActive, + InstanceIDs: []string{"i-worker9"}, + })) + + f.svc.reclaimOrphanedNodegroups(testAccountID, f.kv, cluster) + + var terminated []string + for _, call := range f.worker.terminateCalls { + terminated = append(terminated, call...) + } + assert.ElementsMatch(t, []string{"i-worker1", "i-worker2", "i-worker3"}, terminated) + + for _, ng := range []string{"ng-stuck", "ng-failed"} { + got, err := GetNodegroupRecord(f.kv, cluster, ng) + require.NoError(t, err) + assert.Equal(t, eks.NodegroupStatusCreateFailed, got.Status, ng) + assert.Empty(t, got.InstanceIDs, ng) + } + + got, err := GetNodegroupRecord(f.kv, cluster, "ng-active") + require.NoError(t, err) + assert.Equal(t, eks.NodegroupStatusActive, got.Status) + assert.Equal(t, []string{"i-worker9"}, got.InstanceIDs) +} + func TestNodegroupRecord_CRUDRoundTrip(t *testing.T) { _, _, js := testutil.StartTestJetStream(t) kv, err := GetOrCreateAccountBucket(js, testAccountID) diff --git a/spinifex/handlers/eks/service_impl.go b/spinifex/handlers/eks/service_impl.go index b96e1c4d5..d1cba6e36 100644 --- a/spinifex/handlers/eks/service_impl.go +++ b/spinifex/handlers/eks/service_impl.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "maps" "net" "sort" "strconv" @@ -170,6 +171,11 @@ func (s *EKSServiceImpl) SpawnRegisteredReconcilers() error { if meta.Status != ClusterStatusCreating && meta.Status != ClusterStatusActive { continue } + // Reclaim any nodegroup workers stranded by the restart: a launch that + // was in flight when the prior process died left a CREATING (or + // partially-launched CREATE_FAILED) record whose workers nothing else + // will ever terminate. + s.reclaimOrphanedNodegroups(accountID, acctKV, cluster) // Re-subscribe to missing artifacts; K3s publishes each one-shot message once. if meta.Status == ClusterStatusCreating { if pending := BootstrapPendingKinds(acctKV, cluster, meta); len(pending) > 0 { @@ -310,6 +316,7 @@ func (s *EKSServiceImpl) CreateCluster(input *eks.CreateClusterInput, accountID, PublicAccessCidrs: publicCidrs, }, BuiltinIngress: builtinIngressEnabled(input.Tags), + Tags: aws.StringValueMap(input.Tags), CreatedAt: time.Now().UTC(), } // Claim the cluster name before any launching; duplicate/retry handlers lose the claim. @@ -1226,17 +1233,113 @@ func (s *EKSServiceImpl) DisassociateIdentityProviderConfig(_ *eks.DisassociateI } // --- Tags --- +// +// Store-only against the cluster meta record (no enforcement). Together with +// DescribeCluster echoing create-time tags this gives a stock terraform-aws +// provider a clean default_tags round-trip instead of perpetual drift. Only +// cluster ARNs are backed; other EKS resource ARNs return NotImplemented. -func (s *EKSServiceImpl) TagResource(_ *eks.TagResourceInput, _ string) (*eks.TagResourceOutput, error) { - return nil, notImpl() +func (s *EKSServiceImpl) TagResource(input *eks.TagResourceInput, accountID string) (*eks.TagResourceOutput, error) { + name, ok := clusterNameFromARN(aws.StringValue(input.ResourceArn)) + if !ok { + return nil, notImpl() + } + acctKV, err := s.accountBucket(accountID) + if err != nil { + return nil, err + } + add := aws.StringValueMap(input.Tags) + if err := casUpdateMeta(acctKV, name, func(m *ClusterMeta) bool { + if len(add) == 0 { + return false + } + if m.Tags == nil { + m.Tags = make(map[string]string, len(add)) + } + maps.Copy(m.Tags, add) + return true + }); err != nil { + return nil, eksTagErr(err) + } + return &eks.TagResourceOutput{}, nil } -func (s *EKSServiceImpl) UntagResource(_ *eks.UntagResourceInput, _ string) (*eks.UntagResourceOutput, error) { - return nil, notImpl() +func (s *EKSServiceImpl) UntagResource(input *eks.UntagResourceInput, accountID string) (*eks.UntagResourceOutput, error) { + name, ok := clusterNameFromARN(aws.StringValue(input.ResourceArn)) + if !ok { + return nil, notImpl() + } + acctKV, err := s.accountBucket(accountID) + if err != nil { + return nil, err + } + keys := aws.StringValueSlice(input.TagKeys) + if err := casUpdateMeta(acctKV, name, func(m *ClusterMeta) bool { + changed := false + for _, k := range keys { + if _, ok := m.Tags[k]; ok { + delete(m.Tags, k) + changed = true + } + } + return changed + }); err != nil { + return nil, eksTagErr(err) + } + return &eks.UntagResourceOutput{}, nil } -func (s *EKSServiceImpl) ListTagsForResource(_ *eks.ListTagsForResourceInput, _ string) (*eks.ListTagsForResourceOutput, error) { - return nil, notImpl() +func (s *EKSServiceImpl) ListTagsForResource(input *eks.ListTagsForResourceInput, accountID string) (*eks.ListTagsForResourceOutput, error) { + name, ok := clusterNameFromARN(aws.StringValue(input.ResourceArn)) + if !ok { + return nil, notImpl() + } + acctKV, err := s.accountBucket(accountID) + if err != nil { + return nil, err + } + meta, err := GetClusterMeta(acctKV, name) + if err != nil { + return nil, eksTagErr(err) + } + return &eks.ListTagsForResourceOutput{Tags: aws.StringMap(meta.Tags)}, nil +} + +// accountBucket returns the per-account KV bucket for accountID. +func (s *EKSServiceImpl) accountBucket(accountID string) (nats.KeyValue, error) { + js, err := s.deps.NATSConn.JetStream() + if err != nil { + return nil, fmt.Errorf("jetstream: %w", err) + } + return GetOrCreateAccountBucket(js, accountID) +} + +// clusterNameFromARN extracts the cluster name from an EKS cluster ARN +// (arn:aws:eks:::cluster/), reporting false for any other +// ARN shape (e.g. nodegroup) the tag store does not back. +func clusterNameFromARN(arn string) (string, bool) { + const prefix = "arn:aws:eks:" + if !strings.HasPrefix(arn, prefix) { + return "", false + } + parts := strings.SplitN(arn, ":", 6) + if len(parts) != 6 { + return "", false + } + resType, name, found := strings.Cut(parts[5], "/") + if !found || resType != "cluster" || name == "" { + return "", false + } + return name, true +} + +// eksTagErr maps a meta-store error to the AWS-visible tag-op error: a missing +// cluster surfaces as ResourceNotFound, everything else passes through. +func eksTagErr(err error) error { + if errors.Is(err, ErrClusterNotFound) { + return errors.New(awserrors.ErrorEKSResourceNotFound) + } + return err } // --- helpers --- @@ -1402,6 +1505,9 @@ func clusterMetaToAWS(meta *ClusterMeta) *eks.Cluster { }}, } } + if len(meta.Tags) > 0 { + out.Tags = aws.StringMap(meta.Tags) + } return out } diff --git a/spinifex/handlers/eks/service_impl_test.go b/spinifex/handlers/eks/service_impl_test.go index 963884c51..ec5e0400c 100644 --- a/spinifex/handlers/eks/service_impl_test.go +++ b/spinifex/handlers/eks/service_impl_test.go @@ -339,15 +339,59 @@ func TestEKSServiceImpl_OIDCMethodsReturnNotImplemented(t *testing.T) { require.EqualError(t, err, awserrors.ErrorNotImplemented) } -func TestEKSServiceImpl_TagsMethodsReturnNotImplemented(t *testing.T) { +func TestEKSServiceImpl_ClusterTagRoundTrip(t *testing.T) { svc := setupTestService(t) + js, err := svc.deps.NATSConn.JetStream() + require.NoError(t, err) + kv, err := GetOrCreateAccountBucket(js, testAccountID) + require.NoError(t, err) - _, err := svc.TagResource(&eks.TagResourceInput{ResourceArn: aws.String("arn:aws:eks:us-east-1:111122223333:cluster/c1")}, testAccountID) + const arn = "arn:aws:eks:us-east-1:111122223333:cluster/c1" + require.NoError(t, PutClusterMeta(kv, &ClusterMeta{ + Name: "c1", + Status: ClusterStatusActive, + Tags: map[string]string{"env": "prod"}, + })) + + // Create-time tags are echoed (the drift-killing round-trip). + lt, err := svc.ListTagsForResource(&eks.ListTagsForResourceInput{ResourceArn: aws.String(arn)}, testAccountID) + require.NoError(t, err) + require.Equal(t, "prod", aws.StringValue(lt.Tags["env"])) + + dc, err := svc.DescribeCluster(&eks.DescribeClusterInput{Name: aws.String("c1")}, testAccountID) + require.NoError(t, err) + require.Equal(t, "prod", aws.StringValue(dc.Cluster.Tags["env"])) + + // TagResource merges, UntagResource removes — both store-only. + _, err = svc.TagResource(&eks.TagResourceInput{ + ResourceArn: aws.String(arn), + Tags: aws.StringMap(map[string]string{"team": "platform"}), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.UntagResource(&eks.UntagResourceInput{ + ResourceArn: aws.String(arn), + TagKeys: aws.StringSlice([]string{"env"}), + }, testAccountID) + require.NoError(t, err) + + lt, err = svc.ListTagsForResource(&eks.ListTagsForResourceInput{ResourceArn: aws.String(arn)}, testAccountID) + require.NoError(t, err) + require.Equal(t, "platform", aws.StringValue(lt.Tags["team"])) + _, hasEnv := lt.Tags["env"] + require.False(t, hasEnv) +} + +func TestEKSServiceImpl_TagsNonClusterARNNotImplemented(t *testing.T) { + svc := setupTestService(t) + const ngARN = "arn:aws:eks:us-east-1:111122223333:nodegroup/c1/ng1/abc123" + + _, err := svc.TagResource(&eks.TagResourceInput{ResourceArn: aws.String(ngARN)}, testAccountID) require.EqualError(t, err, awserrors.ErrorNotImplemented) - _, err = svc.UntagResource(&eks.UntagResourceInput{ResourceArn: aws.String("arn:aws:eks:us-east-1:111122223333:cluster/c1")}, testAccountID) + _, err = svc.UntagResource(&eks.UntagResourceInput{ResourceArn: aws.String(ngARN)}, testAccountID) require.EqualError(t, err, awserrors.ErrorNotImplemented) - _, err = svc.ListTagsForResource(&eks.ListTagsForResourceInput{ResourceArn: aws.String("arn:aws:eks:us-east-1:111122223333:cluster/c1")}, testAccountID) + _, err = svc.ListTagsForResource(&eks.ListTagsForResourceInput{ResourceArn: aws.String(ngARN)}, testAccountID) require.EqualError(t, err, awserrors.ErrorNotImplemented) } diff --git a/spinifex/handlers/iam/managed_policies.go b/spinifex/handlers/iam/managed_policies.go new file mode 100644 index 000000000..b3d6118b0 --- /dev/null +++ b/spinifex/handlers/iam/managed_policies.go @@ -0,0 +1,237 @@ +package handlers_iam + +import ( + "encoding/json" + "fmt" +) + +// awsManagedPolicyDocs maps well-known AWS-managed policy ARNs to the grant +// documents AWS publishes for them. Spinifex does not provision managed +// policies (AttachRolePolicy/AttachUserPolicy round-trip the ARN opaquely), +// but assumed-role and user authorization must honour their grants so stock +// EKS roles — whose permissions come entirely from these policies — work +// without a backing document. Unknown managed ARNs are not modeled and resolve +// to no grant (fail-closed deny), never an error. +// +// Documents mirror the AWS-published policies; Condition blocks are omitted +// because Statement carries no Condition field (a minor, deliberate over-grant +// acceptable for a compat shim). +var awsManagedPolicyDocs = map[string]string{ + "arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy": `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "ec2:DescribeInstances", + "ec2:DescribeInstanceTypes", + "ec2:DescribeRouteTables", + "ec2:DescribeSecurityGroups", + "ec2:DescribeSubnets", + "ec2:DescribeVolumes", + "ec2:DescribeVolumesModifications", + "ec2:DescribeVpcs", + "ec2:DescribeDhcpOptions", + "ec2:DescribeNetworkInterfaces", + "ec2:DescribeInstanceTopology" + ], + "Resource": "*" + }, + { + "Sid": "EKSDescribeClusterPolicy", + "Effect": "Allow", + "Action": "eks:DescribeCluster", + "Resource": "*" + } + ] + }`, + + "arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy": `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "ec2:AssignPrivateIpAddresses", + "ec2:AttachNetworkInterface", + "ec2:CreateNetworkInterface", + "ec2:DeleteNetworkInterface", + "ec2:DescribeInstances", + "ec2:DescribeTags", + "ec2:DescribeNetworkInterfaces", + "ec2:DescribeInstanceTypes", + "ec2:DescribeSubnets", + "ec2:DetachNetworkInterface", + "ec2:ModifyNetworkInterfaceAttribute", + "ec2:UnassignPrivateIpAddresses" + ], + "Resource": "*" + }, + { + "Sid": "CreateTags", + "Effect": "Allow", + "Action": "ec2:CreateTags", + "Resource": "arn:aws:ec2:*:*:network-interface/*" + } + ] + }`, + + "arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly": `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "ecr:GetAuthorizationToken", + "ecr:BatchCheckLayerAvailability", + "ecr:GetDownloadUrlForLayer", + "ecr:GetRepositoryPolicy", + "ecr:DescribeRepositories", + "ecr:ListImages", + "ecr:DescribeImages", + "ecr:BatchGetImage", + "ecr:GetLifecyclePolicy", + "ecr:GetLifecyclePolicyPreview", + "ecr:ListTagsForResource", + "ecr:DescribeImageScanFindings" + ], + "Resource": "*" + } + ] + }`, + + "arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryPullOnly": `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "ecr:GetAuthorizationToken", + "ecr:BatchCheckLayerAvailability", + "ecr:GetDownloadUrlForLayer", + "ecr:BatchGetImage" + ], + "Resource": "*" + } + ] + }`, + + "arn:aws:iam::aws:policy/AmazonEKSClusterPolicy": `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "autoscaling:DescribeAutoScalingGroups", + "autoscaling:UpdateAutoScalingGroup", + "ec2:AttachVolume", + "ec2:AuthorizeSecurityGroupIngress", + "ec2:CreateRoute", + "ec2:CreateSecurityGroup", + "ec2:CreateTags", + "ec2:CreateVolume", + "ec2:DeleteRoute", + "ec2:DeleteSecurityGroup", + "ec2:DeleteVolume", + "ec2:DescribeInstances", + "ec2:DescribeRouteTables", + "ec2:DescribeSecurityGroups", + "ec2:DescribeSubnets", + "ec2:DescribeVolumes", + "ec2:DescribeVolumesModifications", + "ec2:DescribeVpcs", + "ec2:DescribeDhcpOptions", + "ec2:DescribeNetworkInterfaces", + "ec2:DescribeAvailabilityZones", + "ec2:DescribeAccountAttributes", + "ec2:DescribeAddresses", + "ec2:DescribeInternetGateways", + "ec2:DetachVolume", + "ec2:ModifyInstanceAttribute", + "ec2:ModifyVolume", + "ec2:RevokeSecurityGroupIngress", + "elasticloadbalancing:*", + "kms:DescribeKey" + ], + "Resource": "*" + }, + { + "Sid": "AllowServiceLinkedRole", + "Effect": "Allow", + "Action": [ + "iam:CreateServiceLinkedRole", + "iam:ListAttachedRolePolicies" + ], + "Resource": "*" + } + ] + }`, + + "arn:aws:iam::aws:policy/AmazonEKSServicePolicy": `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "eks:UpdateClusterVersion", + "ec2:CreateNetworkInterface", + "ec2:CreateNetworkInterfacePermission", + "ec2:DeleteNetworkInterface", + "ec2:DescribeInstances", + "ec2:DescribeNetworkInterfaces", + "ec2:DescribeSecurityGroups", + "ec2:DescribeSubnets", + "ec2:DescribeVpcs", + "ec2:ModifyNetworkInterfaceAttribute", + "route53:AssociateVPCWithHostedZone", + "kms:DescribeKey" + ], + "Resource": "*" + } + ] + }`, +} + +// builtinManagedPolicyParsed is the parsed form of awsManagedPolicyDocs, built +// once at init. A malformed builtin document is a programming error and panics. +var builtinManagedPolicyParsed = func() map[string]PolicyDocument { + parsed := make(map[string]PolicyDocument, len(awsManagedPolicyDocs)) + for arn, raw := range awsManagedPolicyDocs { + var doc PolicyDocument + if err := json.Unmarshal([]byte(raw), &doc); err != nil { + panic(fmt.Sprintf("iam: malformed builtin managed policy %s: %v", arn, err)) + } + parsed[arn] = doc + } + return parsed +}() + +// builtinManagedPolicyDoc returns the modeled grant document for an AWS-managed +// policy ARN. ok is false for managed ARNs Spinifex does not model. +func builtinManagedPolicyDoc(arn string) (PolicyDocument, bool) { + doc, ok := builtinManagedPolicyParsed[arn] + return doc, ok +} + +// resolveAttachedPolicy resolves one attached policy ARN to its grant document +// for authorization. AWS-managed ARNs resolve from the builtin registry, or +// resolve to no grant (include=false) when not modeled — never an error, so a +// role carrying an unmodeled managed policy is denied that grant rather than +// failing the whole request. Customer-managed ARNs are fetched from KV and +// fail closed (error) when unresolvable. +func (s *IAMServiceImpl) resolveAttachedPolicy(accountID, arn string) (doc PolicyDocument, include bool, err error) { + if isAWSManagedPolicyARN(arn) { + if d, ok := builtinManagedPolicyDoc(arn); ok { + return d, true, nil + } + return PolicyDocument{}, false, nil + } + policy, err := s.getPolicyByARN(accountID, arn) + if err != nil { + return PolicyDocument{}, false, fmt.Errorf("resolve policy %s: %w", arn, err) + } + if err := json.Unmarshal([]byte(policy.PolicyDocument), &doc); err != nil { + return PolicyDocument{}, false, fmt.Errorf("parse policy %s: %w", policy.PolicyName, err) + } + return doc, true, nil +} diff --git a/spinifex/handlers/iam/roles.go b/spinifex/handlers/iam/roles.go index b0256c56a..23e31ff30 100644 --- a/spinifex/handlers/iam/roles.go +++ b/spinifex/handlers/iam/roles.go @@ -245,8 +245,15 @@ func (s *IAMServiceImpl) AttachRolePolicy(accountID string, input *iam.AttachRol roleName := *input.RoleName policyARN := *input.PolicyArn - if _, err := s.getPolicyByARN(accountID, policyARN); err != nil { - return nil, err + // AWS-managed policy ARNs (arn:aws:iam::aws:policy/...) are never provisioned + // in Spinifex; stock EKS tooling attaches them (AmazonEKSClusterPolicy, + // AmazonEKSWorkerNodePolicy, AmazonEKS_CNI_Policy, ...). Store them opaquely + // so ListAttachedRolePolicies / DescribeNodegroup round-trip instead of + // failing NoSuchEntity. Customer-managed ARNs must still exist. + if !isAWSManagedPolicyARN(policyARN) { + if _, err := s.getPolicyByARN(accountID, policyARN); err != nil { + return nil, err + } } role, err := s.getRole(accountID, roleName) @@ -314,6 +321,13 @@ func (s *IAMServiceImpl) ListAttachedRolePolicies(accountID string, input *iam.L var attached []*iam.AttachedPolicy for _, arn := range role.AttachedPolicies { + if isAWSManagedPolicyARN(arn) { + attached = append(attached, &iam.AttachedPolicy{ + PolicyArn: aws.String(arn), + PolicyName: aws.String(managedPolicyNameFromARN(arn)), + }) + continue + } policy, err := s.getPolicyByARN(accountID, arn) if err != nil { slog.Warn("ListAttachedRolePolicies: policy not found for ARN", "arn", arn, "err", err) @@ -342,21 +356,36 @@ func (s *IAMServiceImpl) GetRolePolicies(accountID, roleName string) ([]PolicyDo var docs []PolicyDocument for _, arn := range role.AttachedPolicies { - policy, err := s.getPolicyByARN(accountID, arn) + doc, include, err := s.resolveAttachedPolicy(accountID, arn) if err != nil { - return nil, fmt.Errorf("resolve policy %s: %w", arn, err) // fail closed + return nil, err // fail closed } - - var doc PolicyDocument - if err := json.Unmarshal([]byte(policy.PolicyDocument), &doc); err != nil { - return nil, fmt.Errorf("parse policy %s: %w", policy.PolicyName, err) + if include { + docs = append(docs, doc) } - docs = append(docs, doc) } return docs, nil } +// isAWSManagedPolicyARN reports whether arn is an AWS-managed policy ARN +// (arn:aws:iam::aws:policy/...). These are not provisioned in Spinifex but are +// stored and round-tripped opaquely so stock EKS tooling that attaches them +// works without a backing policy document. +func isAWSManagedPolicyARN(arn string) bool { + return strings.HasPrefix(arn, "arn:aws:iam::aws:policy/") +} + +// managedPolicyNameFromARN returns the final path segment of an AWS-managed +// policy ARN, e.g. .../service-role/AmazonEKS_CNI_Policy -> AmazonEKS_CNI_Policy. +func managedPolicyNameFromARN(arn string) string { + name := strings.TrimPrefix(arn, "arn:aws:iam::aws:policy/") + if i := strings.LastIndex(name, "/"); i >= 0 { + name = name[i+1:] + } + return name +} + func (s *IAMServiceImpl) getRole(accountID, roleName string) (*Role, error) { entry, err := s.rolesBucket.Get(accountID + "." + roleName) if err != nil { diff --git a/spinifex/handlers/iam/roles_test.go b/spinifex/handlers/iam/roles_test.go index 25eec60ce..c35da88a3 100644 --- a/spinifex/handlers/iam/roles_test.go +++ b/spinifex/handlers/iam/roles_test.go @@ -2,6 +2,7 @@ package handlers_iam import ( "encoding/json" + "slices" "strings" "testing" @@ -518,6 +519,40 @@ func TestAttachRolePolicy_RoleNotFound(t *testing.T) { assert.Contains(t, err.Error(), awserrors.ErrorIAMNoSuchEntity) } +func TestAttachRolePolicy_AWSManagedPolicyNotSeeded(t *testing.T) { + svc := setupTestIAMService(t) + role := createTestRole(t, svc, "eks-node-role") + // AWS-managed ARN with no backing policy in the store — stock EKS tooling + // attaches these. Must round-trip opaquely, not fail NoSuchEntity. + const managedARN = "arn:aws:iam::aws:policy/service-role/AmazonEKS_CNI_Policy" + + _, err := svc.AttachRolePolicy(testAccountID, &iam.AttachRolePolicyInput{ + RoleName: role.RoleName, + PolicyArn: aws.String(managedARN), + }) + require.NoError(t, err) + + out, err := svc.ListAttachedRolePolicies(testAccountID, &iam.ListAttachedRolePoliciesInput{ + RoleName: role.RoleName, + }) + require.NoError(t, err) + require.Len(t, out.AttachedPolicies, 1) + assert.Equal(t, managedARN, *out.AttachedPolicies[0].PolicyArn) + assert.Equal(t, "AmazonEKS_CNI_Policy", *out.AttachedPolicies[0].PolicyName) + + _, err = svc.DetachRolePolicy(testAccountID, &iam.DetachRolePolicyInput{ + RoleName: role.RoleName, + PolicyArn: aws.String(managedARN), + }) + require.NoError(t, err) + + out, err = svc.ListAttachedRolePolicies(testAccountID, &iam.ListAttachedRolePoliciesInput{ + RoleName: role.RoleName, + }) + require.NoError(t, err) + assert.Empty(t, out.AttachedPolicies) +} + func TestDetachRolePolicy(t *testing.T) { svc := setupTestIAMService(t) role := createTestRole(t, svc, "detach-role") @@ -637,6 +672,63 @@ func TestGetRolePolicies_UnresolvablePolicy(t *testing.T) { assert.Error(t, err) } +// TestGetRolePolicies_AWSManagedResolved proves a stock EKS node role — whose +// grants come entirely from AWS-managed policies — resolves to the builtin +// grant documents, so assumed-role authorization honours them instead of +// denying every call (the regression behind mulga-siv-297). +func TestGetRolePolicies_AWSManagedResolved(t *testing.T) { + svc := setupTestIAMService(t) + role := createTestRole(t, svc, "eks-node-role") + + for _, arn := range []string{ + "arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy", + "arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly", + "arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy", + } { + _, err := svc.AttachRolePolicy(testAccountID, &iam.AttachRolePolicyInput{ + RoleName: role.RoleName, + PolicyArn: aws.String(arn), + }) + require.NoError(t, err) + } + + docs, err := svc.GetRolePolicies(testAccountID, "eks-node-role") + require.NoError(t, err) + require.Len(t, docs, 3) + assert.True(t, policiesGrant(docs, "ec2:DescribeInstances"), "WorkerNodePolicy") + assert.True(t, policiesGrant(docs, "ecr:GetAuthorizationToken"), "ECR ReadOnly") + assert.True(t, policiesGrant(docs, "ec2:CreateNetworkInterface"), "CNI") +} + +// TestGetRolePolicies_UnmodeledManagedSkipped proves an AWS-managed ARN Spinifex +// does not model resolves to no grant (fail-closed deny) rather than erroring +// the whole request. +func TestGetRolePolicies_UnmodeledManagedSkipped(t *testing.T) { + svc := setupTestIAMService(t) + role := createTestRole(t, svc, "unknown-managed-role") + _, err := svc.AttachRolePolicy(testAccountID, &iam.AttachRolePolicyInput{ + RoleName: role.RoleName, + PolicyArn: aws.String("arn:aws:iam::aws:policy/SomeUnmodeledPolicy"), + }) + require.NoError(t, err) + + docs, err := svc.GetRolePolicies(testAccountID, "unknown-managed-role") + require.NoError(t, err) + assert.Empty(t, docs) +} + +// policiesGrant reports whether any statement across docs allows action. +func policiesGrant(docs []PolicyDocument, action string) bool { + for _, d := range docs { + for _, st := range d.Statement { + if slices.Contains(st.Action, action) { + return true + } + } + } + return false +} + // ============================================================================ // Account Scoping // ============================================================================ diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index 605991327..c7e94f51c 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -1283,17 +1283,14 @@ func (s *IAMServiceImpl) GetUserPolicies(accountID, userName string) ([]PolicyDo var docs []PolicyDocument for _, arn := range user.AttachedPolicies { - policy, err := s.getPolicyByARN(accountID, arn) + doc, include, err := s.resolveAttachedPolicy(accountID, arn) if err != nil { // Fail closed: unresolvable policy means we cannot make a safe access decision. - return nil, fmt.Errorf("resolve policy %s: %w", arn, err) + return nil, err } - - var doc PolicyDocument - if err := json.Unmarshal([]byte(policy.PolicyDocument), &doc); err != nil { - return nil, fmt.Errorf("parse policy %s: %w", policy.PolicyName, err) + if include { + docs = append(docs, doc) } - docs = append(docs, doc) } return docs, nil