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.

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

[Feature/NifiCluster] Add ability to set container port network protocol #320

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

mh013370
Copy link
Member

@mh013370 mh013370 commented Nov 13, 2023

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

What's in this PR?

Adds the ability to set the NiFi container port network protocol. This field is new and optional, defaulting to TCP. If one wished to set the protocol for all container ports, you would use the following:

  listenersConfig:
    internalListeners:
      - type: "https"
        name: "https"
        containerPort: 8443
        protocol: "TCP"
      - type: "cluster"
        name: "cluster"
        containerPort: 6007
        protocol: "TCP"
      - type: "s2s"
        name: "s2s"
        containerPort: 10000
        protocol: "TCP"
      - type: "prometheus"
        name: "prometheus"
        containerPort: 9090
        protocol: "TCP"
      - type: "load-balance"
        name: "load-balance"
        containerPort: 6342
        protocol: "TCP"
      - name: "my-custom-listener-port"
        containerPort: 1234
        protocol: "UDP"
    sslSecrets:
      tlsSecretName: "test-nifikop"
      create: true

Why?

It's not currently possible to set anything other than TCP.

Checklist

  • Implementation tested
  • Error handling code meets the guideline
  • Logging code meets the guideline
  • User guide and development docs updated (if needed)
  • Append changelog with changes

@mh013370 mh013370 changed the title 319: Add ability to set container port network protocol Feature[NifiCluster] Add ability to set container port network protocol Nov 13, 2023
@mh013370 mh013370 changed the title Feature[NifiCluster] Add ability to set container port network protocol [Feature/NifiCluster] Add ability to set container port network protocol Nov 13, 2023
@mh013370
Copy link
Member Author

@juldrixx : FYI This requires pushing a new build image, merging #321, and then rebasing this PR on master.

juldrixx
juldrixx previously approved these changes Nov 15, 2023
@juldrixx juldrixx dismissed their stale review November 15, 2023 09:49

Still some issue

@juldrixx
Copy link
Contributor

That still doesn't work. If you change the default value to UDP is still defaulting on TCP.

@mh013370
Copy link
Member Author

That still doesn't work. If you change the default value to UDP is still defaulting on TCP.

well isn't this a fun problem 😄

@juldrixx
Copy link
Contributor

That still doesn't work. If you change the default value to UDP is still defaulting on TCP.

well isn't this a fun problem 😄

Even more when we have

// +kubebuilder:validation:Enum={"DO_NOT_LOAD_BALANCE","PARTITION_BY_ATTRIBUTE","ROUND_ROBIN","SINGLE"}
type ConnectionLoadBalanceStrategy string
// how to load balance the data in this Connection across the nodes in the cluster.
// +kubebuilder:default="DO_NOT_LOAD_BALANCE"
LoadBalanceStrategy ConnectionLoadBalanceStrategy `json:"loadBalanceStrategy,omitempty"

that gives us

loadBalanceStrategy:
 default: DO_NOT_LOAD_BALANCE
 enum:
 - DO_NOT_LOAD_BALANCE
 - PARTITION_BY_ATTRIBUTE
 - ROUND_ROBIN
 - SINGLE
 type: string

And I can change the default value

@juldrixx
Copy link
Contributor

If you change the type from corev1.Protocol to string it works 😕

@juldrixx
Copy link
Contributor

I guess, you can rollback to how it was before without default annotation. It's fine.

@mh013370
Copy link
Member Author

If you change the type from corev1.Protocol to string it works 😕

Hmm. I was hoping to use the k8s types for ease of use and to avoid converting to/from string and corev1.Protocol, but i think due to the // +enum annotation on corev1.Protocol we might not be able to.

I can either change it to a string or remove the default. Which would you prefer?

@juldrixx
Copy link
Contributor

If you change the type from corev1.Protocol to string it works 😕

Hmm. I was hoping to use the k8s types for ease of use and to avoid converting to/from string and corev1.Protocol, but i think due to the // +enum annotation on corev1.Protocol we might not be able to.

I can either change it to a string or remove the default. Which would you prefer?

Remove the default, I asked on Kubebuilder slack how to do it. I'm still waiting for an answer, but depending on the response, I'll make the appropriate change in the future.

@mh013370
Copy link
Member Author

If you change the type from corev1.Protocol to string it works 😕

Hmm. I was hoping to use the k8s types for ease of use and to avoid converting to/from string and corev1.Protocol, but i think due to the // +enum annotation on corev1.Protocol we might not be able to.
I can either change it to a string or remove the default. Which would you prefer?

Remove the default, I asked on Kubebuilder slack how to do it. I'm still waiting for an answer, but depending on the response, I'll make the appropriate change in the future.

pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to change NiFi containerPort protocol (UDP/TCP)
2 participants