Skip to content
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.

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

Fix too restrictive network interface name checking #228

Closed
wants to merge 3 commits into from
Closed

Conversation

thediveo
Copy link

Fix too restrictive network interface name checking

parsePodNetworkObjectName() had too restrictive network interface name checking, trying to enforce RFC 1123 DNS label syntax. However, https://github.com/K8sNetworkPlumbingWG/multi-net-spec/blob/master/%5Bv1%5D%20Kubernetes%20Network%20Custom%20Resource%20Definition%20De-facto%20Standard.md states:

4.1.2.1.5 "interface"
This optional key with value of type string requires the implementation to use the given name for the pod interface resulting from this network attachment. This keys value must be a valid Linux kernel interface name

Fixes #213 and adds tests for correct parsing behavior.

@coveralls
Copy link

coveralls commented Dec 21, 2018

Pull Request Test Coverage Report for Build 685

  • 22 of 22 (100.0%) changed or added relevant lines in 1 file are covered.
  • 171 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+1.9%) to 54.72%

Files with Coverage Reduction New Missed Lines %
k8sclient/k8sclient.go 34 62.5%
types/conf.go 36 32.29%
multus/multus.go 101 56.75%
Totals Coverage Status
Change from base Build 569: 1.9%
Covered Lines: 626
Relevant Lines: 1144

💛 - Coveralls

Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @thediveo -- just a quick note to say that there will probably be delay before the maintainers get together to review this PR / other PRs during the upcoming holidays.

k8sclient/k8sclient.go Show resolved Hide resolved
@dcbw
Copy link
Member

dcbw commented Jan 10, 2019

The code comment about allowed characters is not actually correct. The kernel validates the ifname like so:

bool dev_valid_name(const char *name)
{
	if (*name == '\0')
		return false;
	if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
		return false;
	if (!strcmp(name, ".") || !strcmp(name, ".."))
		return false;

	while (*name) {
		if (*name == '/' || *name == ':' || isspace(*name))
			return false;
		name++;
	}
	return true;
}

@dougbtv
Copy link
Member

dougbtv commented Jan 10, 2019

Also some examples in practice: https://paste.fedoraproject.org/paste/Cm2556XZYIO9IPbZaIWDoQ

@thediveo
Copy link
Author

In the light of dev_valid_name I will reconsider my code to adhere to the nif name restrictions implemented in the Linux kernel.

@rkamudhan rkamudhan self-requested a review February 21, 2019 15:16
@rkamudhan
Copy link
Member

@thediveo As a follow up, you planning to take the @dcbw and @dougbtv suggestion in PR ? if so, we can move this PR as WIP

@thediveo
Copy link
Author

thediveo commented Mar 1, 2019

Yes, I do, so please move to WIP, as I want to bring my PR in sync with the better knowledge about what valid interfaces names in Linux are.

…le "@" in ifnames, but no need to handle "/".
@thediveo
Copy link
Author

thediveo commented Mar 1, 2019

@rkamudhan I've updated my PR to now take dev_valid_name rules into account; I've reverted to the original separation sequence to first split on "/" and then "@", with the latter separation process now accepting multiple "@"s and separating only at the first "@". I've also updated tests too.

For the tests I noticed that it seems to be acceptable to have an empty namespace and network name, and only a network interface name, such as in "@eth42": is this correct?

If it is, then I would suspect my PR to be ready.

@s1061123 s1061123 force-pushed the master branch 2 times, most recently from 1b0b39d to c319f6b Compare March 22, 2019 03:14
@thediveo
Copy link
Author

thediveo commented May 4, 2019

@rkamudhan @dcbw @dougbtv any interest in merging this after I've implemented the suggestions raised before?

@s1061123
Copy link
Member

s1061123 commented Apr 6, 2020

let me close it because RFC1123 is not same as linux kernel interface name convention.

@s1061123 s1061123 closed this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Network interface names restricted to RFC 1123 DNS label syntax
6 participants