-
Notifications
You must be signed in to change notification settings - Fork 198
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.
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
Allow users to use well-known ports for client connections to both ingress controllers #997
Conversation
@@ -113,7 +113,7 @@ func generateExternalPorts(kc *v1beta1.KafkaCluster, brokerIds []int, | |||
Name: fmt.Sprintf(kafkautils.AllBrokerServiceTemplate, "tcp"), | |||
Protocol: string(corev1.ProtocolTCP), | |||
Port: externalListenerConfig.GetAnyCastPort(), | |||
TargetPort: &istioOperatorApi.IntOrString{IntOrString: intstr.FromInt(int(externalListenerConfig.GetAnyCastPort()))}, | |||
TargetPort: &istioOperatorApi.IntOrString{IntOrString: intstr.FromInt(int(externalListenerConfig.GetIngressControllerTargetPort()))}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - looks like named targetPort can't be used here, becasue istio-operator complains about not having an exact value for this field when it reconciles the IstioMeshGateway
resource. But I could me missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's saddening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, take the other PR's comment into account here
…ntroller resources creation
…st kafkacluster API
fc40df3
to
723e843
Compare
723e843
to
45c48e4
Compare
The changes in this validation function are not really required for the original purpose of this PR, it is more for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than my question on one of the cases, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, like the attention to small details like the healthcheck port. Thanks!
Description
Current implementation: when users set this
externalListener[x].anyCastPort
value, Koperator uses this directly as the corresponding container port for the envoy controller containers and target port for the associated services.envoy controller
When users use a well-known port (with port number < 1024), Koperator can handle this for
envoy
controller since theenvoy
container is run asroot
, for example:istioingress controller
When users use a well-known port, Koperator creates a
IstioMeshGateway
object with using that well-known port directly as thetargetPort
for one of the service ports, for example:And the resulting istioingress controller container fails to use this well-known port and therefore the kafka brokers can't be reached by external clients.
What is in this PR?
Introduce a new optional field,
ingressControllerTargetPort
underExternalListenerConfig
to allow users to specify the container port that they would like to use in the ingress controller. It also extends the validation webhook implementation to prevent users from using well-known port for theistioingress
controller's deployment.This way, users can use any
anyCastPort
values for client access while using either of the ingress controllersAdd checks to the KafkaCluster validation webhook (both create and update) to prevent invalid user configurations on external listeners
Type of Change
Checklist