Repository navigation
feat: Add EKS kubeconfig authentication integration (ATMOS-157) - #2149
Merged
Andriy Knysh (aknysh) merged 22 commits intoMar 21, 2026
Conversation
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Scanned Files
|
|
Warning Release Documentation RequiredThis PR is labeled
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f275b9c80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native AWS EKS support: a kubectl exec-token command and EKS SDK client (DescribeCluster/GetToken), kubeconfig manager and update flows (SDK and integration-backed), EKS integration (Execute/Cleanup/Environment), integration-aware env composition and logout cleanup, schema/errors, tests, CLI flags, docs, and dependency/license updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as atmos auth eks-token
participant Manager as Auth Manager
participant AWS as AWS (STS)
participant Stdout
User->>CLI: Run with --cluster-name --region
CLI->>Manager: Authenticate (resolve identity / creds)
Manager->>AWS: Build AWS config from Atmos creds
CLI->>AWS: Presign STS GetCallerIdentity (x-k8s-aws-id header)
AWS-->>CLI: Return presigned URL
CLI->>CLI: base64url encode + prefix "k8s-aws-v1."
CLI->>Stdout: Emit ExecCredential JSON (apiVersion, token, expiration)
sequenceDiagram
participant User
participant CLI as atmos aws eks update-kubeconfig
participant Manager as Auth Manager
participant Integration as EKS Integration
participant EKS as AWS EKS
participant Kube as Kubeconfig Manager
participant FS as File System
User->>CLI: Run with --cluster-name and --integration/--identity
CLI->>Manager: Authenticate identity / obtain creds
Manager->>Integration: Instantiate EKSIntegration (if requested)
Integration->>EKS: DescribeCluster(name)
EKS-->>Integration: Return endpoint, CA, ARN
Integration->>Kube: NewKubeconfigManager(path,mode)
Integration->>Kube: WriteClusterConfig(info, alias, identity, updateMode)
Kube->>FS: Read/merge and atomically write kubeconfig
FS-->>Kube: Write success
Integration-->>CLI: Report success
CLI-->>User: Print success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/aws/eks/update_kubeconfig.go (1)
69-75:⚠️ Potential issue | 🟠 MajorAdd
WithEnvVarsbinding for theintegrationflag to support environment variable configuration.The
integrationflag is missing its environment variable binding. While other flags (stack, profile, region, kubeconfig) have correspondingWithEnvVarscalls,integrationdoesn't. This breaks the configuration precedence model (flags > environment variables > config file > defaults) and makes the flag inconsistent with the rest of the command's settings.Add:
flags.WithEnvVars("integration", "ATMOS_AWS_INTEGRATION"),This allows environment variable support alongside the existing config-file binding provided by
WithViperPrefix("eks").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/eks/update_kubeconfig.go` around lines 69 - 75, The integration flag declared with flags.WithStringFlag("integration", ...) lacks an environment variable binding; add a flags.WithEnvVars("integration", "ATMOS_AWS_INTEGRATION") entry alongside the other WithEnvVars calls so the "integration" flag follows the same precedence (flags > env vars > config via WithViperPrefix("eks") > defaults) as stack/profile/region/kubeconfig and is discoverable from ATMOS_AWS_INTEGRATION.
🧹 Nitpick comments (1)
pkg/auth/cloud/kube/config.go (1)
3-16: Imports need reordering per guidelines.The k8s packages should be in group 2 (3rd-party) between stdlib and Atmos packages.
Suggested import order
import ( "fmt" "os" "path/filepath" "strconv" + "k8s.io/client-go/tools/clientcmd" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + errUtils "github.com/cloudposse/atmos/errors" awsCloud "github.com/cloudposse/atmos/pkg/auth/cloud/aws" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/xdg" - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" - - "k8s.io/client-go/tools/clientcmd" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/kube/config.go` around lines 3 - 16, Reorder the import block in config.go so imports follow: standard library first (fmt, os, filepath, strconv), then third-party k8s packages (clientcmdapi "k8s.io/client-go/tools/clientcmd/api" and "k8s.io/client-go/tools/clientcmd"), and finally internal Atmos packages (errUtils, awsCloud, perf, xdg); group k8s packages together and ensure blank lines separate the three groups so the k8s packages are in the 3rd‑party group between stdlib and Atmos imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth_eks_token_test.go`:
- Around line 13-101: Add integration-style unit tests that actually run the
eks-token command and validate its runtime behavior: write one success test that
sets up a test fixture (via NewTestKit or similar), provides a resolvable
identity, executes authEKSTokenCmd, captures stdout, and asserts the
ExecCredential JSON contains execCredentialAPIVersion and a non-empty token; and
add at least one error-path test that forces resolveDefaultIdentity to return
empty/ambiguous (e.g., no identities or multiple identities), executes
authEKSTokenCmd, and asserts it returns/prints an error matching
errUtils.ErrEKSTokenGeneration (or the command error path) and does not emit a
valid ExecCredential JSON.
In `@cmd/auth_eks_token.go`:
- Around line 29-53: The authEKSTokenCmd command is self-registered and defines
flags directly which bypasses the repo CommandProvider and
flags.NewStandardParser() requirement; refactor by implementing a
CommandProvider that returns the eks-token command (instead of calling
authCmd.AddCommand directly), move flag definitions to use
flags.NewStandardParser() for command-specific flags, and ensure
executeAuthEKSTokenCommand is wired up via the provider so registration follows
the project's registry pattern (replace any direct creation/registration of
authEKSTokenCmd with the provider-based construction and parser usage).
In `@internal/exec/aws_eks_update_kubeconfig.go`:
- Around line 75-78: The code calls flags.GetString("integration") unguarded
which panics when that flag isn't defined; guard the lookup by first checking
flags.Lookup("integration") != nil (or equivalent has/exists method) before
calling flags.GetString("integration"), and only call GetString when the flag
exists—otherwise set integration to the zero value (e.g., "") or skip related
logic; update the usage sites that rely on the integration variable accordingly
(references: flags.GetString("integration"), the integration variable).
- Around line 85-90: The direct-SDK shortcut currently runs before positional
component parsing and ignores --dry-run; tighten the predicate in the blocks
that call executeEKSUpdateKubeconfigDirect so they only take the shortcut when
there are no positional args and dry-run is not requested. Concretely, in the
condition that reads name != "" && stack == "" && profile == "" && roleArn == ""
&& identity != "", add checks to ensure flags.NArg() == 0 (or equivalent check
that no component/stack positional was provided) and that the dry-run flag is
false (e.g. dryRun == false or
!flags.Changed("dry-run")/flags.GetBool("dry-run") as appropriate). Apply the
same tightened predicate to the similar branch at the later lines that also
invoke executeEKSUpdateKubeconfigDirect.
In `@pkg/auth/cloud/aws/config.go`:
- Around line 41-42: Replace the bare return of the SDK error in the AWS config
loader (the return aws.Config{}, err) with a wrapped error using the repository
sentinel from errors/errors.go so upstream callers can use errors.Is; e.g.,
import fmt and the local errors package and return aws.Config{}, fmt.Errorf("%w:
%v", errors.<SentinelNameForAWSConfigLoad>, err) (use the actual sentinel name
defined in errors/errors.go) so the original err is wrapped with the static
sentinel.
In `@pkg/auth/cloud/aws/eks_test.go`:
- Around line 19-26: The test defines a hand-rolled stub mockEKSClient
implementing EKSClient (with DescribeCluster) which will drift from the real
interface; replace this manual mock with a generated mock via
go.uber.org/mock/mockgen: add a //go:generate directive referencing the
EKSClient interface, run mockgen to produce the mock (e.g., generate a mock in
the test package), and update tests to use the generated mock type instead of
mockEKSClient and its describeClusterFn field; ensure tests import and use the
generated mock's EXPECT/On-style methods to stub DescribeCluster so the mock
stays in sync with any EKSClient changes.
In `@pkg/auth/cloud/aws/eks.go`:
- Around line 19-30: The presigned STS GetCallerIdentity request must include
X-Amz-Expires=60 and the token lifetime must align with that presign lifetime:
update usage of eksPresignLifetimeSeconds and eksTokenExpiry by setting
eksTokenExpiry to eksPresignLifetimeSeconds and ensure the presign step for
GetCallerIdentity injects the X-Amz-Expires query parameter before SigV4 signing
(similar to how you wrap to add eksClusterIDHeader). Concretely, find the
presign call that builds the GetCallerIdentity request (references:
GetCallerIdentity presigner/presign method and the constants
eksPresignLifetimeSeconds, eksTokenExpiry, eksClusterIDHeader, eksTokenPrefix)
and wrap the presigner so that it adds the query parameter
X-Amz-Expires=fmt.Sprint(eksPresignLifetimeSeconds) to req.URL.RawQuery prior to
signing; also update the token expiry usage to use eksPresignLifetimeSeconds
instead of 14 minutes.
In `@pkg/auth/cloud/kube/config_test.go`:
- Around line 216-230: The test TestWriteClusterConfig_FilePermissions assumes
POSIX file modes by asserting stat.Mode().Perm() == 0o600 which fails on
Windows; update the test to be OS-aware by detecting runtime.GOOS (or use
os.PathSeparator/exec check) and only assert the POSIX permission for
non-windows platforms, while on Windows assert that the file exists and is
writable/readable (or skip the permission check), keeping the setup using
NewKubeconfigManager and the call to mgr.WriteClusterConfig(info, "dev-eks",
"dev-admin", "merge") unchanged.
In `@pkg/auth/cloud/kube/config.go`:
- Around line 244-251: In writeConfig (method KubeconfigManager.writeConfig)
wrap the error returned by os.Chmod(m.path, m.mode) using the same error
wrapping pattern as the clientcmd.WriteToFile call (i.e. return a fmt.Errorf
that wraps errUtils.ErrKubeconfigWrite and the chmod error) so the chmod failure
is returned consistently; reference os.Chmod, m.path, m.mode and
errUtils.ErrKubeconfigWrite when making the change.
- Around line 185-203: The exec env var ATMOS_IDENTITY is always added to
execEnv even when identityName is empty, causing an empty env var; change the
logic to only append the ExecEnvVar to execEnv when identityName != "" (mirror
the conditional used for adding "--identity" to execArgs) so that execEnv and
execArgs handle identityName consistently—update the block that builds execEnv
(variable execEnv and the ATMOS_IDENTITY entry) to be conditional on
identityName, leaving execArgs and its existing conditional untouched.
- Around line 141-147: The returns in the cleanup block currently return raw
errors from os.Remove(m.path) and clientcmd.WriteToFile(*existing, m.path); wrap
both errors with static sentinel errors (e.g., ErrRemoveConfig and
ErrWriteConfig — create them if missing) using error wrapping
(fmt.Errorf("remove config: %w", ErrRemoveConfig) style or fmt.Errorf("%w: %v",
ErrRemoveConfig, err)) so callers can reliably match on the static error while
preserving the underlying error; apply this wrapping to the os.Remove(m.path)
return and the clientcmd.WriteToFile(*existing, m.path) return, referencing
variables existing and m.path and the WriteToFile call to locate the lines.
In `@pkg/auth/integrations/aws/eks_test.go`:
- Around line 204-207: Replace hardcoded Unix paths in the tests by building
platform-safe paths with filepath.Join; specifically change Kubeconfig.Path
values like "/tmp/kubeconfig" to filepath.Join(t.TempDir(), "kubeconfig") and
update any uses of t.TempDir() + "/..." or forward-slash concatenation in the
same test file (notably the other occurrences around the KubeconfigSettings
usage at the later blocks mentioned) to use filepath.Join instead, and add the
"path/filepath" import if missing.
In `@pkg/auth/integrations/aws/eks.go`:
- Around line 147-170: The cleanup path currently returns early if Alias is
empty and otherwise uses findClusterARN which only does a name-suffix scan,
risking wrong removals; change Cleanup and findClusterARN to resolve the cluster
ARN via the kubeconfig's context->cluster mapping instead of matching on
"cluster/<name>". Specifically: when e.cluster.Alias is empty, look up the
kubeconfig contexts stored in mgr (the manager/kubeconfig object) to find the
context whose user/cluster entry equals the ARN that Execute wrote (derive
contextName from that mapping rather than returning early); update
findClusterARN to inspect the kubeconfig contexts and their cluster fields to
return the exact ARN (not a suffix match), and apply the same fix to the other
block referenced (lines ~224-232) so both Cleanup code paths use context mapping
resolution rather than name-suffix scanning. Ensure you reference and update the
functions/methods: Cleanup, findClusterARN, and any code that sets contextName
in Execute to use the kubeconfig context mapping for exact matches.
In `@pkg/auth/manager_environment_test.go`:
- Around line 34-78: Tests for KUBECONFIG path-list behavior hardcode POSIX
separators and paths; update the tests that reference
composeEnvironmentVariables and appendPathList to build platform-native paths
using filepath.Join (e.g., filepath.Join("path","a")) and to construct expected
combined lists using os.PathListSeparator instead of ":" so assertions use the
OS-native separator and will pass on Windows and Unix.
---
Outside diff comments:
In `@cmd/aws/eks/update_kubeconfig.go`:
- Around line 69-75: The integration flag declared with
flags.WithStringFlag("integration", ...) lacks an environment variable binding;
add a flags.WithEnvVars("integration", "ATMOS_AWS_INTEGRATION") entry alongside
the other WithEnvVars calls so the "integration" flag follows the same
precedence (flags > env vars > config via WithViperPrefix("eks") > defaults) as
stack/profile/region/kubeconfig and is discoverable from ATMOS_AWS_INTEGRATION.
---
Nitpick comments:
In `@pkg/auth/cloud/kube/config.go`:
- Around line 3-16: Reorder the import block in config.go so imports follow:
standard library first (fmt, os, filepath, strconv), then third-party k8s
packages (clientcmdapi "k8s.io/client-go/tools/clientcmd/api" and
"k8s.io/client-go/tools/clientcmd"), and finally internal Atmos packages
(errUtils, awsCloud, perf, xdg); group k8s packages together and ensure blank
lines separate the three groups so the k8s packages are in the 3rd‑party group
between stdlib and Atmos imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96b0d0bf-20e4-457a-ab08-0a2bd289c041
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
NOTICEcmd/auth_eks_token.gocmd/auth_eks_token_test.gocmd/aws/eks/update_kubeconfig.gocmd/aws/eks/update_kubeconfig_test.goerrors/errors.gogo.modinternal/exec/aws_eks_update_kubeconfig.gopkg/auth/cloud/aws/config.gopkg/auth/cloud/aws/ecr.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/cloud/aws/eks.gopkg/auth/cloud/aws/eks_test.gopkg/auth/cloud/kube/config.gopkg/auth/cloud/kube/config_test.gopkg/auth/integrations/aws/ecr.gopkg/auth/integrations/aws/eks.gopkg/auth/integrations/aws/eks_test.gopkg/auth/integrations/registry_test.gopkg/auth/integrations/types.gopkg/auth/manager_environment.gopkg/auth/manager_environment_test.gopkg/auth/manager_integrations.gopkg/auth/manager_logout.gopkg/schema/schema_auth.go
|
Warning Release Documentation RequiredThis PR is labeled
|
Contributor
|
💥 This pull request now has conflicts. Could you fix it Ben (@Benbentwo)? 🙏 |
|
Warning Release Documentation RequiredThis PR is labeled
|
|
Warning Release Documentation RequiredThis PR is labeled
|
1 similar comment
|
Warning Release Documentation RequiredThis PR is labeled
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
pkg/auth/integrations/aws/eks.go (1)
218-236:⚠️ Potential issue | 🟡 MinorSuffix matching can match wrong clusters.
The suffix check
"cluster/" + e.cluster.Namematches any ARN ending with that pattern. Clusters with the same name in different regions or accounts will incorrectly match:
arn:aws:eks:us-east-2:111111:cluster/my-clusterarn:aws:eks:us-west-2:222222:cluster/my-clusterConsider also matching the region from
e.cluster.Regionto narrow the match.💡 Suggested improvement
func (e *EKSIntegration) findClusterARN(mgr *kube.KubeconfigManager) (string, error) { // ... suffix := "cluster/" + e.cluster.Name + regionPrefix := "arn:aws:eks:" + e.cluster.Region + ":" for _, arn := range clusters { - if len(arn) >= len(suffix) && arn[len(arn)-len(suffix):] == suffix { + if strings.HasPrefix(arn, regionPrefix) && strings.HasSuffix(arn, suffix) { return arn, nil } } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/integrations/aws/eks.go` around lines 218 - 236, The suffix-only match in EKSIntegration.findClusterARN can return ARNs from other regions/accounts; modify the logic to also match the cluster region by either parsing the ARN or matching the region component: call mgr.ListClusterARNs() as before but filter ARNs to those containing the region string (e.g. ensure arn contains "arn:aws:eks:"+e.cluster.Region+":"), and then verify the ARN ends with "cluster/"+e.cluster.Name before returning; if multiple still match, optionally prefer exact region+name match or return an explicit ambiguity error.pkg/auth/cloud/kube/config.go (3)
255-260:⚠️ Potential issue | 🟡 MinorWrap
Chmodfailures withErrKubeconfigWrite.Line 260 returns the raw
os.Chmoderror, so callers cannot reliably match kubeconfig write failures.Proposed fix
func (m *KubeconfigManager) writeConfig(config *clientcmdapi.Config) error { if err := clientcmd.WriteToFile(*config, m.path); err != nil { return fmt.Errorf("%w: %w", errUtils.ErrKubeconfigWrite, err) } - return os.Chmod(m.path, m.mode) + if err := os.Chmod(m.path, m.mode); err != nil { + return fmt.Errorf("%w: failed to set permissions on %s: %w", errUtils.ErrKubeconfigWrite, m.path, err) + } + return nil }As per coding guidelines: "Error Handling (MANDATORY): All errors MUST be wrapped using static errors defined in errors/errors.go."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/kube/config.go` around lines 255 - 260, The writeConfig function returns raw os.Chmod errors (from os.Chmod(m.path, m.mode)), preventing callers from matching kubeconfig write failures; change the final return to wrap any Chmod error with the static sentinel errUtils.ErrKubeconfigWrite (e.g., check the error from os.Chmod and if non-nil return fmt.Errorf("%w: %w", errUtils.ErrKubeconfigWrite, err)) so both clientcmd.WriteToFile and Chmod failures are consistently wrapped; reference: KubeconfigManager.writeConfig, m.path, os.Chmod, and errUtils.ErrKubeconfigWrite.
142-147:⚠️ Potential issue | 🟡 MinorRoute cleanup writes through
writeConfig.This block returns raw filesystem errors and bypasses
m.writeConfig, soRemoveClusterConfigloses both sentinel wrapping and the configured file mode on rewrite.Proposed fix
// If the config is now empty, remove the file. if len(existing.Clusters) == 0 && len(existing.Contexts) == 0 && len(existing.AuthInfos) == 0 { - return os.Remove(m.path) + if err := os.Remove(m.path); err != nil { + return fmt.Errorf("%w: failed to remove empty kubeconfig %s: %w", errUtils.ErrKubeconfigWrite, m.path, err) + } + return nil } - return clientcmd.WriteToFile(*existing, m.path) + return m.writeConfig(existing)As per coding guidelines: "Error Handling (MANDATORY): All errors MUST be wrapped using static errors defined in errors/errors.go."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/kube/config.go` around lines 142 - 147, The code in RemoveClusterConfig (using local variable existing) bypasses m.writeConfig by calling os.Remove and clientcmd.WriteToFile directly, losing sentinel error wrapping and configured file mode; change the branch so when existing.Clusters/Contexts/AuthInfos are empty you call m.writeConfig(nil) (or the appropriate removal path via m.writeConfig) instead of os.Remove(m.path), and replace clientcmd.WriteToFile(*existing, m.path) with a call to m.writeConfig(existing) so all errors are wrapped and file mode handling stays centralized in m.writeConfig.
185-191:⚠️ Potential issue | 🟡 MinorSkip
ATMOS_IDENTITYwhen no identity is set.
execArgsonly adds--identityconditionally, butexecEnvalways injectsATMOS_IDENTITY="". That inconsistency is still there and can leak into the exec plugin path unnecessarily.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/kube/config.go` around lines 185 - 191, The exec plugin environment always injects ATMOS_IDENTITY even when none is set; update the execEnv construction in pkg/auth/cloud/kube/config.go so that ATMOS_IDENTITY is only appended when identityName is non-empty (mirroring the conditional in execArgs). Locate the execEnv slice creation (clientcmdapi.ExecEnvVar) and change it to conditionally append the ExecEnvVar{Name: "ATMOS_IDENTITY", Value: identityName} only if identityName != "" so the exec plugin path is not given an empty ATMOS_IDENTITY.
🧹 Nitpick comments (2)
pkg/auth/cloud/aws/eks.go (1)
29-34: Clarify the relationship between presign lifetime and token expiry.The 60-second presign lifetime and 14-minute token expiry are intentionally different (presign URL must be used within 60s, but the resulting token is valid for 14 minutes). A brief inline comment would help future readers understand this isn't a mismatch.
💡 Suggested documentation
// eksTokenExpiry is the default token lifetime for EKS tokens. + // Note: This differs from eksPresignLifetimeSeconds because the presigned URL + // must be used within 60s, but the resulting EKS token is valid for ~14 minutes. eksTokenExpiry = 14 * time.Minute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/aws/eks.go` around lines 29 - 34, The two constants eksPresignLifetimeSeconds and eksTokenExpiry represent different lifetimes: eksPresignLifetimeSeconds (60) is the allowed window to use the presigned URL for obtaining a token, while eksTokenExpiry (14 * time.Minute) is the validity of the token issued after using that presigned URL; add a brief inline comment next to these constants (referencing eksPresignLifetimeSeconds and eksTokenExpiry) clarifying that the presign URL must be used within 60s but the resulting token is valid for 14 minutes so readers understand this is intentional and not a mismatch.cmd/auth_eks_token.go (1)
172-186: Consider checking for default identity in auth config.
resolveDefaultIdentityonly returns an identity when there's exactly one. It doesn't check theDefaultfield on identities. The auth manager'sGetDefaultIdentityhandles multiple defaults and theDefaultflag—consider reusing that logic.💡 Suggested approach
func resolveDefaultIdentity(authConfig *schema.AuthConfig) string { if authConfig == nil || len(authConfig.Identities) == 0 { return "" } // If there's only one identity, use it. if len(authConfig.Identities) == 1 { for name := range authConfig.Identities { return name } } + // Check for explicitly marked default identity. + for name, identity := range authConfig.Identities { + if identity.Default { + return name + } + } + return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_eks_token.go` around lines 172 - 186, resolveDefaultIdentity currently only returns an identity when there's exactly one entry; change it to first check each identity's Default flag (Identity.Default) and return the name of the identity marked default, and if none are marked default fall back to the previous behavior (return the single identity when there's exactly one) or delegate to the existing auth manager helper (GetDefaultIdentity) to reuse its multi-default resolution logic; update resolveDefaultIdentity to iterate authConfig.Identities, prefer Identity.Default, and call GetDefaultIdentity when appropriate so default selection matches the auth manager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/cloud/kube/config.go`:
- Around line 100-110: The "error" branch only checks for an existing cluster
and then calls m.mergeConfig, so context or auth name collisions (alias or
generated auth info name) still cause the file to be mutated; load the existing
kubeconfig (clientcmd.LoadFromFile(m.path)), then check existing.Contexts for
the intended context name (alias) and existing.AuthInfos for the generated auth
info key used in newConfig (the auth name), and if either exists return an
ErrKubeconfigMerge-style error (like the cluster check) instead of proceeding;
only call m.mergeConfig(newConfig) when none of existing.Clusters[info.ARN],
existing.Contexts[alias], or existing.AuthInfos[authName] are present.
- Around line 182-183: The current auth info key `userName := "user-" +
info.Name` can collide across clusters with the same name; change the key to
include a stable unique identifier (e.g. cluster UID or a short hash of cluster
server/endpoint) so it’s unique per cluster. Replace construction of `userName`
to incorporate `info.UID` (or if UID is unavailable, use a deterministic hash of
`info.Name` + cluster server) — e.g. `user-<name>-<uid>` or `user-<name>-<hash>`
— so AuthInfo and contexts won’t be overwritten for same-named clusters.
- Around line 49-56: When customPath is empty, the code currently always falls
back to DefaultKubeconfigPath (Atmos XDG), ignoring an active KUBECONFIG
environment that should take precedence; change the branch handling around
customPath to first check the KUBECONFIG env var and use that value if present,
then only call DefaultKubeconfigPath() when KUBECONFIG is unset. Update the
logic that sets path (the variables customPath and path) and preserve existing
error wrapping using errUtils.ErrKubeconfigPath when DefaultKubeconfigPath()
fails so callers (e.g., internal/exec/aws_eks_update_kubeconfig.go) will update
the same kubeconfig file kubectl would use.
---
Duplicate comments:
In `@pkg/auth/cloud/kube/config.go`:
- Around line 255-260: The writeConfig function returns raw os.Chmod errors
(from os.Chmod(m.path, m.mode)), preventing callers from matching kubeconfig
write failures; change the final return to wrap any Chmod error with the static
sentinel errUtils.ErrKubeconfigWrite (e.g., check the error from os.Chmod and if
non-nil return fmt.Errorf("%w: %w", errUtils.ErrKubeconfigWrite, err)) so both
clientcmd.WriteToFile and Chmod failures are consistently wrapped; reference:
KubeconfigManager.writeConfig, m.path, os.Chmod, and
errUtils.ErrKubeconfigWrite.
- Around line 142-147: The code in RemoveClusterConfig (using local variable
existing) bypasses m.writeConfig by calling os.Remove and clientcmd.WriteToFile
directly, losing sentinel error wrapping and configured file mode; change the
branch so when existing.Clusters/Contexts/AuthInfos are empty you call
m.writeConfig(nil) (or the appropriate removal path via m.writeConfig) instead
of os.Remove(m.path), and replace clientcmd.WriteToFile(*existing, m.path) with
a call to m.writeConfig(existing) so all errors are wrapped and file mode
handling stays centralized in m.writeConfig.
- Around line 185-191: The exec plugin environment always injects ATMOS_IDENTITY
even when none is set; update the execEnv construction in
pkg/auth/cloud/kube/config.go so that ATMOS_IDENTITY is only appended when
identityName is non-empty (mirroring the conditional in execArgs). Locate the
execEnv slice creation (clientcmdapi.ExecEnvVar) and change it to conditionally
append the ExecEnvVar{Name: "ATMOS_IDENTITY", Value: identityName} only if
identityName != "" so the exec plugin path is not given an empty ATMOS_IDENTITY.
In `@pkg/auth/integrations/aws/eks.go`:
- Around line 218-236: The suffix-only match in EKSIntegration.findClusterARN
can return ARNs from other regions/accounts; modify the logic to also match the
cluster region by either parsing the ARN or matching the region component: call
mgr.ListClusterARNs() as before but filter ARNs to those containing the region
string (e.g. ensure arn contains "arn:aws:eks:"+e.cluster.Region+":"), and then
verify the ARN ends with "cluster/"+e.cluster.Name before returning; if multiple
still match, optionally prefer exact region+name match or return an explicit
ambiguity error.
---
Nitpick comments:
In `@cmd/auth_eks_token.go`:
- Around line 172-186: resolveDefaultIdentity currently only returns an identity
when there's exactly one entry; change it to first check each identity's Default
flag (Identity.Default) and return the name of the identity marked default, and
if none are marked default fall back to the previous behavior (return the single
identity when there's exactly one) or delegate to the existing auth manager
helper (GetDefaultIdentity) to reuse its multi-default resolution logic; update
resolveDefaultIdentity to iterate authConfig.Identities, prefer
Identity.Default, and call GetDefaultIdentity when appropriate so default
selection matches the auth manager.
In `@pkg/auth/cloud/aws/eks.go`:
- Around line 29-34: The two constants eksPresignLifetimeSeconds and
eksTokenExpiry represent different lifetimes: eksPresignLifetimeSeconds (60) is
the allowed window to use the presigned URL for obtaining a token, while
eksTokenExpiry (14 * time.Minute) is the validity of the token issued after
using that presigned URL; add a brief inline comment next to these constants
(referencing eksPresignLifetimeSeconds and eksTokenExpiry) clarifying that the
presign URL must be used within 60s but the resulting token is valid for 14
minutes so readers understand this is intentional and not a mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e9dc77f-7907-41e2-9fae-5cc0e81ebcb7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
NOTICEcmd/auth_eks_token.goerrors/errors.gogo.modpkg/auth/cloud/aws/eks.gopkg/auth/cloud/kube/config.gopkg/auth/cloud/kube/config_test.gopkg/auth/integrations/aws/eks.gopkg/auth/integrations/aws/eks_test.gopkg/auth/manager.gopkg/auth/manager_environment.gopkg/auth/manager_environment_test.gopkg/auth/manager_integrations.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/auth/manager_integrations.go
- errors/errors.go
- pkg/auth/integrations/aws/eks_test.go
- NOTICE
Ben (Benbentwo)
force-pushed
the
feature/atmos-157-create-atmos-auth-identity-for-eks-using-aws-go-sdk
branch
from
March 13, 2026 19:31
ee01938 to
4998da0
Compare
Contributor
|
💥 This pull request now has conflicts. Could you fix it Ben (@Benbentwo)? 🙏 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
internal/exec/aws_eks_update_kubeconfig.go (1)
85-90:⚠️ Potential issue | 🟠 MajorTighten the direct-SDK shortcut predicate.
The condition at line 88 runs before positional
componentis parsed (lines 92-95) and ignores--dry-run. This means:
- A command like
atmos aws eks update-kubeconfig mycomponent --stack foo --name cluster --identity idwould take the SDK path, ignoring the component.--dry-runwould still write the kubeconfig.💡 Suggested fix
+ component := "" + if len(args) > 0 { + component = args[0] + } + // If --name is provided without component/stack and without profile/role-arn, // use Go SDK direct path (requires identity). identity, _ := flags.GetString("identity") - if name != "" && stack == "" && profile == "" && roleArn == "" && identity != "" { + if name != "" && component == "" && stack == "" && profile == "" && roleArn == "" && identity != "" && !dryRun { return executeEKSUpdateKubeconfigDirect(name, region, kubeconfig, alias, identity) } - - component := "" - if len(args) > 0 { - component = args[0] - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/aws_eks_update_kubeconfig.go` around lines 85 - 90, The direct-SDK shortcut currently runs too early and ignores the parsed positional component and --dry-run; move and tighten the predicate so it executes only after the positional component is parsed and only when dry-run is not set: after parsing component (variable component) and dryRun (or flags.GetBool("dry-run")), check that name != "" && component == "" && stack == "" && profile == "" && roleArn == "" && identity != "" && dryRun == false, then call executeEKSUpdateKubeconfigDirect(name, region, kubeconfig, alias, identity); keep using flags.GetString("identity") to obtain identity.pkg/auth/cloud/kube/config.go (2)
190-193:⚠️ Potential issue | 🟠 MajorAuthInfo keys aren't unique enough for multiple identities or accounts.
atmos-eks-<name>-<region>collides for the same cluster written under two identities, and for same-named clusters in the same region across accounts. The later merge replaces the earlier exec config, so existing contexts can silently start using the wrong identity. Please key this off a stable cluster identifier plus identity, and centralize the builder so cleanup stays in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/kube/config.go` around lines 190 - 193, The current userName = "atmos-eks-"+info.Name+"-"+info.Region is not unique across identities/accounts; change the auth key generation to use a stable cluster identifier plus the identity (for example combine info.ClusterID or info.UID with the identity ID/ARN) instead of info.Name, and centralize this logic into a single builder function (e.g., BuildKubeAuthUserName or GetKubeAuthKey) used wherever AuthInfo/Context names are created and cleaned up so the same deterministic key is used for both creation and deletion.
150-155:⚠️ Potential issue | 🟡 MinorWrap the cleanup write/remove errors.
These returns bypass the file's static error wrapping, so callers can't reliably match kubeconfig cleanup failures. Reusing
m.writeConfig(existing)for the rewrite path would also keep permission handling consistent with the rest of the manager. As per coding guidelines "Error Handling (MANDATORY): All errors MUST be wrapped using static errors defined in errors/errors.go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/kube/config.go` around lines 150 - 155, The cleanup branch currently returns os.Remove(m.path) or clientcmd.WriteToFile(*existing, m.path) directly, bypassing the manager's static error wrapping and permission handling; change both paths to call m.writeConfig(existing) for the rewrite case (and wrap/remove via a helper that uses the manager's error types) so failures are returned via the same static errors; specifically replace the direct os.Remove(m.path) and clientcmd.WriteToFile(*existing, m.path) returns with calls that use m.writeConfig(existing) (and ensure m.writeConfig handles the remove case and wraps errors with the errors/errors.go static errors).cmd/auth_eks_token_test.go (1)
13-101:⚠️ Potential issue | 🟠 MajorThese tests don't execute the command contract yet.
Everything here inspects static metadata or helper logic, but nothing runs
authEKSTokenCmdand validates the ExecCredential JSON or failure paths. A broken token payload, missing flag wiring, or ambiguous default-identity error would still pass this file. As per coding guidelines "Include integration tests for command flows and test CLI end-to-end when possible with test fixtures" and "Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_eks_token_test.go` around lines 13 - 101, Tests only assert static metadata and helpers; add end-to-end tests that actually execute authEKSTokenCmd and validate JSON ExecCredential output and failure paths. Update the test file to invoke authEKSTokenCmd (use its ExecuteC or RunE entrypoint) with simulated flags (e.g., --cluster-name, --region, --identity) and capture stdout/stderr to assert the produced ExecCredential has execCredentialAPIVersion and correct token fields; add negative tests that trigger resolveDefaultIdentity ambiguity and token generation errors (assert errUtils.ErrEKSTokenGeneration is returned/logged) and use NewTestKit or a mock token provider to simulate success/failure so flag wiring, payload shape, and error handling are exercised. Ensure tests cover missing flags, single vs multiple identities, and JSON schema of the ExecCredential.pkg/auth/integrations/aws/eks.go (1)
148-166:⚠️ Potential issue | 🟠 MajorCleanup still resolves clusters by fuzzy ARN suffix.
When
Aliasis set,Cleanupalready knows the context key and can read the exact cluster ARN from that context. The currentHasSuffix("cluster/"+name)scan matches same-named clusters in other accounts or regions, and map iteration makes the removal target nondeterministic. If you can't resolve an exact match, skip cleanup instead of deleting an arbitrary entry.Also applies to: 212-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/integrations/aws/eks.go` around lines 148 - 166, When cleaning up, don't rely on the fuzzy HasSuffix("cluster/"+name) scan; if e.cluster.Alias is set use that exact kubeconfig context to fetch the cluster key/ARN and pass it to RemoveClusterConfig (modify findClusterARN or the cleanup caller to first check e.cluster.Alias/contextName and read the cluster reference from the kubeconfig contexts map), and if you cannot find an exact match for the context or cluster entry, return nil / skip cleanup instead of falling back to the suffix scan that can delete the wrong cluster; update logic around findClusterARN and the call site that computes contextName/userName so it uses the exact ARN when available and only falls back to a best-effort path if you explicitly decide to keep it (preferably not).cmd/auth_eks_token.go (1)
29-53: 🛠️ Refactor suggestion | 🟠 MajorPlease migrate this command to the registry + standard parser pattern.
Line 227-Line 229 performs direct self-registration and direct Cobra flag wiring, which diverges from the command provider +
flags.NewStandardParser()path used by this repo.As per coding guidelines "Registry Pattern (MANDATORY): New commands MUST use the command registry pattern via
CommandProviderinterface" and "Flag Handling (MANDATORY): Commands MUST useflags.NewStandardParser()for command-specific flags."Also applies to: 226-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_eks_token.go` around lines 29 - 53, The authEKSTokenCmd command is currently being created and wired directly (authEKSTokenCmd and executeAuthEKSTokenCommand) instead of using the repo's registry and standard parser; refactor this by implementing a CommandProvider that returns the command, move flag definitions to use flags.NewStandardParser() for parsing, and register the provider with the central registry instead of self-registering or directly adding Cobra flags; ensure the provider constructs the cobra.Command (using authEKSTokenCmd semantics) but wires flags via the standard parser and uses executeAuthEKSTokenCommand as the RunE handler.
🧹 Nitpick comments (2)
website/src/data/roadmap.js (1)
154-154: Consider documenting why progress moved to 85.This value is valid, but a brief rationale in PR notes/changelog helps avoid confusion when milestones are marked shipped and progress still drops.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/src/data/roadmap.js` at line 154, Add a brief rationale for the changed progress value so reviewers understand why it moved to 85: update the PR description/changelog with a one-line explanation (which milestone shipped or why percentage decreased) and add an inline comment directly above the progress property in roadmap.js (the object containing progress: 85) summarizing that rationale and referencing the affected milestone(s) or ticket IDs.website/docs/cli/commands/aws/aws-eks-update-kubeconfig.mdx (1)
17-22: Add an explicit## Usageheading above the command syntax block.The shell usage snippet is present, but the required Usage section heading is missing.
As per coding guidelines "CLI Command Documentation (MANDATORY) ... Documentation MUST include ... Usage section with shell code block."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/docs/cli/commands/aws/aws-eks-update-kubeconfig.mdx` around lines 17 - 22, Add a "## Usage" heading immediately above the shell code block that shows the command syntax (the block containing "atmos aws eks update-kubeconfig [options]"); update the markdown in aws-eks-update-kubeconfig.mdx so the section reads a second-level heading "## Usage" followed by the existing ```shell code fence to satisfy the CLI Command Documentation requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth_eks_token.go`:
- Around line 113-116: The call to awsCloud.GetToken currently returns the raw
err; update the error handling for token, expiresAt, err :=
awsCloud.GetToken(...) so that any non-nil err is wrapped with the static
sentinel ErrEKSTokenGeneration (e.g., return fmt.Errorf("%w: %v",
errors.ErrEKSTokenGeneration, err)) before returning, ensuring the GetToken call
and ErrEKSTokenGeneration are referenced in the updated return.
In `@pkg/auth/cloud/kube/config.go`:
- Around line 96-122: The switch on updateMode currently treats any
non-"replace" and non-"error" value as "merge", which silently accepts typos;
change the switch to handle "merge" explicitly and have the default branch
return a clear error for unknown updateMode values (include the invalid
updateMode string in the message). Update the switch in the method that calls
m.writeConfig/m.mergeConfig so cases are "replace", "error", and "merge" and the
default returns fmt.Errorf("unknown update mode: %q", updateMode) (or wrap with
an appropriate package error) to fail fast on invalid inputs.
In `@pkg/auth/integrations/aws/eks_test.go`:
- Around line 271-289: The test TestEKSIntegration_Environment_DefaultPath can
touch the real XDG config home because Environment() uses
DefaultKubeconfigPath(); to isolate it, set the XDG_CONFIG_HOME to a temp dir at
the start of the test using t.Setenv("XDG_CONFIG_HOME", t.TempDir()) so
DefaultKubeconfigPath() resolves inside the test temp directory; add this call
at the beginning of TestEKSIntegration_Environment_DefaultPath before calling
integration.Environment().
- Around line 314-348: Add a positive-path unit test that actually writes a
kubeconfig containing the cluster/context/user that EKSIntegration should
manage, calls EKSIntegration.Execute to provision (or directly ensure the
entries exist), then calls EKSIntegration.Cleanup and asserts the kubeconfig no
longer contains the matching context, cluster, or user and that current-context
was updated/cleared appropriately; target the EKSIntegration.Execute and
EKSIntegration.Cleanup methods and use schema.EKSCluster and
schema.KubeconfigSettings with t.TempDir() to create a real kubeconfig file,
asserting idempotency by calling Cleanup twice; convert to table-driven tests to
cover variations (with/without current-context, different aliases) so the
provisioning/removal flow and Execute coverage are exercised.
In `@website/docs/tutorials/eks-kubeconfig-authentication.mdx`:
- Around line 319-325: The doc currently states that multiple kubeconfig paths
are "colon-separated", which is OS-specific and will mislead Windows users;
update the wording in the `eks-kubeconfig-authentication.mdx` section describing
`atmos auth env` / `atmos auth exec` so it explains that `KUBECONFIG` is a
path-list separated by the OS path-list separator (e.g., `:` on Unix, `;` on
Windows) and mention that atmos deduplicates entries, referencing `KUBECONFIG`,
`atmos auth env`, and `atmos auth exec` so readers know the behavior across
platforms.
---
Duplicate comments:
In `@cmd/auth_eks_token_test.go`:
- Around line 13-101: Tests only assert static metadata and helpers; add
end-to-end tests that actually execute authEKSTokenCmd and validate JSON
ExecCredential output and failure paths. Update the test file to invoke
authEKSTokenCmd (use its ExecuteC or RunE entrypoint) with simulated flags
(e.g., --cluster-name, --region, --identity) and capture stdout/stderr to assert
the produced ExecCredential has execCredentialAPIVersion and correct token
fields; add negative tests that trigger resolveDefaultIdentity ambiguity and
token generation errors (assert errUtils.ErrEKSTokenGeneration is
returned/logged) and use NewTestKit or a mock token provider to simulate
success/failure so flag wiring, payload shape, and error handling are exercised.
Ensure tests cover missing flags, single vs multiple identities, and JSON schema
of the ExecCredential.
In `@cmd/auth_eks_token.go`:
- Around line 29-53: The authEKSTokenCmd command is currently being created and
wired directly (authEKSTokenCmd and executeAuthEKSTokenCommand) instead of using
the repo's registry and standard parser; refactor this by implementing a
CommandProvider that returns the command, move flag definitions to use
flags.NewStandardParser() for parsing, and register the provider with the
central registry instead of self-registering or directly adding Cobra flags;
ensure the provider constructs the cobra.Command (using authEKSTokenCmd
semantics) but wires flags via the standard parser and uses
executeAuthEKSTokenCommand as the RunE handler.
In `@internal/exec/aws_eks_update_kubeconfig.go`:
- Around line 85-90: The direct-SDK shortcut currently runs too early and
ignores the parsed positional component and --dry-run; move and tighten the
predicate so it executes only after the positional component is parsed and only
when dry-run is not set: after parsing component (variable component) and dryRun
(or flags.GetBool("dry-run")), check that name != "" && component == "" && stack
== "" && profile == "" && roleArn == "" && identity != "" && dryRun == false,
then call executeEKSUpdateKubeconfigDirect(name, region, kubeconfig, alias,
identity); keep using flags.GetString("identity") to obtain identity.
In `@pkg/auth/cloud/kube/config.go`:
- Around line 190-193: The current userName =
"atmos-eks-"+info.Name+"-"+info.Region is not unique across identities/accounts;
change the auth key generation to use a stable cluster identifier plus the
identity (for example combine info.ClusterID or info.UID with the identity
ID/ARN) instead of info.Name, and centralize this logic into a single builder
function (e.g., BuildKubeAuthUserName or GetKubeAuthKey) used wherever
AuthInfo/Context names are created and cleaned up so the same deterministic key
is used for both creation and deletion.
- Around line 150-155: The cleanup branch currently returns os.Remove(m.path) or
clientcmd.WriteToFile(*existing, m.path) directly, bypassing the manager's
static error wrapping and permission handling; change both paths to call
m.writeConfig(existing) for the rewrite case (and wrap/remove via a helper that
uses the manager's error types) so failures are returned via the same static
errors; specifically replace the direct os.Remove(m.path) and
clientcmd.WriteToFile(*existing, m.path) returns with calls that use
m.writeConfig(existing) (and ensure m.writeConfig handles the remove case and
wraps errors with the errors/errors.go static errors).
In `@pkg/auth/integrations/aws/eks.go`:
- Around line 148-166: When cleaning up, don't rely on the fuzzy
HasSuffix("cluster/"+name) scan; if e.cluster.Alias is set use that exact
kubeconfig context to fetch the cluster key/ARN and pass it to
RemoveClusterConfig (modify findClusterARN or the cleanup caller to first check
e.cluster.Alias/contextName and read the cluster reference from the kubeconfig
contexts map), and if you cannot find an exact match for the context or cluster
entry, return nil / skip cleanup instead of falling back to the suffix scan that
can delete the wrong cluster; update logic around findClusterARN and the call
site that computes contextName/userName so it uses the exact ARN when available
and only falls back to a best-effort path if you explicitly decide to keep it
(preferably not).
---
Nitpick comments:
In `@website/docs/cli/commands/aws/aws-eks-update-kubeconfig.mdx`:
- Around line 17-22: Add a "## Usage" heading immediately above the shell code
block that shows the command syntax (the block containing "atmos aws eks
update-kubeconfig [options]"); update the markdown in
aws-eks-update-kubeconfig.mdx so the section reads a second-level heading "##
Usage" followed by the existing ```shell code fence to satisfy the CLI Command
Documentation requirement.
In `@website/src/data/roadmap.js`:
- Line 154: Add a brief rationale for the changed progress value so reviewers
understand why it moved to 85: update the PR description/changelog with a
one-line explanation (which milestone shipped or why percentage decreased) and
add an inline comment directly above the progress property in roadmap.js (the
object containing progress: 85) summarizing that rationale and referencing the
affected milestone(s) or ticket IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 848398dd-504c-407c-8441-6041f1518790
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
NOTICEcmd/auth_eks_token.gocmd/auth_eks_token_test.gocmd/aws/eks/update_kubeconfig.gocmd/aws/eks/update_kubeconfig_test.goerrors/errors.gogo.modinternal/exec/aws_eks_update_kubeconfig.gopkg/auth/cloud/aws/config.gopkg/auth/cloud/aws/ecr.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/cloud/aws/eks.gopkg/auth/cloud/aws/eks_test.gopkg/auth/cloud/aws/mock_eks_client_test.gopkg/auth/cloud/kube/config.gopkg/auth/cloud/kube/config_test.gopkg/auth/integrations/aws/ecr.gopkg/auth/integrations/aws/eks.gopkg/auth/integrations/aws/eks_test.gopkg/auth/integrations/registry_test.gopkg/auth/integrations/types.gopkg/auth/manager.gopkg/auth/manager_environment.gopkg/auth/manager_environment_test.gopkg/auth/manager_integrations.gopkg/auth/manager_logout.gopkg/schema/schema_auth.gowebsite/blog/2026-03-13-eks-kubeconfig-authentication.mdxwebsite/docs/cli/commands/auth/auth-login.mdxwebsite/docs/cli/commands/auth/eks-token.mdxwebsite/docs/cli/commands/aws/aws-eks-update-kubeconfig.mdxwebsite/docs/cli/configuration/auth/index.mdxwebsite/docs/tutorials/eks-kubeconfig-authentication.mdxwebsite/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/auth/integrations/types.go
- cmd/aws/eks/update_kubeconfig_test.go
- pkg/schema/schema_auth.go
- pkg/auth/integrations/aws/ecr.go
- pkg/auth/manager_environment_test.go
- pkg/auth/cloud/aws/ecr.go
- NOTICE
- pkg/auth/cloud/aws/config.go
- pkg/auth/cloud/aws/ecr_extended_test.go
- pkg/auth/manager.go
Ben (Benbentwo)
force-pushed
the
feature/atmos-157-create-atmos-auth-identity-for-eks-using-aws-go-sdk
branch
from
March 13, 2026 20:14
4998da0 to
24b6a7b
Compare
Contributor
Author
|
CodeRabbit (@coderabbitai) resolve |
Contributor
Author
|
CodeRabbit (@coderabbitai) full review |
Add DI function variables (initCliConfigFn, authenticateForTokenFn, getEKSTokenFn) to token.go for testability, and add tests covering executeTokenCommand and authenticateForToken error/success paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ben (Benbentwo)
force-pushed
the
feature/atmos-157-create-atmos-auth-identity-for-eks-using-aws-go-sdk
branch
from
March 19, 2026 18:36
31b1e2c to
1f7d1f7
Compare
…-for-eks-using-aws-go-sdk
…-for-eks-using-aws-go-sdk
- Replace `atmos auth login` + `kubectl` with `atmos auth exec --identity` and `atmos auth shell --identity` since login does not set KUBECONFIG - Mark `via.identity` as required in integration config table - Add `--identity` flag to all auth login/logout/env commands - Expand CI/CD section without unvalidated code snippets Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move executeEKSUpdateKubeconfigViaIntegration and executeEKSUpdateKubeconfigDirect to cmd/aws/eks/update_kubeconfig_sdk.go - Add integration/identity routing in update_kubeconfig.go RunE - Delete internal/exec/aws_getter.go thin wrappers; yaml_func_aws.go now imports pkg/aws/identity and pkg/aws/organization directly - internal/exec/aws_eks_update_kubeconfig.go retains only the legacy AWS CLI path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
size/xl
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Mar 21, 2026
…-for-eks-using-aws-go-sdk
Andriy Knysh (aknysh)
approved these changes
Mar 21, 2026
Andriy Knysh (aknysh)
deleted the
feature/atmos-157-create-atmos-auth-identity-for-eks-using-aws-go-sdk
branch
March 21, 2026 18:05
|
These changes were released in v1.211.0-rc.2. |
Juan A. (jaguer0)
added a commit
to jaguer0/atmos
that referenced
this pull request
Jul 28, 2026
Add a PRD for native RDS IAM database authentication (`atmos aws rds token`), following the eks-kubeconfig.md and ecr-public-authentication.md precedent (H1 title, no YAML front-matter). Documents the Phase 1 "dumb token" command: two-layer architecture, required --host/--port/--username/--region flags, data.WriteUnmasked output, the ErrRDSTokenGeneration sentinel, an >=85% coverage target, and a phased Implementation Checklist. Companion to the EKS (cloudposse#2149) and ECR Public (cloudposse#2231) auth PRDs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Juan A. (jaguer0)
added a commit
to jaguer0/atmos
that referenced
this pull request
Jul 28, 2026
Add a cloud-layer RDS IAM database-authentication token generator so Atmos can mint short-lived (~15m) SigV4 connection tokens from an Atmos identity without the AWS CLI, mirroring the EKS exec-credential (cloudposse#2149) and ECR Public bearer-token (cloudposse#2231) auth paths. - pkg/auth/cloud/aws/rds.go: GetRDSToken(ctx, creds, endpoint, region, dbUser) builds the token via aws-sdk-go-v2 feature/rds/auth BuildAuthToken, with credentials resolved from the Atmos identity through the existing BuildAWSConfigFromCreds bridge -- never the ambient default chain. Logs metadata only (token_length), never the token value. - errors/errors.go: add static sentinel ErrRDSTokenGeneration beside ErrEKSTokenGeneration; SDK failures wrap it with %w for errors.Is. - go.mod/go.sum: add github.com/aws/aws-sdk-go-v2/feature/rds/auth (go mod tidy); NOTICE: add the Apache-2.0 stanza. Tests: TestGetRDSToken_Success (offline; token contains Action=connect / DBUser / X-Amz-Signature and a future ~15m expiry) and TestGetRDSToken_InvalidCredentials (errors.Is ErrRDSTokenGeneration), written failing-first -- no live RDS call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Juan A. (jaguer0)
added a commit
to jaguer0/atmos
that referenced
this pull request
Jul 28, 2026
Add a PRD for native RDS IAM database authentication (`atmos aws rds token`), following the eks-kubeconfig.md and ecr-public-authentication.md precedent (H1 title, no YAML front-matter). Documents the Phase 1 "dumb token" command: two-layer architecture, required --host/--port/--username/--region flags, data.WriteUnmasked output, the ErrRDSTokenGeneration sentinel, an >=85% coverage target, and a phased Implementation Checklist. Companion to the EKS (cloudposse#2149) and ECR Public (cloudposse#2231) auth PRDs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Juan A. (jaguer0)
added a commit
to jaguer0/atmos
that referenced
this pull request
Jul 28, 2026
Add a cloud-layer RDS IAM database-authentication token generator so Atmos can mint short-lived (~15m) SigV4 connection tokens from an Atmos identity without the AWS CLI, mirroring the EKS exec-credential (cloudposse#2149) and ECR Public bearer-token (cloudposse#2231) auth paths. - pkg/auth/cloud/aws/rds.go: GetRDSToken(ctx, creds, endpoint, region, dbUser) builds the token via aws-sdk-go-v2 feature/rds/auth BuildAuthToken, with credentials resolved from the Atmos identity through the existing BuildAWSConfigFromCreds bridge -- never the ambient default chain. Logs metadata only (token_length), never the token value. - errors/errors.go: add static sentinel ErrRDSTokenGeneration beside ErrEKSTokenGeneration; SDK failures wrap it with %w for errors.Is. - go.mod/go.sum: add github.com/aws/aws-sdk-go-v2/feature/rds/auth (go mod tidy); NOTICE: add the Apache-2.0 stanza. Tests: TestGetRDSToken_Success (offline; token contains Action=connect / DBUser / X-Amz-Signature and a future ~15m expiry) and TestGetRDSToken_InvalidCredentials (errors.Is ErrRDSTokenGeneration), written failing-first -- no live RDS call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
atmos auth loginvia the integration frameworkatmos auth eks-tokencommand: New kubectl exec credential plugin that generates EKS bearer tokens using AWS credentials, eliminating AWS CLI dependencyatmos aws eks update-kubeconfigwith--integrationflag and direct identity-based cluster access without requiring components or stacksatmos auth logout(non-fatal, doesn't block logout)why
atmos auth login, users previously had to manually run AWS CLI commands to generate kubeconfig. This integrates that into the auth flowreferences
docs/prd/eks-kubeconfig.mdcloses #2076
Summary by CodeRabbit
New Features
Documentation
Chores
Tests