Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGetRegion now resolves AWS region with ordered checks: KS_CLOUD_REGION_ENV_VAR, then AWS_REGION, then cluster-name parsing (dot/colon/dash segment strategies), then AWS config region; returns an error if all fail. Tests add env helpers and subtests covering these precedence paths. (46 words) ChangesRegion resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
- Add support for AWS_REGION environment variable (was missing) - Add AWS SDK config auto-detection as fallback - Improve region detection priority: KS_CLOUD_REGION > AWS_REGION > AWS config > cluster name parsing - Improve error messaging to indicate which detection methods were tried - Add tests for AWS_REGION and precedence handling Fixes #1912 Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/eks-region-detection
branch
from
48b49db to
dc3879e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloudsupport/v1/ekssupport_test.go (1)
135-179:⚠️ Potential issue | 🟡 MinorIsolate AWS-related env vars in subtests to avoid test flakiness.
The tests do not save/restore environment variables before modification. IfAWS_REGIONorKS_CLOUD_REGIONare already set in the CI environment, the "AWS_REGION environment variable is checked" subtest (and table-driven loop with empty envRegion cases) can be overridden, causing flaky test behavior. Implement a save/restore helper to ensure deterministic isolation per subtest.🔧 Suggested fix (env save/restore helper)
func TestGetRegion(t *testing.T) { + setEnv := func(t *testing.T, key, val string) func() { + t.Helper() + old, had := os.LookupEnv(key) + if val == "" { + if err := os.Unsetenv(key); err != nil { + t.Fatalf("unset %s: %v", key, err) + } + } else { + if err := os.Setenv(key, val); err != nil { + t.Fatalf("set %s: %v", key, err) + } + } + return func() { + if had { + _ = os.Setenv(key, old) + } else { + _ = os.Unsetenv(key) + } + } + } + tests := []struct { name string cluster string @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + restoreAWS := setEnv(t, "AWS_REGION", "") + restoreKS := setEnv(t, KS_CLOUD_REGION_ENV_VAR, "") + defer restoreAWS() + defer restoreKS() if tt.envRegion != "" { - os.Setenv(KS_CLOUD_REGION_ENV_VAR, tt.envRegion) - defer os.Unsetenv(KS_CLOUD_REGION_ENV_VAR) + if err := os.Setenv(KS_CLOUD_REGION_ENV_VAR, tt.envRegion); err != nil { + t.Fatalf("set %s: %v", KS_CLOUD_REGION_ENV_VAR, err) + } } @@ t.Run("AWS_REGION environment variable is checked", func(t *testing.T) { awsRegion := "ap-southeast-2" - os.Setenv("AWS_REGION", awsRegion) - defer os.Unsetenv("AWS_REGION") + restoreKS := setEnv(t, KS_CLOUD_REGION_ENV_VAR, "") + restoreAWS := setEnv(t, "AWS_REGION", awsRegion) + defer restoreKS() + defer restoreAWS() @@ t.Run("KS_CLOUD_REGION takes precedence over AWS_REGION", func(t *testing.T) { ksRegion := "us-west-2" awsRegion := "eu-west-1" - os.Setenv(KS_CLOUD_REGION_ENV_VAR, ksRegion) - os.Setenv("AWS_REGION", awsRegion) - defer os.Unsetenv(KS_CLOUD_REGION_ENV_VAR) - defer os.Unsetenv("AWS_REGION") + restoreKS := setEnv(t, KS_CLOUD_REGION_ENV_VAR, ksRegion) + restoreAWS := setEnv(t, "AWS_REGION", awsRegion) + defer restoreKS() + defer restoreAWS()
|
Summary:
|
4 similar comments
|
Summary:
|
|
Summary:
|
|
Summary:
|
|
Summary:
|
| return region, nil | ||
| } | ||
|
|
||
| region, present = os.LookupEnv("AWS_REGION") |
matthyx
force-pushed
the
fix/eks-region-detection
branch
from
82364fe to
7f9f15a
Compare
|
Summary:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloudsupport/v1/ekssupport_test.go (1)
174-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading test name and dead
expectedRegion.This case asserts
expectErr: true(correct —"cluster:us-west-2:eks"has only 3 colon-separated segments, the dash branch needs ≥6, andisolateAWSRegionSourcesempties AWS config), so the name "Region is extracted from cluster name with ':' separator" and theexpectedRegion: "us-west-2"field are both misleading and never exercised. Either rename it to reflect that <4-segment colon strings are rejected, or change the input to a real 4+ colon ARN that should succeed and dropexpectErr.💡 Two ways to make the case match its intent
- { - name: "Region is extracted from cluster name with ':' separator", - cluster: "cluster:us-west-2:eks", - expectedRegion: "us-west-2", - expectErr: true, - }, + { + name: "Colon-separated cluster name with <4 segments fails", + cluster: "cluster:us-west-2:eks", + expectErr: true, + },Or, to actually cover the colon-extraction branch:
- { - name: "Region is extracted from cluster name with ':' separator", - cluster: "cluster:us-west-2:eks", - expectedRegion: "us-west-2", - expectErr: true, - }, + { + name: "Region is extracted from colon-separated ARN", + cluster: "arn:aws:eks:us-west-2:123456789012:cluster/foo", + expectedRegion: "us-west-2", + expectErr: false, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudsupport/v1/ekssupport_test.go` around lines 174 - 179, The test case in ekssupport_test.go with name "Region is extracted from cluster name with ':' separator" is misleading because expectErr is true while expectedRegion is set to "us-west-2" and never used; either make the case actually exercise the colon-extraction branch by changing cluster to a valid ARN-like string with ≥4 colon segments (so expectErr=false and expectedRegion="us-west-2"), or rename the test to indicate it expects rejection for <4-segment colon strings and remove/clear expectedRegion; update the test case fields (name, cluster, expectedRegion, expectErr) accordingly and ensure behavior still matches isolateAWSRegionSources/dash-branch expectations.
🧹 Nitpick comments (1)
cloudsupport/v1/ekssupport.go (1)
121-140: ⚡ Quick winExtract
"AWS_REGION"as a constant and validate parsed segments are non-empty.Two improvements to the new resolution path:
Hardcoded
"AWS_REGION"—KS_CLOUD_REGION_ENV_VARis already a constant, but"AWS_REGION"at line 139 is hardcoded inline. Define a constant to match the pattern and keep the env-var surface discoverable.No validation that parsed segments are non-empty. The dot, colon, and dash-split branches return segments without checking they're non-empty strings. For inputs like
"cluster.", the dot split returnssplittedClusterContext[1] == ""withnilerror. Similarly, any cluster name with ≥6 dash-separated parts returnsparts[3:6]joined without validation. Adding&& splittedClusterContext[n] != ""checks before returning prevents silent empty-region results and lets the AWS-config fallback take over.♻️ Suggested adjustment
+const awsRegionEnvVar = "AWS_REGION" + // GetRegion returns the region in which eks cluster is running. func (eksSupport *EKSSupport) GetRegion(cluster string) (string, error) { region, present := os.LookupEnv(KS_CLOUD_REGION_ENV_VAR) if present && region != "" { return region, nil } splittedClusterContext := strings.Split(cluster, ".") - if len(splittedClusterContext) >= 2 { + if len(splittedClusterContext) >= 2 && splittedClusterContext[1] != "" { return splittedClusterContext[1], nil } splittedClusterContext = strings.Split(cluster, ":") - if len(splittedClusterContext) >= 4 { + if len(splittedClusterContext) >= 4 && splittedClusterContext[3] != "" { return splittedClusterContext[3], nil } splittedClusterContext = strings.Split(cluster, "-") - if len(splittedClusterContext) >= 6 { + if len(splittedClusterContext) >= 6 && splittedClusterContext[3] != "" && splittedClusterContext[4] != "" && splittedClusterContext[5] != "" { return strings.Join(splittedClusterContext[3:6], "-"), nil } - region, present = os.LookupEnv("AWS_REGION") + region, present = os.LookupEnv(awsRegionEnvVar) if present && region != "" { return region, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudsupport/v1/ekssupport.go` around lines 121 - 140, Extract the hardcoded "AWS_REGION" into a new constant (following the KS_CLOUD_REGION_ENV_VAR pattern) and update the lookup to use that constant; additionally, in each parsing branch that returns values from splittedClusterContext (dot, colon, and dash branches) add explicit non-empty checks for the target segments (e.g., ensure splittedClusterContext[1] != "" for the dot case, splittedClusterContext[3] != "" for the colon case, and each of splittedClusterContext[3:6] are non-empty before joining) so you only return a valid non-empty region and otherwise fall through to the existing AWS/config fallback; reference the existing variables region, KS_CLOUD_REGION_ENV_VAR, and splittedClusterContext when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cloudsupport/v1/ekssupport_test.go`:
- Around line 174-179: The test case in ekssupport_test.go with name "Region is
extracted from cluster name with ':' separator" is misleading because expectErr
is true while expectedRegion is set to "us-west-2" and never used; either make
the case actually exercise the colon-extraction branch by changing cluster to a
valid ARN-like string with ≥4 colon segments (so expectErr=false and
expectedRegion="us-west-2"), or rename the test to indicate it expects rejection
for <4-segment colon strings and remove/clear expectedRegion; update the test
case fields (name, cluster, expectedRegion, expectErr) accordingly and ensure
behavior still matches isolateAWSRegionSources/dash-branch expectations.
---
Nitpick comments:
In `@cloudsupport/v1/ekssupport.go`:
- Around line 121-140: Extract the hardcoded "AWS_REGION" into a new constant
(following the KS_CLOUD_REGION_ENV_VAR pattern) and update the lookup to use
that constant; additionally, in each parsing branch that returns values from
splittedClusterContext (dot, colon, and dash branches) add explicit non-empty
checks for the target segments (e.g., ensure splittedClusterContext[1] != "" for
the dot case, splittedClusterContext[3] != "" for the colon case, and each of
splittedClusterContext[3:6] are non-empty before joining) so you only return a
valid non-empty region and otherwise fall through to the existing AWS/config
fallback; reference the existing variables region, KS_CLOUD_REGION_ENV_VAR, and
splittedClusterContext when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 676124a2-fb71-48c9-8e64-1ef6a9052c7d
📒 Files selected for processing (2)
cloudsupport/v1/ekssupport.gocloudsupport/v1/ekssupport_test.go
|
Summary:
|
1 similar comment
|
Summary:
|
matthyx
force-pushed
the
fix/eks-region-detection
branch
from
7f9f15a to
5ef6e74
Compare
|
Summary:
|
matthyx
force-pushed
the
fix/eks-region-detection
branch
from
5ef6e74 to
a3af179
Compare
Keep cluster parsing ahead of AWS default config and isolate the EKS region tests from ambient AWS settings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/eks-region-detection
branch
from
a3af179 to
74e1bbd
Compare
|
Summary:
|
1 similar comment
|
Summary:
|
1 participant
Summary
Fixes AWS region detection for EKS clusters, resolving the "failed to get region" error that occurs even when
AWS_REGIONandAWS_PROFILEenvironment variables are properly configured.Changes
AWS_REGIONenvironment variable support - This was previously missing, which was the root cause of the reported issueKS_CLOUD_REGIONenvironment variable (highest priority)AWS_REGIONenvironment variable (newly added)AWS_REGIONusage and precedence handlingRoot Cause
The
GetRegionfunction incloudsupport/v1/ekssupport.goonly checked forKS_CLOUD_REGIONbut didn't check for the standardAWS_REGIONenvironment variable or attempt AWS SDK auto-detection. This caused failures for users with standard AWS configurations.Testing
AWS_REGIONis properly detectedKS_CLOUD_REGIONtakes precedence overAWS_REGIONRelated to kubescape/kubescape#1912
Summary by CodeRabbit
Bug Fixes
Tests