Semver parsing does not terminate CRLF making it returns v0.0.0 when parsing

[antoooks]

What happened?

internal libary cmd/gorepomod/internal/semver/semver.go has a function Parse(raw string) which translate a raw string into Semver object. However, if we supply string with CRLF (\r\n), it will return a default behavior zero which translates to v0.0.0. This makes semver.Parse() function cannot translate kustomize libarary current versioning e.g. kyaml right now is v0.15.1 due to Git commands returns results with CRLF.

Note: the function will not throw an error, but it will throw a default behavior

What did you expect to happen?

The correct behavior

semver.Parse("v0.15.1") will result &Semver{major: 0, minor: 15, patch: 1}

How can we reproduce it (as minimally and precisely as possible)?

Usage:

// Call a git function
currentBranchName, err := gr.run(noHarmDone, "rev-parse", "--abbrev-ref", "release-kyaml/v0.16.3")

// Split branch name string
currentVersionString := strings.Split(currentBranchName, "/")

// Supply raw string to parse function
version, _ := semver.Parse(currentVersionString[1])

Expected output

version, _ := semver.Parse("v0.16.3")
// will return v0.16.3 Semver object
&Semver{major: 0, minor: 16, patch: 3}

Actual output

// return v0.0.0 Semver object
&Semver{major: 0, minor: 0, patch: 0}

Kustomize version

v5.3.0

Operating system

Linux

[k8s-ci-robot]

This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ephesused]

I'm not seeing the same behavior that you're seeing. If it were failing the way you've described, then I would expect to see failures in semver_test.go, particularly TestParse/two.

I tried adding this testcase into semver_test.go, and it came back clean:

func Test5526(t *testing.T) {
	expected := SemVer{major: 0, minor: 15, patch: 1}
	actual, err := Parse("v0.15.1")
	if err != nil {
		t.Errorf("Unexpected error: '%s'", err)
	}
	if !actual.Equals(expected) {
		t.Errorf("Not equal: '%s' and '%s'", actual, expected)
	}
	if actual.String() != "v0.15.1" {
		t.Errorf("String for '%s' not as expected", actual)
	}
}

Will you detail further how to repeat the failure? Thanks.

[koba1t]

Hi @antoooks
Can you investigate and fix this problem?

[antoooks]

Hi @ephesused thanks for pointing that out. The issue happens when I used .Parse() after I supplied version string I got from using git function currentBranchName, err := gr.run(noHarmDone, "rev-parse", "--abbrev-ref", "release-kyaml/v0.16.3")

I split the branch name string to extract the version number currentVersionString := strings.Split(currentBranchName, "/").

After that I supplied the currentVersionString semver.Parse() function, it returns v0.0.0. I have investigate further and finds out that the version string returned by this function call currentVersionString := strings.Split(currentBranchName, "/") has a trailing CRLF \r\n, so semver.Parse() function need to clean this before parsing the string. I will add the functionality together with CI automation on this PR #5520

Will change the title and description accordingly

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[antoooks]

/remove-lifecycle stale

[ephesused]

Looking at this one again, I would recommend that the caller trim its whitespace prior to invoking .Parse(). Whitespace is not allowed in a semver, so I don't think semver.Parse() should allow whitespace either.

I see different behavior than you do when I try to parse v0.15.1\r\n. We both get the zero value, but where you don't get an error, I do:

strconv.Atoi: parsing "1\r\n": invalid syntax

I added this locally to TestParse and it runs clean:

	"trailing whitespace": {
		raw:    "v0.15.1\r\n",
		v:      zero,
		errMsg: "strconv.Atoi: parsing \"1\\r\\n\": invalid syntax",
	},

That fits what I would expect. Based on what I'm seeing, I believe .Parse() is acting properly.

If .Parse() isn't giving you an error in this scenario, I think the better route is to figure out what the difference is in our scenarios, and fix things so that it throws the error properly for you.

[antoooks]

Looking at this one again, I would recommend that the caller trim its whitespace prior to invoking .Parse(). Whitespace is not allowed in a semver, so I don't think semver.Parse() should allow whitespace either.

I see different behavior than you do when I try to parse v0.15.1\r\n. We both get the zero value, but where you don't get an error, I do:

strconv.Atoi: parsing "1\r\n": invalid syntax

I added this locally to TestParse and it runs clean:

  "trailing whitespace": {
  	raw:    "v0.15.1\r\n",
  	v:      zero,
  	errMsg: "strconv.Atoi: parsing \"1\\r\\n\": invalid syntax",
  },

That fits what I would expect. Based on what I'm seeing, I believe .Parse() is acting properly.

If .Parse() isn't giving you an error in this scenario, I think the better route is to figure out what the difference is in our scenarios, and fix things so that it throws the error properly for you.

Thanks for pointing it out, I have included trim inside Parse() on my PR (#5449) because I need it for the feature I'm working on. I see your point and will include such testing on my changes as well

[antoooks]

Hi @ephesused , I am able to reproduce the condition. So you cannot use "v0.15.1\r\n" because that is a string and strconv.Atoi() will read \r and \n literally, as runes, though it will trigger the error handling condition anyway, it does not represent this issue. So to reproduce correctly you have to use fmt.Sprintf("v0.15.1\r\n"), anyways I will use "v0.15.1\r\n" since it's a better practice according to the linter.

And secondly, throwing error means it works as expected. It's just that the test does not consume the return value and only look for the error that is being thrown together with zero. It is the same case as v1.x.222 test case (another non-digit test case), only difference being the non-digit for CRLF case is /r/n and is placed at the last digit.

On the source code you can see this conditional:

// kustomize/cmd/gorepomod/internal/semver.go, L55-58

      n[i], err = strconv.Atoi(fields[i])
      if err != nil {
	      return zero, err
      }

Which makes it supposedly throw zero or v0.0.0 upon non-digit input.

[ephesused]

Sorry, I don't understand what you're describing. If you have a test case that shows the incorrect semver.Parse() behavior, will you please copy-and-paste it in an update to this ticket?

From what I see, semver.Parse() is behaving properly, and adhering to the semantic versioning standard. That is why I don't think it should change. Instead, I believe the caller should trim whitespace prior to calling semver.Parse().