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

Improve external access implementaton and documentation via NodePort #121

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

vuldin
Copy link
Member

@vuldin vuldin commented Jul 20, 2022

This PR updates how the external access configuration is handled in values.yaml, and then focused on NodePort service as the default method for providing external access:

  • update advertised listeners so each listener has expected values
  • NodePort uses top-level external section
  • NodePort uses each listener
  • kafka listener named internal is now required (doesn't have to be
    first in list)
  • remove unneeded cluster service

I started down the path of doing the same level of work for LoadBalancer, but I decided to break the PR into separate NodePort and LoadBalancer PRs... expect a LoadBalancer PR soon with similar details.

NodePort was used prior to this PR. The issue with the previous version related to advertised listeners, and handling listeners properly (kafka and the other listener groups), the values.yaml layout, and the documentation.

I'm surprised people had as much success as they did with the previous version especially with how advertised listeners worked before. There was this template variable $advertiseAddress that was used for every kafka listener, so there was no way to use a different host name for different listeners (internal, external, etc.): https://github.com/redpanda-data/helm-charts/pull/121/files#diff-f74d1889126f524c4b7e7e7836a376c58b0ca5aabe3720e03732b0e25d6ee380

Also each listener can take a list of listeners, but the helm chart did not use any of them but the first entry for most listener groups (this is still a problem in other areas that will be resolved in subsequent PRs): https://github.com/redpanda-data/helm-charts/pull/121/files#diff-69ec53469d658295718b28d4286a5ca368657dd3179a72c4ef50e928e88568cbL59

A big change is to the values.yaml layout to include a top-level external section along with the ability for each listener to have its own external section to customize behavior per listener if needed. This isn't too noticeable in terms of added functionality... it's benefit is more around ease-of-use due to and simpler values.yaml: https://github.com/redpanda-data/helm-charts/pull/121/files#diff-b46fb23874338d25278532c8599e01bd3f82a6045c00316c889e5747f7acee47

And then also the documentation, which hopefully covers most of what needs to be considered in order to ensure NodePort external access will work in whatever scenario and/or environment. There's a balance between just a TLDR and explaining everything, and then I'm also sure I didn't cover all details as much as could be. Some things may also be left out that should be included (let me know if you think that's the case): https://github.com/redpanda-data/helm-charts/pull/121/files#diff-79616ba1246321de4561899f03fca4d6b0fdf535391a81a1ab55349c88705385

@vuldin vuldin requested review from BenPope, r-vasquez and redp01 and removed request for BenPope July 20, 2022 04:49
README.md Outdated
Comment on lines 42 to 48
[Install minikube](https://k8s-docs.netlify.app/en/docs/tasks/tools/install-minikube/) (if needed), then start a 4-node cluster:

```sh
minikube start
minikube start --nodes 4 --extra-config=apiserver.service-node-port-range=8081-65535
```

This command starts minikube with 4 nodes (1 control plane, 3 worker nodes) and will extend the NodePort range to include default ports for Redpanda services.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to install a 4-node cluster rather than a single node, since the default external access method is via NodePort. So each node where Redpanda runs will open the same node, allowing access directly to each broker for external clients. But this only works if we have at least the same number of nodes as replicas in the statefulset, which is 3 by default.

Comment on lines +7 to +8
- [External access via LoadBalancer](./with-external-via-loadbalancer/README.md)
- [External access via Ingress](./with-external-via-ingress/README.md)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files are what will be added in later PRs (one per file). Right now they don't exist.

Comment on lines +95 to +101
# Optional external section
external:
enabled: false
# enabled: true
# Type of external access (options are NodePort and LoadBalancer)
# type: NodePort
# External port
# This listener port will be used for the external port if this is not included
port: 31644
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is optional for each listener. If it doesn't exist and external is enabled for all listeners, then the NodePort service will attempt to use the port (in this case on line 94) as the NodePort. By default this will result in the service failing to start with an error if you haven't configured Kubernetes to expand the NodePort range. I included details on how to do this for minikube in the startup section of the main README. I wasn't able to find details on how to do this in kind though, and this is entirely optional.

Comment on lines 125 to 124
name: internal
port: 9093
# TODO make tls section optional
tls:
enabled: false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A kafka listener with the name internal is required, as this name is how the code identifies this listener as always being advertised internally rather than externally. This is because the external section is optional, so if global external access is enabled by not explicitly disabled in this listener then it would not be an internal listener without this additional logic.

@@ -180,7 +227,7 @@ storage:
# used to store Redpanda's data, otherwise `hostPath` is used.
persistentVolume:
# TODO possibly replace with storage.enabled
enabled: true
enabled: false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird line to include in this PR, but it was required to make sure minikube would work. More investigation into why is needed, and there is a ticket in the v2 project related to this: #79

Comment on lines +198 to +205
external:
# Default external access value for all listeners except RPC
# External config doesn't apply to RPC listeners as they are never externally accessible
# These values can be overridden by each listener if needed
enabled: true
# Default external access type (options are NodePort and LoadBalancer)
# TODO include IP range for load balancer that support it: https://github.com/redpanda-data/helm-charts/issues/106
type: NodePort
domain: local
# annotations:
# For example:
# cloud.google.com/load-balancer-type: "Internal"
# service.beta.kubernetes.io/aws-load-balancer-type: nlb
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most important change to values.yaml as far as this PR is concerned. This is the top-level external section.

@vuldin
Copy link
Member Author

vuldin commented Aug 2, 2022

Latest update resolves merge conflicts, updates Redpanda to v22.1.6, and ensures minikube starts with enough memory given to the worker nodes to satisfy default memory settings for Redpanda.

@vuldin vuldin force-pushed the external-access-via-nodeport branch from c1ade48 to 9f7d685 Compare August 2, 2022 21:09
- update advertised listeners so each listener has expected values
- NodePort uses top-level external section
- NodePort uses each listener
- kafka listener named internal is now required (doesn't have to be
  first in list)
- remove unneeded cluster service
@vuldin vuldin force-pushed the external-access-via-nodeport branch from 9f7d685 to 267680f Compare August 2, 2022 22:33
@vuldin
Copy link
Member Author

vuldin commented Aug 2, 2022

While verifying merge conflict resolutions, I noticed the NodePort documentation section order was off, so this is also resolved.

@vuldin vuldin merged commit 6e2f1f5 into redpanda-data:v2 Aug 2, 2022
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.

1 participant