UDN: Basic layer2 service support #4653
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/unit-testing
kyrtapz
requested review from
ricky-rav
and removed request for
JacobTanenbaum
kyrtapz
force-pushed
the
l2_pod_svc
branch
2 times, most recently
from
227ef5b
to
7e9a903
Compare
tssurya
added
the
feature/user-defined-network-segmentation
kyrtapz
force-pushed
the
l2_pod_svc
branch
12 times, most recently
from
2eda103
to
d37e487
Compare
|
/cc |
kyrtapz
force-pushed
the
l2_pod_svc
branch
2 times, most recently
from
6278ae9
to
046c09e
Compare
github-actions
bot
added
feature/egress-ip
6 tasks
kyrtapz
force-pushed
the
l2_pod_svc
branch
5 times, most recently
from
50f14b4
to
341e8d5
Compare
kyrtapz
changed the title
kyrtapz
force-pushed
the
l2_pod_svc
branch
2 times, most recently
from
8c5e740
to
6201f3f
Compare
github-actions
bot
added
the
kind/documentation
4 tasks
trozet
reviewed
Sep 19, 2024
go-controller/pkg/ovn/egressgw.go
Outdated
| @@ -631,7 +632,7 @@ func getExternalIPsGR(watchFactory *factory.WatchFactory, nodeName string) ([]*n | |||
| // deletePodSNATOps creates ovsdb operation that removes per pod SNAT rules towards the nodeIP that are applied to the GR where the pod resides | |||
| // used when disableSNATMultipleGWs=true | |||
| func deletePodSNATOps(nbClient libovsdbclient.Client, ops []ovsdb.Operation, gwRouterName string, extIPs, podIPNets []*net.IPNet) ([]ovsdb.Operation, error) { | |||
| nats, err := buildPodSNAT(extIPs, podIPNets) | |||
| nats, err := buildPodSNAT(extIPs, podIPNets, "") | |||
There was a problem hiding this comment.
deletePodSNATOps is used by secondary network controller. Why do you pass ""? Looking closer at this,
func isEquivalentNAT(existing *nbdb.NAT, searched *nbdb.NAT) bool {
looks like it is not comparing match criteria. I wonder if that was something missed by the conditional snat update in ovnk?
There was a problem hiding this comment.
You are right, I modified isEquivalentNAT to take the match into account and fixed the delete path.
| @@ -66,7 +67,7 @@ func (cm *NetworkControllerManager) NewNetworkController(nInfo util.NetInfo) (na | |||
| case ovntypes.Layer3Topology: | |||
| return ovn.NewSecondaryLayer3NetworkController(cnci, nInfo) | |||
| case ovntypes.Layer2Topology: | |||
| return ovn.NewSecondaryLayer2NetworkController(cnci, nInfo), nil | |||
| return ovn.NewSecondaryLayer2NetworkController(cnci, nInfo) | |||
There was a problem hiding this comment.
on commit "L2 services support" it would be good to have a commit message describing in general what you did to make it work
| // kubernetes controller-manager | ||
| err := oc.svcController.Run(5, oc.stopChan, runRepair, useLBGroups, oc.svcTemplateSupport) | ||
| if err != nil { | ||
| klog.Errorf("Error running OVN Kubernetes Services controller: %v", err) |
There was a problem hiding this comment.
would it be good to list the name of the network that the service controller couldn't start for?
Signed-off-by: Patryk Diak <[email protected]>
Signed-off-by: Patryk Diak <[email protected]>
Add OVS flows that redirect traffic from the masquerade subnet towards UDN enabled services to the default gateway router. For shared gateway mode a static route is added to redirect the service traffic to the management port. Modified the staticRouteCleanup to ensure that the new routes are not getting removed. Signed-off-by: Patryk Diak <[email protected]>
Signed-off-by: Patryk Diak <[email protected]>
In case of L2 networks only traffic leaving the cluster should be SNATed. Additonally change the order of SNAT cleanup to ensure that it happens for L2 networks. Signed-off-by: Patryk Diak <[email protected]>
Run the generic services controller for each primary layer2 network. It takes care of configuring the load blancers and load balancer groups for each service in a namespace using the primary layer2 network. Signed-off-by: Patryk Diak <[email protected]>
Signed-off-by: Patryk Diak <[email protected]>
This commit does not introduce functional changes. It makes it possible to add more networks to the services unit tests. Signed-off-by: Patryk Diak <[email protected]>
Signed-off-by: Patryk Diak <[email protected]>
trozet
reviewed
Sep 20, 2024
| @@ -1033,7 +1033,7 @@ func (oc *SecondaryLayer3NetworkController) StartServiceController(wg *sync.Wait | |||
| // kubernetes controller-manager | |||
| err := oc.svcController.Run(5, oc.stopChan, runRepair, useLBGroups, oc.svcTemplateSupport) | |||
| if err != nil { | |||
| klog.Errorf("Error running OVN Kubernetes Services controller: %v", err) | |||
| klog.Errorf("Error running OVN Kubernetes Services controller for network %s: %v", oc.GetNetworkName(), err) | |||
There was a problem hiding this comment.
not really your problem, but i just noticed if we dont start the services controller we just log an error and continue? we should be propagating an error somehow.
trozet
previously approved these changes
Sep 20, 2024
kyrtapz
dismissed
trozet’s stale review
The merge-base changed after approval.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for Layer2 services for cluster networked endpoints.
Based on #4567.