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

Add loadBalancer option to the chart #74

Closed
wants to merge 10 commits into from
Closed

Conversation

zurajm
Copy link
Contributor

@zurajm zurajm commented Jan 19, 2021

Hi, this adds an option for specifying loadBalancer, ExternalTrafficPolicy and loadBalancerIP. I'm using MetaLB in local k3s cluster so hostPort is not an option for me.
Is somehow related to issue #56

@zurajm zurajm changed the title Lb Add loadBalancer option to the chart Jan 19, 2021
@micw
Copy link
Contributor

micw commented Jan 20, 2021

Hello,
thank you for the PR. I hope i find time at weekend to completely review it.

I know that there had also been requests for "NodePort" service type. I'm not sure if we ever support this (since all variants with NodePort I'm aware of creates open relay because the source IP get lost). Nevertheless, we should maybe make this more generic:

  • one option to enable "hostPort" which should default to true for backward compatibility
  • another option to set the service type which defaults to ClusterIP for backward compatibility
  • more options to add externalTrafficPolicy and/or loadBalancerIP to the services.
  • maybe also for setting service annotations like metallb.universe.tf/address-pool

Kind regards,
Michael.

@zurajm
Copy link
Contributor Author

zurajm commented Jan 20, 2021

Hi Michael,
it make sense to go with more generic approach. I will look at this and extend this PR..
Regards,

@zurajm
Copy link
Contributor Author

zurajm commented Jan 24, 2021

I think I covered all viable scenarios:

  • hostPort (enabled default)
  • service.type (ClusterIP default)
  • externalTrafficPolicy and loadBalancerIP (taken into account if service.type: loadBalancer)
  • annotations

Regards, Miha

Copy link

@wesparish wesparish left a comment

Choose a reason for hiding this comment

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

This looks good, can we please get it merged for those of us using MetalLB?

@ChipWolf
Copy link

ChipWolf commented Mar 18, 2021

Agree, we also need this change.

@micw
Copy link
Contributor

micw commented Apr 25, 2021

May it be that hostPort is not enabled by default in this PR?

https://github.com/Mailu/helm-charts/pull/74/files#diff-50d55ccb9dc108e0535254e89f32431b5c0342b715685820c80f5f5ff3410a03R132

Otherwise, it looks good to me.

I also saw #84 and ask myself how it relates to this one. E.g. there are ports that should never be exposed public (e.g. 10025 and 8000). Would it make sense to go the "additional service way"?

@micw
Copy link
Contributor

micw commented Jun 28, 2021

I would close this in favour to #84. I think having an extra service is a better way.

@micw micw closed this Oct 26, 2021
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.

4 participants