Standalone Egress NAT #299

p-strusiewiczsurmacki-mobica · 2024-08-08T14:40:45Z

This PR introduces Standalone Egress NAT as discussed in #274 .

coil-controller is now divided into coil-ipam-controller and coil-egress-controller.
coild has now configuration flags to disable/enable egress and/or IPAM features.
~~- coil has now capabilities fields that can be used to disable/enable IPAM/Egress.~~
If IPAM is disabled, coild can be run as secondary CNI using CNI chaining as described in setup.md.
In case IPAM is disabled, veth aliases will now use pod's UUID instead of container's ID (couldn't get e2e test working using container's ID, I believe container ID is changed during pod restart, but as IPAM is disabled it is not updated as required).

PR was tested using egress related E2E tests with both Kindnet and Calico and tests that are provided in the repository passed.

terassyi · 2024-08-26T04:30:50Z

@p-strusiewiczsurmacki-mobica
I'm reviewing this, so please wait a little longer.

v2/pkg/cnirpc/cni.proto

v2/config/pod/coil-egress-controller.yaml

v2/cmd/coil-egress-controller/sub/root.go

v2/runners/coild_server.go

v2/runners/coild_server_test.go

v2/pkg/nodenet/pod.go

v2/controllers/egress_watcher.go

v2/pkg/nodenet/pod.go

terassyi · 2024-09-20T06:31:10Z

PR was tested using egress related E2E tests with both Kindnet and Calico and tests that are provided in the repository passed.

Thank you for checking it.
I also ran stand-alone egress mode with Kindnet, and it works well.

It's better if we can run e2e tests for stand-alone egress mode.
What do you think?

terassyi · 2024-09-20T06:33:04Z

Hi, @p-strusiewiczsurmacki-mobica !

Thank you for waiting.

The main changes look good to me.
So I left some nit comments.
Please check it.

p-strusiewiczsurmacki-mobica · 2024-09-24T15:05:20Z

PR was tested using egress related E2E tests with both Kindnet and Calico and tests that are provided in the repository passed.

Thank you for checking it. I also ran stand-alone egress mode with Kindnet, and it works well.

It's better if we can run e2e tests for stand-alone egress mode. What do you think?

I agree this would be great. However I need to think about how this test should be done. The problem with kindnet is, that it requires different CNI config on each node, so I would need to add some script that would get node subnet from the kind cluster's nodes after the cluster was created and use that data to update CNI config after make coil-install is executed.

terassyi · 2024-09-26T06:25:51Z

I would need to add some script that would get node subnet from the kind cluster's nodes after the cluster was created and use that data to update CNI config after make coil-install is executed.

Yes, it isn't easy to configure the cluster to use kindnet and coil.

OK, in this PR, it's OK no e2e test for egress-only mode.
Thanks.

p-strusiewiczsurmacki-mobica · 2024-09-26T11:50:53Z

@terassyi I've added egress-only e2e tests to e2e/Makefile and instructions on how to use those to docs/setup.md.

v2/e2e/Makefile

v2/e2e/coil_test.go

docs/setup.md

v2/e2e/coil_test.go

v2/e2e/Makefile

v2/e2e/coil_test.go

p-strusiewiczsurmacki-mobica · 2024-10-17T15:02:11Z

@terassyi Just one important thing - it turned out that egress-controller pods has to use hostNetwork as, otherwise, it seems to be unable to reach the apiserver if Coild is used as primary CNI.
So I've added the hostNetwork to the manifest once again.

terassyi · 2024-10-21T05:36:03Z

@p-strusiewiczsurmacki-mobica
Thank you for reporting.

So I've added the hostNetwork to the manifest once again.

In this PR, it's ok to turn on hostNetwork.

When I tried to turn out hostNetwork, I found that egess-controller couldn't communicate with the api-server.
I'm not sure why it can't reach api-server.

But, I think hostNetwork is an unnecessary setting.
So, I will create an investigation issue after merging this.

terassyi · 2024-10-21T06:56:13Z

