bgpv1: Don't use net package for addressing

[YutaroHayakawa]

We're now discouraging using net package for addressing. Convert all
net.IP and net.IPNet to netip.Addr and netip.Prefix. During the
conversion, routerID resolution code is duplicated to two places. As a
part of the cleanup, I consolidated them into the single function.

Related: #24246

bgpv1: Don't use net package for addressing

[harsimran-pabla]

much awaited net/netip, amazing to see this change!

[YutaroHayakawa]

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) with L7 policy Tests NodePort with L7 Policy

Failure Output

FAIL: Request from testclient-rlzkz pod to service tftp://[fd04::11]:30278/hello failed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/2049/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

[YutaroHayakawa]

k8s-1.25-kernel-4.19: #25096
runtime: Looks like it checkouts old commit. I guess it didn't trigger in the last push? Let me manually rerun.

[YutaroHayakawa]

/test-1.25-4.19

[YutaroHayakawa]

/test-runtime

[dylandreimerink]

I did not know about this package until now, this is very nice!

[YutaroHayakawa]

/test-runtime

[YutaroHayakawa]

Fixed minor grammar mistakes in the commit message. Also wanted to try triggering the test-runtime with the right commit again.

[YutaroHayakawa]

/test

[YutaroHayakawa]

Very weird... GHA now started to pick older commits for other workflows: https://github.com/cilium/cilium/actions/runs/4927081052/jobs/8803538691?pr=25313

[YutaroHayakawa]

Ok, I have no luck today. I don't know if this is due to the ongoing GH incident, but I'll retry tomorrow.

[YutaroHayakawa]

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unexpected missed tail calls

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2186/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

[YutaroHayakawa]

Let me close and open this PR. Some workflow keeps grabbing the old commits.

[YutaroHayakawa]

Very weird... Seems like we cannot recover from here.

[YutaroHayakawa]

/test

[YutaroHayakawa]

Go-related checks / lint: https://github.com/cilium/cilium/actions/runs/4956257837/jobs/8866490366?pr=25313

• GH got mad after the outage, and it no longer checkouts the correct latest changes

ClusterMesh: https://github.com/cilium/cilium/actions/runs/4956670271

• 1.1.1.1 test is failing, which is known to be unstable

Travis: https://github.com/cilium/cilium/pull/25313/checks?check_run_id=13426991588

• GH got mad after the outage, and it no longer checkouts the correct latest changes

k8s-1.26-kernel-net-next: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2244/

• Failing on the VM boot up.

runtime: https://jenkins.cilium.io/job/Cilium-PR-Runtime-net-next/6638/

• GH got mad after the outage, and it no longer checkouts the correct latest changes

[YutaroHayakawa]

/ci-multicluster

[YutaroHayakawa]

/test-1.26-net-next

[YutaroHayakawa]

ClusterMesh: Filed a new issue #25442

[YutaroHayakawa]

/ci-multicluster

[YutaroHayakawa]

For golang lint and precheck, I run these tests locally and they are passing.

$ git checkout yutaro/bgp-cplane/use-netip-prefix
$ make lint
docker run --rm -v `pwd`:/app -w /app docker.io/golangci/golangci-lint:v1.52.2@sha256:3d2f4240905054c7efa7f4e98ba145c12a16995bbc3e605300e21400a1665cb6 golangci-lint run

$ make precheck
contrib/scripts/check-logging-subsys-field.sh
contrib/scripts/check-fmt.sh
contrib/scripts/check-log-newlines.sh
contrib/scripts/check-test-tags.sh
contrib/scripts/check-assert-deep-equals.sh
contrib/scripts/lock-check.sh
contrib/scripts/check-viper.sh
contrib/scripts/custom-vet-check.sh
contrib/scripts/rand-check.sh

For runtime and Travis tests, I think it's safe to ignore them since they don't test the code path modified in this PR, and integration-test is passing.

[dylandreimerink]

The Travis test fails with the following error:

 GO     operator/cilium-operator
