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

feat(kiali-server): add custom nodeport for service #248

Merged
merged 9 commits into from
Feb 19, 2024

Conversation

emreberber
Copy link
Contributor

@emreberber emreberber commented Feb 14, 2024

Related Issue ➜ Kiali Server Helm Chart Support Custom NodePort

When Kiali Server is deployed to hundreds of Kubernetes clusters, it would be very good to have this feature to access all clusters with the same nodePort. This PR was prepared to use a custom port for NodePort.

In Values, we can get the nodePort value as follows:

server:
  port: 20001
  nodePort: 32475
  observability:
    metrics:
      enabled: true
      port: 9090
      nodePort: 32476
  web_root: ""

We can use it in the Service as follows:

{{- if eq .Values.deployment.service_type nodePort }}
    nodePort: {{ .Values.server.nodePort }}
{{- end }}

#7093

kiali-server/templates/service.yaml Outdated Show resolved Hide resolved
kiali-server/templates/service.yaml Outdated Show resolved Hide resolved
@jmazzitelli
Copy link
Contributor

Note to Kiali maintainers - do not merge this until an operator PR is submitted that provides the same feature. See: kiali/kiali#7093 (comment)

This will require another community member or Kiali maintainer to take on that work.

@jmazzitelli jmazzitelli added do not merge requires operator PR Requires changes to the operator labels Feb 14, 2024
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

I do not think the metrics endpoint should need to define a nodePort. See comments that follow.

kiali-server/templates/service.yaml Outdated Show resolved Hide resolved
kiali-server/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

use snake_case

kiali-server/values.yaml Outdated Show resolved Hide resolved
kiali-server/templates/service.yaml Outdated Show resolved Hide resolved
@jmazzitelli
Copy link
Contributor

jmazzitelli commented Feb 15, 2024

Another thing I just realized here - now that there is a fixed nodePort defined as a default, there is a greater chance of a collision and therefore deployment failure.

In current behavior (before this PR), k8s will pick an unused port and assign that as the service's nodePort. Now, with this PR, that is not the case. If there is something already on port 32475, the deployment will fail.

We need to maintain backwards compatibility - the behavior should be the same as before - a dynamic nodePort will be assigned by default. Only if the (optional) server.node_port is set will that be explicitly used for the node port.

That would retain backward compatibility and will reduce the chance for deployment failure in the default case, while at the same time still providing the ability for the user to configure the service with an explicit node port if that is desired.

To see how this will be done in the operator, see the operator PR.

@jmazzitelli
Copy link
Contributor

Operator PR is ready to go. Will wait for this PR to finish before the operator PR gets merged.

emreberber and others added 4 commits February 16, 2024 09:52
Co-authored-by: John Mazzitelli <mazz@redhat.com>
Co-authored-by: John Mazzitelli <mazz@redhat.com>
Co-authored-by: John Mazzitelli <mazz@redhat.com>
Co-authored-by: John Mazzitelli <mazz@redhat.com>
@jmazzitelli jmazzitelli self-requested a review February 16, 2024 11:06
@jmazzitelli jmazzitelli dismissed their stale review February 16, 2024 11:07

changes done

Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

Waiting on one further update. See #248 (comment) for the motivation behind this.

My suggestions to implement that are below.

kiali-server/templates/service.yaml Outdated Show resolved Hide resolved
kiali-server/values.yaml Outdated Show resolved Hide resolved
emreberber and others added 3 commits February 19, 2024 20:41
Co-authored-by: John Mazzitelli <mazz@redhat.com>
Co-authored-by: John Mazzitelli <mazz@redhat.com>
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

Code looks good. Did a couple quick smoke tests:

  1. Set node port to 32444:
helm install --namespace istio-system --set deployment.service_type=NodePort --set server.node_port=32444 kiali-server _output/charts/kiali-server-1.81.0-SNAPSHOT.tgz

results in:

$ kubectl get svc -n istio-system -l app.kubernetes.io/name=kiali -ojsonpath='{..spec.type}{"\n"}'
NodePort
$ kubectl get svc -n istio-system -l app.kubernetes.io/name=kiali -ojsonpath='{..spec.ports[?(@.name=="http")].nodePort}{"\n"}'
32444
  1. Re-install, but this time do not specify node_port - old behavior remains - it chooses a free random port:
$ helm uninstall --namespace istio-system kiali-server
$ helm install --namespace istio-system --set deployment.service_type=NodePort kiali-server _output/charts/kiali-server-1.81.0-SNAPSHOT.tgz
$ kubectl get svc -n istio-system -l app.kubernetes.io/name=kiali -ojsonpath='{..spec.type}{"\n"}'
NodePort
$ kubectl get svc -n istio-system -l app.kubernetes.io/name=kiali -ojsonpath='{..spec.ports[?(@.name=="http")].nodePort}{"\n"}'
32515
  1. Finally, don't set the service_type either:
$ helm uninstall --namespace istio-system kiali-server
$ helm install --namespace istio-system kiali-server _output/charts/kiali-server-1.81.0-SNAPSHOT.tgz
$ kubectl get svc -n istio-system -l app.kubernetes.io/name=kiali -ojsonpath='{..spec.type}{"\n"}'
ClusterIP
$ kubectl get svc -n istio-system -l app.kubernetes.io/name=kiali -ojsonpath='{..spec.ports[?(@.name=="http")]}{"\n"}' | jq
{
  "appProtocol": "http",
  "name": "http",
  "port": 20001,
  "protocol": "TCP",
  "targetPort": 20001
}

@jmazzitelli jmazzitelli merged commit 9646112 into kiali:master Feb 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires operator PR Requires changes to the operator
Projects
Development

Successfully merging this pull request may close these issues.

2 participants