https://github.com/cybozu-go/coil/pull/299/files#diff-87c0c48de880a4de757f70cabed8e310b213adaebd0974c60386af47de4aeb8bR81

Could you update the manifests target to be able to select generating manifests for only ipam or egress related or both?

p-strusiewiczsurmacki-mobica · 2024-10-23T11:57:03Z

https://github.com/cybozu-go/coil/pull/299/files#diff-87c0c48de880a4de757f70cabed8e310b213adaebd0974c60386af47de4aeb8bR81

Could you update the manifests target to be able to select generating manifests for only ipam or egress related or both?

@terassyi Done. :)

manifests, manifests-egress and manifests-ipam are now available to be used.

It seems that controller-gen only accepts directories for manifest generation, so I had to add some workaround for that (copying *_webhook.go files to temporary folder) but I think it should work OK.

terassyi · 2024-10-28T08:01:26Z

When trying to run e2e test, it fails with following error.

Error: accumulating resources: accumulation err='accumulating resources from '../../../../config/default/egress/v4': read /home/terashima/go/src/github.com/cybozu-go/coil/v2/config/default/egress/v4: is a directory': recursed accumulation of path '/home/terashima/go/src/github.com/cybozu-go/coil/v2/config/default/egress/v4': trouble configuring builtin PatchStrategicMergeTransformer with config: `
paths:
- ../../webhook_manifests_patch.yaml
`: open /home/terashima/go/src/github.com/cybozu-go/coil/v2/config/default/webhook_manifests_patch.yaml: no such file or directory
error: no objects passed to apply

I ran following commands.

$ make -C .. manifests
$ WITH_KINDNET=true TEST_IPV6=false make start
$ make install-coil-egress-v4

p-strusiewiczsurmacki-mobica · 2024-10-28T14:49:44Z

@terassyi

Error: accumulating resources: accumulation err='accumulating resources from '../../../../config/default/egress/v4': read /home/terashima/go/src/github.com/cybozu-go/coil/v2/config/default/egress/v4: is a directory': recursed accumulation of path '/home/terashima/go/src/github.com/cybozu-go/coil/v2/config/default/egress/v4': trouble configuring builtin PatchStrategicMergeTransformer with config: `
paths:
- ../../webhook_manifests_patch.yaml
`: open /home/terashima/go/src/github.com/cybozu-go/coil/v2/config/default/webhook_manifests_patch.yaml: no such file or directory
error: no objects passed to apply

I believe you're missing a file v2/config/default/webhook_manifests_patch.yaml which is generated by make certs target (this is something i did not change).

So, you'd have to run

make -C .. manifests && make -C .. certs
WITH_KINDNET=true TEST_IPV6=false make start
make install-coil-egress-v4

terassyi · 2024-10-30T02:43:13Z

@p-strusiewiczsurmacki-mobica

Thanks!
I could run e2e tests for each configuration, and it seems all e2e tests work well.

It seems procedures to run e2e tests are getting complex, so cloud you update e2e/README.md?

terassyi · 2024-10-30T02:47:54Z

It seems webhook-related tests in small test fail.
Could you fix this test?

terassyi · 2024-10-30T02:56:59Z

Lastly, I want you to update CI to run e2e tests for all combinations, such as egress-only and egress-only, with ipv6 and egress+ipam, etc.

When you want to run CI, please mention me.
I respond as quickly as I can.

p-strusiewiczsurmacki-mobica · 2024-10-31T11:10:51Z

@terassyi Small test should be fixed and actions were added to the CI.

Should I squash the commits before it'll be merged?

terassyi · 2024-11-01T04:52:23Z

Thanks!

It passes all CIs.

Should I squash the commits before it'll be merged?

Yes, please.

p-strusiewiczsurmacki-mobica · 2024-11-04T12:00:20Z

@terassyi It's squashed now :)

terassyi · 2024-11-05T09:27:23Z

@p-strusiewiczsurmacki-mobica

Thanks!

I found small test seems to be flaky.
Cloud you fix this?

https://github.com/cybozu-go/coil/actions/runs/11663759016

