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

stack: directly expose smee when relay is disabled #116

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

clwluvw
Copy link
Contributor

@clwluvw clwluvw commented Jul 22, 2024

Description

If dhrelay is disabled, smee needs to be exposed directly via loadbalancer instead.

Why is this needed

Currently, if dhrelay is disabled, smee can't be accessed.

{{- if .Values.stack.lbClass }}
loadBalancerClass: {{ .Values.stack.lbClass }}
{{- end }}
loadBalancerIP: {{ .Values.stack.loadBalancerIP }}
Copy link
Member

Choose a reason for hiding this comment

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

this looks to be the same value as the stack service object. I dont think 1 load balancer IP for 2 services is going to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already tried and it can work as the ports are different. But I was considering whether we should use hostNetwork for smee in this situation or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacobweinstock Can you take a look again? I guess this one makes more sense.

@@ -183,9 +184,12 @@ spec:
- port: {{ .Values.smee.syslog.port }}
protocol: UDP
name: {{ .Values.smee.syslog.name }}
{{- end }}
{{- if .Values.stack.relay.enabled }}
Copy link
Member

Choose a reason for hiding this comment

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

port 67 for Smee is used for unicast traffic. The relay container is used for broadcast traffic. The relay being enabled should not gate this port.

Copy link
Contributor Author

@clwluvw clwluvw Jul 23, 2024

Choose a reason for hiding this comment

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

It's for when the loadbalancer IP is the same as host IP (single node installation). Also this would be unnecessary when relay is disabled.
Do you have any reason why to keep it when relay is disabled?
Or maybe I misunderstood the purpose of port 67 exposed by the lb? isn't that pointing to the dhrelay in stack pod?

Copy link
Member

Choose a reason for hiding this comment

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

It points at Smee not the relay. It's for unicast DHCP messages like when a DHCP relay from a switch or router points at Smee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the selector is pointing to the stack:

selector:
    {{- with .Values.stack.selector }}
    {{- toYaml . | nindent 4 }}
    {{- end }}

and the stack is maintaining relay for port 67:

- name: {{ .Values.stack.relay.name }}
  image: {{ .Values.stack.relay.image }}
  args: ["-m", "{{ .Values.stack.relay.presentGiaddrAction }}", "-c", "{{ .Values.stack.relay.maxHopCount }}", "-id", "{{ $dhcpInterfaceName }}", "-iu", "eth0", "-U", "eth0", "smee.{{ .Release.Namespace }}.svc.{{ .Values.stack.clusterDomain }}."]
  ports:
  - containerPort: 67
    protocol: UDP
    name: DHCP

Am I mistaken?

Avoid conflicts when smee is using hostNetwork and loadbalancer IP
is the same as hostNetwork.

Signed-off-by: Seena Fallah <[email protected]>
@jacobweinstock jacobweinstock added the ready-to-merge Mergify: Ready for Merging label Oct 24, 2024
@mergify mergify bot merged commit 42acdee into tinkerbell:main Oct 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Mergify: Ready for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants