EdgeFS: Allow control over K8s service type via CRD #3516
Conversation
pkg/operator/edgefs/isgw/isgw.go
Outdated
| @@ -139,7 +139,7 @@ func (c *ISGWController) makeISGWService(name, svcname, namespace string, isgwSp | |||
| }, | |||
| Spec: v1.ServiceSpec{ | |||
| Selector: labels, | |||
| Type: v1.ServiceTypeNodePort, | |||
| Type: v1.ServiceType(isgwSpec.ServiceType), | |||
There was a problem hiding this comment.
I think better to add method to resolve string of isgwSpec.ServiceType to correct v1.ServiceType, due to typo in service.ServiceType
There was a problem hiding this comment.
There is already code to handle the error in the event the operator tries to create a bad service, although it is a bit lacking (for example, if a service fails to start, the pod will be left dangling).
2019-07-25 13:21:02.926979 E | edgefs-op-s3: failed to create s3 s3-cola. failed to create s3 service. Service "rook-edgefs-s3-s3-cola" is invalid: spec.type: Unsupported value: "NidePort": supported values: "ClusterIP", "ExternalName", "LoadBalancer", "NodePort"
I had assumed that this would be sufficient, since it is the exact error a user would receive if they tried to create an off-spec Service in K8s. I can certainly add checks to validateService() if you think that would be a better way of handling the input.
There was a problem hiding this comment.
I think, better solution is to create method like this:
func ParseServiceType(serviceType string) ServiceType {
if strings.EqualFold(serviceType, string(ServiceTypeNodePort)) {
return ServiceTypeNodePort
}
return ServiceTypeClusterIP
}
What do you think?
There was a problem hiding this comment.
And add current service type to log plz
There was a problem hiding this comment.
that looks very similar to something I was doing before. I'll bring it back.
There was a problem hiding this comment.
Added ServiceType check to the validateService() function
| httpPort := v1.ServicePort{Name: "port", Port: int32(s3Spec.Port), Protocol: v1.ProtocolTCP} | ||
| httpsPort := v1.ServicePort{Name: "secure-port", Port: int32(s3Spec.SecurePort), Protocol: v1.ProtocolTCP} | ||
|
|
||
| if s3Spec.ExternalPort != 0 { |
There was a problem hiding this comment.
In case of ClusterIP service type, should be possible to add NodePorts?
There was a problem hiding this comment.
No, by specification NodePort cannot be used for a ServiceType of 'ClusterIP'. The K8s apiserver will reject this if it is invalid
2019-07-25 13:33:28.254349 E | edgefs-op-s3: failed to create s3 s3-cola. failed to create s3 service. Service "rook-edgefs-s3-s3-cola" is invalid: [spec.ports[1].nodePort: Forbidden: may not be used when `type` is 'ClusterIP', spec.ports[2].nodePort: Forbidden: may not be used when `type` is 'ClusterIP']
Same comments as above. If this should be validated by the operator instead of the K8s API, I can add those checks.
There was a problem hiding this comment.
Yes, i think you shoud add check to operator. Thanks
There was a problem hiding this comment.
Added port check to the validateService() function
NullOranje
force-pushed
the
edgefs-configurable-servicetypes
branch
2 times, most recently
from
d66677d
to
d0cdb5c
Compare
Signed-off-by: Nick McKinney <nmckinney@breakpoint-labs.com>
NullOranje
force-pushed
the
edgefs-configurable-servicetypes
branch
from
d0cdb5c
to
d9aec6b
Compare
Description of your changes:
Allow users to define Kubernetes users to define ServiceType and NodePort via the CRD spec. This changes the current behavior, which deploys all services into the K8s cluster as NodePort services on auto-selected ports. The intent is to provide users more control over services and make accessing services via other methods simpler (e.g., LaodBalancer, ingress controller). It also reduces the number of ports exposed outside of the Kubernetes cluster.
Which issue is resolved by this Pull Request:
N/A
Checklist:
make codegen) has been run to update object specifications, if necessary.[test edgefs]