p-strusiewiczsurmacki-mobica · 2024-11-06T15:27:03Z

@terassyi I've fixed the warnings and added some fixes for the CI jobs.
One last thing that is bugging me are the controller-runtime's/go-client's cache errors in the small tests, like this one:

W1105 08:58:09.635773   24935 reflector.go:470] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:232: watch of *v1.PodDisruptionBudget ended with: an error on the server ("unable to decode an event from the watch stream: context canceled") has prevented the request from succeeding

But I believe that this might be out of our reach here, as it seems it is somewhat known issue with controller runtime's cache.

I've tried disabling the cache altogether but for some reason it did not work for me. I believe those were also not introduced by my changes as I can see them in other runs of CI, e.g: https://github.com/cybozu-go/coil/actions/runs/10804727159/job/29970647406

If you see anything else I could fix just let me know.

RefluxMeds · 2024-11-11T09:08:38Z

Hi @terassyi,
Have you had time to check this? It would be very beneficial for our org.
Thanks!

terassyi · 2024-11-12T07:58:58Z

@RefluxMeds

Hi, I'm checking this to be able to merge.
Please wait a little longer!

terassyi · 2024-11-22T07:49:59Z

@p-strusiewiczsurmacki-mobica

I re-reviewed all changes again, and I want to discuss the need for enable-spam/egress flags in the coil CNI plugin.

I think it's enough to handle these flags in coild.

Now, we check the features we want to use by values given by the coil CNI plugin.
However, could also has the same flags.
I think we can use coild's enable-ipam/egress flags instead of coil CNI's flags.

https://github.com/cybozu-go/coil/pull/299/files#diff-f7d3c7316366cfd19ef46b412e3ab6937dc7d6607b49a73b613e3fecd639d875R181

The only concern I have now is following the code.

https://github.com/cybozu-go/coil/pull/299/files#diff-e193be60a714369d09504366326bc0f86dff95f1dabd9e1bd16e027dac81acb2R33

But we can solve this by moving this error handling to coild.

If we can keep the CNI configuration simple and no changes, it's better for all users.

p-strusiewiczsurmacki-mobica · 2024-11-25T12:04:19Z

@terassyi
OK, I've removed the capabilities settings from the CNI binary itself. The configuration will be now handled by coild flags. I've also moved the check you've mentioned to coild and changed small tests a bit to reflect that.

v2/runners/coild_server.go

terassyi · 2024-12-02T08:40:24Z

v2/runners/coild_server.go

@@ -88,7 +92,8 @@ func (n natSetup) Hook(l []GWNets, log *zap.Logger) func(ipv4, ipv6 net.IP) erro
 }

 // NewCoildServer returns an implementation of cnirpc.CNIServer for coild.
-func NewCoildServer(l net.Listener, mgr manager.Manager, nodeIPAM ipam.NodeIPAM, podNet nodenet.PodNetwork, setup NATSetup, logger *zap.Logger) manager.Runnable {
+func NewCoildServer(l net.Listener, mgr manager.Manager, nodeIPAM ipam.NodeIPAM, podNet nodenet.PodNetwork, setup NATSetup, cfg *config.Config, logger *zap.Logger,
+	aliasFunc func(interfaces map[string]bool, conf *nodenet.PodNetConf, logger *zap.Logger, pod *corev1.Pod) error) manager.Runnable {


Why did you add the callback function to add the interface alias?

So, in egress only mode we need to set interface alias on Add call. However the function that sets alias is dependent on the existance of the pod's veth pair which is not created for simpe-test. I've added the callback function so nil returning function can be passed here which makes it possible to run simple-test for egress-only mode.

At least that's the best I was able to come up with.

Thanks.
It's ok to me!

v2/runners/coild_server.go

terassyi · 2024-12-02T08:58:58Z

@p-strusiewiczsurmacki-mobica
Thank you for fixing!

I added some comments :)
Please check it!

p-strusiewiczsurmacki-mobica · 2025-01-13T18:57:50Z

Hi @terassyi - sorry you had to wait so long. I was on a leave for some time.

Anyway - I've fixed the issues you've found and added answer to your question regarding the callback function. I'll tests the changes tomorrow, but can I ask you to trigger the test workflow anyway?

terassyi · 2025-01-14T07:10:02Z

Hi, @p-strusiewiczsurmacki-mobica
thank you for updating!

I approved to run CI👍

p-strusiewiczsurmacki-mobica · 2025-01-14T12:41:28Z

~~That's weird. I see that the small tests failed in the CI, but I can't see that error to happen on my machine.~~

~~EDIT: CI no my fork works as well: https://github.com/p-strusiewiczsurmacki-mobica/coil/actions/runs/12753248695/job/35544494665?pr=2~~

Well, I reworked those tests a bit. Let's see now.

EDIT: It works on my fork: https://github.com/p-strusiewiczsurmacki-mobica/coil/actions/runs/12769527188

terassyi

Thank you for updating!

I reviewed the whole changes and added some small comments.
After reflecting on these, we can merge this!

.github/workflows/ci.yaml

v2/Makefile

Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]> Co-authored-by: Tomoya Terashima <[email protected]>

