Skip to content

Commit 6ae55c0

Browse files
johscheuerjohscheuer
authored
Fix a bug in the connection string parser when a cluster has a single letter as name (#2240)
* Fix a bug in the connection string parser when a cluster has a single letter as name * Improve error messages and add additional tests
1 parent 90b5c53 commit 6ae55c0

File tree

5 files changed

+223
-31
lines changed

5 files changed

+223
-31
lines changed

‎api/v1beta2/foundationdb_process_address.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,10 @@ func (address ProcessAddress) MarshalJSON() ([]byte, error) {
156156
// representation.
157157
func ParseProcessAddress(address string) (ProcessAddress, error) {
158158
result := ProcessAddress{}
159+
if address == "" {
160+
return result, fmt.Errorf("cannot parse empty address")
161+
}
162+
159163
if strings.HasSuffix(address, "(fromHostname)") {
160164
address = strings.TrimSuffix(address, "(fromHostname)")
161165
result.FromHostname = true

‎api/v1beta2/foundationdbcluster_types.go

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1713,10 +1713,6 @@ func (cluster *FoundationDBCluster) GetLogServersPerPod() int {
17131713
// connection string.
17141714
var alphanum = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
17151715

1716-
// connectionStringPattern provides a regular expression for parsing the
1717-
// connection string.
1718-
var connectionStringPattern = regexp.MustCompile("(?m)^([^#][^:@]+):([^:@]+)@(.*)$")
1719-
17201716
// ConnectionString models the contents of a cluster file in a structured way
17211717
type ConnectionString struct {
17221718
// DatabaseName provides an identifier for the database which persists
@@ -1731,32 +1727,70 @@ type ConnectionString struct {
17311727
Coordinators []string `json:"coordinators,omitempty"`
17321728
}
17331729

1730+
// isAlphanumeric regex to validate: value containing alphanumeric characters (a-z, A-Z, 0-9).
1731+
var isAlphanumeric = regexp.MustCompile(`^[a-zA-Z0-9]+$`)
1732+
1733+
// isAlphanumericWithUnderScore regex to validate: value containing alphanumeric characters (a-z, A-Z, 0-9) and underscores.
1734+
var isAlphanumericWithUnderScore = regexp.MustCompile(`^[a-zA-Z0-9_]+$`)
1735+
17341736
// ParseConnectionString parses a connection string from its string
17351737
// representation
17361738
func ParseConnectionString(str string) (ConnectionString, error) {
1737-
components := connectionStringPattern.FindStringSubmatch(str)
1738-
if components == nil {
1739-
return ConnectionString{}, fmt.Errorf("invalid connection string %s", str)
1739+
firstSplit := strings.SplitN(str, ":", 2)
1740+
if len(firstSplit) != 2 {
1741+
return ConnectionString{}, fmt.Errorf("invalid connection string: %s, could not split string to get database description", str)
1742+
}
1743+
1744+
// The description is a logical description of the database using alphanumeric characters (a-z, A-Z, 0-9) and underscores.
1745+
description := firstSplit[0]
1746+
if !isAlphanumericWithUnderScore.MatchString(description) {
1747+
return ConnectionString{}, fmt.Errorf("invalid connection string: %s, database description can only contain alphanumeric characters (a-z, A-Z, 0-9) and underscores", str)
1748+
}
1749+
1750+
secondSplit := strings.SplitN(firstSplit[1], "@", 2)
1751+
if len(secondSplit) != 2 {
1752+
return ConnectionString{}, fmt.Errorf("invalid connection string: %s, could not split string to get generation ID", str)
17401753
}
17411754

1742-
coordinatorsStrings := strings.Split(components[3], ",")
1743-
coordinators := make([]string, len(coordinatorsStrings))
1744-
for idx, coordinatorsString := range coordinatorsStrings {
1755+
// The ID is an arbitrary value containing alphanumeric characters (a-z, A-Z, 0-9).
1756+
generationID := secondSplit[0]
1757+
if !isAlphanumeric.MatchString(generationID) {
1758+
return ConnectionString{}, fmt.Errorf("invalid connection string: %s, generation ID can only contain alphanumeric characters (a-z, A-Z, 0-9)", str)
1759+
}
1760+
1761+
coordinatorsStrings := strings.Split(secondSplit[1], ",")
1762+
coordinators := make([]string, 0, len(coordinatorsStrings))
1763+
for _, coordinatorsString := range coordinatorsStrings {
17451764
coordinatorAddress, err := ParseProcessAddress(coordinatorsString)
17461765
if err != nil {
1747-
return ConnectionString{}, err
1766+
return ConnectionString{}, fmt.Errorf("invalid connection string: %s, could not parse coordinator address: %s, got error: %w", str, coordinatorAddress, err)
17481767
}
17491768

1750-
coordinators[idx] = coordinatorAddress.String()
1769+
coordinators = append(coordinators, coordinatorAddress.String())
1770+
}
1771+
1772+
if len(coordinators) == 0 {
1773+
return ConnectionString{}, fmt.Errorf("invalid connection string: %s, connection string must contain at least one coordinator", str)
17511774
}
17521775

17531776
return ConnectionString{
1754-
components[1],
1755-
components[2],
1777+
description,
1778+
generationID,
17561779
coordinators,
17571780
}, nil
17581781
}
17591782

1783+
// SanitizeConnectionStringDescription will replace "-" to "_".
1784+
func SanitizeConnectionStringDescription(str string) string {
1785+
return strings.ReplaceAll(str, "-", "_")
1786+
}
1787+
1788+
// Validate will return nil if the connection string is valid or an error if the connection string is not valid.
1789+
func (str *ConnectionString) Validate() error {
1790+
_, err := ParseConnectionString(str.String())
1791+
return err
1792+
}
1793+
17601794
// String formats a connection string as a string
17611795
func (str *ConnectionString) String() string {
17621796
return fmt.Sprintf("%s:%s@%s", str.DatabaseName, str.GenerationID, strings.Join(str.Coordinators, ","))

‎api/v1beta2/foundationdbcluster_types_test.go

Lines changed: 164 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -804,32 +804,183 @@ var _ = Describe("[api] FoundationDBCluster", func() {
804804
},
805805
}
806806

807-
coordinatorsStr := []string{
807+
coordinatorsList := []string{
808808
"127.0.0.1:4500",
809809
"127.0.0.2:4500",
810810
"127.0.0.3:4500",
811811
}
812812

813813
When("parsing the connection string", func() {
814-
It("should be parsed correctly", func() {
815-
str, err := ParseConnectionString("test:[email protected]:4500,127.0.0.2:4500,127.0.0.3:4500")
816-
Expect(err).NotTo(HaveOccurred())
817-
Expect(str.DatabaseName).To(Equal("test"))
818-
Expect(str.GenerationID).To(Equal("abcd"))
819-
Expect(str.Coordinators).To(Equal(coordinatorsStr))
814+
var str ConnectionString
815+
var err error
816+
var input string
820817

821-
str, err = ParseConnectionString("test:abcd")
822-
Expect(err).To(HaveOccurred())
823-
Expect(err.Error()).To(Equal("invalid connection string test:abcd"))
818+
JustBeforeEach(func() {
819+
str, err = ParseConnectionString(input)
820+
})
821+
822+
When("the input is valid", func() {
823+
BeforeEach(func() {
824+
input = "test:[email protected]:4500,127.0.0.2:4500,127.0.0.3:4500"
825+
})
826+
827+
It("should be parsed correctly", func() {
828+
Expect(err).NotTo(HaveOccurred())
829+
Expect(str.DatabaseName).To(Equal("test"))
830+
Expect(str.GenerationID).To(Equal("abcd"))
831+
Expect(str.Coordinators).To(ConsistOf(coordinatorsList))
832+
})
833+
})
834+
835+
When("the input is invalid", func() {
836+
BeforeEach(func() {
837+
input = "test:abcd"
838+
})
839+
840+
It("should return an error", func() {
841+
Expect(err).To(MatchError(Equal("invalid connection string: test:abcd, could not split string to get generation ID")))
842+
})
843+
})
844+
845+
When("the input has an invalid char", func() {
846+
BeforeEach(func() {
847+
input = "te-st:abcd"
848+
})
849+
850+
It("should return an error", func() {
851+
Expect(err).To(MatchError(Equal("invalid connection string: te-st:abcd, database description can only contain alphanumeric characters (a-z, A-Z, 0-9) and underscores")))
852+
})
853+
})
854+
855+
When("the input has an empty description", func() {
856+
BeforeEach(func() {
857+
input = ":[email protected]:4500,127.0.0.2:4500,127.0.0.3:4500"
858+
})
859+
860+
It("should return an error", func() {
861+
Expect(err).To(MatchError(Equal("invalid connection string: :[email protected]:4500,127.0.0.2:4500,127.0.0.3:4500, database description can only contain alphanumeric characters (a-z, A-Z, 0-9) and underscores")))
862+
})
863+
})
864+
865+
When("the input has an empty generation ID", func() {
866+
BeforeEach(func() {
867+
input = "test:@127.0.0.1:4500,127.0.0.2:4500,127.0.0.3:4500"
868+
})
869+
870+
It("should return an error", func() {
871+
Expect(err).To(MatchError(Equal("invalid connection string: test:@127.0.0.1:4500,127.0.0.2:4500,127.0.0.3:4500, generation ID can only contain alphanumeric characters (a-z, A-Z, 0-9)")))
872+
})
873+
})
874+
875+
When("the input has no coordinators", func() {
876+
BeforeEach(func() {
877+
input = "test:abcd@"
878+
})
879+
880+
It("should return an error", func() {
881+
Expect(err).To(MatchError(Equal("invalid connection string: test:abcd@, could not parse coordinator address: <nil>, got error: cannot parse empty address")))
882+
})
883+
})
884+
885+
When("the input is valid with a single char description", func() {
886+
BeforeEach(func() {
887+
input = "s:[email protected]:4500,127.0.0.2:4500,127.0.0.3:4500"
888+
})
889+
890+
It("should be parsed correctly", func() {
891+
Expect(err).NotTo(HaveOccurred())
892+
Expect(str.DatabaseName).To(Equal("s"))
893+
Expect(str.GenerationID).To(Equal("abcd"))
894+
Expect(str.Coordinators).To(ConsistOf(coordinatorsList))
895+
})
896+
})
897+
898+
When("the input is valid with multiple underscores", func() {
899+
BeforeEach(func() {
900+
input = "fdb_cluster_52v1bpr8:[email protected]:4500:tls,100.82.71.5:4500:tls,100.82.119.151:4500:tls,100.82.122.125:4500:tls,100.82.76.240:4500:tls"
901+
})
902+
903+
It("should be parsed correctly", func() {
904+
Expect(err).NotTo(HaveOccurred())
905+
Expect(str.DatabaseName).To(Equal("fdb_cluster_52v1bpr8"))
906+
Expect(str.GenerationID).To(Equal("rhUbBjrtyweZBQO1U3Td81zyP9d46yEh"))
907+
Expect(str.Coordinators).To(ConsistOf([]string{
908+
"100.82.81.253:4500:tls",
909+
"100.82.71.5:4500:tls",
910+
"100.82.119.151:4500:tls",
911+
"100.82.122.125:4500:tls",
912+
"100.82.76.240:4500:tls",
913+
}))
914+
})
915+
})
916+
917+
When("the input is valid with DNS entries", func() {
918+
BeforeEach(func() {
919+
input = "fdb_cluster_52v1bpr8:rhUbBjrtyweZBQO1U3Td81zyP9d46yEh@coordinator1.test.svc.cluster.local:4500:tls,coordinator2.test.svc.cluster.local:4500:tls,coordinator2.test.svc.cluster.local:4500:tls"
920+
})
921+
922+
It("should be parsed correctly", func() {
923+
Expect(err).NotTo(HaveOccurred())
924+
Expect(str.DatabaseName).To(Equal("fdb_cluster_52v1bpr8"))
925+
Expect(str.GenerationID).To(Equal("rhUbBjrtyweZBQO1U3Td81zyP9d46yEh"))
926+
Expect(str.Coordinators).To(ConsistOf([]string{
927+
"coordinator1.test.svc.cluster.local:4500:tls",
928+
"coordinator2.test.svc.cluster.local:4500:tls",
929+
"coordinator2.test.svc.cluster.local:4500:tls",
930+
}))
931+
})
932+
})
933+
934+
When("the input is valid with IPv6 entries", func() {
935+
BeforeEach(func() {
936+
input = "fdb_cluster_52v1bpr8:rhUbBjrtyweZBQO1U3Td81zyP9d46yEh@[0100::2]:4500:tls,[0100::3]:4500:tls,[0100::4]:4500:tls"
937+
})
938+
939+
It("should be parsed correctly", func() {
940+
Expect(err).NotTo(HaveOccurred())
941+
Expect(str.DatabaseName).To(Equal("fdb_cluster_52v1bpr8"))
942+
Expect(str.GenerationID).To(Equal("rhUbBjrtyweZBQO1U3Td81zyP9d46yEh"))
943+
Expect(str.Coordinators).To(ConsistOf([]string{
944+
"[100::2]:4500:tls",
945+
"[100::3]:4500:tls",
946+
"[100::4]:4500:tls",
947+
}))
948+
})
949+
})
950+
951+
When("the input is valid with IPv6 and IPv4 entries", func() {
952+
BeforeEach(func() {
953+
input = "fdb_cluster_52v1bpr8:[email protected]:4500:tls,100.82.71.5:4500:tls,100.82.119.151:4500:tls,[0100::3]:4500:tls,[0100::4]:4500:tls"
954+
})
955+
956+
It("should be parsed correctly", func() {
957+
Expect(err).NotTo(HaveOccurred())
958+
Expect(str.DatabaseName).To(Equal("fdb_cluster_52v1bpr8"))
959+
Expect(str.GenerationID).To(Equal("rhUbBjrtyweZBQO1U3Td81zyP9d46yEh"))
960+
Expect(str.Coordinators).To(ConsistOf([]string{
961+
"100.82.81.253:4500:tls",
962+
"100.82.71.5:4500:tls",
963+
"100.82.119.151:4500:tls",
964+
"[100::3]:4500:tls",
965+
"[100::4]:4500:tls",
966+
}))
967+
})
824968
})
825969
})
826970

971+
DescribeTable("sanitize connection string description", func(input string, expected string) {
972+
Expect(SanitizeConnectionStringDescription(input)).To(Equal(expected))
973+
},
974+
Entry("without hyphen", "test", "test"),
975+
Entry("with hyphen", "test-string", "test_string"),
976+
)
977+
827978
When("formatting the connection string", func() {
828979
It("should be formatted correctly", func() {
829980
str := ConnectionString{
830981
DatabaseName: "test",
831982
GenerationID: "abcd",
832-
Coordinators: coordinatorsStr,
983+
Coordinators: coordinatorsList,
833984
}
834985
Expect(str.String()).To(Equal("test:[email protected]:4500,127.0.0.2:4500,127.0.0.3:4500"))
835986
})
@@ -840,7 +991,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
840991
str := ConnectionString{
841992
DatabaseName: "test",
842993
GenerationID: "abcd",
843-
Coordinators: coordinatorsStr,
994+
Coordinators: coordinatorsList,
844995
}
845996
err := str.GenerateNewGenerationID()
846997
Expect(err).NotTo(HaveOccurred())
@@ -853,7 +1004,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
8531004
str := ConnectionString{
8541005
DatabaseName: "test",
8551006
GenerationID: "abcd",
856-
Coordinators: coordinatorsStr,
1007+
Coordinators: coordinatorsList,
8571008
}
8581009
Expect(str.HasCoordinators(coordinators)).To(BeTrue())
8591010
// We have to copy the slice to prevent to modify the original slice

‎controllers/cluster_controller.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030
"k8s.io/apimachinery/pkg/runtime/serializer/yaml"
3131
"k8s.io/apimachinery/pkg/types"
3232
"k8s.io/apimachinery/pkg/util/uuid"
33-
"regexp"
3433
"sigs.k8s.io/controller-runtime/pkg/builder"
3534
"sigs.k8s.io/controller-runtime/pkg/handler"
3635
"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -490,8 +489,6 @@ func (r *FoundationDBClusterReconciler) releaseLock(logger logr.Logger, cluster
490489
return lockClient.ReleaseLock()
491490
}
492491

493-
var connectionStringNameRegex, _ = regexp.Compile("[^A-Za-z0-9_]")
494-
495492
// clusterSubReconciler describes a class that does part of the work of
496493
// reconciliation for a cluster.
497494
type clusterSubReconciler interface {

‎controllers/generate_initial_cluster_file.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func (g generateInitialClusterFile) reconcile(ctx context.Context, r *Foundation
9797
if cluster.Spec.PartialConnectionString.DatabaseName != "" {
9898
clusterName = cluster.Spec.PartialConnectionString.DatabaseName
9999
} else {
100-
clusterName = connectionStringNameRegex.ReplaceAllString(cluster.Name, "_")
100+
clusterName = fdbv1beta2.SanitizeConnectionStringDescription(cluster.Name)
101101
}
102102

103103
connectionString := fdbv1beta2.ConnectionString{DatabaseName: clusterName}
@@ -151,6 +151,12 @@ func (g generateInitialClusterFile) reconcile(ctx context.Context, r *Foundation
151151
connectionString.Coordinators = append(connectionString.Coordinators, coordinator.GetCoordinatorAddress(cluster, currentLocality).String())
152152
}
153153

154+
// Ensure that the connection string is in a valid format.
155+
err = connectionString.Validate()
156+
if err != nil {
157+
return &requeue{curError: err}
158+
}
159+
154160
cluster.Status.ConnectionString = connectionString.String()
155161
err = r.updateOrApply(ctx, cluster)
156162
if err != nil {

0 commit comments

Comments
 (0)