Skip to content

Add draft for KaaS LB standard - #169

Merged
joshmue merged 20 commits into
SovereignCloudStack:169-kaas-lb-standard-draftfrom
joshmue:standard_kaas_lb
Jun 26, 2024
Merged

Add draft for KaaS LB standard#169
joshmue merged 20 commits into
SovereignCloudStack:169-kaas-lb-standard-draftfrom
joshmue:standard_kaas_lb

Conversation

@joshmue

@joshmue joshmue commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

Addresses SovereignCloudStack/issues#227

Signed-off-by: Joshua Mühlfort muehlfort@gonicus.de

Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
@joshmue
joshmue requested a review from garloff December 19, 2022 17:12
@joshmue

joshmue commented Jan 9, 2023

Copy link
Copy Markdown
Contributor Author

Per discussions in the latest container team meeting, for this decision record, we'll de-scope client IP preservation completely.

To sum up:

  • enabling the health monitor at octavia by default will help ensuring that setting externalTrafficPolicy: Local will not result in frequent connectivity issues, which is the major point for this decision record anyway.
  • externalTrafficPolicy: Local will not have the effect of that the actual client IP is visible to the application server. It will only cause Kubernetes to stop doing SNAT, which will only expose the direct TCP client IP - which is the Octavia HAProxy.

So currently, client IP preservation - at least on IP level - does not seem to be possible.

Comparing with other clouds, as far as I can tell:
Google and Azure implement Load Balancers at at lower (router) level (aka DSR?), making preserving the actual source IP super easy. (Without understanding specifics, AWS also somehow implements client IP preservation).
OpenStack Octavia makes use of higher (proxy) level, requiring an extra implementation at this high level.

TODO: Even though this will not trivial to implement, create a backlog item for this, as feature parity with major clouds is desired.

Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
@joshmue

joshmue commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

I pretty much rewrote the record.

I know that the result of the container team call was to actively de-scope client IP preservation from this document, but there is too much pointing towards it to be able to just omit it.

I added a few considerations and three more detailed ways to go.

@garloff garloff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very well written ADR, thanks!
I do however have a few suggestions, which I'd like us to consider before we finalize this.

Note: We do have the option to defer the IP preservation to a secondary ADR, but I do guess we need another discussion: Supporting TrafficPolicy: Local without supporting IP preservation might be a confusing outcome. On the other hand currently (withou health monitor) things appear to work and then break by only having every third request (in a cluster with 3 worker nodes and one nginx replica) reaching nginx. Much worse breakage, more easily detectable though.

Comment thread Standards/scs-XXXX-vN-kaas_load_balancers.md Outdated
Comment thread Standards/scs-XXXX-vN-kaas_load_balancers.md
Comment thread Standards/scs-XXXX-vN-kaas_load_balancers.md Outdated
Comment thread Standards/scs-XXXX-vN-kaas_load_balancers.md Outdated
Comment thread Standards/scs-XXXX-vN-kaas_load_balancers.md Outdated
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
Signed-off-by: Joshua Mühlfort <muehlfort@gonicus.de>
@garloff
garloff requested a review from jschoone April 24, 2023 08:15
@garloff garloff added Container Issues or pull requests relevant for Team 2: Container Infra and Tooling standards Issues / ADR / pull requests relevant for standardization & certification labels Apr 24, 2023
@jschoone jschoone linked an issue Aug 25, 2023 that may be closed by this pull request
9 tasks
@joshmue

joshmue commented Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

This PR only requires the bare minimum (a working Service implementation of type=LoadBalancer), so I do not see any blocker for this PR.

Of course, this also means that the standard does not provide too much value other than showing off alternatives, being the basis for other standards, or kick starting implementation of e2e tests. So technically, it would not really matter whether this is merged or abandoned.
I'd opt to do either, though.

@mbuechse @garloff @jschoone How to proceed?

@cah-hbaum

Copy link
Copy Markdown
Contributor

This PR only requires the bare minimum (a working Service implementation of type=LoadBalancer), so I do not see any blocker for this PR.

Of course, this also means that the standard does not provide too much value other than showing off alternatives, being the basis for other standards, or kick starting implementation of e2e tests. So technically, it would not really matter whether this is merged or abandoned. I'd opt to do either, though.

@mbuechse @garloff @jschoone How to proceed?

I'm neither of the mentioned people, but IMO this is a good document. But I agree with your assessment that the overall value isn't that big. While reading it, I had the impression, that this is more of a Decision Record. So personally, I would just change this to a DR and take it as a base for a future standard.

@joshmue

joshmue commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

I'm neither of the mentioned people

Your feedback is welcome, of course. Thanks! Just wanted to limit to mentioning as much as possible.

But I agree with your assessment that the overall value isn't that big. While reading it, I had the impression, that this is more of a Decision Record. So personally, I would just change this to a DR and take it as a base for a future standard.

Indeed, I structured it a little bit DR-like. I'm still a bit unsure whether wording should be adjusted to be more standard-like or making it a DR completely. Mostly because this would have been most likely a standard if the result had been different. Should that make a difference? Also, it's been more that a year now, so the decision may be reevaluated. 🤷‍♂️

@jschoone

jschoone commented May 8, 2024

Copy link
Copy Markdown
Contributor

I'm neither of the mentioned people

Your feedback is welcome, of course. Thanks! Just wanted to limit to mentioning as much as possible.

But I agree with your assessment that the overall value isn't that big. While reading it, I had the impression, that this is more of a Decision Record. So personally, I would just change this to a DR and take it as a base for a future standard.

Indeed, I structured it a little bit DR-like. I'm still a bit unsure whether wording should be adjusted to be more standard-like or making it a DR completely. Mostly because this would have been most likely a standard if the result had been different. Should that make a difference? Also, it's been more that a year now, so the decision may be reevaluated. 🤷‍♂️

Hi @cah-hbaum and @joshmue,
sorry for the very late response, I was pretty sure I answered 🤦
Now I forgot what it was.
I also think there's no blocker and I like the idea from Hannes to just make a DR out of it

@mbuechse

Copy link
Copy Markdown
Contributor

Is this still relevant? I will close this PR if nothing happens by July 31st.

@cah-hbaum

Copy link
Copy Markdown
Contributor

I wanted to update this yesterday, but since this is a repository fork from @joshmue, I couldn't really do that.

@joshmue

joshmue commented Jun 26, 2024

Copy link
Copy Markdown
Contributor Author

Seems like I cannot change the source branch to some branch in this repo (not directly anyway).

@cah-hbaum would you prefer to

  1. close this and create a new PR from a new branch in this repo OR
  2. merge this PR into a new branch in this repo, then create another PR from the newly created branch to main?

@cah-hbaum

Copy link
Copy Markdown
Contributor

I think merging this into a new branch and then updating this branch would be best.
I'm gonna set that up.

@cah-hbaum
cah-hbaum changed the base branch from main to 169-kaas-lb-standard-draft June 26, 2024 08:28
@cah-hbaum

Copy link
Copy Markdown
Contributor

@josmue Please merge, we will address other comments in the following PR.

@cah-hbaum
cah-hbaum self-requested a review June 26, 2024 08:32

@cah-hbaum cah-hbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved here, will review in the upcoming PR!

@joshmue
joshmue merged commit 78dbc62 into SovereignCloudStack:169-kaas-lb-standard-draft Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Container Issues or pull requests relevant for Team 2: Container Infra and Tooling standards Issues / ADR / pull requests relevant for standardization & certification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write ADR for load-balancers

5 participants