Skip to content

OCPBUGS-55962: Inter advertised UDN isolation configurable #2569

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+23 −1
Open

OCPBUGS-55962: Inter advertised UDN isolation configurable #2569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion go-controller/pkg/controllermanager/controller_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ func (cm *ControllerManager) CleanupStaleNetworks(validNetworks ...util.NetInfo)
}
}

if util.IsRouteAdvertisementsEnabled() {
if util.IsRouteAdvertisementsEnabled() && !util.IsLooseUDNIsolation() {
// Remove stale subnets from the advertised networks address set used for isolation
// NOTE: network reconciliation will take care of removing the subnets for existing networks that are no longer
// advertised.
Expand Down Expand Up @@ -530,6 +530,10 @@ func (cm *ControllerManager) Reconcile(_ string, _, _ util.NetInfo) error {
}

func (cm *ControllerManager) configureAdvertisedNetworkIsolation() error {
if util.IsLooseUDNIsolation() {
klog.Infof("Skip creating global advertised networks addressset in loose UDN isolation mode")
return nil
}
Comment on lines +533 to +536
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we actually ensure the address set is not there if start with loose UDN isolation?

Copy link
Contributor

Choose a reason for hiding this comment

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

My assumption was that moving ovn-k to loose mode would only be "supported" when there is no advertised networks. If that is not the case I agree we need to add the necessary cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that was my assumption as well, AFAIU this is a stopgap arrangement for testing loose UDN isolation mode in a lab. This commit will be removed once it's implemented in upstream with a proper UDN isolation specific API.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is an assumption, can we pass it by @trozet in case he is aware of something else?

Copy link
Contributor

@jcaamano jcaamano May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So toggling this knob won't do anything for networks that were already advertised. It would only affect networks that became advertised after.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my current opinion is that we should do this upstream with a global config flag to enable/disable "RoutedUDNIsolation", default enabled. In the future we might make this a per RA thing, where it might be "UDNIsolation": default strict, maybe another mode is "AllowExternalRouting".

I think the code should sync the current state if the flag is flipped on or off. So if someone toggles the flag on an existing cluster, it should remove the address set, ACL, whatever that was configured to enforce isolation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@trozet @jcaamano agree to your thoughts, here is upstream PR as suggested above: ovn-kubernetes/ovn-kubernetes#5276. PTAL.

addressSetFactory := addressset.NewOvnAddressSetFactory(cm.nbClient, config.IPv4Mode, config.IPv6Mode)
_, err := addressSetFactory.EnsureAddressSet(ovn.GetAdvertisedNetworkSubnetsAddressSetDBIDs())
return err
Expand Down
10 changes: 10 additions & 0 deletions go-controller/pkg/ovn/udn_isolation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,11 @@ func BuildAdvertisedNetworkSubnetsDropACL(advertisedNetworkSubnetsAddressSet add
// pass "(ip[4|6].src == <UDN_SUBNET> && ip[4|6].dst == <UDN_SUBNET>)" 1100
// drop "(ip[4|6].src == $<ALL_ADV_SUBNETS> && ip[4|6].dst == $<ALL_ADV_SUBNETS>)" 1050
func (bnc *BaseNetworkController) addAdvertisedNetworkIsolation(nodeName string) error {
if util.IsLooseUDNIsolation() {
klog.Infof("The network %s is configured with loose isolation mode, skip adding tier-0 drop ACL rule",
bnc.GetNetworkName())
return nil
}
var passMatches, cidrs []string
var ops []ovsdb.Operation

Expand Down Expand Up @@ -363,6 +368,11 @@ func (bnc *BaseNetworkController) addAdvertisedNetworkIsolation(nodeName string)
// deleteAdvertisedNetworkIsolation deletes advertised network isolation rules from the given node switch.
// It removes the network CIDRs from the global advertised networks addresset together with the ACLs on the node switch.
func (bnc *BaseNetworkController) deleteAdvertisedNetworkIsolation(nodeName string) error {
if util.IsLooseUDNIsolation() {
klog.Infof("The network %s is configured with loose isolation mode, skip deleting tier-0 drop ACL rule",
bnc.GetNetworkName())
return nil
}
Comment on lines +371 to +375
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we actually ensure the ACL is not there if start with loose UDN isolation?

addrSet, err := bnc.addressSetFactory.GetAddressSet(GetAdvertisedNetworkSubnetsAddressSetDBIDs())
if err != nil {
return fmt.Errorf("failed to get advertised subnets addresset %s for network %s: %w", GetAdvertisedNetworkSubnetsAddressSetDBIDs(), bnc.GetNetworkName(), err)
Expand Down
8 changes: 8 additions & 0 deletions go-controller/pkg/util/multi_network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"net"
"os"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -1554,3 +1555,10 @@ func ParseNetworkName(networkName string) (udnNamespace, udnName string) {
}
return "", ""
}

// IsLooseUDNIsolation returns true of `UDN_ISOLATION_MODE` env variable is set to "loose" value.
// In "loose" mode, the network controller skips programming network isolation rules for advertised
// UDN networks and this will allow pod to pod communication among advertised UDN networks.
func IsLooseUDNIsolation() bool {
return os.Getenv("UDN_ISOLATION_MODE") == "loose"
}