terassyi · 2025-01-24T09:06:49Z

@p-strusiewiczsurmacki-mobica
Thanks a lot!
I approved this.

For merging, I'm testing compatibility.
Because, from this PR, coil-controller will separate coil-ipam-controller and coil-egress-controller.
I believe that there are no behavior changes, but some resources(for example, ServiceAccount and ValidatingWebhookConfiguration) are affected by this change.

We also have to consider how to migrate the controller separation.

Now, I'm not sure I work on this as another PR.
Anyway, please wait for a moment to make a decision.

p-strusiewiczsurmacki-mobica mentioned this pull request Aug 8, 2024

[WiP] Documentation changes for egress separation #297

Closed

tkna requested a review from terassyi August 9, 2024 05:12

terassyi reviewed Sep 20, 2024

View reviewed changes

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch 2 times, most recently from 66abdf2 to df10c75 Compare September 24, 2024 15:00

terassyi self-requested a review October 4, 2024 01:45

terassyi reviewed Oct 4, 2024

View reviewed changes

v2/e2e/Makefile Outdated Show resolved Hide resolved

v2/e2e/coil_test.go Outdated Show resolved Hide resolved

docs/setup.md Outdated Show resolved Hide resolved

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch 2 times, most recently from 59227cc to 2eb29ee Compare October 10, 2024 16:06

terassyi reviewed Oct 16, 2024

View reviewed changes

v2/e2e/coil_test.go Outdated Show resolved Hide resolved

v2/e2e/Makefile Outdated Show resolved Hide resolved

v2/e2e/coil_test.go Outdated Show resolved Hide resolved

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch 2 times, most recently from 2c074c1 to 8958f4b Compare October 17, 2024 14:55

p-strusiewiczsurmacki-mobica requested a review from terassyi October 23, 2024 11:57

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch from a058344 to 9ed43c5 Compare October 31, 2024 11:05

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch from 9ed43c5 to 09d9a0e Compare November 4, 2024 11:59

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch from b065851 to 7483dee Compare November 6, 2024 15:17

terassyi reviewed Dec 2, 2024

View reviewed changes

terassyi self-requested a review December 2, 2024 08:56

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch from dbaeacb to a5be1f8 Compare January 13, 2025 18:03

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch from a5be1f8 to 2f91b00 Compare January 14, 2025 14:16

terassyi reviewed Jan 21, 2025

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

v2/Makefile Outdated Show resolved Hide resolved

Standalone Egress NAT

edae8ae

Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]> Co-authored-by: Tomoya Terashima <[email protected]>

p-strusiewiczsurmacki-mobica force-pushed the standalone-egress branch from 3d7c727 to edae8ae Compare January 22, 2025 19:16

terassyi approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standalone Egress NAT #299

Standalone Egress NAT #299

p-strusiewiczsurmacki-mobica commented Aug 8, 2024 •

edited

Loading

terassyi commented Aug 26, 2024

