Skip to content
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

Ko 461 nodeport service v2 #177

Merged
merged 16 commits into from
Jul 28, 2020
Merged

Ko 461 nodeport service v2 #177

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0032058
Initial NodePortConfig added
respringer Jul 24, 2020
177ebd0
added configgen port setting
respringer Jul 24, 2020
f714499
Passes HOST_IP to configbuilder
respringer Jul 24, 2020
cd34088
replaced accidentally removed function
respringer Jul 24, 2020
111a036
WIP - create nodeport service
respringer Jul 24, 2020
88765b2
WIP - created nodeport test
respringer Jul 24, 2020
bb2c9d4
WIP - test fixes
respringer Jul 24, 2020
12729e4
fixed a port and introduced a couple of test helper methods
respringer Jul 27, 2020
2c0e453
do not set a native port in the normal service if the nodeport servic…
respringer Jul 27, 2020
9e91c5c
use cql port instead of cqlSsl port in the test
respringer Jul 28, 2020
7bccdff
preserve ClusterIP of existing services
respringer Jul 28, 2020
9cb1852
the test runs
respringer Jul 28, 2020
15004dd
updated the user doc for nodeport
respringer Jul 28, 2020
50bc3e5
Port names now use SSL instead of Ssl
respringer Jul 28, 2020
8848f81
Introduced DefaultCqlPort and DefaultBroadcastPort
respringer Jul 28, 2020
380e7d9
Do not remove native port from normal service when nodeport service i…
respringer Jul 28, 2020
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
14 changes: 14 additions & 0 deletions charts/cass-operator-chart/templates/customresourcedefinition.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,20 @@ spec:
- serverSecretName
type: object
type: object
networking:
properties:
nodePort:
properties:
broadcast:
type: integer
broadcastSsl:
type: integer
cql:
type: integer
cqlSsl:
type: integer
type: object
type: object
nodeSelector:
additionalProperties:
type: string
Expand Down
18 changes: 18 additions & 0 deletions docs/user/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,24 @@ spec:
serverImage: private-docker-registry.example.com/dse-img/dse:5f6e7d8c
```

## Configuring a NodePort service

A NodePort service may be requested by setting the following fields:

networking:
nodePort:
cql: 30001
broadcast: 30002

The SSL versions of the ports may be requested:

networking:
nodePort:
cqlSql: 30010
broadcastSql: 30020
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cqlSql: 30010
broadcastSql: 30020
cqlSsl: 30010
broadcastSsl: 30020

Copy link
Collaborator

Choose a reason for hiding this comment

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

and actually maybe SSL should be in caps, because golang idioms

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doh!

Copy link
Collaborator

Choose a reason for hiding this comment

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

broadcastSsl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was a typo. I pushed a fix.


If any of the nodePort fields have been configured then a NodePort service will be created that routes from the specified external port to the identically numbered internal port. Cassandra will be configured to listen on the specified ports. Additionally, the standard service will no longer define a "native" port.

# Using Your Cluster

## Connecting from inside the Kubernetes cluster
Expand Down
26 changes: 24 additions & 2 deletions mage/ginkgo/lib.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ func (ns *NsWrapper) WaitForOutputContainsAndLog(description string, kcmd kubect
Expect(execErr).ToNot(HaveOccurred())
}


func (ns *NsWrapper) WaitForDatacenterCondition(dcName string, conditionType string, value string) {
step := fmt.Sprintf("checking that dc condition %s has value %s", conditionType, value)
json := fmt.Sprintf("jsonpath={.status.conditions[?(.type=='%s')].status}", conditionType)
Expand All @@ -223,7 +222,6 @@ func (ns *NsWrapper) WaitForDatacenterCondition(dcName string, conditionType str
ns.WaitForOutputAndLog(step, k, value, 600)
}


func (ns *NsWrapper) WaitForDatacenterToHaveNoPods(dcName string) {
step := "checking that no dc pods remain"
json := "jsonpath={.items}"
Expand Down Expand Up @@ -369,3 +367,27 @@ func (ns NsWrapper) HelmInstall(chartPath string) {
err := helm_util.Install(chartPath, "cass-operator", ns.Namespace, overrides)
mageutil.PanicOnError(err)
}

// Note that the actual value will be cast to a string before the comparison with the expectedValue
func (ns NsWrapper) ExpectKeyValue(m map[string]interface{}, key string, expectedValue string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ExpectKeyValues(), which takes a whole map, and the helper function ExpectKeyValue() are a first-pass at a generic way to validate key/values. Right now everything is cast to a string. I added a helper for dealing with Float64 values because the Ports from the service were coming back as Float64s for some reason. I am open to suggestion for improvements, but we may want to handle them in a follow-up ticket.

actualValue, ok := m[key].(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand the intent of this code correctly, I don't think it works the way you expect. Floats, ints, etc. cannot be cast to strings in this manner:

https://play.golang.org/p/lIg2pmmYczZ

You would have to parse them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This tries to interpret the value as a string. If that was possible, then ok is set to true. If that was not possible, for instance, the value was a float64, then ok is set to false.

if !ok {
// Note: floats will end up as strings with six decimal points
// example: "12.000000"
tryFloat64, ok := m[key].(float64)
if !ok {
msg := fmt.Sprintf("Actual value for key %s is not expected type", key)
err := fmt.Errorf(msg)
Expect(err).ToNot(HaveOccurred())
}
actualValue = fmt.Sprintf("%f", tryFloat64)
}
Expect(actualValue).To(Equal(expectedValue), "Expected %s %s to be %s", key, m[key], expectedValue)
}

// Compare all key/values from an expected map to an actual map
func (ns NsWrapper) ExpectKeyValues(actual map[string]interface{}, expected map[string]string) {
for key := range expected {
ns.ExpectKeyValue(actual, key, expected[key])
}
}
14 changes: 14 additions & 0 deletions operator/deploy/crds/cassandra.datastax.com_cassandradatacenters_crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,20 @@ spec:
- serverSecretName
type: object
type: object
networking:
properties:
nodePort:
properties:
broadcast:
type: integer
broadcastSsl:
type: integer
cql:
type: integer
cqlSsl:
type: integer
type: object
type: object
nodeSelector:
additionalProperties:
type: string
Expand Down
83 changes: 80 additions & 3 deletions operator/pkg/apis/cassandra/v1beta1/cassandradatacenter_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,29 @@ type CassandraDatacenterSpec struct {
// Cassandra users to bootstrap
Users []CassandraUser `json:"users,omitempty"`

Networking *NetworkingConfig `json:"networking,omitempty"`

AdditionalSeeds []string `json:"additionalSeeds,omitempty"`

Reaper *ReaperConfig `json:"reaper,omitempty"`
}

type NetworkingConfig struct {
NodePort *NodePortConfig `json:"nodePort,omitempty"`
}

type NodePortConfig struct {
Cql int `json:"cql,omitempty"`
CqlSsl int `json:"cqlSsl,omitempty"`
Broadcast int `json:"broadcast,omitempty"`
BroadcastSsl int `json:"broadcastSsl,omitempty"`
}

// Is the NodePort service enabled?
func (dc *CassandraDatacenter) IsNodePortEnabled() bool {
return dc.Spec.Networking != nil && dc.Spec.Networking.NodePort != nil
}

type DseWorkloads struct {
AnalyticsEnabled bool `json:"analyticsEnabled,omitempty"`
GraphEnabled bool `json:"graphEnabled,omitempty"`
Expand Down Expand Up @@ -491,6 +509,10 @@ func (dc *CassandraDatacenter) GetDatacenterServiceName() string {
return dc.Spec.ClusterName + "-" + dc.Name + "-service"
}

func (dc *CassandraDatacenter) GetNodePortServiceName() string {
return dc.Spec.ClusterName + "-" + dc.Name + "-node-port-service"
}

func (dc *CassandraDatacenter) ShouldGenerateSuperuserSecret() bool {
return len(dc.Spec.SuperuserSecretName) == 0
}
Expand Down Expand Up @@ -533,13 +555,28 @@ func (dc *CassandraDatacenter) GetConfigAsJSON() (string, error) {
}
}

cql := 0
cqlSsl := 0
broadcast := 0
broadcastSsl := 0
if dc.IsNodePortEnabled() {
cql = dc.Spec.Networking.NodePort.Cql
cqlSsl = dc.Spec.Networking.NodePort.CqlSsl
broadcast = dc.Spec.Networking.NodePort.Broadcast
broadcastSsl = dc.Spec.Networking.NodePort.BroadcastSsl
}

modelValues := serverconfig.GetModelValues(
seeds,
dc.Spec.ClusterName,
dc.Name,
graphEnabled,
solrEnabled,
sparkEnabled)
sparkEnabled,
cql,
cqlSsl,
broadcast,
broadcastSsl)

var modelBytes []byte

Expand Down Expand Up @@ -569,21 +606,61 @@ func (dc *CassandraDatacenter) GetConfigAsJSON() (string, error) {
return modelParsed.String(), nil
}

// Gets the defined CQL port for NodePort.
// 0 will be returned if NodePort is not configured.
// The SSL port will be returned if it is defined,
// otherwise the normal CQL port will be used.
func (dc *CassandraDatacenter) GetNodePortCqlPort() int {
if !dc.IsNodePortEnabled() {
return 0
}

if dc.Spec.Networking.NodePort.CqlSsl != 0 {
return dc.Spec.Networking.NodePort.CqlSsl
} else if dc.Spec.Networking.NodePort.Cql != 0 {
return dc.Spec.Networking.NodePort.Cql
} else {
return 9042
}
}

// Gets the defined broadcast/intranode port for NodePort.
// 0 will be returned if NodePort is not configured.
// The SSL port will be returned if it is defined,
// otherwise the normal broadcast port will be used.
func (dc *CassandraDatacenter) GetNodePortBroadcastPort() int {
if !dc.IsNodePortEnabled() {
return 0
}

if dc.Spec.Networking.NodePort.BroadcastSsl != 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The port fallback logic is a bit squirrelly and went through multiple designs via slack/standup conversations. What I have landed on is nodeport ssl ports take first priority, then nodeport non-ssl ports, then the defaults of 9042 for native and 7000 for broadcast. If the nodeport native/cql port is defined, then the native port will be omitted from the normal service.

return dc.Spec.Networking.NodePort.BroadcastSsl
} else if dc.Spec.Networking.NodePort.Broadcast != 0 {
return dc.Spec.Networking.NodePort.Broadcast
} else {
return 7000
}
}

// GetContainerPorts will return the container ports for the pods in a statefulset based on the provided config
func (dc *CassandraDatacenter) GetContainerPorts() ([]corev1.ContainerPort, error) {

cqlPort := 9042
broadcastPort := 7000
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be time to make these constants somewhere. Like defaultCqlPort or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I will do that.


ports := []corev1.ContainerPort{
{
// Note: Port Names cannot be more than 15 characters
Name: "native",
ContainerPort: 9042,
ContainerPort: int32(cqlPort),
},
{
Name: "inter-node-msg",
ContainerPort: 8609,
},
{
Name: "intra-node",
ContainerPort: 7000,
ContainerPort: int32(broadcastPort),
},
{
Name: "tls-intra-node",
Expand Down
42 changes: 42 additions & 0 deletions operator/pkg/apis/cassandra/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.

54 changes: 50 additions & 4 deletions operator/pkg/reconciliation/constructor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,20 @@ func newServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev1.Servi
service := makeGenericHeadlessService(dc)
service.ObjectMeta.Name = svcName
service.Spec.Ports = []corev1.ServicePort{
// Note: Port Names cannot be more than 15 characters
{
Name: "native", Port: 9042, TargetPort: intstr.FromInt(9042),
},
{
Name: "mgmt-api", Port: 8080, TargetPort: intstr.FromInt(8080),
},
}

// If NodePort is enabled, it will take control of the native port
if !dc.IsNodePortEnabled() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why disable this when node port is configured vs just put the port number here? I think taking it out might break some folks with in cluster clients

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will put it back.

service.Spec.Ports = append(service.Spec.Ports,
corev1.ServicePort{
Name: "native", Port: 9042, TargetPort: intstr.FromInt(9042),
},
)
}

addHashAnnotation(service)

return service
Expand Down Expand Up @@ -122,6 +127,39 @@ func newEndpointsForAdditionalSeeds(dc *api.CassandraDatacenter) *corev1.Endpoin
return &endpoints
}

// newNodePortServiceForCassandraDatacenter creates a headless service owned by the CassandraDatacenter,
// that preserves the client source IPs
func newNodePortServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev1.Service {
service := makeGenericHeadlessService(dc)
service.ObjectMeta.Name = dc.GetNodePortServiceName()

service.Spec.Type = "NodePort"
// Note: ClusterIp = "None" is not valid for NodePort
service.Spec.ClusterIP = ""
service.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal

cqlPort := dc.GetNodePortCqlPort()
broadcastPort := dc.GetNodePortBroadcastPort()

service.Spec.Ports = []corev1.ServicePort{
// Note: Port Names cannot be more than 15 characters
{
Name: "broadcast",
Port: int32(broadcastPort),
NodePort: int32(broadcastPort),
TargetPort: intstr.FromInt(broadcastPort),
},
{
Name: "native",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JeremiahDJordan do you think cql is a better name here? what about broadcast vs. internode?

Copy link
Member

Choose a reason for hiding this comment

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

Broadcast is wrong. I would call it internode. Both internode and native protocol (cql) ports have broadcast settings.

Port: int32(cqlPort),
NodePort: int32(cqlPort),
TargetPort: intstr.FromInt(cqlPort),
},
}

return service
}

// newAllPodsServiceForCassandraDatacenter creates a headless service owned by the CassandraDatacenter,
// which covers all server pods in the datacenter, whether they are ready or not
func newAllPodsServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev1.Service {
Expand Down Expand Up @@ -504,6 +542,12 @@ func buildInitContainers(dc *api.CassandraDatacenter, rackName string) ([]corev1
}
serverCfg.VolumeMounts = []corev1.VolumeMount{serverCfgMount}

// Convert the bool to a string for the env var setting
useHostIpForBroadcast := "false"
if dc.IsNodePortEnabled() {
useHostIpForBroadcast = "true"
}

configData, err := dc.GetConfigAsJSON()
if err != nil {
return nil, err
Expand All @@ -512,6 +556,8 @@ func buildInitContainers(dc *api.CassandraDatacenter, rackName string) ([]corev1
serverCfg.Env = []corev1.EnvVar{
{Name: "CONFIG_FILE_DATA", Value: configData},
{Name: "POD_IP", ValueFrom: selectorFromFieldPath("status.podIP")},
{Name: "HOST_IP", ValueFrom: selectorFromFieldPath("status.hostIP")},
{Name: "USE_HOST_IP_FOR_BROADCAST", Value: useHostIpForBroadcast},
{Name: "RACK_NAME", Value: rackName},
{Name: "PRODUCT_VERSION", Value: serverVersion},
{Name: "PRODUCT_NAME", Value: dc.Spec.ServerType},
Expand Down
Loading