Bug 1763728: fix 'oc adm release extract' version injection #131
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@sallyom: This pull request references Bugzilla bug 1763728, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/valid-bug
sallyom
commented
Oct 22, 2019
| @@ -492,17 +492,16 @@ func (o *ExtractOptions) extractCommand(command string) error { | |||
|
|
|||
| // copy the input to disk | |||
| if target.InjectReleaseImage { | |||
Contributor
There was a problem hiding this comment.
Why are we using release image and release image name under the same flag? They are unrelated - oc and Openshift-install should be using this flag for replace, rather than checking on binary name.
Contributor
Author
There was a problem hiding this comment.
good point, thanks! reworking
Contributor
Author
There was a problem hiding this comment.
pushed an update, added InjectReleaseVersion bool
smarterclayton
suggested changes
Oct 22, 2019
Contributor
smarterclayton
left a comment
smarterclayton
left a comment
There was a problem hiding this comment.
I'd like some structural changes to separate these out, if you're using a flag on the binary to denote what to replace they should be split by version or image, and you should only replace when denoted.
Contributor
Author
thanks for taking a look, will do! I'll back up and modify the original change to be in line w/ your suggestions, that will fix the bug ; ) |
sallyom
force-pushed
the
fix-version-inject
branch
2 times, most recently
from
4014c6f to
e621670
Compare
openshift-ci-robot
added
size/L
sallyom
force-pushed
the
fix-version-inject
branch
3 times, most recently
from
f2167e5 to
5b6f08e
Compare
sallyom
commented
Oct 23, 2019
| Command string | ||
| Optional bool | ||
|
|
||
| TargetName string |
Contributor
Author
There was a problem hiding this comment.
This isn't used anywhere.
Contributor
Author
|
added image built from this PR to latest published release, then used oc built from this pr to extract oc from that release, then used the extracted oc to extract openshift-install: $ oc adm release extract --command oc quay.io/sallyom/release:test
$ ./oc version
Client Version: 4.3.0-0.ci-2019-10-23-103858
Server Version: 4.3.0-0.ci-2019-10-23-061559
Kubernetes Version: v1.16.0-beta.2+3012553
$ ./oc adm release extract --command openshift-install quay.io/sallyom/release:test
$ ./openshift-install version
./openshift-install unreleased-master-2003-g42a87f05c2e1a42ad815ec8f37b839fd5952cbfc-dirty
built from commit 42a87f05c2e1a42ad815ec8f37b839fd5952cbfc
release image quay.io/sallyom/release@sha256:6ef6426b9181001bee5bc6d693a8205e0bcff55ef4469b99ba7ada66cb7c8811 |
sallyom
force-pushed
the
fix-version-inject
branch
4 times, most recently
from
e10e379 to
4247b0d
Compare
sallyom
force-pushed
the
fix-version-inject
branch
from
4247b0d to
f55426f
Compare
sallyom
commented
Oct 23, 2019
| // the binary has been altered, but the replaced release name is empty which is incorrect | ||
| return version.Info{}, "", fmt.Errorf("release name location is empty, this binary was incorrectly generated") | ||
| // the oc binary will not be pinned to Release Metadata:Version | ||
| return version.Info{}, "", fmt.Errorf("oc version was incorrectly replaced during extract") |
Contributor
Author
There was a problem hiding this comment.
The oc binary doesn't depend on the version being pinned to the release, but if we're meddling w/ the binary and the result isn't what is expected, it should return an error? oc works the same either way, though, so I could see returning a warning for these, instead. like:
klog.Warningf("oc version was incorrectly replaced during extract")
return Get(), "", nil
Contributor
Author
There was a problem hiding this comment.
I can't figure this out, when I extract oc from the release image from this PR, I get an empty releaseName (match found, but len(releaseName)==0. However, when I create a release from a published release w/ new cli image (built from this PR, but locally), and extract oc from that, releaseName is the expected 4.3.0-0.ci-2019-10-23-blah - maybe the ci-op/release:latest image is in flux w/ multiple pushes/test restarts so the Release Metadata: Version is absent? not sure.
Contributor
There was a problem hiding this comment.
👍 for the warning and printing the built-in version.
Contributor
There was a problem hiding this comment.
For the 2nd, let's land this and we'll check you still have a few followups to fix. Side note, have you checked with hexeditor that binary extracted from the release image build from this PR?
Contributor
Author
|
/test images |
soltysh
approved these changes
Oct 24, 2019
| errmsg = fmt.Sprintf("warning: Unable to inject release image info into %s, could not determine version", target.Command) | ||
| default: | ||
| errmsg = fmt.Sprintf("warning: Unable to inject release image info into %s, installer will not be locked to the correct image", target.Command) | ||
| matched, err = copyAndReplaceReleaseInfo(w, r, 4*1024, exactReleaseImage) |
Contributor
There was a problem hiding this comment.
copyAndReplaceReleaseImage
| // release image info string | ||
| binaryReplacement = "\x00_RELEASE_IMAGE_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00" | ||
| // installerReplacement is the location within the installer binary that we can insert our release image info string | ||
| installerReplacement = "\x00_RELEASE_IMAGE_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00" |
| // the binary has been altered, but the replaced release name is empty which is incorrect | ||
| return version.Info{}, "", fmt.Errorf("release name location is empty, this binary was incorrectly generated") | ||
| // the oc binary will not be pinned to Release Metadata:Version | ||
| return version.Info{}, "", fmt.Errorf("oc version was incorrectly replaced during extract") |
Contributor
There was a problem hiding this comment.
👍 for the warning and printing the built-in version.
| // the binary has been altered, but the replaced release name is empty which is incorrect | ||
| return version.Info{}, "", fmt.Errorf("release name location is empty, this binary was incorrectly generated") | ||
| // the oc binary will not be pinned to Release Metadata:Version | ||
| return version.Info{}, "", fmt.Errorf("oc version was incorrectly replaced during extract") |
Contributor
There was a problem hiding this comment.
For the 2nd, let's land this and we'll check you still have a few followups to fix. Side note, have you checked with hexeditor that binary extracted from the release image build from this PR?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sallyom, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@sallyom: All pull requests linked via external trackers have merged. Bugzilla bug 1763728 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 16, 2019
…aller We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88), but we want to inject it into the installer to to avoid potential confusion from: $ oc adm release extract --command=openshift-install quay.io/openshift-release-dev/ocp-release:4.2.7 $ ./openshift-install version ./openshift-install v4.2.5 built from commit 425e4ff0037487e32571258640b39f56d5ee5572 release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9 This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [1]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 16, 2019
…aller We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88), but we want to inject it into the installer to to avoid potential confusion from: $ oc adm release extract --command=openshift-install quay.io/openshift-release-dev/ocp-release:4.2.7 $ ./openshift-install version ./openshift-install v4.2.5 built from commit 425e4ff0037487e32571258640b39f56d5ee5572 release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9 This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [1]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 16, 2019
…aller We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88), but we want to inject it into the installer to to avoid potential confusion from: $ oc adm release extract --command=openshift-install quay.io/openshift-release-dev/ocp-release:4.2.7 $ ./openshift-install version ./openshift-install v4.2.5 built from commit 425e4ff0037487e32571258640b39f56d5ee5572 release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9 This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [1]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 16, 2019
…aller We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88), but we want to inject it into the installer to to avoid potential confusion from: $ oc adm release extract --command=openshift-install quay.io/openshift-release-dev/ocp-release:4.2.7 $ ./openshift-install version ./openshift-install v4.2.5 built from commit 425e4ff0037487e32571258640b39f56d5ee5572 release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9 This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [1]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 19, 2019
…lace We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [2]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 19, 2019
…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [2]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 23, 2019
…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. The Fprintf warning follows package precedent. Clayton [2]: > In these commands warnings are printed with fprintf. > > Klog is for debugging and servers, not end users. In action extracting from [3]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: openshift#167 (comment) [3]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
wking
added a commit
to wking/oc
that referenced
this pull request
Nov 23, 2019
…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoided that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinguish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amount of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. The Fprintf warning follows package precedent. Clayton [2]: > In these commands warnings are printed with fprintf. > > Klog is for debugging and servers, not end users. In action extracting from [3]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: openshift#167 (comment) [3]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
openshift-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/oc
that referenced
this pull request
Dec 3, 2019
…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoided that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinguish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amount of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. The Fprintf warning follows package precedent. Clayton [2]: > In these commands warnings are printed with fprintf. > > Klog is for debugging and servers, not end users. In action extracting from [3]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: openshift#167 (comment) [3]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since this merged, when using an extracted oc to extract openshift-install|oc, command fails, because the shared binary replacement location. This PR is the fix.
/cc @soltysh