diff --git a/README.md b/README.md index fc4922b7..c3d15a76 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,70 @@ list, which briefly interrupts traffic on that port. Setting it to an empty value (`""`) sends an empty CIDR list to CloudStack — it does not block all traffic. +#### `service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-name` + +**Type:** String + +**Default:** Not set (no stickiness policy) + +**Description:** Creates a CloudStack **LB stickiness policy** on every load balancer rule belonging +to the service, making the load balancer keep a client on the same backend node between requests. + +The value is the CloudStack stickiness method name and is passed through to CloudStack, which +validates it against the methods the network's load balancer provider offers. The VirtualRouter +(HAProxy) provider supports `LbCookie`, `AppCookie` and `SourceBased`. An unsupported method makes +the service fail to sync with an `error creating stickiness policy` error. + +Each service port has its own load balancer rule, so a service exposing several ports gets one +policy per port, all with the same method and parameters. + +**Use Case:** Applications that keep per-client state in the backend — a session held in process +memory, for example — and therefore need successive requests from one client to land on the same +node. + +**Example:** +```yaml +apiVersion: v1 +kind: Service +metadata: + name: my-service + annotations: + service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-name: "LbCookie" + service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-param: "name=SERVERID" +spec: + type: LoadBalancer +``` + +**Note:** Removing the annotation deletes the stickiness policy and leaves the load balancer rule in +place. Changing either the method name or the parameters replaces the policy: the controller deletes +the existing policy and creates a new one, which resets whatever affinity state the load balancer +was holding. + +#### `service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-param` + +**Type:** String (comma-separated `key=value` list) + +**Default:** Not set (no parameters) + +**Description:** Parameters for the stickiness method selected by +`service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-name`. Which keys are +accepted depends on the method — `LbCookie` and `AppCookie` take a cookie `name`, `SourceBased` +takes `tablesize` and `expire`. CloudStack validates the keys, so an unknown parameter makes the +service fail to sync. + +This annotation has no effect on its own: without a method name no policy is created. + +**Format:** Comma-separated `key=value` pairs. Spaces around entries are trimmed, and only the first +`=` separates key from value, so a value may itself contain `=`. Entries without a `=` are ignored +rather than rejected, an empty value (`key=`) is passed through as an empty string, and if a key +repeats, the last occurrence wins. + +**Example:** +```yaml + service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-name: "AppCookie" + service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-param: "name=JSESSIONID,mode=insert" +``` + #### `service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller` **Type:** Boolean (`"true"` or `"false"`) @@ -262,6 +326,14 @@ annotation for it. Any other value makes the service fail to sync with `unsupported load balancer affinity`. Other CloudStack algorithms, such as `leastconn`, cannot currently be selected. +The algorithm is separate from stickiness. `spec.sessionAffinity: ClientIP` picks the load balancer +algorithm, while a CloudStack stickiness policy — cookie-based affinity, for instance — is +configured with the +[`stickiness-method-name`](#servicebetakubernetesiocloudstack-load-balancer-stickiness-method-name) +and +[`stickiness-method-param`](#servicebetakubernetesiocloudstack-load-balancer-stickiness-method-param) +annotations. The two can be used together. + ### VPC Networks VPC networks are supported. VPC networks normally do not offer the Firewall service, so the diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index ffbdd7cd..77a8167d 100644 --- a/cloudstack_loadbalancer.go +++ b/cloudstack_loadbalancer.go @@ -56,6 +56,9 @@ const ( // associated the IP address. This annotation is set by the controller when it associates // an unallocated IP, and is used to determine if the IP should be disassociated on deletion. ServiceAnnotationLoadBalancerIPAssociatedByController = "service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller" //nolint:gosec + + ServiceAnnotationLoadBalancerStickinessMethodName = "service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-name" + ServiceAnnotationLoadBalancerStickinessParam = "service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-param" ) type loadBalancer struct { @@ -69,6 +72,7 @@ type loadBalancer struct { networkID string projectID string rules map[string]*cloudstack.LoadBalancerRule + stickinessPolicies map[string]*cloudstack.LBStickinessPolicyStickinesspolicy ipAssociatedByController bool } @@ -181,12 +185,36 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s // Delete the rule from the map, to prevent it being deleted. delete(lb.rules, lbRuleName) } + + stickinessPolicy, stickinessPolicyNeedsUpdate, err := lb.checkStickinessPolicy(lbRule, service) + if err != nil { + return nil, err + } + if stickinessPolicyNeedsUpdate { + if stickinessPolicy != nil { + klog.V(4).Infof("Recreate stickiness policy: %v", lbRuleName) + if err := lb.deleteStickinessPolicy(stickinessPolicy.Id); err != nil { + return nil, err + } + delete(lb.stickinessPolicies, lbRule.Id) + } else { + klog.V(4).Infof("Creating stickiness policy: %v", lbRuleName) + } + if _, err := lb.createStickinessPolicy(lbRuleName, lbRule.Id, service); err != nil { + return nil, err + } + // Remove from map to mark as handled (map tracks initial state for comparison) + delete(lb.stickinessPolicies, lbRule.Id) + } } else { klog.V(4).Infof("Creating load balancer rule: %v", lbRuleName) lbRule, err = lb.createLoadBalancerRule(lbRuleName, port, protocol, service) if err != nil { return nil, err } + if _, err := lb.createStickinessPolicy(lbRuleName, lbRule.Id, service); err != nil { + return nil, err + } klog.V(4).Infof("Assigning hosts (%v) to load balancer rule: %v", lb.hostIDs, lbRuleName) if err = lb.assignHostsToRule(lbRule, lb.hostIDs); err != nil { @@ -434,10 +462,11 @@ func (cs *CSCloud) GetLoadBalancerName(ctx context.Context, clusterName string, // getLoadBalancer retrieves the IP address and ID and all the existing rules it can find. func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, error) { lb := &loadBalancer{ - CloudStackClient: cs.client, - name: cs.GetLoadBalancerName(context.TODO(), "", service), - projectID: cs.projectID, - rules: make(map[string]*cloudstack.LoadBalancerRule), + CloudStackClient: cs.client, + name: cs.GetLoadBalancerName(context.TODO(), "", service), + projectID: cs.projectID, + rules: make(map[string]*cloudstack.LoadBalancerRule), + stickinessPolicies: make(map[string]*cloudstack.LBStickinessPolicyStickinesspolicy), } p := cs.client.LoadBalancer.NewListLoadBalancerRulesParams() @@ -462,6 +491,19 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, erro lb.ipAddr = lbRule.Publicip lb.ipAddrID = lbRule.Publicipid + + lbStickinessPoliciesParams := cs.client.LoadBalancer.NewListLBStickinessPoliciesParams() + lbStickinessPoliciesParams.SetLbruleid(lbRule.Id) + lbStickinessPolicies, err := cs.client.LoadBalancer.ListLBStickinessPolicies(lbStickinessPoliciesParams) + if err != nil { + return nil, fmt.Errorf("error retrieving stickiness policies: %v", err) + } + // CloudStack returns a policy wrapper per rule even when the rule has no + // stickiness policy, with an empty Stickinesspolicy list inside it. + if len(lbStickinessPolicies.LBStickinessPolicies) > 0 && + len(lbStickinessPolicies.LBStickinessPolicies[0].Stickinesspolicy) > 0 { + lb.stickinessPolicies[lbRule.Id] = &lbStickinessPolicies.LBStickinessPolicies[0].Stickinesspolicy[0] + } } klog.V(4).Infof("Load balancer %v contains %d rule(s)", lb.name, len(lb.rules)) @@ -649,6 +691,60 @@ func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error) { return cidrList, nil } +func (lb *loadBalancer) checkStickinessPolicy(lbRule *cloudstack.LoadBalancerRule, service *corev1.Service) (*cloudstack.LBStickinessPolicyStickinesspolicy, bool, error) { + stickinessPolicy := lb.stickinessPolicies[lbRule.Id] + stickinessMethodName := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerStickinessMethodName, "") + stickinessMethodParam := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerStickinessParam, "") + stickinessMethodParams := parseStickinessParams(stickinessMethodParam) + + // If no policy exists and no method name is specified, no action needed + if stickinessPolicy == nil { + if stickinessMethodName == "" { + return nil, false, nil + } + klog.V(4).Infof("sticky policy not found for rule: %v", lbRule.Name) + return nil, true, nil + } + + // If policy exists but method name is not specified, policy should be deleted + if stickinessMethodName == "" { + klog.V(4).Infof("sticky policy exists but annotation removed for rule: %v", lbRule.Name) + return stickinessPolicy, true, nil + } + + // Policy exists and method name is specified - check if it matches + klog.V(4).Infof("sticky policy found for rule: %v", lbRule.Name) + if stickinessPolicy.Methodname != stickinessMethodName { + klog.V(4).Infof("sticky policy method name does not match: %v", lbRule.Name) + return stickinessPolicy, true, nil + } + + // Check if params match + if len(stickinessPolicy.Params) != len(stickinessMethodParams) { + klog.V(4).Infof("sticky policy params length does not match: %v", lbRule.Name) + return stickinessPolicy, true, nil + } + + // Check if all keys in stickinessPolicy.Params match stickinessMethodParams + for key, value := range stickinessPolicy.Params { + if stickinessMethodParams[key] != value { + klog.V(4).Infof("sticky policy param %v does not match: %v", key, value) + return stickinessPolicy, true, nil + } + } + + // Check if all keys in stickinessMethodParams exist in stickinessPolicy.Params + for key := range stickinessMethodParams { + if _, exists := stickinessPolicy.Params[key]; !exists { + klog.V(4).Infof("sticky policy missing param: %v", key) + return stickinessPolicy, true, nil + } + } + + // Policy matches desired state + return stickinessPolicy, false, nil +} + // checkLoadBalancerRule checks if the rule already exists and if it does, if it can be updated. If // it does exist but cannot be updated, it will delete the existing rule so it can be created again. func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) { @@ -715,6 +811,45 @@ func (lb *loadBalancer) updateLoadBalancerRule(lbRuleName string, protocol LoadB return err } +// createStickinessPolicy creates a new stickiness policy and returns it. +func (lb *loadBalancer) createStickinessPolicy(lbRuleName string, lbRuleId string, service *corev1.Service) (*cloudstack.LBStickinessPolicyStickinesspolicy, error) { + stickinessMethodName := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerStickinessMethodName, "") + stickinessMethodParam := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerStickinessParam, "") + // If the stickiness method name is not set, we don't need to create a stickiness policy. + if stickinessMethodName == "" { + return nil, nil + } + p := lb.LoadBalancer.NewCreateLBStickinessPolicyParams(lbRuleId, stickinessMethodName, lbRuleName) + + params := parseStickinessParams(stickinessMethodParam) + p.SetParam(params) + + stickinessPolicy, err := lb.LoadBalancer.CreateLBStickinessPolicy(p) + if err != nil { + return nil, fmt.Errorf("error creating stickiness policy: %v", err) + } + if len(stickinessPolicy.Stickinesspolicy) == 0 { + return nil, fmt.Errorf("error creating stickiness policy: no policy returned for load balancer rule %v", lbRuleName) + } + return &cloudstack.LBStickinessPolicyStickinesspolicy{ + Methodname: stickinessPolicy.Stickinesspolicy[0].Methodname, + Params: stickinessPolicy.Stickinesspolicy[0].Params, + Id: stickinessPolicy.Stickinesspolicy[0].Id, + Name: stickinessPolicy.Stickinesspolicy[0].Name, + State: stickinessPolicy.Stickinesspolicy[0].State, + }, nil +} + +// deleteStickinessPolicy deletes a stickiness policy. +func (lb *loadBalancer) deleteStickinessPolicy(stickinessPolicyId string) error { + p := lb.LoadBalancer.NewDeleteLBStickinessPolicyParams(stickinessPolicyId) + + if _, err := lb.LoadBalancer.DeleteLBStickinessPolicy(p); err != nil { + return fmt.Errorf("error deleting stickiness policy %v: %v", stickinessPolicyId, err) + } + return nil +} + // createLoadBalancerRule creates a new load balancer rule and returns it's ID. func (lb *loadBalancer) createLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service) (*cloudstack.LoadBalancerRule, error) { p := lb.LoadBalancer.NewCreateLoadBalancerRuleParams( @@ -772,6 +907,7 @@ func (lb *loadBalancer) deleteLoadBalancerRule(lbRule *cloudstack.LoadBalancerRu // Delete the rule from the map as it no longer exists delete(lb.rules, lbRule.Name) + delete(lb.stickinessPolicies, lbRule.Id) return nil } @@ -1136,6 +1272,23 @@ func getStringFromServiceAnnotation(service *corev1.Service, annotationKey strin return defaultSetting } +// parseStickinessParams parses a comma-separated string of key=value pairs into a map. +// Empty values and malformed entries are ignored. +func parseStickinessParams(paramString string) map[string]string { + params := make(map[string]string) + for _, param := range strings.Split(paramString, ",") { + param = strings.TrimSpace(param) + if param == "" { + continue + } + parts := strings.SplitN(param, "=", 2) + if len(parts) == 2 { + params[parts[0]] = parts[1] + } + } + return params +} + // getBoolFromServiceAnnotation searches a given v1.Service for a specific annotationKey and either returns the annotation's boolean value or a specified defaultSetting func getBoolFromServiceAnnotation(service *corev1.Service, annotationKey string, defaultSetting bool) bool { klog.V(4).Infof("getBoolFromServiceAnnotation(%s/%s, %v, %v)", service.Namespace, service.Name, annotationKey, defaultSetting) diff --git a/cloudstack_loadbalancer_test.go b/cloudstack_loadbalancer_test.go index 4bbf38e7..9cdcfe43 100644 --- a/cloudstack_loadbalancer_test.go +++ b/cloudstack_loadbalancer_test.go @@ -3277,9 +3277,35 @@ func TestGetLoadBalancer(t *testing.T) { }, } + // rule-1 has a stickiness policy configured, rule-2 has none. + stickyResp := &cloudstack.ListLBStickinessPoliciesResponse{ + Count: 1, + LBStickinessPolicies: []*cloudstack.LBStickinessPolicy{ + { + Lbruleid: "rule-1", + Stickinesspolicy: []cloudstack.LBStickinessPolicyStickinesspolicy{ + { + Id: "policy-1", + Name: "test-service-tcp-80", + Methodname: "LbCookie", + Params: map[string]string{"cookie-name": "SERVERID"}, + }, + }, + }, + }, + } + emptyStickyResp := &cloudstack.ListLBStickinessPoliciesResponse{ + Count: 0, + LBStickinessPolicies: []*cloudstack.LBStickinessPolicy{}, + } + gomock.InOrder( mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams), mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(listResp, nil), + mockLB.EXPECT().NewListLBStickinessPoliciesParams().Return(&cloudstack.ListLBStickinessPoliciesParams{}), + mockLB.EXPECT().ListLBStickinessPolicies(gomock.Any()).Return(stickyResp, nil), + mockLB.EXPECT().NewListLBStickinessPoliciesParams().Return(&cloudstack.ListLBStickinessPoliciesParams{}), + mockLB.EXPECT().ListLBStickinessPolicies(gomock.Any()).Return(emptyStickyResp, nil), ) cs := &CSCloud{ @@ -3299,6 +3325,14 @@ func TestGetLoadBalancer(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } + if len(lb.stickinessPolicies) != 1 { + t.Errorf("stickinessPolicies count = %d, want %d", len(lb.stickinessPolicies), 1) + } + if policy, ok := lb.stickinessPolicies["rule-1"]; !ok { + t.Errorf("stickinessPolicies missing entry for %q", "rule-1") + } else if policy.Id != "policy-1" { + t.Errorf("stickiness policy ID = %q, want %q", policy.Id, "policy-1") + } if lb.ipAddr != "203.0.113.1" { t.Errorf("ipAddr = %q, want %q", lb.ipAddr, "203.0.113.1") } @@ -3685,3 +3719,488 @@ func TestVerifyHosts(t *testing.T) { } }) } + +func TestParseStickinessParams(t *testing.T) { + tests := []struct { + name string + input string + want map[string]string + }{ + { + name: "empty string returns empty map", + input: "", + want: map[string]string{}, + }, + { + name: "single pair", + input: "cookie-name=SERVERID", + want: map[string]string{"cookie-name": "SERVERID"}, + }, + { + name: "multiple pairs", + input: "cookie-name=SERVERID,mode=insert", + want: map[string]string{"cookie-name": "SERVERID", "mode": "insert"}, + }, + { + name: "whitespace around entries is trimmed", + input: " cookie-name=SERVERID , mode=insert ", + want: map[string]string{"cookie-name": "SERVERID", "mode": "insert"}, + }, + { + name: "entry without separator is ignored", + input: "cookie-name=SERVERID,bogus", + want: map[string]string{"cookie-name": "SERVERID"}, + }, + { + name: "trailing comma is ignored", + input: "mode=insert,", + want: map[string]string{"mode": "insert"}, + }, + { + name: "only separators returns empty map", + input: ",,,", + want: map[string]string{}, + }, + { + name: "empty value is preserved", + input: "cookie-name=", + want: map[string]string{"cookie-name": ""}, + }, + { + name: "value containing separator is kept intact", + input: "expr=a=b", + want: map[string]string{"expr": "a=b"}, + }, + { + name: "duplicate key keeps last value", + input: "mode=insert,mode=rewrite", + want: map[string]string{"mode": "rewrite"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parseStickinessParams(tt.input) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("parseStickinessParams(%q) = %v, want %v", tt.input, got, tt.want) + } + }) + } +} + +// stickinessTestService builds a service carrying the stickiness annotations. An +// empty method or param string omits that annotation entirely. +func stickinessTestService(methodName, params string) *corev1.Service { + annotations := map[string]string{} + if methodName != "" { + annotations[ServiceAnnotationLoadBalancerStickinessMethodName] = methodName + } + if params != "" { + annotations[ServiceAnnotationLoadBalancerStickinessParam] = params + } + return &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + Annotations: annotations, + }, + } +} + +func TestCheckStickinessPolicy(t *testing.T) { + lbRule := &cloudstack.LoadBalancerRule{Id: "rule-id", Name: "test-service-tcp-80"} + + tests := []struct { + name string + existingPolicy *cloudstack.LBStickinessPolicyStickinesspolicy + methodName string + params string + wantPolicy bool // true when the existing policy is expected back + wantNeedsUpdate bool + }{ + { + name: "no policy and no annotation is a no-op", + existingPolicy: nil, + methodName: "", + wantPolicy: false, + wantNeedsUpdate: false, + }, + { + name: "no policy with annotation needs creation", + existingPolicy: nil, + methodName: "LbCookie", + wantPolicy: false, + wantNeedsUpdate: true, + }, + { + name: "policy with annotation removed needs deletion", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "LbCookie", + }, + methodName: "", + wantPolicy: true, + wantNeedsUpdate: true, + }, + { + name: "matching method with no params is up-to-date", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "LbCookie", + Params: map[string]string{}, + }, + methodName: "LbCookie", + params: "", + wantPolicy: true, + wantNeedsUpdate: false, + }, + { + name: "matching method and params is up-to-date", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "AppCookie", + Params: map[string]string{"cookie-name": "SERVERID", "mode": "insert"}, + }, + methodName: "AppCookie", + params: "cookie-name=SERVERID,mode=insert", + wantPolicy: true, + wantNeedsUpdate: false, + }, + { + name: "method name mismatch needs recreation", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "LbCookie", + }, + methodName: "AppCookie", + wantPolicy: true, + wantNeedsUpdate: true, + }, + { + name: "extra desired param needs recreation", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "AppCookie", + Params: map[string]string{"cookie-name": "SERVERID"}, + }, + methodName: "AppCookie", + params: "cookie-name=SERVERID,mode=insert", + wantPolicy: true, + wantNeedsUpdate: true, + }, + { + name: "removed desired param needs recreation", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "AppCookie", + Params: map[string]string{"cookie-name": "SERVERID", "mode": "insert"}, + }, + methodName: "AppCookie", + params: "cookie-name=SERVERID", + wantPolicy: true, + wantNeedsUpdate: true, + }, + { + name: "param value mismatch needs recreation", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "AppCookie", + Params: map[string]string{"cookie-name": "SERVERID"}, + }, + methodName: "AppCookie", + params: "cookie-name=JSESSIONID", + wantPolicy: true, + wantNeedsUpdate: true, + }, + { + name: "renamed param key of equal count needs recreation", + existingPolicy: &cloudstack.LBStickinessPolicyStickinesspolicy{ + Id: "policy-id", + Methodname: "AppCookie", + Params: map[string]string{"cookie-name": ""}, + }, + methodName: "AppCookie", + params: "mode=", + wantPolicy: true, + wantNeedsUpdate: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lb := &loadBalancer{ + stickinessPolicies: map[string]*cloudstack.LBStickinessPolicyStickinesspolicy{}, + } + if tt.existingPolicy != nil { + lb.stickinessPolicies[lbRule.Id] = tt.existingPolicy + } + + policy, needsUpdate, err := lb.checkStickinessPolicy(lbRule, stickinessTestService(tt.methodName, tt.params)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if tt.wantPolicy && policy != tt.existingPolicy { + t.Errorf("policy = %v, want the existing policy %v", policy, tt.existingPolicy) + } + if !tt.wantPolicy && policy != nil { + t.Errorf("policy = %v, want nil", policy) + } + if needsUpdate != tt.wantNeedsUpdate { + t.Errorf("needsUpdate = %v, want %v", needsUpdate, tt.wantNeedsUpdate) + } + }) + } +} + +func TestCreateStickinessPolicy(t *testing.T) { + t.Run("no method annotation is a no-op", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No expectations on the mock; any API call would fail the test. + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + policy, err := lb.createStickinessPolicy("test-service-tcp-80", "rule-id", stickinessTestService("", "")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if policy != nil { + t.Errorf("policy = %v, want nil", policy) + } + }) + + t.Run("creates policy from annotations", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + createParams := &cloudstack.CreateLBStickinessPolicyParams{} + createResp := &cloudstack.CreateLBStickinessPolicyResponse{ + Lbruleid: "rule-id", + Stickinesspolicy: []cloudstack.CreateLBStickinessPolicyResponseStickinesspolicy{ + { + Id: "policy-id", + Name: "test-service-tcp-80", + Methodname: "AppCookie", + Params: map[string]string{"cookie-name": "SERVERID"}, + State: "Active", + }, + }, + } + + gomock.InOrder( + mockLB.EXPECT().NewCreateLBStickinessPolicyParams("rule-id", "AppCookie", "test-service-tcp-80").Return(createParams), + mockLB.EXPECT().CreateLBStickinessPolicy(createParams).Return(createResp, nil), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + policy, err := lb.createStickinessPolicy("test-service-tcp-80", "rule-id", stickinessTestService("AppCookie", "cookie-name=SERVERID")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if policy == nil { + t.Fatal("expected a policy, got nil") + } + if policy.Id != "policy-id" { + t.Errorf("policy ID = %q, want %q", policy.Id, "policy-id") + } + if policy.Methodname != "AppCookie" { + t.Errorf("policy method = %q, want %q", policy.Methodname, "AppCookie") + } + if policy.Name != "test-service-tcp-80" { + t.Errorf("policy name = %q, want %q", policy.Name, "test-service-tcp-80") + } + if policy.State != "Active" { + t.Errorf("policy state = %q, want %q", policy.State, "Active") + } + if !reflect.DeepEqual(policy.Params, map[string]string{"cookie-name": "SERVERID"}) { + t.Errorf("policy params = %v, want %v", policy.Params, map[string]string{"cookie-name": "SERVERID"}) + } + }) + + t.Run("API error is returned", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + createParams := &cloudstack.CreateLBStickinessPolicyParams{} + apiErr := fmt.Errorf("create policy API error") + + gomock.InOrder( + mockLB.EXPECT().NewCreateLBStickinessPolicyParams("rule-id", "LbCookie", "test-service-tcp-80").Return(createParams), + mockLB.EXPECT().CreateLBStickinessPolicy(createParams).Return(nil, apiErr), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + policy, err := lb.createStickinessPolicy("test-service-tcp-80", "rule-id", stickinessTestService("LbCookie", "")) + if err == nil { + t.Fatal("expected an error, got nil") + } + if policy != nil { + t.Errorf("policy = %v, want nil", policy) + } + if !strings.Contains(err.Error(), "error creating stickiness policy") { + t.Errorf("error = %q, want it to mention creating the stickiness policy", err.Error()) + } + }) +} + +func TestDeleteStickinessPolicy(t *testing.T) { + t.Run("deletes policy by ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + deleteParams := &cloudstack.DeleteLBStickinessPolicyParams{} + + gomock.InOrder( + mockLB.EXPECT().NewDeleteLBStickinessPolicyParams("policy-id").Return(deleteParams), + mockLB.EXPECT().DeleteLBStickinessPolicy(deleteParams).Return(&cloudstack.DeleteLBStickinessPolicyResponse{Success: true}, nil), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + if err := lb.deleteStickinessPolicy("policy-id"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("API error is returned", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + deleteParams := &cloudstack.DeleteLBStickinessPolicyParams{} + apiErr := fmt.Errorf("delete policy API error") + + gomock.InOrder( + mockLB.EXPECT().NewDeleteLBStickinessPolicyParams("policy-id").Return(deleteParams), + mockLB.EXPECT().DeleteLBStickinessPolicy(deleteParams).Return(nil, apiErr), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + err := lb.deleteStickinessPolicy("policy-id") + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "error deleting stickiness policy policy-id") { + t.Errorf("error = %q, want it to mention deleting the stickiness policy", err.Error()) + } + }) +} + +// TestGetLoadBalancerEmptyStickinessPolicyList covers the shape CloudStack +// actually returns for a rule with no stickiness policy: a policy wrapper with +// an empty inner Stickinesspolicy list. +func TestGetLoadBalancerEmptyStickinessPolicyList(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{ + {Id: "rule-1", Name: "test-service-tcp-80", Publicip: "203.0.113.1", Publicipid: "ip-123"}, + }, + } + stickyResp := &cloudstack.ListLBStickinessPoliciesResponse{ + Count: 1, + LBStickinessPolicies: []*cloudstack.LBStickinessPolicy{ + { + Lbruleid: "rule-1", + Stickinesspolicy: []cloudstack.LBStickinessPolicyStickinesspolicy{}, + }, + }, + } + + gomock.InOrder( + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(listResp, nil), + mockLB.EXPECT().NewListLBStickinessPoliciesParams().Return(&cloudstack.ListLBStickinessPoliciesParams{}), + mockLB.EXPECT().ListLBStickinessPolicies(gomock.Any()).Return(stickyResp, nil), + ) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + }, + } + + lb, err := cs.getLoadBalancer(service) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(lb.rules) != 1 { + t.Errorf("rules count = %d, want %d", len(lb.rules), 1) + } + if len(lb.stickinessPolicies) != 0 { + t.Errorf("stickinessPolicies count = %d, want %d", len(lb.stickinessPolicies), 0) + } +} + +// TestCreateStickinessPolicyEmptyResponse ensures an empty create response is +// reported as an error rather than dereferenced. +func TestCreateStickinessPolicyEmptyResponse(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + createParams := &cloudstack.CreateLBStickinessPolicyParams{} + createResp := &cloudstack.CreateLBStickinessPolicyResponse{ + Lbruleid: "rule-id", + Stickinesspolicy: []cloudstack.CreateLBStickinessPolicyResponseStickinesspolicy{}, + } + + gomock.InOrder( + mockLB.EXPECT().NewCreateLBStickinessPolicyParams("rule-id", "LbCookie", "test-service-tcp-80").Return(createParams), + mockLB.EXPECT().CreateLBStickinessPolicy(createParams).Return(createResp, nil), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + policy, err := lb.createStickinessPolicy("test-service-tcp-80", "rule-id", stickinessTestService("LbCookie", "")) + if err == nil { + t.Fatal("expected an error, got nil") + } + if policy != nil { + t.Errorf("policy = %v, want nil", policy) + } + if !strings.Contains(err.Error(), "no policy returned") { + t.Errorf("error = %q, want it to mention that no policy was returned", err.Error()) + } +} diff --git a/go.mod b/go.mod index 24e177ad..be0b8af9 100644 --- a/go.mod +++ b/go.mod @@ -3,13 +3,14 @@ module github.com/apache/cloudstack-kubernetes-provider go 1.23.0 require ( - github.com/apache/cloudstack-go/v2 v2.19.0 + github.com/apache/cloudstack-go/v2 v2.19.1 github.com/blang/semver/v4 v4.0.0 github.com/spf13/pflag v1.0.5 go.uber.org/mock v0.5.0 gopkg.in/gcfg.v1 v1.2.3 k8s.io/api v0.24.17 k8s.io/apimachinery v0.24.17 + k8s.io/client-go v0.24.17 k8s.io/cloud-provider v0.24.17 k8s.io/component-base v0.24.17 k8s.io/klog/v2 v2.80.1 @@ -95,7 +96,6 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/apiserver v0.24.17 // indirect - k8s.io/client-go v0.24.17 // indirect k8s.io/component-helpers v0.24.17 // indirect k8s.io/controller-manager v0.24.17 // indirect k8s.io/kube-openapi v0.0.0-20220328201542-3ee0da9b0b42 // indirect diff --git a/go.sum b/go.sum index 85fd3e0f..423e8ea9 100644 --- a/go.sum +++ b/go.sum @@ -52,8 +52,8 @@ github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRF github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho= github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kdvxnR2qWY= -github.com/apache/cloudstack-go/v2 v2.19.0 h1:YHLw770MmgiqXx6NRFYw2Nr7DpnylLhLG2KYNCftgnc= -github.com/apache/cloudstack-go/v2 v2.19.0/go.mod h1:p/YBUwIEkQN6CQxFhw8Ff0wzf1MY0qRRRuGYNbcb1F8= +github.com/apache/cloudstack-go/v2 v2.19.1 h1:1K5O4NZpdWzOZUN6XuaVNsdX+QnoFRc5VE/oc1nUckQ= +github.com/apache/cloudstack-go/v2 v2.19.1/go.mod h1:p/YBUwIEkQN6CQxFhw8Ff0wzf1MY0qRRRuGYNbcb1F8= github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY= github.com/benbjohnson/clock v1.0.3/go.mod h1:bGMdMPoPVvcYyt1gHDf4J2KE153Yf9BuiUKYMaxlTDM= github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8=