Skip to content

Commit 3ce572a

Browse files
gujingitgujingit
authored
bugfix: add max len check for workloadName & serviceName. (sgl-project#58)
feat: svcName changed to new format "s-rbg.Name-role.Name"
1 parent 4271d50 commit 3ce572a

File tree

6 files changed

+231
-44
lines changed

6 files changed

+231
-44
lines changed

‎api/workloads/v1alpha1/helper.go‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import (
55
"encoding/hex"
66
"errors"
77
"fmt"
8-
"unicode"
8+
"strings"
99
)
1010

1111
func (rbg *RoleBasedGroup) GetCommonLabelsFromRole(role *RoleSpec) map[string]string {
@@ -37,14 +37,32 @@ func (rbg *RoleBasedGroup) GetGroupSize() int {
3737
}
3838

3939
func (rbg *RoleBasedGroup) GetWorkloadName(role *RoleSpec) string {
40-
return fmt.Sprintf("%s-%s", rbg.Name, role.Name)
40+
if rbg == nil {
41+
return ""
42+
}
43+
44+
workloadName := fmt.Sprintf("%s-%s", rbg.Name, role.Name)
45+
46+
// Kubernetes name length is limited to 63 characters
47+
if len(workloadName) > 63 {
48+
workloadName = workloadName[:63]
49+
workloadName = strings.TrimRight(workloadName, "-")
50+
}
51+
return workloadName
4152
}
4253

54+
// GetServiceName Because ServiceName needs to follow DNS naming conventions,
55+
// which do not allow names to start with a number. Therefore, the s- prefix
56+
// is added to the service name to meet this requirement.
4357
func (rbg *RoleBasedGroup) GetServiceName(role *RoleSpec) string {
44-
if len(rbg.Name) > 0 && unicode.IsDigit(rune(rbg.Name[0])) {
45-
return fmt.Sprintf("s-%s-%s", rbg.Name, role.Name)
58+
svcName := fmt.Sprintf("s-%s-%s", rbg.Name, role.Name)
59+
if len(svcName) > 63 {
60+
svcName = svcName[:63]
61+
// After truncation, trim trailing hyphens (and ensure the name ends with an alphanumeric)
62+
// to maintain DNS-1123/DNS-1035 validity.
63+
svcName = strings.TrimRight(svcName, "-")
4664
}
47-
return fmt.Sprintf("%s-%s", rbg.Name, role.Name)
65+
return svcName
4866
}
4967

5068
func (rbg *RoleBasedGroup) GetRole(roleName string) (*RoleSpec, error) {

‎pkg/discovery/config_builder.go‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
package discovery
22

33
import (
4+
"context"
45
"fmt"
56
"strings"
67

78
"github.com/google/go-cmp/cmp"
89
"github.com/google/go-cmp/cmp/cmpopts"
910
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
"sigs.k8s.io/controller-runtime/pkg/client"
1012
"sigs.k8s.io/rbgs/pkg/utils"
1113

1214
"sigs.k8s.io/yaml"
@@ -16,8 +18,9 @@ import (
1618
)
1719

1820
type ConfigBuilder struct {
19-
rbg *workloadsv1alpha1.RoleBasedGroup
20-
role *workloadsv1alpha1.RoleSpec
21+
client client.Client
22+
rbg *workloadsv1alpha1.RoleBasedGroup
23+
role *workloadsv1alpha1.RoleSpec
2124
}
2225

2326
type ClusterConfig struct {
@@ -44,13 +47,18 @@ type Instance struct {
4447
}
4548

4649
func (b *ConfigBuilder) Build() ([]byte, error) {
50+
roles, err := b.buildRolesInfo()
51+
if err != nil {
52+
return nil, err
53+
}
54+
4755
config := ClusterConfig{
4856
Group: GroupInfo{
4957
Name: b.rbg.Name,
5058
Size: len(b.rbg.Spec.Roles),
5159
Roles: b.getRoleNames(),
5260
},
53-
Roles: b.buildRolesInfo(),
61+
Roles: roles,
5462
}
5563
return yaml.Marshal(config)
5664
}
@@ -63,20 +71,27 @@ func (b *ConfigBuilder) getRoleNames() []string {
6371
return names
6472
}
6573

66-
func (b *ConfigBuilder) buildRolesInfo() RolesInfo {
74+
func (b *ConfigBuilder) buildRolesInfo() (RolesInfo, error) {
6775
roles := make(RolesInfo)
6876
for _, role := range b.rbg.Spec.Roles {
77+
instances, err := b.buildInstances(&role)
78+
if err != nil {
79+
return nil, err
80+
}
6981
roles[role.Name] = RoleInstances{
7082
Size: int(*role.Replicas),
71-
Instances: b.buildInstances(&role),
83+
Instances: instances,
7284
}
7385
}
74-
return roles
86+
return roles, nil
7587
}
7688

77-
func (b *ConfigBuilder) buildInstances(role *workloadsv1alpha1.RoleSpec) []Instance {
89+
func (b *ConfigBuilder) buildInstances(role *workloadsv1alpha1.RoleSpec) ([]Instance, error) {
7890
instances := make([]Instance, 0, *role.Replicas)
79-
serviceName := b.rbg.GetServiceName(role)
91+
serviceName, err := utils.GetCompatibleHeadlessServiceName(context.TODO(), b.client, b.rbg, role)
92+
if err != nil {
93+
return nil, fmt.Errorf("GetCompatibleHeadlessServiceName error: %s", err.Error())
94+
}
8095

8196
for i := 0; i < int(*role.Replicas); i++ {
8297
instance := Instance{
@@ -91,7 +106,7 @@ func (b *ConfigBuilder) buildInstances(role *workloadsv1alpha1.RoleSpec) []Insta
91106

92107
instances = append(instances, instance)
93108
}
94-
return instances
109+
return instances, nil
95110
}
96111

97112
func generatePortKey(port corev1.ServicePort) string {
@@ -127,9 +142,11 @@ func semanticallyEqualConfigmap(old, new *corev1.ConfigMap) (bool, string) {
127142

128143
opts := cmp.Options{
129144
objectMetaIgnoreOpts,
130-
cmpopts.SortSlices(func(a, b metav1.OwnerReference) bool {
131-
return a.UID < b.UID // Make OwnerReferences comparison order-insensitive
132-
}),
145+
cmpopts.SortSlices(
146+
func(a, b metav1.OwnerReference) bool {
147+
return a.UID < b.UID // Make OwnerReferences comparison order-insensitive
148+
},
149+
),
133150
cmpopts.EquateEmpty(),
134151
}
135152

‎pkg/discovery/config_builder_test.go‎

Lines changed: 117 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,23 +6,30 @@ import (
66
"github.com/google/go-cmp/cmp"
77
corev1 "k8s.io/api/core/v1"
88
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
"k8s.io/apimachinery/pkg/runtime"
10+
"sigs.k8s.io/controller-runtime/pkg/client"
11+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
912
workloadsv1alpha1 "sigs.k8s.io/rbgs/api/workloads/v1alpha1"
1013
)
1114

1215
// TestConfigBuilder_Build tests the Build method of ConfigBuilder
1316
func TestConfigBuilder_Build(t *testing.T) {
1417
replicas3 := int32(3)
1518
replicas1 := int32(1)
19+
schema := runtime.NewScheme()
20+
_ = corev1.AddToScheme(schema)
1621

1722
tests := []struct {
1823
name string
24+
client client.Client
1925
rbg *workloadsv1alpha1.RoleBasedGroup
2026
role *workloadsv1alpha1.RoleSpec
2127
expected string
2228
wantErr bool
2329
}{
2430
{
25-
name: "simple cluster config",
31+
name: "simple cluster config",
32+
client: fake.NewClientBuilder().WithScheme(schema).Build(),
2633
rbg: &workloadsv1alpha1.RoleBasedGroup{
2734
ObjectMeta: metav1.ObjectMeta{
2835
Name: "test-cluster",
@@ -62,6 +69,83 @@ func TestConfigBuilder_Build(t *testing.T) {
6269
- worker
6370
- leader
6471
size: 2
72+
roles:
73+
leader:
74+
instances:
75+
- address: leader-0.s-test-cluster-leader
76+
ports:
77+
api: 6443
78+
size: 1
79+
worker:
80+
instances:
81+
- address: worker-0.s-test-cluster-worker
82+
ports:
83+
http: 8080
84+
- address: worker-1.s-test-cluster-worker
85+
ports:
86+
http: 8080
87+
- address: worker-2.s-test-cluster-worker
88+
ports:
89+
http: 8080
90+
size: 3
91+
`,
92+
wantErr: false,
93+
},
94+
{
95+
name: "config with oldSvc",
96+
client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(
97+
&corev1.Service{
98+
ObjectMeta: metav1.ObjectMeta{
99+
Name: "test-cluster-leader",
100+
Namespace: "default",
101+
},
102+
}, &corev1.Service{
103+
ObjectMeta: metav1.ObjectMeta{
104+
Name: "test-cluster-worker",
105+
Namespace: "default",
106+
},
107+
},
108+
).Build(),
109+
rbg: &workloadsv1alpha1.RoleBasedGroup{
110+
ObjectMeta: metav1.ObjectMeta{
111+
Name: "test-cluster",
112+
Namespace: "default",
113+
},
114+
Spec: workloadsv1alpha1.RoleBasedGroupSpec{
115+
Roles: []workloadsv1alpha1.RoleSpec{
116+
{
117+
Name: "worker",
118+
Replicas: &replicas3,
119+
ServicePorts: []corev1.ServicePort{
120+
{
121+
Name: "http",
122+
Port: 8080,
123+
},
124+
},
125+
},
126+
{
127+
Name: "leader",
128+
Replicas: &replicas1,
129+
ServicePorts: []corev1.ServicePort{
130+
{
131+
Name: "api",
132+
Port: 6443,
133+
},
134+
},
135+
},
136+
},
137+
},
138+
},
139+
role: &workloadsv1alpha1.RoleSpec{
140+
Name: "worker",
141+
Replicas: &replicas3,
142+
},
143+
expected: `group:
144+
name: test-cluster
145+
roles:
146+
- worker
147+
- leader
148+
size: 2
65149
roles:
66150
leader:
67151
instances:
@@ -85,10 +169,12 @@ roles:
85169
wantErr: false,
86170
},
87171
{
88-
name: "role with unnamed ports",
172+
name: "role with unnamed ports",
173+
client: fake.NewClientBuilder().WithScheme(schema).Build(),
89174
rbg: &workloadsv1alpha1.RoleBasedGroup{
90175
ObjectMeta: metav1.ObjectMeta{
91-
Name: "test-cluster",
176+
Name: "test-cluster",
177+
Namespace: "default",
92178
},
93179
Spec: workloadsv1alpha1.RoleBasedGroupSpec{
94180
Roles: []workloadsv1alpha1.RoleSpec{
@@ -119,7 +205,7 @@ roles:
119205
roles:
120206
web:
121207
instances:
122-
- address: web-0.test-cluster-web
208+
- address: web-0.s-test-cluster-web
123209
ports:
124210
port80: 80
125211
port443: 443
@@ -128,7 +214,8 @@ roles:
128214
wantErr: false,
129215
},
130216
{
131-
name: "rbg name start with numeric",
217+
name: "rbg name start with numeric",
218+
client: fake.NewClientBuilder().WithScheme(schema).Build(),
132219
rbg: &workloadsv1alpha1.RoleBasedGroup{
133220
ObjectMeta: metav1.ObjectMeta{
134221
Name: "1-test-cluster",
@@ -196,8 +283,9 @@ roles:
196283
t.Run(
197284
tt.name, func(t *testing.T) {
198285
b := &ConfigBuilder{
199-
rbg: tt.rbg,
200-
role: tt.role,
286+
client: tt.client,
287+
rbg: tt.rbg,
288+
role: tt.role,
201289
}
202290
got, err := b.Build()
203291
if (err != nil) != tt.wantErr {
@@ -284,12 +372,19 @@ func TestConfigBuilder_buildRolesInfo(t *testing.T) {
284372
},
285373
},
286374
}
375+
scheme := runtime.NewScheme()
376+
_ = corev1.AddToScheme(scheme)
377+
_ = workloadsv1alpha1.AddToScheme(scheme)
287378

288379
b := &ConfigBuilder{
289-
rbg: rbg,
380+
client: fake.NewClientBuilder().WithScheme(scheme).Build(),
381+
rbg: rbg,
290382
}
291383

292-
rolesInfo := b.buildRolesInfo()
384+
rolesInfo, err := b.buildRolesInfo()
385+
if err != nil {
386+
t.Errorf("buildRolesInfo() error = %v", err)
387+
}
293388

294389
// Verify number of roles
295390
if len(rolesInfo) != 2 {
@@ -474,21 +569,29 @@ func TestConfigBuilder_buildInstances(t *testing.T) {
474569
},
475570
}
476571

572+
scheme := runtime.NewScheme()
573+
_ = corev1.AddToScheme(scheme)
574+
_ = workloadsv1alpha1.AddToScheme(scheme)
575+
477576
b := &ConfigBuilder{
478-
rbg: rbg,
479-
role: role,
577+
client: fake.NewClientBuilder().WithScheme(scheme).Build(),
578+
rbg: rbg,
579+
role: role,
480580
}
481581

482-
instances := b.buildInstances(role)
582+
instances, err := b.buildInstances(role)
583+
if err != nil {
584+
t.Errorf("buildInstances() error = %v", err)
585+
}
483586

484587
// Verify number of instances
485588
if len(instances) != int(replicas) {
486589
t.Errorf("Expected %d instances, got %d", replicas, len(instances))
487590
}
488591

489592
// Verify first instance
490-
if instances[0].Address != "server-0.test-cluster-server" {
491-
t.Errorf("Expected address 'server-0.test-cluster-server', got '%s'", instances[0].Address)
593+
if instances[0].Address != "server-0.s-test-cluster-server" {
594+
t.Errorf("Expected address 's-server-0.test-cluster-server', got '%s'", instances[0].Address)
492595
}
493596

494597
if len(instances[0].Ports) != 2 {

‎pkg/discovery/injector.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ func (i *DefaultInjector) InjectConfig(
5454
logger := log.FromContext(ctx)
5555

5656
builder := &ConfigBuilder{
57-
rbg: rbg,
58-
role: role,
57+
client: i.client,
58+
rbg: rbg,
59+
role: role,
5960
}
6061

6162
const (

0 commit comments

Comments
 (0)