Skip to content

Pod IP selection rules for dual-stack environments #794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jul 2, 2021
Merged

Pod IP selection rules for dual-stack environments #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3be2b9d
Add support for using a specific IP from the podIPs list.
brownleej Jun 26, 2021
9b3e0bc
Use the configured pod IP index to determine which public IP to use f…
brownleej Jun 27, 2021
20d18ae
Allow specifying an arbitrary pattern to determine which IP in the po…
brownleej Jun 27, 2021
f3aa981
Temporarily allow specifying the pod IP pattern as an annotation to h…
brownleej Jun 27, 2021
dd85473
Fix issues with parsing IPv6 addresses.
brownleej Jun 27, 2021
07dea65
Add an option to disable readiness probes.
brownleej Jun 27, 2021
a3887b0
Update documentation.
brownleej Jun 27, 2021
04a9cc0
Add a comment on the sidecar probe config in the default test cluster.
brownleej Jun 27, 2021
c68b424
Fix an old variable name.
brownleej Jun 28, 2021
71a6f07
Style fixes.
brownleej Jun 28, 2021
a29d083
Remove the black formatter from our python linting.
brownleej Jun 28, 2021
3dc3a25
Replace the custom pod IP patterns with a more explicit selector for …
brownleej Jun 30, 2021
f62a936
Update based on PR feedback.
brownleej Jul 2, 2021
fe13130
Fix a use of a renamed method.
brownleej Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 45 additions & 12 deletions api/v1beta1/foundationdbcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,12 @@ type FoundationDBClusterSpec struct {

// Services defines the configuration for services that sit in front of our
// pods.
// Deprecated: Use Routing instead.
Services ServiceConfig `json:"services,omitempty"`

// Routing defines the configuration for routing to our pods.
Routing RoutingConfig `json:"routing,omitempty"`

// IgnoreUpgradabilityChecks determines whether we should skip the check for
// client compatibility when performing an upgrade.
IgnoreUpgradabilityChecks bool `json:"ignoreUpgradabilityChecks,omitempty"`
Expand Down Expand Up @@ -1568,23 +1572,28 @@ type ProcessAddress struct {
// representation.
func ParseProcessAddress(address string) (ProcessAddress, error) {
result := ProcessAddress{}
components := strings.Split(address, ":")

if len(components) < 2 {
ipEnd := strings.Index(address, "]:") + 1
Copy link
Member

Choose a reason for hiding this comment

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

Instead of building this custom logic I would prefer to use net.SplitHostPort that should make everything more robust.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would that work with the custom flags in our addresses like the :tls suffix?

Copy link
Member

Choose a reason for hiding this comment

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

I got a solution for that in mind (more or less the same FDB currently does). I would work on that in #352 that should simplify thee logic here. For the PR itself that's fine and we can improve things step by step.

if ipEnd == 0 {
ipEnd = strings.Index(address, ":")
}
if ipEnd == -1 {
return result, fmt.Errorf("invalid address: %s", address)
}

result.IPAddress = components[0]
result.IPAddress = address[:ipEnd]

port, err := strconv.Atoi(components[1])
components := strings.Split(address[ipEnd+1:], ":")

port, err := strconv.Atoi(components[0])
if err != nil {
return result, err
}
result.Port = port

if len(components) > 2 {
result.Flags = make(map[string]bool, len(components)-2)
for _, flag := range components[2:] {
if len(components) > 1 {
result.Flags = make(map[string]bool, len(components)-1)
for _, flag := range components[1:] {
result.Flags[flag] = true
}
}
Expand Down Expand Up @@ -1813,10 +1822,13 @@ type DataCenter struct {
// ContainerOverrides provides options for customizing a container created by
// the operator.
type ContainerOverrides struct {
// EnableLivenessProbe defines if the sidecar should have a livenessProbe in addition
// to the readinessProbe. This setting will be enabled per default in the 1.0.0 release.
// EnableLivenessProbe defines if the sidecar should have a livenessProbe.
// This setting will be ignored on the main container.
EnableLivenessProbe *bool `json:"enableLivenessProbe,omitempty"`

// EnableReadinessProbe defines if the sidecar should have a readinessProbe.
// This setting will be ignored on the main container.
EnableLivenessProbe bool `json:"enableLivenessProbe,omitempty"`
EnableReadinessProbe *bool `json:"enableReadinessProbe,omitempty"`

// EnableTLS controls whether we should be listening on a TLS connection.
EnableTLS bool `json:"enableTls,omitempty"`
Expand Down Expand Up @@ -1994,13 +2006,13 @@ func (cluster *FoundationDBCluster) GetLockID() string {
// NeedsExplicitListenAddress determines whether we pass a listen address
// parameter to fdbserver.
func (cluster *FoundationDBCluster) NeedsExplicitListenAddress() bool {
source := cluster.Spec.Services.PublicIPSource
source := cluster.Spec.Routing.PublicIPSource
return source != nil && *source == PublicIPSourceService
}

// GetPublicIPSource returns the set PublicIPSource or the default PublicIPSourcePod
func (cluster *FoundationDBCluster) GetPublicIPSource() PublicIPSource {
source := cluster.Spec.Services.PublicIPSource
source := cluster.Spec.Routing.PublicIPSource
if source == nil {
return PublicIPSourcePod
}
Expand Down Expand Up @@ -2383,6 +2395,7 @@ type LockDenyListEntry struct {
}

// ServiceConfig allows configuring services that sit in front of our pods.
// Deprecated: Use RoutingConfig instead.
type ServiceConfig struct {
// Headless determines whether we want to run a headless service for the
// cluster.
Expand All @@ -2395,6 +2408,26 @@ type ServiceConfig struct {
PublicIPSource *PublicIPSource `json:"publicIPSource,omitempty"`
}

// RoutingConfig allows configuring routing to our pods, and services that sit
// in front of them.
type RoutingConfig struct {
// Headless determines whether we want to run a headless service for the
// cluster.
HeadlessService *bool `json:"headlessService,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking we never use the headless service for routing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but I felt that "Routing" was closer than "Service" for this group of settings, since in the long term the plan is to use the headless service for discovery. Do you have any other suggestions for what we could call this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I felt like Routing captured the intent of these fields better than Service, but I'm open to other suggestions about how to make the meaning clearer.

Copy link
Member

Choose a reason for hiding this comment

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

What about Discovery? I mean that's also an overloaded term. Otherwise I don't have any better idea :( Like always:

"There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors." :)


// PublicIPSource specifies what source a process should use to get its
// public IPs.
//
// This supports the values `pod` and `service`.
PublicIPSource *PublicIPSource `json:"publicIPSource,omitempty"`

// PodIPFamily tells the pod which family of IP addresses to use.
// You can use 4 to represent IPv4, and 6 to represent IPv6.
// This feature is only supported in FDB 7.0 or later, and requires
// dual-stack support in your Kubernetes environment.
PodIPFamily *int `json:"podIPFamily,omitempty"`
}

// RequiredAddressSet provides settings for which addresses we need to listen
// on.
type RequiredAddressSet struct {
Expand Down
21 changes: 21 additions & 0 deletions api/v1beta1/foundationdbcluster_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,6 +2276,14 @@ var _ = Describe("[api] FoundationDBCluster", func() {
}))
Expect(address.String()).To(Equal("127.0.0.1:4501"))

address, err = ParseProcessAddress("[::1]:4501")
Expect(err).NotTo(HaveOccurred())
Expect(address).To(Equal(ProcessAddress{
IPAddress: "[::1]",
Port: 4501,
}))
Expect(address.String()).To(Equal("[::1]:4501"))

address, err = ParseProcessAddress("127.0.0.1:bad")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("strconv.Atoi: parsing \"bad\": invalid syntax"))
Expand Down Expand Up @@ -2810,6 +2818,19 @@ var _ = Describe("[api] FoundationDBCluster", func() {
},
},
}),
Entry("TLS IPv6",
testCase{
cmdline: "/usr/bin/fdbserver --class=stateless --cluster_file=/var/fdb/data/fdb.cluster --datadir=/var/fdb/data --locality_instance_id=stateless-9 --locality_machineid=machine1 --locality_zoneid=zone1 --logdir=/var/log/fdb-trace-logs --loggroup=test --public_address=[::1]:4500:tls --seed_cluster_file=/var/dynamic-conf/fdb.cluster",
expected: []ProcessAddress{
{
IPAddress: "[::1]",
Port: 4500,
Flags: map[string]bool{
"tls": true,
},
},
},
}),
Entry("TLS and no-TLS",
testCase{
cmdline: "/usr/bin/fdbserver --class=stateless --cluster_file=/var/fdb/data/fdb.cluster --datadir=/var/fdb/data --locality_instance_id=stateless-9 --locality_machineid=machine1 --locality_zoneid=zone1 --logdir=/var/log/fdb-trace-logs --loggroup=test --public_address=1.2.3.4:4501,1.2.3.4:4500:tls --seed_cluster_file=/var/dynamic-conf/fdb.cluster",
Expand Down
41 changes: 41 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,6 +1320,8 @@ spec:
properties:
enableLivenessProbe:
type: boolean
enableReadinessProbe:
type: boolean
enableTls:
type: boolean
env:
Expand Down Expand Up @@ -7564,6 +7566,15 @@ spec:
x-kubernetes-int-or-string: true
type: object
type: object
routing:
properties:
headlessService:
type: boolean
podIPFamily:
type: integer
publicIPSource:
type: string
type: object
runningVersion:
type: string
seedConnectionString:
Expand All @@ -7579,6 +7590,8 @@ spec:
properties:
enableLivenessProbe:
type: boolean
enableReadinessProbe:
type: boolean
enableTls:
type: boolean
env:
Expand Down
8 changes: 6 additions & 2 deletions config/tests/base/cluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ spec:
# Use fake fault domains to support running in a single-node Kubernetes
# cluster.
key: foundationdb.org/none
services:
headless: true
routing:
headlessService: true
automationOptions:
replacements:
# Enable automatic replacements. This will be the default in the future.
Expand All @@ -20,6 +20,10 @@ spec:
# Enable a dedicated cluster controller process to test for surprising
# behavior with having a process class with an underscore.
cluster_controller: 1
sidecarContainer:
# Change default probes to match the defaults we will apply in the future.
enableReadinessProbe: false
enableLivenessProbe: true
processes:
general:
customParameters:
Expand Down
2 changes: 1 addition & 1 deletion controllers/add_pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func (a AddPods) Reconcile(r *FoundationDBClusterReconciler, context ctx.Context

pod.ObjectMeta.Annotations[fdbtypes.LastConfigMapKey] = configMapHash

if *cluster.Spec.Services.PublicIPSource == fdbtypes.PublicIPSourceService {
if *cluster.Spec.Routing.PublicIPSource == fdbtypes.PublicIPSourceService {
service := &corev1.Service{}
err = r.Get(context, types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}, service)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions controllers/add_services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (a AddServices) Reconcile(r *FoundationDBClusterReconciler, context ctx.Con
}
}

if *cluster.Spec.Services.PublicIPSource == fdbtypes.PublicIPSourceService {
if *cluster.Spec.Routing.PublicIPSource == fdbtypes.PublicIPSourceService {
for _, processGroup := range cluster.Status.ProcessGroups {
if processGroup.Remove {
continue
Expand Down Expand Up @@ -90,7 +90,7 @@ func (a AddServices) Reconcile(r *FoundationDBClusterReconciler, context ctx.Con

// GetHeadlessService builds a headless service for a FoundationDB cluster.
func GetHeadlessService(cluster *fdbtypes.FoundationDBCluster) *corev1.Service {
headless := cluster.Spec.Services.Headless
headless := cluster.Spec.Routing.HeadlessService
if headless == nil || !*headless {
return nil
}
Expand Down
8 changes: 4 additions & 4 deletions controllers/add_services_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ var _ = Describe("add_services", func() {
BeforeEach(func() {
cluster = createDefaultCluster()
source := fdbtypes.PublicIPSourceService
cluster.Spec.Services.PublicIPSource = &source
cluster.Spec.Routing.PublicIPSource = &source
enabled := true
cluster.Spec.Services.Headless = &enabled
cluster.Spec.Routing.HeadlessService = &enabled

err = k8sClient.Create(context.TODO(), cluster)
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -108,7 +108,7 @@ var _ = Describe("add_services", func() {
Context("with the pod public IP source", func() {
BeforeEach(func() {
source := fdbtypes.PublicIPSourcePod
cluster.Spec.Services.PublicIPSource = &source
cluster.Spec.Routing.PublicIPSource = &source
})

It("should not requeue", func() {
Expand Down Expand Up @@ -161,7 +161,7 @@ var _ = Describe("add_services", func() {
Context("with the headless service disabled", func() {
BeforeEach(func() {
enabled := false
cluster.Spec.Services.Headless = &enabled
cluster.Spec.Routing.HeadlessService = &enabled
})

It("should not requeue", func() {
Expand Down
Loading