Skip to content

fixes registry logic for case:#144

Open
rdallman wants to merge 3 commits intomasterfrom
fix-registry-logic
Open

fixes registry logic for case:#144
rdallman wants to merge 3 commits intomasterfrom
fix-registry-logic

Conversation

@rdallman
Copy link
Copy Markdown
Contributor

@rdallman rdallman commented Jan 19, 2018

previously, we if the following value for FN_REGISTRY was provided, we ignored
it if the image contained a slash, see example:

(func.yaml has image: rdallman/yodawg)

$ FN_REGISTRY=localhost:5000/ cli deploy --app myapp
Deploying rdallman/yodawg to app: myapp at path: /hello
Bumped to version 0.0.7
Building image rdallman/yodawg:0.0.7 ...
Pushing rdallman/yodawg:0.0.7 to docker registry...The push refers to a repository [docker.io/rdallman/yodawg]

with this patch, it's fixed:

$ FN_REGISTRY=localhost:5000/ cli deploy --app myapp
Deploying rdallman/yodawg to app: myapp at path: /hello
Bumped to version 0.0.11
Building image localhost:5000/rdallman/yodawg:0.0.11 .
Pushing localhost:5000/rdallman/yodawg:0.0.11 to docker registry...The push refers to a repository [localhost:5000/rdallman/yodawg]

this does mean that anyone relying on the logic of their image having a slash
in it not using an FN_REGISTRY they provided (seems odd) will be broken, the
code kind of implies this was the case but maybe not. anyway, this behavior
seems like what we want, i.e. 'if a REGISTRY_URL is provided, prepend it to
everything unless it's a fully qualified image that specifies a registry URL
itself'. to enumerate:

FN_REGISTRY!="" && image=="yodawg" -> image = FN_REGISTRY || image
FN_REGISTRY!="" && image=="yo/dawg" -> image = FN_REGISTRY || image
FN_REGISTRY!="" && image=="localhost:5000/yo/dawg" -> image = image

