CNI: add host-side interface info to cni.Result #24901

nayihz · 2023-04-14T13:38:48Z

Add host-side interface info to cni.Result, which allows bandwidth CNI to work with Cilium

maintainer-s-little-helper · 2023-04-14T13:47:10Z

Commit b1d47cba365c97754a87cf892552a0df27ce85fd does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

tommyp1ckles · 2023-04-24T02:28:45Z

@czybjtu thanks for the PR 😄

One question, later on in the function IPs get their interface index added: https://github.com/cilium/cilium/blob/5b042476dd6f18c70bc7c0d7401cc596abf04b24/plugins/cilium-cni/main.go#L608

Would this still work if we add the host level interface first?

nayihz · 2023-04-24T03:15:20Z

Would this still work if we add the host level interface first?

Thanks for catching this. Have fixed it. PTAL.

tommyp1ckles · 2023-04-25T18:19:21Z

plugins/cilium-cni/main.go

@@ -552,6 +557,9 @@ func cmdAdd(args *skel.CmdArgs) (err error) {
 			err = fmt.Errorf("unable to prepare IP addressing for '%s': %s", ep.Addressing.IPV6, err)
 			return
 		}
+		// Add to the result the Interface as index of Interfaces
+		ipConfig.Interface = cniTypesV1.Int(len(res.Interfaces))


Shouldn't this be:

Suggested change

ipConfig.Interface = cniTypesV1.Int(len(res.Interfaces))

ipConfig.Interface = cniTypesV1.Int(len(res.Interfaces)-1)

You cannot do it this way because the index of an array starts from 0.

Sorry I'm still confused,

On line: https://github.com/cilium/cilium/pull/24901/files#diff-87793c7d9c6d6a10f34d6a3efefed53dac8abd700cb05cfb2a77844ec990b744R523

So at this point res.Interfaces is either of length 0 or 1, if ip4 and ip6 are enabled then we're going to be referencing 2, which is out of bounds in the Interfaces array?

Does that make sense? This function is a bit confusing so I might be missing something here.

On line L523, we add the host-side veth info to res.Interface which has no ip.
On line L561 and L577, ipConfig was refer to container-side veth info. ipConfig is the last element that will be added to res.Interface slice. So I think

// ipConfig.Interface represents its index in this slice. ipConfig.Interface = cniTypesV1.Int(len(res.Interfaces))

is what we want.

Does that still work if datapathmodeveth isn't enabled?

(Line 504)

Yes. We add host side veth info to res.Interfaces only when datapathmodeveth enabled. Everything is the same as before if datapathmodeveth was not enabled.

Ok I see now, thanks for the clarification.

joestringer · 2023-05-05T02:09:12Z

/test

joestringer · 2023-05-05T02:10:03Z

I've kicked off the tests to ensure there are no regressions introduced by this. Other than that I think we are just looking for @tommyp1ckles' feedback on the remaining open thread. Thanks for contributing :)

tommyp1ckles · 2023-05-08T23:46:09Z

/test-1.25-4.19

tommyp1ckles · 2023-05-08T23:46:20Z

/test-1.26-net-next

tommyp1ckles · 2023-05-08T23:46:30Z

/test-1.16-4.19

tommyp1ckles · 2023-05-08T23:46:35Z

/test-1.24-5.4

tommyp1ckles · 2023-05-08T23:47:47Z

Changes LGTM 🙏 , just gonna make sure tests are passing

tommyp1ckles · 2023-05-09T14:07:26Z

/test

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

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Error deleting resource /home/jenkins/workspace/Cilium-PR-K8s-1.25-kernel-4.19/src/github.com/cilium/cilium/test/k8s/manifests/host-policies.yaml: Cannot retrieve "cilium-pwzd7"'s policy revision: cannot get policy revision: ""

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

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.

github-actions · 2023-06-09T02:05:28Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

tommyp1ckles · 2023-06-15T06:45:34Z

@czybjtu sorry, just getting back to this one, do you mind rebasing and we can make sure the tests pass.

nayihz · 2023-06-15T08:45:56Z

@czybjtu sorry, just getting back to this one, do you mind rebasing and we can make sure the tests pass.

Done. PTAL.

tommyp1ckles · 2023-06-16T07:10:24Z

/test

tommyp1ckles

Looks good, just going to make sure tests pass

nayihz · 2023-06-16T07:33:43Z

❌ command "curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 --retry 3 --retry-all-errors --retry-delay 3 [http://google.com:80](http://google.com/)" failed: command failed: curl: (28) Resolving timed out after 2000 milliseconds

This doesn't seem to be related to this PR.

ti-mo · 2023-06-20T09:57:28Z

I've approved the tests to run. Could you please add a commit description with some of the context you've outlined in the original issue? right now it's hard to tell what the intention is behind the change by just looking at the commit.

