Improving circular dependency checking for new-build#10067
Improving circular dependency checking for new-build#10067openshift-bot merged 1 commit intoopenshift:masterfrom jupierce:dev/jupierce/bug/7453
Conversation
|
So does this handle: ? that seems like the most likely error path a user would hit. or a variant of that in which even without the --to, the generated Output value ends up matching the image referenced in the source repo's dockerfile. |
|
That will issue a warning, yes. My examples are more contrived since I wanted them to have the potential for success. --to=centos/ruby-22-centos7 will always fail since the user will lack the credentials to push to docker.io. One important note: all of these examples generate a warning, but otherwise proceed. I preserved the existing logic that turns cycle detection into a warning if the user specifies --to. |
sure, i didn't feel like creating an example with actual repo a user might have access to, but the concept is the same. (and there are people w/ those credentials) |
|
Understood. In either case, it should be caught. It is a similar codepath to the first bullet in the PR -- just with a defaulting builder image. |
pkg/generate/app/cmd/newapp.go
Outdated
| // Some code paths add namespace and others don't. Make things | ||
| // consistent. | ||
| if len(ref.Namespace) == 0 { | ||
| ref.Namespace = ns |
There was a problem hiding this comment.
ns is going to be the input namespace here, right? if we're looking at the output image, we should be setting the ns to the origin(current/chosen) namespace, not the input namespace.
There was a problem hiding this comment.
Yes. I just couldn't come up with a scenario in which they would differ and got too aggressive in trimming the code :-) . Next commit removes this assumption.
|
I'm missing tests to accompany the code changes. |
|
@rhcarvalho Tests cases underway. Just wanted to know if the approach seemed like it was on the right track. |
i think so. |
|
Just an update.. Adding test cases is conceptually simple, but a magic flag used by newapp is taking time to figure out. If it is not set, the client does not populate the Tags element of ImageStream resources (apparently for backwards compatibility with older servers). In the existing test cases, ImageStream objects are being created without Tags, but the new checkCircularReferences requires them. I will be adding backwards compatibility to checkCircularReferences to support tagless ImageStreams, but I would like the test case to exercise the newer ImageStream Spec. https://github.com/openshift/origin/blob/master/pkg/generate/app/imagestreamlookup.go#L201-L203 |
|
[test] |
|
re[test] |
1 similar comment
|
re[test] |
|
@bparees Just to get back on your radar. |
test/integration/newapp_test.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| want := "--> WARNING: output image of \"default/centos/ruby-22-centos7:latest\" must be different than input\n" |
There was a problem hiding this comment.
can we change this actual warning message:
s/must.*/should be different from the input to avoid circular build triggering\n/
(since it's a warning, not an error, it doesn't make sense to say "must")
|
@bparees @rhcarvalho Added support for IST->IST references. Ready for another round of review. |
| isNS = c.OriginNamespace | ||
| } | ||
|
|
||
| // Otherwise, we are tracing an IST reference |
There was a problem hiding this comment.
is there a test for this scenario? it wasn't obvious to me when i scanned the unit tests.
There was a problem hiding this comment.
To be clear, this line a verbose guard to let someone know that they are misusing the current method's implementation. e.g. If a new type of To/From ObjectReference is encountered, this path will annoy folks and remind them the cycle check logic needs to be updated -- but do so without breaking newapp.
In short, I don't believe newapp's current implementation can actually trigger the error. I will add a test case to cover the code, but it will need to invoke checkCircularReferences with contrived data.
There was a problem hiding this comment.
hm? i'm referring to the scenario where someone uses an imagestreamtag as an input but that imagestreamtag is a "following" imagestreamtag, rather than one that directly points to a docker pull spec. that can happen, right? (unless we have resolved the indirect reference earlier in new-app code)
There was a problem hiding this comment.
I mistakenly thought you were talking about the if ref.Kind != "ImageStreamTag" . I will add a test for the IST->IST.
|
pkg/generate/app/cmd/newapp.go
Outdated
| } | ||
|
|
||
| // followRefToDockerImage follows a buildconfig...To/From reference until it t | ||
| // erminates in docker image information. This can include dereferencing chains |
There was a problem hiding this comment.
looks like "terminates" was broken into the 2 lines
There was a problem hiding this comment.
erminate (verb) - meaning to improperly line break comments in code.
fixed :)
|
@bparees TestBuildOutputCycleWithFollowingTag now checks for following tag support. |
|
lgtm [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7808/) (Image: devenv-rhel7_4806) |
|
Evaluated for origin merge up to 76800c3 |
|
Evaluated for origin test up to 76800c3 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7808/) |
|
@oatmealraisin Over to you! |
|
@jupierce merci! |
6 participants
Fixes #7453
Problematic invocations I found:
oc new-build --docker-image=jupierce/ruby-22-centos7 --to-docker=true --to=jupierce/ruby-22-centos7 https://github.com/openshift/ruby-hello-worldoc new-build --docker-image=jupierce/ruby-22-centos7:latest --to-docker=true --to=jupierce/ruby-22-centos7 https://github.com/openshift/ruby-hello-worldoc new-build -D $'FROM jupierce/ruby-22-centos7' --to ruby-22-centos7oc new-build -D $'FROM jupierce/ruby-22-centos7:latest' --to ruby-22-centos7fwiw, I found a way to sneak past the existing check without --to-docker=true with a sequence like:
oc new-build https://github.com/openshift/ruby-hello-worldoc new-build --image-stream=ruby-hello-world --to=ruby-hello-world https://github.com/openshift/ruby-hello-world --name=cycleIt introduced a namespace attribute which caused the DeepEqual to fail. This PR accounts for this.
This will not fix docker image pushes to the local registry from causing a circular build process. For example:
oc new-build https://github.com/openshift/ruby-hello-world --to-docker=true --to=172.30.96.226:5000/myproject/ruby-22-centos7(Where --to is pointing to the openshift docker registry/authorized project)
Adding additional cycle inducing commands that involve IST to IST references (this requires oc create of centos7 image streams and assumes latest points to 10.0):
oc new-build -D "FROM openshift/wildfly:latest" --to-docker=true --to openshift/wildfly-100-centos7