Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
The diff you're trying to view is too large. We only load the first 3000 changed files.
26 changes: 26 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# IDE
.idea

# Binaries for programs and plugins
*.exe
*.exe~
*.dll
*.so
*.dylib

# Test binary, built with `go test -c`
*.test

# Output of the go coverage tool
*.out

# Directories with variable content
bin/*
config/*

# golangci-lint cache
.cache/*

# Git
.git
.gitignore
4 changes: 3 additions & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.25-openshift-4.22 AS builder
WORKDIR /go/src/github.com/openshift/cluster-cloud-controller-manager-operator
COPY . .
RUN make build
RUN make build &&\
gzip /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/cloud-controller-manager-aws-tests-ext

FROM registry.ci.openshift.org/ocp/4.22:base-rhel9
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/cluster-controller-manager-operator .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/config-sync-controllers .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/azure-config-credentials-injector .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/manifests manifests
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/cloud-controller-manager-aws-tests-ext.gz /usr/bin/cloud-controller-manager-aws-tests-ext.gz

LABEL io.openshift.release.operator true
10 changes: 9 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ unit:
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path --bin-dir $(PROJECT_DIR)/bin --index https://raw.githubusercontent.com/openshift/api/master/envtest-releases.yaml)" ./hack/ci-test.sh

# Build operator binaries
build: operator config-sync-controllers azure-config-credentials-injector
build: operator config-sync-controllers azure-config-credentials-injector cloud-controller-manager-aws-tests-ext

operator:
go build -o bin/cluster-controller-manager-operator cmd/cluster-cloud-controller-manager-operator/main.go
Expand All @@ -48,6 +48,14 @@ config-sync-controllers:
azure-config-credentials-injector:
go build -o bin/azure-config-credentials-injector cmd/azure-config-credentials-injector/main.go

cloud-controller-manager-aws-tests-ext:
cd cmd/cloud-controller-manager-aws-tests-ext && \
GO111MODULE=on CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) GOPROXY=$(GOPROXY) go build \
-trimpath \
-ldflags="$(LDFLAGS)" \
-o=../../bin/cloud-controller-manager-aws-tests-ext .


# Run against the configured Kubernetes cluster in ~/.kube/config
run: verify manifests
go run cmd/cluster-cloud-controller-manager-operator/main.go
Expand Down
196 changes: 196 additions & 0 deletions cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
package e2e

import (
"context"
"fmt"
"strings"

"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
ec2 "github.com/aws/aws-sdk-go-v2/service/ec2"
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
elbv2 "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2"
elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/kubernetes/test/e2e/framework"
)

// AWS helpers

// getAWSClientLoadBalancer creates an AWS ELBv2 client using default credentials configured in the environment.
func getAWSClientLoadBalancer(ctx context.Context) (*elbv2.Client, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like this should be createAWSClientLoadBalancer, is there a reason we don't use that?

cfg, err := config.LoadDefaultConfig(ctx)
if err != nil {
return nil, fmt.Errorf("unable to load AWS config: %v", err)
}
return elbv2.NewFromConfig(cfg), nil
}

// getAWSLoadBalancerFromDNSName finds a load balancer by DNS name using the AWS ELBv2 client.
func getAWSLoadBalancerFromDNSName(ctx context.Context, elbClient *elbv2.Client, lbDNSName string) (*elbv2types.LoadBalancer, error) {
var foundLB *elbv2types.LoadBalancer
framework.Logf("describing load balancers with DNS %s", lbDNSName)

paginator := elbv2.NewDescribeLoadBalancersPaginator(elbClient, &elbv2.DescribeLoadBalancersInput{})
for paginator.HasMorePages() {
page, err := paginator.NextPage(ctx)
if err != nil {
return nil, fmt.Errorf("failed to describe load balancers: %v", err)
}

framework.Logf("found %d load balancers in page", len(page.LoadBalancers))
// Search for the load balancer with matching DNS name in this page
for i := range page.LoadBalancers {
if aws.ToString(page.LoadBalancers[i].DNSName) == lbDNSName {
foundLB = &page.LoadBalancers[i]
framework.Logf("found load balancer with DNS %s", aws.ToString(foundLB.DNSName))
break
}
}
if foundLB != nil {
break
}
}

if foundLB == nil {
return nil, fmt.Errorf("no load balancer found with DNS name: %s", lbDNSName)
}

return foundLB, nil
}

// isFeatureEnabled checks if an OpenShift feature gate is enabled by querying the
// FeatureGate resource named "cluster" using the typed OpenShift config API.
//
// This function uses the official OpenShift config/v1 API types for type-safe
// access to feature gate information, providing better performance and maintainability
// compared to dynamic client approaches.
//
// Parameters:
// - ctx: Context for the API call
// - featureName: Name of the feature gate to check (e.g., "AWSServiceLBNetworkSecurityGroup")
//
// Returns:
// - bool: true if the feature is enabled, false if disabled or not found
// - error: error if the API call fails
//
// Note: For HyperShift clusters, this checks the management cluster's feature gates.
// To check hosted cluster feature gates, use the hosted cluster's kubeconfig.
func isFeatureEnabled(ctx context.Context, featureName string) (bool, error) {
// Get the REST config
restConfig, err := framework.LoadConfig()
if err != nil {
return false, fmt.Errorf("failed to load kubeconfig: %v", err)
}

// Create typed config client (more efficient than dynamic client)
configClient, err := configv1client.NewForConfig(restConfig)
if err != nil {
return false, fmt.Errorf("failed to create config client: %v", err)
}

// Get the FeatureGate resource using typed API
featureGate, err := configClient.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{})
if err != nil {
return false, fmt.Errorf("failed to get FeatureGate 'cluster': %v", err)
}

// Iterate through the feature gates status (typed structs)
for _, fg := range featureGate.Status.FeatureGates {
// Check enabled list
for _, enabled := range fg.Enabled {
if string(enabled.Name) == featureName {
framework.Logf("Feature %s is enabled (version %s)", featureName, fg.Version)
return true, nil
}
}

// Check disabled list
for _, disabled := range fg.Disabled {
if string(disabled.Name) == featureName {
framework.Logf("Feature %s is disabled (version %s)", featureName, fg.Version)
return false, nil
}
}
}

// Feature not found in either list
framework.Logf("Feature %s not found in FeatureGate status", featureName)
return false, nil
}

// getAWSClientEC2 creates an AWS EC2 client using default credentials configured in the environment.
func getAWSClientEC2(ctx context.Context) (*ec2.Client, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

similar question here about get vs create.

cfg, err := config.LoadDefaultConfig(ctx)
if err != nil {
return nil, fmt.Errorf("unable to load AWS config: %v", err)
}
return ec2.NewFromConfig(cfg), nil
}

// getAWSSecurityGroup retrieves a security group by ID using the AWS EC2 client.
func getAWSSecurityGroup(ctx context.Context, ec2Client *ec2.Client, sgID string) (*ec2types.SecurityGroup, error) {
framework.Logf("describing security group %s", sgID)
Copy link
Contributor

Choose a reason for hiding this comment

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

how chatty is this log?

input := &ec2.DescribeSecurityGroupsInput{
GroupIds: []string{sgID},
}

result, err := ec2Client.DescribeSecurityGroups(ctx, input)
if err != nil {
return nil, fmt.Errorf("failed to describe security group %s: %v", sgID, err)
}

if len(result.SecurityGroups) == 0 {
return nil, fmt.Errorf("security group %s not found", sgID)
}

return &result.SecurityGroups[0], nil
}

// getAWSSecurityGroupRules gets the security group rules for the given security group IDs.
func getAWSSecurityGroupRules(ctx context.Context, ec2Client *ec2.Client, groups []string) ([]ec2types.IpPermission, error) {
rules := []ec2types.IpPermission{}
for _, group := range groups {
sg, err := getAWSSecurityGroup(ctx, ec2Client, group)
if err != nil {
return nil, err
}
rules = append(rules, sg.IpPermissions...)
}
return rules, nil
}

// securityGroupExists checks if a security group exists by ID.
// Returns true if it exists, false if it doesn't exist or was deleted.
func securityGroupExists(ctx context.Context, ec2Client *ec2.Client, sgID string) (bool, error) {
framework.Logf("checking if security group %s exists", sgID)
input := &ec2.DescribeSecurityGroupsInput{
GroupIds: []string{sgID},
}

_, err := ec2Client.DescribeSecurityGroups(ctx, input)
if err != nil {
// Check if it's a "not found" error
if ec2IsNotFoundError(err) {
framework.Logf("security group %s does not exist", sgID)
return false, nil
}
return false, fmt.Errorf("failed to check security group %s: %v", sgID, err)
}

framework.Logf("security group %s exists", sgID)
return true, nil
}

// ec2IsNotFoundError checks if an error is an EC2 "not found" error.
func ec2IsNotFoundError(err error) bool {
if err == nil {
return false
}
// Check for common EC2 not found error messages
errMsg := err.Error()
return strings.Contains(errMsg, "InvalidGroup.NotFound") ||
strings.Contains(errMsg, "InvalidGroupId.NotFound") ||
strings.Contains(errMsg, "InvalidGroup.Malformed")
Comment on lines +187 to +195
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

head -200 cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go | tail -50

Repository: openshift/cluster-cloud-controller-manager-operator

Length of output: 1737


🏁 Script executed:

# First, let's see the exact function and surrounding context
sed -n '180,200p' cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go

Repository: openshift/cluster-cloud-controller-manager-operator

Length of output: 562


🏁 Script executed:

# Also check the imports at the top of the file
head -50 cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go

Repository: openshift/cluster-cloud-controller-manager-operator

Length of output: 2027


🏁 Script executed:

# Check if smithy-go is in the project dependencies
grep -r "smithy-go" . --include="*.go" --include="go.mod" | head -20

Repository: openshift/cluster-cloud-controller-manager-operator

Length of output: 3526


🏁 Script executed:

# Search for other error handling patterns in the codebase
rg "errors\.As" --type go | head -20

Repository: openshift/cluster-cloud-controller-manager-operator

Length of output: 2473


🏁 Script executed:

# Look for AWS error handling examples in the codebase
rg "ErrorCode()" --type go | head -20

Repository: openshift/cluster-cloud-controller-manager-operator

Length of output: 2770


🌐 Web query:

AWS SDK v2 Go error handling InvalidGroup.NotFound InvalidGroup.Malformed smithy APIError

💡 Result:

In AWS SDK for Go v2, EC2 service errors like InvalidGroup.NotFound / InvalidGroup.Malformed are typically surfaced as an error chain (often wrapped in *smithy.OperationError). The most robust way to handle them is to unwrap with errors.As and inspect the Smithy APIError interface (code/message/fault). (docs.aws.amazon.com)

import (
	"errors"
	"fmt"

	"github.com/aws/smithy-go"
)

func isInvalidGroup(err error) (code string, ok bool) {
	var apiErr smithy.APIError
	if errors.As(err, &apiErr) {
		switch apiErr.ErrorCode() { // e.g. "InvalidGroup.NotFound", "InvalidGroup.Malformed"
		case "InvalidGroup.NotFound", "InvalidGroup.Malformed":
			return apiErr.ErrorCode(), true
		}
	}
	return "", false
}

func example(err error) {
	if code, ok := isInvalidGroup(err); ok {
		// handle specifically (wrong SG id format, wrong region/VPC, deleted SG, eventual consistency, etc.)
		fmt.Println("security group error:", code)
		return
	}
	// generic handling
	fmt.Println("other error:", err)
}

Notes:

  • smithy.APIError is the generic interface implemented by SDK service/protocol errors and exposes ErrorCode() / ErrorMessage() (and ErrorFault()). (pkg.go.dev)
  • InvalidGroup.NotFound / InvalidGroup.Malformed are EC2 API error codes you should compare as strings (they’re documented by EC2; they aren’t always generated as dedicated Go exception types for the Query protocol). (docs.aws.amazon.com)

Citations:


Use typed AWS SDK v2 error handling instead of string matching; InvalidGroup.Malformed indicates bad input, not a deleted resource.

The current implementation treats InvalidGroup.Malformed as equivalent to "not found", which is semantically incorrect. Malformed IDs represent input validation failures, not resource deletions. This causes securityGroupExists() to return (false, nil) for invalid IDs, masking input errors and producing false-positive cleanup checks.

Use smithy.APIError with errors.As() to properly handle AWS SDK v2 errors and only check for actual "not found" codes:

Proposed fix
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
@@
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
 	elbv2 "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2"
 	elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
+	"github.com/aws/smithy-go"
@@
 func ec2IsNotFoundError(err error) bool {
 	if err == nil {
 		return false
 	}
-	// Check for common EC2 not found error messages
-	errMsg := err.Error()
-	return strings.Contains(errMsg, "InvalidGroup.NotFound") ||
-		strings.Contains(errMsg, "InvalidGroupId.NotFound") ||
-		strings.Contains(errMsg, "InvalidGroup.Malformed")
+	var apiErr smithy.APIError
+	if errors.As(err, &apiErr) {
+		code := apiErr.ErrorCode()
+		return code == "InvalidGroup.NotFound" || code == "InvalidGroupId.NotFound"
+	}
+	return false
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func ec2IsNotFoundError(err error) bool {
if err == nil {
return false
}
// Check for common EC2 not found error messages
errMsg := err.Error()
return strings.Contains(errMsg, "InvalidGroup.NotFound") ||
strings.Contains(errMsg, "InvalidGroupId.NotFound") ||
strings.Contains(errMsg, "InvalidGroup.Malformed")
func ec2IsNotFoundError(err error) bool {
if err == nil {
return false
}
var apiErr smithy.APIError
if errors.As(err, &apiErr) {
code := apiErr.ErrorCode()
return code == "InvalidGroup.NotFound" || code == "InvalidGroupId.NotFound"
}
return false
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go` around lines 187 -
195, The ec2IsNotFoundError helper incorrectly treats "InvalidGroup.Malformed"
as a not-found case; change it to use typed AWS SDK v2 error handling by casting
the error to smithy.APIError via errors.As and inspect apiErr.ErrorCode(),
returning true only for "InvalidGroup.NotFound" and "InvalidGroupId.NotFound"
and false otherwise (do not treat "InvalidGroup.Malformed" as not-found so
callers like securityGroupExists() can surface it as an input error). Ensure the
function still returns false for nil errors and returns false for non-API
errors.

}
Loading