This commit adds host-side interface name and mac address to cni.Result according to CNI Specification (https://www.cni.dev/docs/spec/#section-5-result-types) Signed-off-by: czybjtu <smartczy@outlook.com>

nayihz · 2023-06-23T02:23:42Z

Updated the commit message. PTAL.

tommyp1ckles · 2023-06-23T17:57:47Z

not sure why tests failed, possible GH Actions issue 😕

/test

ldelossa · 2023-06-26T15:59:31Z

@czybjtu GH actions is looking really off here. Can you rebase this PR and force push?

maintainer-s-little-helper · 2023-06-28T01:30:20Z

Commit 8445e2b does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-06-28T01:30:21Z

Commit 8445e2b does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

nayihz · 2023-06-28T01:38:48Z

I have some problems when rebase upstream because the branch name has changed(master -> main). I will open a new same pr for this.
Sorry for inconvenience.

nayihz · 2023-06-28T02:23:30Z

#26518

New pr has been created. PTAL

nayihz requested a review from a team as a code owner April 14, 2023 13:38

nayihz requested a review from tommyp1ckles April 14, 2023 13:38

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 14, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 14, 2023

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Apr 14, 2023

tommyp1ckles reviewed Apr 25, 2023

View reviewed changes

joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 5, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 5, 2023

nayihz requested a review from a team as a code owner May 9, 2023 09:36

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 9, 2023

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 16, 2023

tommyp1ckles approved these changes Jun 16, 2023

View reviewed changes

ti-mo added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jun 20, 2023

ti-mo added the kind/bug This is a bug in the Cilium logic. label Jun 20, 2023

add host-side interface info to cni.Result

e87513f

This commit adds host-side interface name and mac address to cni.Result according to CNI Specification (https://www.cni.dev/docs/spec/#section-5-result-types) Signed-off-by: czybjtu <smartczy@outlook.com>

Merge branch 'cilium:main' into add_hostside_veth_info

8445e2b

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 28, 2023

nayihz closed this by deleting the head repository Jun 28, 2023

tommyp1ckles mentioned this pull request Jun 28, 2023

CNI: add host-side interface info to cni.Result #26518

Merged

joestringer removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNI: add host-side interface info to cni.Result #24901

CNI: add host-side interface info to cni.Result #24901

nayihz commented Apr 14, 2023 •

edited by joestringer

maintainer-s-little-helper bot commented Apr 14, 2023

tommyp1ckles commented Apr 24, 2023

nayihz commented Apr 24, 2023

tommyp1ckles Apr 25, 2023

nayihz Apr 26, 2023

tommyp1ckles Apr 28, 2023

nayihz Apr 28, 2023

tommyp1ckles May 5, 2023 •

edited

nayihz May 5, 2023

tommyp1ckles May 8, 2023

joestringer commented May 5, 2023

joestringer commented May 5, 2023 •

edited

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 9, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

github-actions bot commented Jun 9, 2023

tommyp1ckles commented Jun 15, 2023

nayihz commented Jun 15, 2023

tommyp1ckles commented Jun 16, 2023

tommyp1ckles left a comment

nayihz commented Jun 16, 2023

ti-mo commented Jun 20, 2023

nayihz commented Jun 23, 2023 •

edited

tommyp1ckles commented Jun 23, 2023

ldelossa commented Jun 26, 2023

maintainer-s-little-helper bot commented Jun 28, 2023

maintainer-s-little-helper bot commented Jun 28, 2023

nayihz commented Jun 28, 2023

nayihz commented Jun 28, 2023

	ipConfig.Interface = cniTypesV1.Int(len(res.Interfaces))
	ipConfig.Interface = cniTypesV1.Int(len(res.Interfaces)-1)

CNI: add host-side interface info to cni.Result #24901

CNI: add host-side interface info to cni.Result #24901

Conversation

nayihz commented Apr 14, 2023 • edited by joestringer

maintainer-s-little-helper bot commented Apr 14, 2023

tommyp1ckles commented Apr 24, 2023

nayihz commented Apr 24, 2023

tommyp1ckles Apr 25, 2023

Choose a reason for hiding this comment

nayihz Apr 26, 2023

Choose a reason for hiding this comment

tommyp1ckles Apr 28, 2023

Choose a reason for hiding this comment

nayihz Apr 28, 2023

Choose a reason for hiding this comment

tommyp1ckles May 5, 2023 • edited

Choose a reason for hiding this comment

nayihz May 5, 2023

Choose a reason for hiding this comment

tommyp1ckles May 8, 2023

Choose a reason for hiding this comment

joestringer commented May 5, 2023

joestringer commented May 5, 2023 • edited

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 8, 2023

tommyp1ckles commented May 9, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

github-actions bot commented Jun 9, 2023

tommyp1ckles commented Jun 15, 2023

nayihz commented Jun 15, 2023

tommyp1ckles commented Jun 16, 2023

tommyp1ckles left a comment

Choose a reason for hiding this comment

nayihz commented Jun 16, 2023

ti-mo commented Jun 20, 2023

nayihz commented Jun 23, 2023 • edited

tommyp1ckles commented Jun 23, 2023

ldelossa commented Jun 26, 2023

maintainer-s-little-helper bot commented Jun 28, 2023

maintainer-s-little-helper bot commented Jun 28, 2023

nayihz commented Jun 28, 2023

nayihz commented Jun 28, 2023

nayihz commented Apr 14, 2023 •

edited by joestringer

tommyp1ckles May 5, 2023 •

edited

joestringer commented May 5, 2023 •

edited

tommyp1ckles commented May 9, 2023 •

edited by maintainer-s-little-helper bot

nayihz commented Jun 23, 2023 •

edited