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

mc rm to support entire namespace removal #2265

Merged
merged 1 commit into from
Dec 2, 2017
Merged

Conversation

poornas
Copy link
Contributor

@poornas poornas commented Sep 29, 2017

Fixes #2250
Currently, mc rm --recursive --force works on buckets but not on the entire site. This PR adds support for rm --recursive to delete entire site.

To distinguish full site removal from removing a bucket and provide appropriate warning to the user, the user needs to supply --dangerous flag in addition to the --recursive and --force flags for full site rm.

The rm --recursive --force option will continue to work as before at the bucket level or on the file system.

@codecov-io
Copy link

codecov-io commented Sep 29, 2017

Codecov Report

Merging #2265 into master will decrease coverage by 0.18%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
- Coverage   10.95%   10.76%   -0.19%     
==========================================
  Files         102      102              
  Lines        7624    10042    +2418     
==========================================
+ Hits          835     1081     +246     
- Misses       6654     8823    +2169     
- Partials      135      138       +3
Impacted Files Coverage Δ
cmd/rm-main.go 0% <0%> (ø) ⬆️
cmd/client-s3.go 13.89% <0%> (-0.68%) ⬇️
cmd/pipechan.go 87.8% <0%> (-12.2%) ⬇️
cmd/session.go 35.89% <0%> (-6.48%) ⬇️
pkg/hookreader/hookreader.go 19.23% <0%> (-5.77%) ⬇️
cmd/humanized-duration.go 59.57% <0%> (-4.53%) ⬇️
cmd/client-url.go 57.85% <0%> (-2.33%) ⬇️
pkg/probe/probe.go 42.59% <0%> (-2.24%) ⬇️
cmd/ls.go 10.71% <0%> (-1.45%) ⬇️
cmd/config.go 20.56% <0%> (-1.4%) ⬇️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65c582c...c65be4d. Read the comment docs.

cmd/rm-main.go Outdated
$ {{.HelpName}} --recursive --namespace s3

6. Remove all buckets and objects older than '90' days from entire namespace
$ {{.HelpName}} --recursive --namespace --older-than=90 s3
Copy link
Member

Choose a reason for hiding this comment

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

Tabbing is off.

