[occm] Don't allow internal Services to share an LB #2190

dulek · 2023-04-07T17:51:11Z

What this PR does / why we need it:
Feature of sharing LBs seems to have some logic flaws. One of them is the problem of mixing internal and external Services on a single LB. As a FIP is tied to the LB and not individual listener, it means that if one Service attached the FIP to the Service, all the other Services will be available on that FIP. This may lead to accidental exposure of an internal Service which can potentially be pretty bad.

This commits attempts to limit the number of cases when the user can shoot themselves in the foot and makes it impossible for the internal Services to be secondary on a share load balancer. Moreover a condition is added that prevents secondary Services to create FIPs at all, so that accidental exposure of primary internal service by a secondary external one is solved.

There is no other way to reliably do that prevention as we only save truncated name into the tags of the LB, so there's no way to get list of all Services in a load balancer without listing all of them (in every namespace) and looking through their LB ID annotation. Even with that it's still really complicated to make decisions.

Cases (P - primary, S - secondary, E - external, I - internal):

P S
E E - all good
E I - prevented, all good
I I - prevented, not great, because it's technically a working
      combination, but it's a cost
I E - external one won't create FIP, good

Updates (after #2168 merges):
E E -> E I - we'll get error on the second Service, good
E E -> I E - FIP gets detached, E won't be able to readd it, good.

Which issue this PR fixes(if applicable):
fixes #2174

Special notes for reviewers:
Cases are listed

Release note:

[openstack-cloud-controller-manager] OCCM will no longer allow internal Services to share a load balancer with another Service.

Feature of sharing LBs seems to have some logic flaws. One of them is the problem of mixing internal and external Services on a single LB. As a FIP is tied to the LB and not individual listener, it means that if one Service attached the FIP to the Service, all the other Services will be available on that FIP. This may lead to accidental exposure of an internal Service which can potentially be pretty bad. This commits attempts to limit the number of cases when the user can shoot themselves in the foot and makes it impossible for the internal Services to be secondary on a share load balancer. Moreover a condition is added that prevents secondary Services to create FIPs at all, so that accidental exposure of primary internal service by a secondary external one is solved. There is no other way to reliably do that prevention as we only save truncated name into the tags of the LB, so there's no way to get list of all Services in a load balancer without listing all of them (in every namespace) and looking through their LB ID annotation. Even with that it's still really complicated to make decisions. Cases (P - primary, S - secondary, E - external, I - internal): P S E E - all good E I - prevented, all good I I - prevented, not great, because it's technically a working combination, but it's a cost I E - external one won't create FIP, good Updates (after kubernetes#2168 merges): E E -> E I - we'll get error on the second Service, good E E -> I E - FIP gets detached, E won't be able to readd it, good.

jichenjc · 2023-04-11T03:40:30Z

pkg/openstack/loadbalancer.go

@@ -928,6 +929,13 @@ func (lbaas *LbaasV2) getServiceAddress(clusterName string, service *corev1.Serv
 	}
 	klog.V(4).Infof("Found floating ip %v by loadbalancer port id %q", floatIP, portID)

+	// we cannot add a FIP to a shared LB when we're a secondary Service or we risk adding it to an internal
+	// Service and exposing it to the world unintentionally.


adding it to an internal ===> curious line 917 we return directly if it's internal.. so will it possible that described scenario can happen?

It's the case when we create an internal Service A as an LB owner and then a secondary external Service B on the same LB. B is external, so normally it would attempt to create the FIP for the LB, exposing A externally in the process. The idea is to prevent secondary Services from creating FIPs.

It has to be done this way, because when we're a secondary Service, there is no reliable way to get the primary Service besides listing all the Services and looking for LB ID in the annotations. The LB name and tags are not reliable because they're truncated to 255 characters.

ok, thanks for the detailed info

jichenjc · 2023-04-11T09:45:51Z

/approve

k8s-ci-robot · 2023-04-11T09:45:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/openstack/OWNERS~~ [jichenjc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jichenjc · 2023-04-12T01:22:24Z

/lgtm

I'm clarifying that 2.5 is and Octavia *API* version, adding a note about FIP being shared too and explaining changes from kubernetes#2190.

I'm clarifying that 2.5 is and Octavia *API* version, adding a note about FIP being shared too and explaining changes from #2190.

PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs.

…ling (#2360) * Compare proper LB name for shared LBs With shared LBs we distinguish the elements by tagging them with the proper name of the LB that would be created for a Service if it wasn't created as shared. This commit fixes that comparison for listener deletion as code was always comparing the name of the primary LB. * Fix shared LBs tests PR #2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function.

…ling (kubernetes#2360) * Fix shared LBs tests PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function.

…ling (#2360) (#2367) * Fix shared LBs tests PR #2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function.

…ubernetes#2360) * Fix shared LBs tests PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function.

…2360) (#2430) * Fix shared LBs tests PR #2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function. Co-authored-by: Michal Dulko <mdulko@redhat.com>

…ling (kubernetes#2360) * Compare proper LB name for shared LBs With shared LBs we distinguish the elements by tagging them with the proper name of the LB that would be created for a Service if it wasn't created as shared. This commit fixes that comparison for listener deletion as code was always comparing the name of the primary LB. * Fix shared LBs tests PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function.

…ling (#2360) (#2537) * Compare proper LB name for shared LBs With shared LBs we distinguish the elements by tagging them with the proper name of the LB that would be created for a Service if it wasn't created as shared. This commit fixes that comparison for listener deletion as code was always comparing the name of the primary LB. * Fix shared LBs tests PR #2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 7, 2023

k8s-ci-robot requested a review from anguslees April 7, 2023 17:51

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 7, 2023

k8s-ci-robot requested a review from FengyunPan2 April 7, 2023 17:51

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 7, 2023

dulek force-pushed the no-mixed-shared branch from e69bb9a to 5a75e04 Compare April 7, 2023 18:19

jichenjc reviewed Apr 11, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2023

k8s-ci-robot assigned jichenjc Apr 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023

k8s-ci-robot merged commit 788ea08 into kubernetes:master Apr 12, 2023

dulek added a commit to dulek/cloud-provider-openstack that referenced this pull request Apr 13, 2023

Shared LBs - improve docs

76356a2

I'm clarifying that 2.5 is and Octavia *API* version, adding a note about FIP being shared too and explaining changes from kubernetes#2190.

dulek mentioned this pull request Apr 13, 2023

[occm] Shared LBs - improve docs #2195

Merged

k8s-ci-robot pushed a commit that referenced this pull request Apr 17, 2023

Shared LBs - improve docs (#2195)

31ce345

I'm clarifying that 2.5 is and Octavia *API* version, adding a note about FIP being shared too and explaining changes from #2190.

dulek mentioned this pull request Apr 17, 2023

[occm] Remove FIP on internal annotation addition #2168

Merged

dulek added a commit to dulek/cloud-provider-openstack that referenced this pull request Sep 7, 2023

Fix shared LBs tests

4efeaf4

PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs.

dulek added a commit to dulek/cloud-provider-openstack that referenced this pull request Sep 8, 2023

Fix shared LBs tests

87f208c

PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs.

dulek added a commit to dulek/cloud-provider-openstack that referenced this pull request Sep 8, 2023

Fix shared LBs tests

d7910fc

PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs.

dulek added a commit to dulek/cloud-provider-openstack that referenced this pull request Sep 11, 2023

Fix shared LBs tests

403c54d

PR kubernetes#2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs.

dulek mentioned this pull request Sep 12, 2023

[occm] Make sure we don't mask LB tests failures and fix what was failing #2360

Merged

This was referenced Sep 19, 2023

Backport 2360 #2366

Closed

[occm] Make sure we don't mask LB tests failures and fix what was failing #2367

Merged

modzilla99 mentioned this pull request Sep 24, 2024

[OCCM] Shared Loadbalancer behavior #2675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[occm] Don't allow internal Services to share an LB #2190

[occm] Don't allow internal Services to share an LB #2190

dulek commented Apr 7, 2023 •

edited

Loading

jichenjc Apr 11, 2023

dulek Apr 11, 2023

jichenjc Apr 11, 2023

jichenjc commented Apr 11, 2023

k8s-ci-robot commented Apr 11, 2023

jichenjc commented Apr 12, 2023

[occm] Don't allow internal Services to share an LB #2190

[occm] Don't allow internal Services to share an LB #2190

Conversation

dulek commented Apr 7, 2023 • edited Loading

jichenjc Apr 11, 2023

Choose a reason for hiding this comment

dulek Apr 11, 2023

Choose a reason for hiding this comment

jichenjc Apr 11, 2023

Choose a reason for hiding this comment

jichenjc commented Apr 11, 2023

k8s-ci-robot commented Apr 11, 2023

jichenjc commented Apr 12, 2023

dulek commented Apr 7, 2023 •

edited

Loading