terassyi commented Sep 20, 2024

terassyi commented Sep 20, 2024

p-strusiewiczsurmacki-mobica commented Sep 24, 2024

terassyi commented Sep 26, 2024

p-strusiewiczsurmacki-mobica commented Sep 26, 2024

p-strusiewiczsurmacki-mobica commented Oct 17, 2024

terassyi commented Oct 21, 2024

terassyi commented Oct 21, 2024

p-strusiewiczsurmacki-mobica commented Oct 23, 2024

terassyi commented Oct 28, 2024

p-strusiewiczsurmacki-mobica commented Oct 28, 2024

terassyi commented Oct 30, 2024

terassyi commented Oct 30, 2024

terassyi commented Oct 30, 2024

p-strusiewiczsurmacki-mobica commented Oct 31, 2024

terassyi commented Nov 1, 2024

p-strusiewiczsurmacki-mobica commented Nov 4, 2024

terassyi commented Nov 5, 2024

p-strusiewiczsurmacki-mobica commented Nov 6, 2024 •

edited

Loading

RefluxMeds commented Nov 11, 2024

terassyi commented Nov 12, 2024

terassyi commented Nov 22, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Nov 25, 2024 •

edited

Loading

terassyi Dec 2, 2024

p-strusiewiczsurmacki-mobica Jan 13, 2025

terassyi Jan 15, 2025

terassyi commented Dec 2, 2024

p-strusiewiczsurmacki-mobica commented Jan 13, 2025 •

edited

Loading

terassyi commented Jan 14, 2025

p-strusiewiczsurmacki-mobica commented Jan 14, 2025 •

edited

Loading

terassyi left a comment

terassyi commented Jan 24, 2025

Standalone Egress NAT #299

Are you sure you want to change the base?

Standalone Egress NAT #299

Conversation

p-strusiewiczsurmacki-mobica commented Aug 8, 2024 • edited Loading

terassyi commented Aug 26, 2024

terassyi commented Sep 20, 2024

terassyi commented Sep 20, 2024

p-strusiewiczsurmacki-mobica commented Sep 24, 2024

terassyi commented Sep 26, 2024

p-strusiewiczsurmacki-mobica commented Sep 26, 2024

p-strusiewiczsurmacki-mobica commented Oct 17, 2024

terassyi commented Oct 21, 2024

terassyi commented Oct 21, 2024

p-strusiewiczsurmacki-mobica commented Oct 23, 2024

terassyi commented Oct 28, 2024

p-strusiewiczsurmacki-mobica commented Oct 28, 2024

terassyi commented Oct 30, 2024

terassyi commented Oct 30, 2024

terassyi commented Oct 30, 2024

p-strusiewiczsurmacki-mobica commented Oct 31, 2024

terassyi commented Nov 1, 2024

p-strusiewiczsurmacki-mobica commented Nov 4, 2024

terassyi commented Nov 5, 2024

p-strusiewiczsurmacki-mobica commented Nov 6, 2024 • edited Loading

RefluxMeds commented Nov 11, 2024

terassyi commented Nov 12, 2024

terassyi commented Nov 22, 2024 • edited Loading

p-strusiewiczsurmacki-mobica commented Nov 25, 2024 • edited Loading

terassyi Dec 2, 2024

Choose a reason for hiding this comment

p-strusiewiczsurmacki-mobica Jan 13, 2025

Choose a reason for hiding this comment

terassyi Jan 15, 2025

Choose a reason for hiding this comment

terassyi commented Dec 2, 2024

p-strusiewiczsurmacki-mobica commented Jan 13, 2025 • edited Loading

terassyi commented Jan 14, 2025

p-strusiewiczsurmacki-mobica commented Jan 14, 2025 • edited Loading

terassyi left a comment

Choose a reason for hiding this comment

terassyi commented Jan 24, 2025

p-strusiewiczsurmacki-mobica commented Aug 8, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Nov 6, 2024 •

edited

Loading

terassyi commented Nov 22, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Nov 25, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Jan 13, 2025 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Jan 14, 2025 •

edited

Loading