@poornas poornas force-pushed the rr2 branch 3 times, most recently from 11fd120 to 1b66a05 Compare September 29, 2017 21:42
cmd/rm-main.go Outdated
isNamespace := ctx.Bool("namespace")
isNamespaceRemoval := false
for _, url := range ctx.Args() {
if isHostName(url) {
Copy link
Member

Choose a reason for hiding this comment

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

This method does not work for removing a directory with non-aliases. For example mc rm mylocaldir would fail. Basically we need to differentiate alias and local dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this to incorporate feedback

cmd/rm-main.go Outdated

if (isRecursive || isStdin) && isNamespaceRemoval && !isNamespace {
fatalIf(errDummy().Trace(),
"Full namespace removal requires --namespace option. This operation removes all the buckets on your namespace. Please review carefully before performing this *DANGEROUS* operation.")
Copy link
Member

Choose a reason for hiding this comment

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

This check is not interesting. removing local file system, we don't need any restriction like that.

cmd/rm-main.go Outdated
// print the rmMessage only if content is a bucket or object. clnt.List lists
// prefixes as well which unifies the S3 listing with folder listing - however
// these prefixes are not actual objects on s3 and the content.Time defaults to
// zero. Filter these out to prevent erroneous rmMessage being printed to console.
Copy link
Member

Choose a reason for hiding this comment

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

You can skip entries those are ending with / and having zero time for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

cmd/rm-main.go Outdated
if !ok {
close(contentCh)
return exitStatus(globalErrorExitStatus)
}
Copy link
Member

Choose a reason for hiding this comment

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

We would need to exit on first error here. This check leads to continuation. Below is the right logic

case pErr, ok := <-errorCh:
	if ok {
		errorIf(pErr.Trace(urlString), "Failed to remove `"+urlString+"`.")
		switch pErr.ToGoError().(type) {
		case PathInsufficientPermission:
			// Ignore Permission error.
			continue
		}
	}
	close(contentCh)
	return exitStatus(globalErrorExitStatus)

IMO, having ok is not required because pErr will be nil on closed channel and we exit anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this.

cmd/rm-main.go Outdated
func removeSiteRecursive(url string, isIncomplete bool, isFake bool, older int) (err error) {
clnt, pErr := newClient(url)
if pErr != nil {
errorIf(pErr.Trace(url), "Failed to remove `"+url+"` recursively.")
Copy link
Member

Choose a reason for hiding this comment

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

This is not appropriate error message here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

This is fine because of pErr.Trace(url). Sorry for the confusion.

cmd/rm-main.go Outdated
err = removeRecursive(url, isIncomplete, isFake, older)
if isHostName(url) {
if isNamespace {
err = removeSiteRecursive(url, isIncomplete, isFake, older)
Copy link
Member

Choose a reason for hiding this comment

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

Unable to understand the need of this function.

Current code fails for below commands

$ mc rm --recursive --fake --force play
mc: <ERROR> Failed to remove `play` recursively. Bucket `` does not exist.

What I am able to visualize with --namespace flag is by running mc rm --recursive --fake --force play, it could get list of buckets from play alias and keep calling removeRecursive() with each bucket URL.

cmd/rm-main.go Outdated
@@ -264,6 +293,43 @@ func removeRecursive(url string, isIncomplete bool, isFake bool, older int) erro
return nil
}

func isHostName(url string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Add unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this function.

cmd/rm-main.go Outdated
@@ -245,7 +246,7 @@ func removeRecursive(url string, isIncomplete bool, isFake bool, older int) erro
// prefixes as well which unifies the S3 listing with folder listing - however
// these prefixes are not actual objects on s3 and the content.Time defaults to
// zero. Filter these out to prevent erroneous rmMessage being printed to console.
if !content.Time.IsZero() {
if !(content.Time.IsZero() && strings.HasSuffix(urlString, string(filepath.Separator))) {
Copy link
Member

Choose a reason for hiding this comment

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

S3 service sends / as separator whereas on windows \. We would need to check whether this works on windows or not.

cmd/rm-main.go Outdated
@@ -351,7 +344,7 @@ func mainRm(ctx *cli.Context) error {
// Support multiple targets.
for _, url := range ctx.Args() {
if isRecursive {
if isHostName(url) {
if !isAliasURLDir(url) && (strings.Count(url, string(filepath.Separator)) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isAliasURLDir is used internally by the checkCopySyntax function. Here the intent is to make sure only the alias is entered, and it is at the root level not a subdirectory.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

You should look at the cp-url-syntax.go implementation to understand the how to guess the URL types.

checkCopySyntax

@poornas poornas force-pushed the rr2 branch 2 times, most recently from b2a0a38 to 42f05f9 Compare October 25, 2017 22:55
@poornas
Copy link
Contributor Author

poornas commented Oct 25, 2017

@harshavardhana and @balamurugana , updated the PR with a different approach - recursive removal is handled agnostic to fs/s3.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

There is some inconsistent error messaging here

$ mc rm --recursive s3
mc: <ERROR> Removal requires --force option. This operation is *IRREVERSIBLE*. Please review carefully before performing this *DANGEROUS* operation. 
$ mc rm --recursive --force s3
mc: <ERROR> Full site removal requires --dangerous option. This operation removes all the buckets on your namespace. Please review carefully before performing this *DESTRUCTIVE* operation. 

Requires like two steps to get to --dangerous

$ mc rm --recursive --force --dangerous s3
Removing `s3/aead/manual.txt`.
Removing `s3/aead/my-encrypted-object.txt`.
Removing `s3/aead`.
Removing `s3/ersan-ersan`.
Removing `s3/kannappan/list.go`.
Removing `s3/kannappan/1/2/3`.
Removing `s3/kannappan/a/b/`.
Removing `s3/kannappan`.
Removing `s3/kannappan1`.
Removing `s3/mc-mint-test-bucket-10442/2123123\123`.
Removing `s3/mc-mint-test-bucket-10442`.
panic: close of nil channel

goroutine 7 [running]:
github.com/minio/mc/cmd.(*s3Client).Remove.func1(0xc4200860c0, 0xc420086060, 0xc420349480, 0xc42030fd00, 0xc42000ea30, 0xc42000ea38, 0x405000, 0xc42000ea40, 0xc42005c15c)
	/home/harsha/mygo/src/github.com/minio/mc/cmd/client-s3.go:723 +0x3ff
created by github.com/minio/mc/cmd.(*s3Client).Remove
	/home/harsha/mygo/src/github.com/minio/mc/cmd/client-s3.go:666 +0x144

Also there seems to be a bug when doing the entire removal, looks like due to an error.

@harshavardhana
Copy link
Member

To distinguish full site removal from removing a bucket and provide appropriate warning to the user, the user needs to supply --dangerous flag in addition to the --recursive and --force flags for full site rm.

Tested the PR mc rm -r --force play/ seems to delete everything, but strangely --dangerous flag is removed was that intended?

@poornas
Copy link
Contributor Author

poornas commented Nov 1, 2017

that was an intermediate commit - updated the PR to fix messaging as well.

@harshavardhana
Copy link
Member

Can you sqaush the commits here,. Will test once again after they are squashed.

@poornas
Copy link
Contributor Author

poornas commented Nov 5, 2017

done.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

LGTM, minor changes.

cmd/rm-main.go Outdated
@@ -82,16 +88,22 @@ EXAMPLES:
1. Remove a file.
$ {{.HelpName}} 1999/old-backup.tgz

2. Remove all objects recursively.
2. Remove all objects recursively from a bucket.
Copy link
Member

Choose a reason for hiding this comment

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

Remove all objects recursively from bucket 'jazz-songs' matching 'louis' prefix.

cmd/rm-main.go Outdated
$ {{.HelpName}} --recursive s3/jazz-songs/louis/

3. Remove all objects older than '90' days.
3. Remove all objects older than '90' days from a bucket.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above but with 'older than 90 days'

cmd/rm-main.go Outdated
$ {{.HelpName}} --recursive --older-than=90 s3/jazz-songs/louis/

4. Remove all objects read from STDIN.
$ {{.HelpName}} --force --stdin

5. Drop all incomplete uploads on 'jazz-songs' bucket.
5. Remove all buckets and objects recursively from host
Copy link
Member

Choose a reason for hiding this comment

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

Remove all buckets and objects recursively from S3 host

@poornas poornas force-pushed the rr2 branch 2 times, most recently from a79ab47 to 0c847db Compare November 5, 2017 18:35
@poornas
Copy link
Contributor Author

poornas commented Nov 13, 2017

@harshavardhana and @balamurugana , review comments were addressed. needs a revisit.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

LGTM tested

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

minor comments

cmd/rm-main.go Outdated
isDangerous := ctx.Bool("dangerous")
isNamespaceRemoval := false
for _, url := range ctx.Args() {
if !isAliasURLDir(url) && (strings.Count(url, string(filepath.Separator)) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. Please add a comment about meaning of this logic.
  2. () around strings. is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cmd/rm-main.go Outdated
Size: content.Size,
})

if !(content.Time.IsZero() && strings.HasSuffix(urlString, string(filepath.Separator))) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment about this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

cmd/rm-main.go Outdated
for _, url := range ctx.Args() {
// check if url represents just the namespace without bucket or object prefix specified
// in the url
if !isAliasURLDir(url) && strings.Count(url, string(filepath.Separator)) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

A valid input s3/ and unusual input s3// wouldn't work here. We need to take care properly i.e. we shouldn't use strings.Count() inside use url to alias conversion.

If mentioned input is not supported, we have to error out properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct @balamurugana , added additional validation so that user input errors are caught right away.

cmd/rm-main.go Outdated
}
if alias != "" && path == "" && isBucketEmpty {
isNamespaceRemoval = true
break
Copy link
Member

@balamurugana balamurugana Nov 16, 2017

Choose a reason for hiding this comment

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

The whole checker logic is confusing to me. In rm we should not check for bucket or object which is completely abstracted by client interface. I see below logic is good enough

url = filepath.ToSlash(url)
if !isAliasURLDir(url) {
    alias, path := url2Alias(url)
    isNamespaceRemoval = (path == "")
}

Also you need to aware that ctx.Args() may come as multiple different values for single run e.g.
ctx.Args() = ["mysite", "play/my-bucket", "play/another-bucket/my/prefix", "mydir", "mydir2/my/path"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without the check on lines 157-158, errors like mc rm play// wont be caught. Earlier it was being caught in s3 api because bucket name was not permitted to be empty. Now by allowing namespace removal, bucket name can be empty and there is no way to validate cases like multiple consecutive / delimiters being entered as input. Lines 160-163 can be replaced by the logic you proposed.

Copy link
Member

Choose a reason for hiding this comment

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

I see url = filepath.ToSlash(url) is wrong, you should use url = filepath.Clean(url)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

cmd/rm-main.go Outdated
isDangerous := ctx.Bool("dangerous")
isNamespaceRemoval := false
for _, url := range ctx.Args() {
url = filepath.Clean(url)
Copy link
Member

Choose a reason for hiding this comment

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

you could use path.Clean() to work with MS Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't filepath.Clean do the same?

Copy link
Member

Choose a reason for hiding this comment

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

no

Copy link
Member

Choose a reason for hiding this comment

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

path.Clean() doesn't do anything on windows @balamurugana @poornas - filepath.Clean() should be used for cross platform friendly behavior.

Copy link
Member

Choose a reason for hiding this comment

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

See below. This had been discussed long back

package main

import (
	"fmt"
	"path"
	"path/filepath"
)

func main() {
	fmt.Println("filepath.Clean():")
	fmt.Println(filepath.Clean("/s3"))
	fmt.Println(filepath.Clean("s3/"))
	fmt.Println(filepath.Clean("s3/mybucket//object//"))
	fmt.Println(filepath.Clean(`\s3`))
	fmt.Println(filepath.Clean(`s3\`))
	fmt.Println(filepath.Clean(`s3\mybucket\\object\\`))

	fmt.Println("path.Clean():")
	fmt.Println(path.Clean("/s3"))
	fmt.Println(path.Clean("s3/"))
	fmt.Println(path.Clean("s3/mybucket//object//"))
	fmt.Println(path.Clean(`\s3`))
	fmt.Println(path.Clean(`s3\`))
	fmt.Println(path.Clean(`s3\mybucket\\object\\`))
}

On MS Windows

filepath.Clean():
\s3
s3
s3\mybucket\object
\s3
s3
s3\mybucket\object
path.Clean():
/s3
s3
s3/mybucket/object
\s3
s3\
s3\mybucket\\object\\

On GNU/Linux

filepath.Clean():
/s3
s3
s3/mybucket/object
\s3
s3\
s3\mybucket\\object\\
path.Clean():
/s3
s3
s3/mybucket/object
\s3
s3\
s3\mybucket\\object\\

Copy link
Member

Choose a reason for hiding this comment

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

i don't think so you understand the problem of using path.Clean() , path.Clean() doesn't work on windows since it never cleans paths such as \\ or C:\mypath\...\otherdir\ . path.Clean() doesn't understand C:\ etc. so it would be wrong to use path.Clean() for cross platform reasons.

For mc we do support such paths and need to behave properly for windows natively once we do filepath.Clean() if we want all the \ to / then we should use filepath.ToSlash() to convert the paths properly.

Using code like in this manner filepath.ToSlash(filepath.Clean(p)) would work for all operating systems.

Copy link
Member

@balamurugana balamurugana Nov 19, 2017

Choose a reason for hiding this comment

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

How is it possible to store object name containing \ with filepath.ToSlash(filepath.Clean(p))?

If this is unsupported by mc, go ahead and do the change you mentioned. Also check whether you handle UNC path correctly.

Copy link
Member

@harshavardhana harshavardhana Nov 19, 2017

Choose a reason for hiding this comment

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

filepath.ToSlash is used to do proper Windows translation, filepath.ToSlash has no effect on Unixes. Now if you are arguing that if there are files which have \ on Linux then they are stored fine with this mechanism as well. Now on windows there is no filename which explicitly contains \ since its a separator.

Here is how the behavior is

  • filepath.ToSlash converts all \ on Windows to /
  • filepath.ToSlash converts all / to / on Unixes.

filepath.ToSlash is not ubiquitously converting \ to / it is a platform dependent behavior - https://play.golang.org/p/fFPdEZoAEf

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana
Copy link
Member

@poornas can you squash your changes? would it possible to bring in some unit tests as well if possible?

cmd/rm-main.go Outdated
isDangerous := ctx.Bool("dangerous")
isNamespaceRemoval := false
for _, url := range ctx.Args() {
url = path.Clean(url)
Copy link
Member

Choose a reason for hiding this comment

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

It is required to perhaps bring change like this instead filepath.ToSlash(filepath.Clean(url)) here, for windows paths such as C:\ path.Clean will not work.

Also question for me why is this path.Clean needed? is it for isAliasURLDir() ? A comment would be needed i think here.

Copy link
Contributor Author

@poornas poornas Nov 28, 2017

Choose a reason for hiding this comment

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

@harshavardhana , on further thinking, the path.Clean and isAliasURLDir() check are superfluous and can be dispensed with altogether.

Copy link
Member

Choose a reason for hiding this comment

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

Okay that would be better.

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Revoking +1 till finalize on path.Clean() or filepath.Clean() on windows. Also test with UNC path as well.

@poornas
Copy link
Contributor Author

poornas commented Nov 28, 2017

@harshavardhana , have not introduced stand alone functions that can be unit tested here. It might be worthwhile to add a mint cleanup function in the functional tests that exercises this code - but that will cleanup play server and may not be entirely good.

@harshavardhana
Copy link
Member

@harshavardhana , have not introduced stand alone functions that can be unit tested here. It might be worthwhile to add a mint cleanup function in the functional tests that exercises this code - but that will cleanup play server and may not be entirely good.

Okay that makes sense.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/rm-main.go Outdated
isDangerous := ctx.Bool("dangerous")
isNamespaceRemoval := false
for _, url := range ctx.Args() {
_, path := url2Alias(url)
Copy link
Member

@balamurugana balamurugana Nov 28, 2017

Choose a reason for hiding this comment

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

What are all expected behaviors for below command line arguments?

1. mc rm mylocaldir
2. mc rm mylocaldir/
3. mc rm mylocalfile
4. mc rm c:
5. mc rm c:\
6. mc rm \\myserver\myfolder\myfile

Copy link
Contributor Author

@poornas poornas Nov 28, 2017

Choose a reason for hiding this comment

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

➜  mc git:(rr2) mc rm mylocaldir 
Removing `mylocaldir`.
➜  mc git:(rr2) mkdir mylocaldir
➜  mc git:(rr2) touch mylocaldir/testfile
➜  mc git:(rr2) ✗ mc rm mylocaldir 
Removing `mylocaldir`.
mc: <ERROR> Failed to remove `mylocaldir`. remove mylocaldir: directory not empty
➜  mc git:(rr2) ✗ mc rm mylocaldir 
Removing `mylocaldir`.
mc: <ERROR> Failed to remove `mylocaldir`. remove mylocaldir: directory not empty

Copy link
Contributor Author

@poornas poornas Nov 28, 2017

Choose a reason for hiding this comment

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

however, removing the isAliasURLDir() check will interfere with mc rm messaging in recursive removal. Which is why the check was introduced in the first place - for filtering out fs.

➜  mc git:(rr2) ✗ mc rm --r --force mylocaldir 
mc: <ERROR> Full site removal requires --dangerous option. This operation removes all the buckets on your namespace. Please review carefully before performing this *DESTRUCTIVE* operation. ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harshavardhana and @balamurugana , added the isAliasURLDir() check back and a comment on why it is needed.

cmd/rm-main.go Outdated
for _, url := range ctx.Args() {
// namespace removal applies only for non FS. So filter out if passed url represents a directory
if !isAliasURLDir(url) {
_, path := url2Alias(url)
Copy link
Member

Choose a reason for hiding this comment

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

Does url2Alias() take care of returning clean path including windows UNC path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, see test output below..

C:\go\bin> cd ..\src\github.com\minio\mc
C:\go\src\github.com\minio\mc [rr2 ≡]> mc rm --r --force \\myserver\myfolder\myfile
mc.exe: <ERROR> Failed to remove `\\myserver\myfolder\myfile` recursively. open \\myserver\myfolder\myfile: The network name cannot be found.
C:\go\src\github.com\minio\mc [rr2 ≡]> mc rm --r --force C:
Removing `C:.git\FETCH_HEAD`.

C:\go\src\github.com\minio\mc> mc rm C:\
Removing `C:\`.
mc.exe: <ERROR> Failed to remove `C:\`. Insufficient permissions to access this file `C:\`
C:\go\src\github.com\minio\mc>
C:\go\src\github.com\minio\mc> mc rm C:
Removing `C:`.
mc.exe: <ERROR> Failed to remove `C:`. remove C:: The process cannot access the file because it is being used by a
ther process.
C:\go\src\github.com\minio\mc>

C:\go\src\github.com\minio\mc> mc rm --r --force  C:
Removing `C:cmd\mc_test.go`.

~> mc mb a/b/c/d
~> mc rm a
Removing `a`.
mc.exe: <ERROR> Failed to remove `a`. remove a: The directory is not empty.
~> mc rm \\TMP\ss\
mc.exe: <ERROR> Failed to remove `\\TMP\ss\`. GetFileAttributesEx \\TMP\ss: The network name cannot be
~> mc rm C:\Users\vagrant\a\\\
Removing `C:\Users\vagrant\a\\\`.
mc.exe: <ERROR> Failed to remove `C:\Users\vagrant\a\\\`. remove C:\Users\vagrant\a\\\: The directory
~> mc rm C:\Users\vagrant\a\\\b\\\c\\\d
Removing `C:\Users\vagrant\a\\\b\\\c\\\d`.
~>

Copy link
Member

Choose a reason for hiding this comment

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

We used path.Clean() to take care aliases like s3 and s3/. Hope return path from url2Alias() is clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, added a comment for that as well.

cmd/rm-main.go Outdated
isNamespaceRemoval := false
for _, url := range ctx.Args() {
// clean path for aliases like s3/
url = filepath.ToSlash(filepath.Clean(url))
Copy link
Member

@balamurugana balamurugana Nov 29, 2017

Choose a reason for hiding this comment

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

interestingly UNC path using / is working in go1.9.2 by breaking the UNC path specification. I would recommend to add this comment as you convert any path separator to / here.

Copy link
Member

Choose a reason for hiding this comment

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

It is not breaking @balamurugana they automatically convert it internally properly. Before any Win32 calls are made os package converts them to necessary values.

FWIW Win32 calls support / path separators as well.

Copy link
Member

Choose a reason for hiding this comment

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

Check on windows how it behaves UNC path with /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C:\GO\src\github.com\minio\mc [(0f4e134...) +0 ~1 -0 !]> mc mb a/b/c/d/e
Bucket created successfully `a/b/c/d/e`.
C:\GO\src\github.com\minio\mc [(0f4e134...) +0 ~1 -0 !]> tree a
Folder PATH listing for volume Windows 10
Volume serial number is 00000200 4286:2744
C:\GO\SRC\GITHUB.COM\MINIO\MC\A
└───b
    └───c
        └───d
            └───e
C:\GO\src\github.com\minio\mc [(0f4e134...) +0 ~1 -0 !]> mc rm a/b/c/d/e
Removing `a/b/c/d/e`.
C:\GO\src\github.com\minio\mc [(0f4e134...) +0 ~1 -0 !]>
C:\GO\src\github.com\minio\mc [(0f4e134...) +0 ~1 -0 !]> mc rm //network1/blah/blah
mc.exe: <ERROR> Failed to remove `//network1/blah/blah`. Requested file `/blah/blah` not found
C:\GO\src\github.com\minio\mc [(0f4e134...) +0 ~1 -0 !]> mc rm --r --force //network1/blah/blah
mc.exe: <ERROR> Failed to remove `//network1/blah/blah` recursively. open /blah/blah: The system canno
h specified.

Copy link
Member

Choose a reason for hiding this comment

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

No surprise go applications are working cleanly. See my above comment. Just add a comment what exactly i mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@harshavardhana
Copy link
Member

Moving this to blocked as requested by @poornas until @abperiasamy gives final review on the changes and UI.

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

LGTM

This requires --dangerous flag in addition to --recursive and --force

Change namespace identification check to be more robust to user
input errors like mc rm --r --force play//
@poornas
Copy link
Contributor Author

poornas commented Dec 2, 2017

after discussion with @abperiasamy , changed messaging for the --force and --dangerous flags and error message if flags are not supplied for site wide rm command. This is unblocked now

@harshavardhana harshavardhana merged commit 1e7b4c9 into minio:master Dec 2, 2017
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.

4 participants