Skip to content

fix(aws): fail instead of falling back to 0.0.0.0/0 for security group #650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ArangoGutierrez merged 1 commit into from
Feb 13, 2026
Merged

fix(aws): fail instead of falling back to 0.0.0.0/0 for security group #650

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions pkg/provider/aws/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,15 +352,14 @@ func (p *Provider) createSecurityGroup(cache *AWS) error {
ipRanges := []types.IpRange{}

// First lookup for the IP address of the user
cidr := "0.0.0.0/0"
if publicIP, err := utils.GetIPAddress(); err == nil {
cidr = publicIP
p.log.Info("Using detected public IP for security group: %s", cidr)
} else {
p.log.Warning("Could not detect public IP, using 0.0.0.0/0: %v", err)
publicIP, err := utils.GetIPAddress()
if err != nil {
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing cancelLoading call before returning error. Following the codebase pattern (seen throughout this file and in pkg/provider/aws/cluster.go:222, pkg/provider/aws/nlb.go:68, etc.), errors that occur during a loading operation should call cancelLoading(logger.ErrLoadingFailed) before returning. The loading spinner was started at line 326 but is not properly cancelled when IP detection fails.

Suggested change
if err != nil {
if err != nil {
cancelLoading(logger.ErrLoadingFailed)

Copilot uses AI. Check for mistakes.
return fmt.Errorf("could not detect public IP for security group (set ingressCidr explicitly): %w", err)
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message references "ingressCidr" but the actual field name in the API is "ingressIpRanges" (as seen in api/holodeck/v1alpha1/types.go line 108). This inconsistency could confuse users. The error message should use the correct field name from the API spec.

Suggested change
return fmt.Errorf("could not detect public IP for security group (set ingressCidr explicitly): %w", err)
return fmt.Errorf("could not detect public IP for security group (set ingressIpRanges explicitly): %w", err)

Copilot uses AI. Check for mistakes.
Comment on lines +355 to +357
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description references "Audit Finding #34 (LOW)" suggesting a security audit, but the linked PR #34 is about bumping golang.org/x/net dependency, not a security audit of this code. This appears to be an error in the PR description - either the wrong issue/PR was referenced, or the audit finding number is incorrect.

Copilot uses AI. Check for mistakes.
}
Comment on lines +355 to 358
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks a valid use case where users provide explicit ingressIpRanges. According to the documentation (docs/guides/ip-detection.md and docs/commands/create.md), when users provide ingressIpRanges, the detected IP should be added in addition to those ranges. However, if IP detection fails (e.g., due to corporate firewall or proxy), the code will now error out even when the user has provided valid ingressIpRanges.

The fix should only fail when BOTH conditions are true: (1) IP detection fails AND (2) ingressIpRanges is empty. Otherwise, the code should continue with the user-provided ranges.

Suggested logic:

  • Try to detect public IP
  • If detection succeeds, add it to ipRanges
  • If detection fails AND len(p.Spec.IngressIpRanges) == 0, return error
  • If detection fails BUT user provided ingressIpRanges, log a warning and continue

Copilot uses AI. Check for mistakes.
cidr := publicIP
p.log.Info("Using detected public IP for security group: %s", cidr)

// Add the auto-detected IP or fallback to the map and list
// Add the auto-detected IP to the map and list
ipRangeMap[cidr] = true
ipRanges = append(ipRanges, types.IpRange{
CidrIp: &cidr,
Expand Down
Loading