Some features improvement #11

x-lhan · 2017-11-02T18:03:52Z

What does this PR do?

Provides support for registries that have basic/token auth mode enabled(Unable to handle authentication #1)
Provides a flag to ignore SSL verification errors(Disable SSL validation #2)
Fixes a potential problem that may lead to unnecessary tag removal(Deleting a tag also deletes all other images with this image ID. #9)
This only happens when duplicate tags share the same hash. If one should be removed but the other should remain, both will be deleted.
Some parameter adjustments around repo filter(Please see the comparison between previous and new behavior section below)
Add dockerize support for a docker image with a small footage(~7MB)

What issues does this PR fix or reference?

Previous Behavior

Parameters -repo and -tag were used for filtering repo/tag by regexp
Parameter -repos was used as the maximum repo count to fetch

New Behavior

Parameter changes include:
-username string
username for docker login
-password string
password for docker login
-insecure
ignore https verification error
-repos
matching repositories by name (allows multiple value separates by ,); If specifies will ignore -repo_regexp
-repo_regexp string
matching repositories (allows regexp) (default ".")
-max_repos int
max number of repositories to garbage collect (default to no limit)
-tag_regexp string
matching tags (allows regexp) (default ".")

Add back maximum repo count as parameter

yan-foto · 2017-11-06T14:53:06Z

Wow! this is a huge PR. Thanks for the effort. Please give me some time to review it.

yan-foto

I think from this commit, the only thing that we should keep are the insecure, user, and password flags. However, we should find a better way to read passwords and avoid plain texts in the command line (readable through history)

yan-foto · 2017-11-08T12:26:55Z

main.go

 	log "github.com/Sirupsen/logrus"
 	"github.com/docker/distribution/context"
 	schema2 "github.com/docker/distribution/manifest/schema2"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/distribution/registry/client"
+	"github.com/heroku/docker-registry-client/registry"


I'm not sure why we need this!?

yan-foto · 2017-11-08T12:28:14Z

main.go

+	// registry username (default = "")
+	username = flag.String("username", "", "registry username to login")
+	// registry password (default = "")
+	password = flag.String("password", "", "registry password to login")


I'm not fond of Plaintext passwords as args, we should do this in an extra step (just as mysql -u root -p does)

yan-foto · 2017-11-08T12:30:25Z

main.go

 	// Create registry object
-	r, err := client.NewRegistry(*registryURL, nil)
+	r, err := client.NewRegistry(ctx, *registryURL, wrapTransport)


Context is not used anymore in Docker API. See this: 9931cc7

yan-foto · 2017-11-08T12:32:31Z

main.go

@@ -125,7 +154,7 @@ func main() {
 			logger.Fatalf("Could not parse repo from name! (err: %v)", err)
 		}

-		repo, err := client.NewRepository(repoName, *registryURL, nil)
+		repo, err := client.NewRepository(ctx, repoName, *registryURL, wrapTransport)


Same as above. ctx is not required.

yan-foto · 2017-11-08T12:38:39Z

main.go

 	// Maximum age of iamges to consider for deletion in days (default = 0)
 	day = flag.Int("day", 0, "max age in days")
 	// Maximum age of months to consider for deletion in days (default = 0)
 	month = flag.Int("month", 0, "max age in months")
 	// Maximum age of iamges to consider for deletion in years (default = 0)
 	year = flag.Int("year", 0, "max age in days")
 	// Regexp for images (default = .*)
-	repoRegexp = flag.String("repo", ".*", "matching repositories (allows regexp)")
+	repoRegexp = flag.String("repo", ".*", "matching repositories (allows mulitple value seperates by ,)")


This is redundant as it's equal to '|' in a regular expression and IMHO does not provide any added value. 🙃

yan-foto · 2017-11-08T12:41:13Z

main.go

 	if err != nil {
 		log.Fatalf("Could not create registry object! (err: %s", err)
 	}

+	fetchAll := strings.Compare(*repoRegexp, ".*") == 0


This is the side-effect when we use 2 different parameters for the same goal.

yan-foto

Could you please elaborate on this bug? I'm not following what exactly is happening.

yan-foto

Have you tested this? My favorite base image is alpine. Moreover, this guide is a generic way of generating a Docker image. I would remove it from the README and put it at best in the wiki

yan-foto

For such changes please use git rebase

yan-foto

As commented in the first commit of this PR, we should stick to regex and not complicate the whole procedure.

yan-foto · 2017-11-08T13:01:54Z

So I reviewed the first commits and the rest seems to be fixing typos and what seems to be reverting some changes. The following points are what to be done and what will be merged:

Add insecure flag
Read username in plaintext and password in next step hidden (similar to mysql -u username -p)

The following is going to be removed:

New repo_regexp (see first review comments)
Fix wrt. Deleting a tag also deletes all other images with this image ID. #9

I appreciate the PR. 👍

x-lhan · 2017-11-08T19:07:30Z

Thank you for your throughly review. Here are some illustrations regarding to your concerns:

Plaintext password
I agree with you. So I will try to add a parameter which promotes user to enter password. But I also think plaintext password parameter should be reserved for user who does not think this is an issue and will appreciate the non-interrupt configure(e.g. run as docker container and manage password from configuration system)
Tag remove bug Deleting a tag also deletes all other images with this image ID. #9
This happens if two or more tags have the same blob backed:
if an image is tagged 3 times(no changes in between) with different names(e.g. latest, stable and snapshot-1) and pushed to the registry, running deckscrubber cmd (with the default latest of 1) instead of the latest tag remain all 3 tags will be removed.
Repo Regex modification
This modification is mainly not enforcing user to query all repo. This is required for users who do not authorized to do so. (With token-based auth mode, users can be assigned to only access to limited repos.)
Dockerize
Yes I have tested without any problem. Sure I can move this section into wiki.

Let me know if you have any different opinions.

yan-foto · 2017-11-09T12:45:11Z

It makes sense! Thanks. Please try to avoid commits that fix previous commits and just rebase if you think something needs to be changed. Otherwise the review procedure would be really confusing 😅

So I think we agree on all points 😄 !

x-lhan · 2017-11-09T20:10:09Z

Thank you for the tips. 👍
Just update the code according to our discussion. (Not sure how to submit a PR for wiki but I removed from README)

yan-foto · 2017-11-10T10:46:44Z

README.md

@@ -33,6 +33,8 @@ We run our own private registry on a server with limited storage and it was only
      username for docker login
 -password string
      password for docker login
+-promote
+      promote user to enter docker login password; If specified will ignore `-password`


I'd leave this out and see if any value is given after -password or not. If not, prompt user.

yan-foto · 2017-11-10T10:47:44Z

main.go

+
+	if *enterPwd {
+		fmt.Println("Enter registry login password:")
+		fmt.Scanln(password)


This is not the proper way to read passwords in terminal. See here

yan-foto

Please see comments.

2. Upgraded to support latest docker distribution API 3. Added support for native module 4. Fix an import package error owing renaming

x-lhan added 8 commits October 24, 2017 21:24

📝 Add token/basic auth

bc790f8

📝 fix a bug that will delete unnecssry tag when have the same digest

c897017

📝 Add dockerize doc

a94d8cd

📝 fix some typo in the previous commit

306b5d1

add repo regexp match back

00ec833

📝 make dockerize more easy

b57358a

Add back maximum repo count as parameter

📝 revert doc to avoid conflict

c0fa90c

🐛 Fix a outOfBound error of latest tags

98546e7

yan-foto reviewed Nov 8, 2017

View reviewed changes

yan-foto requested changes Nov 8, 2017

View reviewed changes

yan-foto reviewed Nov 8, 2017

View reviewed changes

yan-foto requested changes Nov 8, 2017

View reviewed changes

x-lhan added 2 commits November 9, 2017 11:50

📝 Added Parameter for user to enter password

bb8899f

Move dockerize doc from readme to wiki

770ba4a

yan-foto reviewed Nov 10, 2017

View reviewed changes

yan-foto requested changes Nov 10, 2017

View reviewed changes

yan-foto mentioned this pull request Dec 6, 2017

how to input username/password for private registry ? #13

Closed

x-lhan added 2 commits March 1, 2019 12:55

Improve debug log output for error

e352cb7

1. Added support for schema1 manifest

33d0bab

2. Upgraded to support latest docker distribution API 3. Added support for native module 4. Fix an import package error owing renaming

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some features improvement #11

Some features improvement #11

x-lhan commented Nov 2, 2017 •

edited

Loading

yan-foto commented Nov 6, 2017

yan-foto left a comment

yan-foto Nov 8, 2017

yan-foto Nov 8, 2017

yan-foto Nov 8, 2017

yan-foto Nov 8, 2017

yan-foto Nov 8, 2017

yan-foto Nov 8, 2017

yan-foto left a comment

yan-foto left a comment

yan-foto left a comment

yan-foto left a comment

yan-foto commented Nov 8, 2017

x-lhan commented Nov 8, 2017

yan-foto commented Nov 9, 2017 •

edited

Loading

x-lhan commented Nov 9, 2017

yan-foto Nov 10, 2017

yan-foto Nov 10, 2017

yan-foto left a comment

Some features improvement #11

Are you sure you want to change the base?

Some features improvement #11

Conversation

x-lhan commented Nov 2, 2017 • edited Loading

yan-foto commented Nov 6, 2017

yan-foto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yan-foto left a comment

Choose a reason for hiding this comment

yan-foto left a comment

Choose a reason for hiding this comment

yan-foto left a comment

Choose a reason for hiding this comment

yan-foto left a comment

Choose a reason for hiding this comment

yan-foto commented Nov 8, 2017

x-lhan commented Nov 8, 2017

yan-foto commented Nov 9, 2017 • edited Loading

x-lhan commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yan-foto left a comment

Choose a reason for hiding this comment

x-lhan commented Nov 2, 2017 •

edited

Loading

yan-foto commented Nov 9, 2017 •

edited

Loading