Ko 461 nodeport service v2 #177
Conversation
respringer
requested review from
bradfordcp,
jimdickinson,
johntrimble and
sandoichi
as code owners
docs/user/README.md
Outdated
| cqlSql: 30010 | ||
| broadcastSql: 30020 |
There was a problem hiding this comment.
| cqlSql: 30010 | |
| broadcastSql: 30020 | |
| cqlSsl: 30010 | |
| broadcastSsl: 30020 |
There was a problem hiding this comment.
and actually maybe SSL should be in caps, because golang idioms
| @@ -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) { | |||
There was a problem hiding this comment.
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.
| return 0 | ||
| } | ||
|
|
||
| if dc.Spec.Networking.NodePort.BroadcastSsl != 0 { |
There was a problem hiding this comment.
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.
| { | ||
| Name: "mgmt-api", Port: 8080, TargetPort: intstr.FromInt(8080), | ||
| }, | ||
| } | ||
|
|
||
| // If NodePort is enabled, it will take control of the native port | ||
| if !dc.IsNodePortEnabled() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I will put it back.
| TargetPort: intstr.FromInt(broadcastPort), | ||
| }, | ||
| { | ||
| Name: "native", |
There was a problem hiding this comment.
@JeremiahDJordan do you think cql is a better name here? what about broadcast vs. internode?
There was a problem hiding this comment.
Broadcast is wrong. I would call it internode. Both internode and native protocol (cql) ports have broadcast settings.
docs/user/README.md
Outdated
| networking: | ||
| nodePort: | ||
| cqlSql: 30010 | ||
| broadcastSql: 30020 |
There was a problem hiding this comment.
That was a typo. I pushed a fix.
|
|
||
| // 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) { | ||
| actualValue, ok := m[key].(string) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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 |
There was a problem hiding this comment.
It might be time to make these constants somewhere. Like defaultCqlPort or something.
There was a problem hiding this comment.
Good idea, I will do that.
|
By the way, the PR looks great! |
A NodePort service may be requested by setting the following fields:
The SSL versions of the ports may be requested:
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.