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 invalid supportbundle temporary name #1150

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

weiqiangt
Copy link
Contributor

@weiqiangt weiqiangt commented Aug 26, 2020

In afero 1.2.2, the TempFile does not accept wildcard and thus the filename will
contain an asterisk. Windows does not allow filename to have an asterisk.

Fixes #1148.

@weiqiangt
Copy link
Contributor Author

/test-all

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

@weiqiangt I feel like I am missing something. When I look at the documentation for afero.TempFile and at the implementation (https://github.com/spf13/afero/blob/64002605e424363ac9aeb8c945c6dbfc06bd0d62/ioutil.go#L178), it seems that it should work. Could you explain where the issue comes from?

Also in your commit message, you mean "asterisk", not "asteroid".

@weiqiangt
Copy link
Contributor Author

weiqiangt commented Aug 26, 2020

@weiqiangt I feel like I am missing something. When I look at the documentation for afero.TempFile and at the implementation (https://github.com/spf13/afero/blob/64002605e424363ac9aeb8c945c6dbfc06bd0d62/ioutil.go#L178), it seems that it should work. Could you explain where the issue comes from?

Also in your commit message, you mean "asterisk", not "asteroid".

Yeah, you're right. But the afero we're using in Antrea is 1.2.2 which has the following implementation.

func TempFile(fs Fs, dir, prefix string) (f File, err error) {
	if dir == "" {
		dir = os.TempDir()
	}

	nconflict := 0
	for i := 0; i < 10000; i++ {
		name := filepath.Join(dir, prefix+nextSuffix())
		f, err = fs.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)
		if os.IsExist(err) {
			if nconflict++; nconflict > 10 {
				randmu.Lock()
				rand = reseed()
				randmu.Unlock()
			}
			continue
		}
		break
	}
	return
}

Now, we have two options to solve this issue, upgrade or edit like this PR. I would prefer the prior one.

Yes, asterisk 🤦‍♂️.

The `TempFile` does not accept wildcard and thus the filename will
contains asterisk. Windows does not allow filename to have asterisk.
@antoninbas
Copy link
Contributor

I also like the upgrade option

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests.
  • /skip-e2e: to skip e2e tests.
  • /test-conformance: to trigger conformance tests.
  • /skip-conformance: to skip conformance tests.
  • /test-whole-conformance: to trigger all conformance tests on linux.
  • /skip-whole-conformance: to skip all conformance tests on linux.
  • /test-networkpolicy: to trigger networkpolicy tests.
  • /skip-networkpolicy: to skip networkpolicy tests.
  • /test-windows-conformance: to trigger windows conformance tests.
  • /skip-windows-conformance: to skip windows conformance tests.
  • /test-windows-networkpolicy: to trigger windows networkpolicy tests.
  • /skip-windows-networkpolicy: to skip windows networkpolicy tests.
  • /test-hw-offload: to trigger ovs hardware offload test.
  • /skip-hw-offload: to skip ovs hardware offload test.
  • /test-all: to trigger all tests (except whole conformance).
  • /skip-all: to skip all tests (except whole conformance).

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@weiqiangt
Copy link
Contributor Author

/test-all

@weiqiangt weiqiangt merged commit 7118d3c into antrea-io:master Aug 26, 2020
@weiqiangt weiqiangt deleted the fix-tmp-bundle-name branch August 26, 2020 08:11
antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Aug 27, 2020
The `TempFile` does not accept wildcard and thus the filename will
contains asterisk. Windows does not allow filename to have asterisk.

Fixes antrea-io#1148.
antoninbas pushed a commit that referenced this pull request Aug 28, 2020
The `TempFile` does not accept wildcard and thus the filename will
contains asterisk. Windows does not allow filename to have asterisk.

Fixes #1148.
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 22, 2020
The `TempFile` does not accept wildcard and thus the filename will
contains asterisk. Windows does not allow filename to have asterisk.

Fixes antrea-io#1148.
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.

antctl supportbundle doesn't work on Windows Nodes
5 participants