Add Listener port validation

[mihaialexandrescu]

Q	A
Bug fix?	no
New feature?	no
API breaks?	no
Deprecations?	no
Related tickets	fixes #913
License	Apache 2.0

What's in this PR?

This PR aims to add some validation for the port numbers under spec.listenersConfig of the KafkaCluster CR.

Example config pointing out the fields this PR adds validation for :

apiVersion: kafka.banzaicloud.io/v1beta1
kind: KafkaCluster
spec:
  brokers:
    - id: 0
      brokerConfigGroup: "default"
    ... ...
  ... ...
  listenersConfig:
    internalListeners:
      - type: "plaintext"
        name: "internal"
        containerPort: 29092     <<<  
        usedForInnerBrokerCommunication: true
      ... ...
    externalListeners:
      - type: "plaintext"
        name: "external1"
        externalStartingPort: 19090     <<< 
        containerPort: 9094    <<<
      ... ...
  ... ...

The validation logic added in this PR falls under 2 categories :

1. OpenAPIv3 Schema validations (via kubebuilder markers)

• this performs an initial sanity check (value between 1 and 65535; 0 is acceptable for externalStartingPort) directly on the input value
• applied in this PR to both containerPort (via CommonListenerSpec; applies to both internal and external listeners) and to externalListeners[].externalStartingPort

2. New functions added to the validating webhook (for both Create and Update operations) for KafkaCluster to check that:

• the external port we get from externalStartingPort + brokers[].id is sane (1 < value < 65535)
• the containerPort is not duplicated between listeners because that would lead to a Duplicate value error when creating the Kafka broker Service (headless or "all-brokers") (just a general rule of K8s Services)
  • sample of such an error for the headless service (truncated ; note the same Port reused under different spec.ports[index]):

    "errorVerbose":"creating resource failed: Service "kafka-headless" is invalid: [spec.ports[1]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", AppProtocol:(*string)(nil), Port:29095, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}, spec.ports[3]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", AppProtocol:(*string)(nil), Port:29095, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}]

This PR also includes :

• updates to the validatingwebhook CR for the RBAC to register for Create operations (as well as the already existing Update operations) and what I thought are the necessary translations of that into the helm charts.
• unit tests for the new functions used by the webhook.
• a new sentinel error for the externalStartingPort case, an exported function to help "recognize" it and the unit test for it (IsAdmissionInvalidExternalListenerPort())

The new unit tests use testify/require.
Outdated: I chose to do matching with the ElementsMatch function because order doesn't matter even though it is deterministic. With the current test cases the Equal function could be used just as well.
Update: In kafkacluster_validator_test.go matching is done with require.Equal() per the advice and explanations received during review.

Why?

I reckon this deals with most of #913 (and a bit more).

Additional context

Tested on a real cluster (with make deploy) and, for the webhook, tested both Create and Update operations (with both kubectl apply and kubectl edit for Update).

Example inputs and results:

1. OpenAPIv3 Schema validation:

    externalListeners:
      - type: "plaintext"
        name: "external1"
        externalStartingPort: 65536   << illegal value
        containerPort: 9094

Results in :

The KafkaCluster "kafka" is invalid: spec.listenersConfig.externalListeners.externalStartingPort: Invalid value: 65536: spec.listenersConfig.externalListeners.externalStartingPort in body should be less than or equal to 65535

Also:

    internalListeners:
      - type: "plaintext"
        name: "internal"
        containerPort: 89092
        usedForInnerBrokerCommunication: true

Results in :

The KafkaCluster "kafka" is invalid: spec.listenersConfig.internalListeners.containerPort: Invalid value: 89092: spec.listenersConfig.internalListeners.containerPort in body should be less than or equal to 65535

2. Webhook validation :

apiVersion: kafka.banzaicloud.io/v1beta1
kind: KafkaCluster
spec:
  brokers:
    - id: 0
      brokerConfigGroup: "default"
    - id: 1
      brokerConfigGroup: "default"
    - id: 200
      brokerConfigGroup: "default"
  listenersConfig:
    internalListeners:
      - type: "plaintext"
        name: "internal"
        containerPort: 29095    <<< subsequent uses of this port number (regardless of listener type) are reported as "Duplicate value" errors
        usedForInnerBrokerCommunication: true
      - type: "plaintext"
        name: "controller"
        containerPort: 29095     <<< duplicate value
        usedForInnerBrokerCommunication: false
        usedForControllerCommunication: true
    externalListeners:
      - type: "plaintext"
        name: "external1"
        externalStartingPort: 65535   <<< causes external port for brokers 1 and 200 to be out of range
        containerPort: 9094
      - type: "plaintext"
        name: "external2"
        externalStartingPort: 65400   <<< causes external port for broker 200 to be out of range
        containerPort: 29095     <<< duplicate value

Results in :

The KafkaCluster "kafka" is invalid:
* spec.listenersConfig.internalListeners[1].containerPort: Duplicate value: 29095
* spec.listenersConfig.externalListeners[1].containerPort: Duplicate value: 29095
* spec.listenersConfig.externalListeners[0].externalStartingPort: Invalid value: 65535: invalid external listener starting port number: ExternalListener 'external1' would generate external access port numbers (externalStartingPort + Broker ID) that are out of range (not between 1 and 65535) for brokers [1 200]
* spec.listenersConfig.externalListeners[1].externalStartingPort: Invalid value: 65400: invalid external listener starting port number: ExternalListener 'external2' would generate external access port numbers (externalStartingPort + Broker ID) that are out of range (not between 1 and 65535) for brokers [200]

We get the same messages/errors when using kubectl apply/create and when using kubectl edit (depending on the operation Create/Update being performed).

externalListeners[].accessMethod: NodePort

The checks proposed in this PR address generic well-known bounds for port numbers.
They don't address in any way the more restrictive port range at play when externalListeners[].accessMethod: NodePort --> the issue I detect here is that the nodePort range is a kubeapiserver input flag/parameter and I don't know how to query for that... also, even if we could do that we can't guarantee that a value is not already taken by another service. For this case, I think we'll have to keep relying on errors from the K8s Service controller during deployment.

Naming scheme

Please also provide feedback on the name and message string of the new sentinel error in pkg/webhooks/errors.go and the exported function meant to "recognize it" (IsAdmission...()).

Please let me know if there are other cases I missed in the unit tests or, worse, the logic.

[panyuenlau]

Thanks for the contribution! Left some comments

[Kuvesz]

Looks good to me with the update Darren proposed regarding the t.Errorf vs Require logic!

[Kuvesz]

LGTM! Thanks for the work. :)

[pregnor]

LGTM at a glance with 1 comment.

[bartam1]

LGTM! Have one suggestion.

[panyuenlau]

Feel free to merge