of course, if FN_REGISTRY is not provided, the image is not modified. shame
this thing exists :(

previously, we if the following value for FN_REGISTRY was provided, we ignored
it if the image contained a slash, see example:

(`func.yaml` has `image: rdallman/yodawg`)

``sh
$ FN_REGISTRY=localhost:5000/ cli deploy --app myapp
Deploying rdallman/yodawg to app: myapp at path: /hello
Bumped to version 0.0.7
Building image rdallman/yodawg:0.0.7 ...
Pushing rdallman/yodawg:0.0.7 to docker registry...The push refers to a repository [docker.io/rdallman/yodawg]
```

with this patch, it's fixed:

```sh
$ FN_REGISTRY=localhost:5000/ cli deploy --app myapp
Deploying rdallman/yodawg to app: myapp at path: /hello
Bumped to version 0.0.11
Building image localhost:5000/rdallman/yodawg:0.0.11 .
Pushing localhost:5000/rdallman/yodawg:0.0.11 to docker registry...The push refers to a repository [localhost:5000/rdallman/yodawg]
```

this does mean that anyone relying on the logic of their image having a slash
in it not using an FN_REGISTRY they provided (seems odd) will be broken, the
code kind of implies this was the case but maybe not. anyway, this behavior
seems like what we want, i.e. 'if a REGISTRY_URL is provided, prepend it to
everything unless it's a fully qualified image that specifies a registry URL
itself'. to enumerate:

FN_REGISTRY!="" && image=="yodawg"                  -> image = FN_REGISTRY || image
FN_REGISTRY!="" && image=="yo/dawg"                 -> image = FN_REGISTRY || image
FN_REGISTRY!="" && image=="localhost:5000/yo/dawg"  -> image = image

of course, if FN_REGISTRY is not provided, the image is not modified. shame
this thing exists :(
@rdallman
Copy link
Copy Markdown
Contributor Author

closes fnproject/fn#699 -- the image was never pushed to the registry. i can confirm that using this version of the cli, an image provided with a registry ends up in there, and fn server also pulls it and runs it correctly.

@rdallman
Copy link
Copy Markdown
Contributor Author

rdallman commented Jan 19, 2018

meh, this FN_REGISTRY idea is dumb.

a hostname (without a port) and a user name will both look the same, i'm not sure it's possible to move forward...

FN_REGISTRY is a host and a host or username, prepend
FN_REGISTRY is a user name and image is a single word (no slash), prepend
FN_REGISTRY is a user name and image has username (slash), don't prepend

since host and user name can look the same... there's no identifiable way around this. can we toss out the user name concept please? a user is not a registry. I understand the thinking around it. FN_REGISTRY_USERNAME makes this all make a whole lot more sense.

Comment thread funcfile.go

// prefix FN_REGISTRY, if provided (important) AND if the image does not
// itself contain a registry.
if len(strings.SplitN(fname, "/", 3)) < 3 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

localhost:5000/mygroup/myimage would match this though?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this one works https://play.golang.org/p/eY2LSGhSDr7 but i think ultimately this logic is flawed trying to have FN_REGISTRY either be a username, a registry, or a registry with username. it seems like the proper fix is making FN_REGISTRY be a registry, and having an additional FN_REGISTRY_USERNAME for cli convenience shenanigans, maybe something slightly easier there but I'm not sure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah I'm all for moving to more explicit values for those vars. 👍

@treeder
Copy link
Copy Markdown
Contributor

treeder commented Feb 12, 2018

I think making FN_REGISTRY fully qualified makes sense, eg: FN_REGISTRY=docker.io/NAMESPACE .

Would that be enough to resolve all of this? Docker refers to it as namespace since it can be an org too, not just a username.

@rdallman
Copy link
Copy Markdown
Contributor Author

I think making FN_REGISTRY fully qualified makes sense, eg: FN_REGISTRY=docker.io/NAMESPACE .

Would that be enough to resolve all of this? Docker refers to it as namespace since it can be an org too, not just a username.

good idea, that might work. the only confusing case then is if the function 'name' (image) is NAMESPACE/image and a user provides my.registry.io/path/to/NAMESPACE we have to blow up, since if FN_REGISTRY is required to be fully qualified a user can't simply use FN_REGISTRY=my.registry.io. allowing / suggesting FN_REGISTRY to contain NAMESPACE is mostly the source of issues, but I'm not sure there's any getting around it completely.

@treeder
Copy link
Copy Markdown
Contributor

treeder commented Feb 12, 2018

What if we don't allow NAMESPACE in the function names (shouldn't really be there anyways)?

@rdallman
Copy link
Copy Markdown
Contributor Author

I don't really like the whole FN_REGISTRY experience to begin with, asking the wrong dude. ya got the wrong lebowski

@rdallman
Copy link
Copy Markdown
Contributor Author

it's already relatively confusing that function name becomes an image name, i'm not sure that makes that any better. the other point would be it's a total pain if you're on a team with a shared repo you'd have to set FN_REGISTRY in some build script or your env to toggle between various things. in a team that uses different orgs/repo names to box stuff like dev/hello stage/hello in a shared registry, this becomes a real pain since that can no longer be encoded into the function/image name. it makes sense to me to allow a full my.registry.io/my/docker as an image name, i maintain it's confusing this is a function name esp in that context. personally i prefer not using FN_REGISTRY at all since i have various functions that go to fnproject or to rdallman on dhub. would be nice to resolve all this for sure..

@rikgibson
Copy link
Copy Markdown
Contributor

@rdallman seems this should be closed off - any objections?

@rdallman
Copy link
Copy Markdown
Contributor Author

afaik this still doesn't work, there's an ambiguity to resolve around FN_REGISTRY's definition, per #144 (comment) -- maybe it's fixed now?

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