Copilot commented on code in PR #90:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/90#discussion_r2618499162
##########
cloudstack_loadbalancer_test.go:
##########
@@ -764,3 +765,2932 @@ func TestCheckLoadBalancerRule(t *testing.T) {
}
})
}
+
+func TestRuleToString(t *testing.T) {
+ tests := []struct {
+ name string
+ rule *cloudstack.FirewallRule
+ want string
+ }{
+ {
+ name: "TCP rule",
+ rule: &cloudstack.FirewallRule{
+ Protocol: "tcp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.1",
+ Startport: 80,
+ Endport: 80,
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.1:[80-80] (tcp)}",
+ },
+ {
+ name: "UDP rule",
+ rule: &cloudstack.FirewallRule{
+ Protocol: "udp",
+ Cidrlist: "192.168.0.0/16",
+ Ipaddress: "203.0.113.2",
+ Startport: 53,
+ Endport: 53,
+ },
+ want: "{[192.168.0.0/16] -> 203.0.113.2:[53-53] (udp)}",
+ },
+ {
+ name: "TCP rule with port range",
+ rule: &cloudstack.FirewallRule{
+ Protocol: "tcp",
+ Cidrlist: "0.0.0.0/0",
+ Ipaddress: "203.0.113.3",
+ Startport: 8000,
+ Endport: 8999,
+ },
+ want: "{[0.0.0.0/0] -> 203.0.113.3:[8000-8999] (tcp)}",
+ },
+ {
+ name: "ICMP rule",
+ rule: &cloudstack.FirewallRule{
+ Protocol: "icmp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.4",
+ Icmptype: 8,
+ Icmpcode: 0,
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.4 [8,0] (icmp)}",
+ },
+ {
+ name: "unknown protocol",
+ rule: &cloudstack.FirewallRule{
+ Protocol: "gre",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.6",
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.6 (gre)}",
+ },
+ {
+ name: "nil rule",
+ rule: nil,
+ want: "nil",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got := ruleToString(tt.rule)
+ if got != tt.want {
+ t.Errorf("ruleToString() = %q, want %q", got,
tt.want)
+ }
+ })
+ }
+}
+
+func TestRulesToString(t *testing.T) {
+ tests := []struct {
+ name string
+ rules []*cloudstack.FirewallRule
+ want string
+ }{
+ {
+ name: "empty list",
+ rules: []*cloudstack.FirewallRule{},
+ want: "",
+ },
+ {
+ name: "single rule",
+ rules: []*cloudstack.FirewallRule{
+ {
+ Protocol: "tcp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.1",
+ Startport: 80,
+ Endport: 80,
+ },
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.1:[80-80] (tcp)}",
+ },
+ {
+ name: "multiple rules",
+ rules: []*cloudstack.FirewallRule{
+ {
+ Protocol: "tcp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.1",
+ Startport: 80,
+ Endport: 80,
+ },
+ {
+ Protocol: "udp",
+ Cidrlist: "192.168.0.0/16",
+ Ipaddress: "203.0.113.2",
+ Startport: 53,
+ Endport: 53,
+ },
+ {
+ Protocol: "icmp",
+ Cidrlist: "0.0.0.0/0",
+ Ipaddress: "203.0.113.3",
+ Icmptype: 8,
+ Icmpcode: 0,
+ },
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.1:[80-80] (tcp)},
{[192.168.0.0/16] -> 203.0.113.2:[53-53] (udp)}, {[0.0.0.0/0] -> 203.0.113.3
[8,0] (icmp)}",
+ },
+ {
+ name: "rules with nil rule",
+ rules: []*cloudstack.FirewallRule{
+ {
+ Protocol: "tcp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.1",
+ Startport: 80,
+ Endport: 80,
+ },
+ nil,
+ {
+ Protocol: "udp",
+ Cidrlist: "192.168.0.0/16",
+ Ipaddress: "203.0.113.2",
+ Startport: 53,
+ Endport: 53,
+ },
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.1:[80-80] (tcp)},
nil, {[192.168.0.0/16] -> 203.0.113.2:[53-53] (udp)}",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got := rulesToString(tt.rules)
+ if got != tt.want {
+ t.Errorf("rulesToString() = %q, want %q", got,
tt.want)
+ }
+ })
+ }
+}
+
+func TestRulesMapToString(t *testing.T) {
+ tests := []struct {
+ name string
+ rules map[*cloudstack.FirewallRule]bool
+ want string
+ }{
+ {
+ name: "empty map",
+ rules: map[*cloudstack.FirewallRule]bool{},
+ want: "",
+ },
+ {
+ name: "single rule",
+ rules: map[*cloudstack.FirewallRule]bool{
+ {
+ Protocol: "tcp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.1",
+ Startport: 80,
+ Endport: 80,
+ }: true,
+ },
+ want: "{[10.0.0.0/8] -> 203.0.113.1:[80-80] (tcp)}",
+ },
+ {
+ name: "multiple rules",
+ rules: map[*cloudstack.FirewallRule]bool{
+ {
+ Protocol: "tcp",
+ Cidrlist: "10.0.0.0/8",
+ Ipaddress: "203.0.113.1",
+ Startport: 80,
+ Endport: 80,
+ }: true,
+ {
+ Protocol: "udp",
+ Cidrlist: "192.168.0.0/16",
+ Ipaddress: "203.0.113.2",
+ Startport: 53,
+ Endport: 53,
+ }: false,
+ {
+ Protocol: "icmp",
+ Cidrlist: "0.0.0.0/0",
+ Ipaddress: "203.0.113.3",
+ Icmptype: 8,
+ Icmpcode: 0,
+ }: true,
+ },
+ // Note: Map iteration order is non-deterministic, so
we need to check
+ // that all rules are present, not the exact order
+ want: "", // We'll check this differently
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got := rulesMapToString(tt.rules)
+
+ if tt.want == "" {
+ // For maps, we can't predict order, so check
that all rules are present
+ if len(tt.rules) == 0 {
+ if got != "" {
+ t.Errorf("rulesMapToString() =
%q, want empty string", got)
+ }
+ return
+ }
+
+ // Check that all rules are present in the
output
+ expectedRules := make([]string, 0,
len(tt.rules))
+ for rule := range tt.rules {
+ expectedRules = append(expectedRules,
ruleToString(rule))
+ }
+
+ // Split the output and check each rule is
present
+ if got != "" {
+ parts := strings.Split(got, ", ")
+ if len(parts) != len(expectedRules) {
+ t.Errorf("rulesMapToString()
returned %d rules, want %d", len(parts), len(expectedRules))
+ return
+ }
+
+ // Check that all expected rules are in
the output
+ for _, expectedRule := range
expectedRules {
+ found := false
+ for _, part := range parts {
+ if part == expectedRule
{
+ found = true
+ break
+ }
+ }
+ if !found {
+
t.Errorf("rulesMapToString() missing rule %q in output %q", expectedRule, got)
+ }
+ }
+ } else if len(expectedRules) > 0 {
+ t.Errorf("rulesMapToString() = empty
string, want rules to be present")
+ }
+ } else {
+ if got != tt.want {
+ t.Errorf("rulesMapToString() = %q, want
%q", got, tt.want)
+ }
+ }
+ })
+ }
+}
+
+func TestGetPublicIPAddress(t *testing.T) {
+ t.Run("IP found and allocated", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ listParams := &cloudstack.ListPublicIpAddressesParams{}
+ resp := &cloudstack.ListPublicIpAddressesResponse{
+ Count: 1,
+ PublicIpAddresses: []*cloudstack.PublicIpAddress{
+ {
+ Id: "ip-123",
+ Ipaddress: "203.0.113.1",
+ Allocated: "2023-01-01T00:00:00+0000",
+ },
+ },
+ }
+
+ gomock.InOrder(
+
mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(listParams),
+
mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(resp, nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ },
+ }
+
+ err := lb.getPublicIPAddress("203.0.113.1")
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if lb.ipAddr != "203.0.113.1" {
+ t.Errorf("ipAddr = %q, want %q", lb.ipAddr,
"203.0.113.1")
+ }
+ if lb.ipAddrID != "ip-123" {
+ t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID,
"ip-123")
+ }
+ })
+
+ t.Run("IP found but not allocated - associates", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ listParams := &cloudstack.ListPublicIpAddressesParams{}
+ resp := &cloudstack.ListPublicIpAddressesResponse{
+ Count: 1,
+ PublicIpAddresses: []*cloudstack.PublicIpAddress{
+ {
+ Id: "ip-123",
+ Ipaddress: "203.0.113.1",
+ Allocated: "",
+ },
+ },
+ }
+
+ networkResp := &cloudstack.Network{
+ Id: "net-123",
+ Vpcid: "",
+ Service: []cloudstack.NetworkServiceInternal{},
+ }
+
+ associateParams := &cloudstack.AssociateIpAddressParams{}
+ associateResp := &cloudstack.AssociateIpAddressResponse{
+ Id: "ip-123",
+ Ipaddress: "203.0.113.1",
+ }
+
+ gomock.InOrder(
+
mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(listParams),
+
mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(resp, nil),
+ mockNetwork.EXPECT().GetNetworkByID("net-123",
gomock.Any()).Return(networkResp, 1, nil),
+
mockAddress.EXPECT().NewAssociateIpAddressParams().Return(associateParams),
+
mockAddress.EXPECT().AssociateIpAddress(gomock.Any()).Return(associateResp,
nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ Network: mockNetwork,
+ },
+ networkID: "net-123",
+ ipAddr: "203.0.113.1",
+ }
+
+ err := lb.getPublicIPAddress("203.0.113.1")
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if lb.ipAddr != "203.0.113.1" {
+ t.Errorf("ipAddr = %q, want %q", lb.ipAddr,
"203.0.113.1")
+ }
+ if lb.ipAddrID != "ip-123" {
+ t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID,
"ip-123")
+ }
+ })
+
+ t.Run("IP not found", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ listParams := &cloudstack.ListPublicIpAddressesParams{}
+ resp := &cloudstack.ListPublicIpAddressesResponse{
+ Count: 0,
+ PublicIpAddresses: []*cloudstack.PublicIpAddress{},
+ }
+
+ gomock.InOrder(
+
mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(listParams),
+
mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(resp, nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ },
+ }
+
+ err := lb.getPublicIPAddress("203.0.113.1")
+ if err == nil {
+ t.Fatalf("expected error for IP not found")
+ }
+ if !strings.Contains(err.Error(), "could not find IP address") {
+ t.Errorf("error message = %q, want to contain 'could
not find IP address'", err.Error())
+ }
+ })
+
+ t.Run("multiple IPs found", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ listParams := &cloudstack.ListPublicIpAddressesParams{}
+ resp := &cloudstack.ListPublicIpAddressesResponse{
+ Count: 2,
+ PublicIpAddresses: []*cloudstack.PublicIpAddress{
+ {Id: "ip-1", Ipaddress: "203.0.113.1"},
+ {Id: "ip-2", Ipaddress: "203.0.113.1"},
+ },
+ }
+
+ gomock.InOrder(
+
mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(listParams),
+
mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(resp, nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ },
+ }
+
+ err := lb.getPublicIPAddress("203.0.113.1")
+ if err == nil {
+ t.Fatalf("expected error for multiple IPs found")
+ }
+ if !strings.Contains(err.Error(), "Found 2 addresses") {
+ t.Errorf("error message = %q, want to contain 'Found 2
addresses'", err.Error())
+ }
+ })
+
+ t.Run("error retrieving IP", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ listParams := &cloudstack.ListPublicIpAddressesParams{}
+ apiErr := fmt.Errorf("API error")
+
+ gomock.InOrder(
+
mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(listParams),
+
mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(nil, apiErr),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ },
+ }
+
+ err := lb.getPublicIPAddress("203.0.113.1")
+ if err == nil {
+ t.Fatalf("expected error")
+ }
+ if !strings.Contains(err.Error(), "error retrieving IP
address") {
+ t.Errorf("error message = %q, want to contain 'error
retrieving IP address'", err.Error())
+ }
+ })
+
+ t.Run("project ID handling", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ listParams := &cloudstack.ListPublicIpAddressesParams{}
+ resp := &cloudstack.ListPublicIpAddressesResponse{
+ Count: 1,
+ PublicIpAddresses: []*cloudstack.PublicIpAddress{
+ {
+ Id: "ip-123",
+ Ipaddress: "203.0.113.1",
+ Allocated: "2023-01-01T00:00:00+0000",
+ },
+ },
+ }
+
+ gomock.InOrder(
+
mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(listParams),
+
mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(resp, nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ },
+ projectID: "proj-123",
+ }
+
+ err := lb.getPublicIPAddress("203.0.113.1")
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if lb.ipAddr != "203.0.113.1" {
+ t.Errorf("ipAddr = %q, want %q", lb.ipAddr,
"203.0.113.1")
+ }
+ if lb.ipAddrID != "ip-123" {
+ t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID,
"ip-123")
+ }
+ })
+}
+
+func TestAssociatePublicIPAddress(t *testing.T) {
+ t.Run("associate IP for regular network", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ networkResp := &cloudstack.Network{
+ Id: "net-123",
+ Vpcid: "",
+ Service: []cloudstack.NetworkServiceInternal{},
+ }
+
+ associateParams := &cloudstack.AssociateIpAddressParams{}
+ associateResp := &cloudstack.AssociateIpAddressResponse{
+ Id: "ip-123",
+ Ipaddress: "203.0.113.1",
+ }
+
+ gomock.InOrder(
+ mockNetwork.EXPECT().GetNetworkByID("net-123",
gomock.Any()).Return(networkResp, 1, nil),
+
mockAddress.EXPECT().NewAssociateIpAddressParams().Return(associateParams),
+
mockAddress.EXPECT().AssociateIpAddress(gomock.Any()).Return(associateResp,
nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ Network: mockNetwork,
+ },
+ networkID: "net-123",
+ }
+
+ err := lb.associatePublicIPAddress()
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if lb.ipAddr != "203.0.113.1" {
+ t.Errorf("ipAddr = %q, want %q", lb.ipAddr,
"203.0.113.1")
+ }
+ if lb.ipAddrID != "ip-123" {
+ t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID,
"ip-123")
+ }
+ if !lb.ipAssociatedByController {
+ t.Errorf("ipAssociatedByController = false, want true")
+ }
+ })
+
+ t.Run("associate IP for VPC network", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ networkResp := &cloudstack.Network{
+ Id: "net-123",
+ Vpcid: "vpc-456",
+ Service: []cloudstack.NetworkServiceInternal{},
+ }
+
+ associateParams := &cloudstack.AssociateIpAddressParams{}
+ associateResp := &cloudstack.AssociateIpAddressResponse{
+ Id: "ip-123",
+ Ipaddress: "203.0.113.1",
+ }
+
+ gomock.InOrder(
+ mockNetwork.EXPECT().GetNetworkByID("net-123",
gomock.Any()).Return(networkResp, 1, nil),
+
mockAddress.EXPECT().NewAssociateIpAddressParams().Return(associateParams),
+
mockAddress.EXPECT().AssociateIpAddress(gomock.Any()).Return(associateResp,
nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{
+ Address: mockAddress,
+ Network: mockNetwork,
+ },
+ networkID: "net-123",
+ }
+
+ err := lb.associatePublicIPAddress()
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if lb.ipAddr != "203.0.113.1" {
+ t.Errorf("ipAddr = %q, want %q", lb.ipAddr,
"203.0.113.1")
+ }
+ if lb.ipAddrID != "ip-123" {
+ t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID,
"ip-123")
+ }
+ if !lb.ipAssociatedByController {
+ t.Errorf("ipAssociatedByController = false, want true")
+ }
+ if lb.ipAddr != "203.0.113.1" {
+ t.Errorf("ipAddr = %q, want %q", lb.ipAddr,
"203.0.113.1")
+ }
+ if lb.ipAddrID != "ip-123" {
+ t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID,
"ip-123")
+ }
+ if !lb.ipAssociatedByController {
+ t.Errorf("ipAssociatedByController = false, want true")
+ }
Review Comment:
Duplicate assertions detected. Lines 1364-1372 repeat the exact same checks
as lines 1355-1363. These duplicate assertions should be removed as they
provide no additional test value and make the test unnecessarily verbose.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