# github.com/cilium/cilium/pkg/bgpv1/manager
../pkg/bgpv1/manager/advertisements_reconciler.go:107:6: unknown field Net in struct literal of type "github.com/cilium/cilium/pkg/bgpv1/types".Advertisement
../pkg/bgpv1/manager/advertisements_reconciler.go:107:27: advrt.Net undefined (type "github.com/cilium/cilium/pkg/bgpv1/types".Advertisement has no field or method Net)
make[1]: *** [Makefile:14: cilium-agent] Error 1
make: *** [Makefile:59: daemon] Error 2

Same for the runtime test:

12:13:28      runtime: #15 72.06 CGO_ENABLED=0 go build -mod=vendor -ldflags '-X "github.com/cilium/cilium/pkg/version.ciliumVersion=1.14.0-dev 7e26f334d8 2023-05-12T08:30:47+00:00" -s -w -X "github.com/cilium/cilium/pkg/proxy.requiredEnvoyVersionSHA=4350471813b173839df78f7a1ea5d77b5cdf714b" -X "github.com/cilium/cilium/pkg/datapath/loader.DatapathSHA256=813a4a12226bba103827831c800082a316d36275abd90f07d726b23097b45bbb" ' -tags=osusergo,lockdebug  -o cilium-agent
12:13:28      runtime: #15 85.86 # github.com/cilium/cilium/pkg/bgpv1/manager
12:13:28      runtime: #15 85.86 ../pkg/bgpv1/manager/advertisements_reconciler.go:107:6: unknown field Net in struct literal of type "github.com/cilium/cilium/pkg/bgpv1/types".Advertisement
12:13:28      runtime: #15 85.86 ../pkg/bgpv1/manager/advertisements_reconciler.go:107:27: advrt.Net undefined (type "github.com/cilium/cilium/pkg/bgpv1/types".Advertisement has no field or method Net)
12:13:28      runtime: #15 96.14 make[1]: Leaving directory '/go/src/github.com/cilium/cilium/daemon'
12:13:28      runtime: #15 96.14 make[1]: *** [Makefile:14: cilium-agent] Error 1
12:13:28      runtime: #15 96.14 make: *** [Makefile:56: build-container] Error 2

That does not seem like a flake :(

[YutaroHayakawa]

@dylandreimerink That is a GH issue. It picks an old commit, as I mentioned in this comment (#25313 (comment)). In the latest commit, it is fixed.

https://github.com/cilium/cilium/pull/25313/files#diff-0bf546cea985047b084104fb137a2205d0060842e36f92bf5660ecbbe51cfe88L102-R107

If it still has that issue, every workflow compiles the source should also fail, but they don't.

@YutaroHayakawa FYI - I triggered rebase from the GH UI.

[aditighag]

Here is the snapshot of the test runs. Let's rebase and re-run the failing Travis and runtime tests. For go linters, Yutaro ran the tests locally.
We can fast track this PR without running the full CI once the basic test runs are completed.

[aditighag]

@YutaroHayakawa There are still failures after rebase. PTAL. I'm removing the ready-to-merge label for now. Feel free to add it back with notes in case there are failures.

[YutaroHayakawa]

Ashame, I noticed I was stupid 🤦 I kept saying the failure was due to the GitHub issue, but it wasn't. The CI noticed line 107 of the advertisement_reconsiler.go has an issue that uses Advertisement.Net, which is renamed in this PR, but in the preview of this PR, it looks like this, and we don't have such a problem. So. I thought GitHub picked up the old commit, which had a similar issue (I forgot to rename some Net fields in the old commit).

However, what I was missing was the CI rebases my changes on top of the latest main, and the latest main recently got a change in line 107 (a0254cd#diff-0bf546cea985047b084104fb137a2205d0060842e36f92bf5660ecbbe51cfe88R107).

My local branch was not rebased on top of the latest main. That's why retry never helped. Now I pushed the fix on top of the latest main, and now the CI recovered.

My bad 🤦 thanks @aditighag @dylandreimerink for stopping before I break the tree 🙇

[YutaroHayakawa]

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "tftp://192.168.56.12:32112/hello" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2311/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

[YutaroHayakawa]

k8s-1.26-kernel-net-next: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2311/

#25411

[YutaroHayakawa]

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "tftp://[fd04::11]:31279/hello" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2313/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

[YutaroHayakawa]

k8s-1.26-kernel-net-next: Again failing with #25411

[YutaroHayakawa]

/test-1.26-net-next