Bug 1773257: pkg/cli/admin/release/extract_tools: Inject release version into installer

[wking]

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, #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, #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, #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 4.3.0-0.nightly-2019-11-13-233341:

$ 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

CC @sallyom

[wking]

/assign @soltysh

[wking]

e2e-aws:

fail [github.com/openshift/origin/test/extended/builds/new_app.go:56]: Unexpected error:
    <*errors.errorString | 0xc0033f1400>: {
        s: "The build \"a234567890123456789012345678901234567890123456789012345678-1\" status is \"Failed\"",
    }
    The build "a234567890123456789012345678901234567890123456789012345678-1" status is "Failed"
occurred
...
failed: (1m15s) 2019-11-16T17:16:11 "[Feature:Builds][Conformance] oc new-app  should succeed with a --name of 58 characters [Suite:openshift/conformance/parallel/minimal]"

/retest

[openshift-ci-robot]

@wking: This pull request references Bugzilla bug 1773257, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1773257: pkg/cli/admin/release/extract_tools: Inject release version into installer

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.

[sallyom]

@wking, awesome refactor and cleanup, thank you : )
Do you see any compatibility issues? I've noticed this:

• oc-built-fr-this-pr adm extract --command openshift-install <today's current release image> fails, w/ Unable to make all expected replacements in openshift-install. Remaining: release version
  that will be ok?

afaict only way to get past the hump w/ this is a release image w/ both installer + this PR baked in. That leaves a backward compatibility issue w/ extracting openshift-install

when I build a release w/ this PR installer + this PR oc, I can extract successfully and see:

$ ./openshift-install version
./openshift-install 4.3.0-0.ci-2019-11-16-153916
built from commit e24708d81be4c161afd952b2308127dfabc1f097
release image quay.io/sallyom/release@sha256:e47b9cd6918aef880eff07e0e16b3af7bc49e72a03ae798887bf6d0b47493c8a

[wking]

Unable to make all expected replacements in openshift-install. Remaining: release version

Expected until openshift/installer#2682, and harmless noise until then.

...backward compatibility issue...

More on this in the Because 13215d7, f55426f, and ded896d all touched the version marker... paragraph of my initial post here. I think we're fine as long as we settle down before we cut a 4.3 RC.

[sallyom]

Expected until openshift/installer#2682, and harmless noise until then.

If it's harmless, make a note of it? Or, drop from Warning to Info, and level >1?

[wking]

If it's harmless, make a note of it? Or, drop from Warning to Info, and level >1?

If you want to avoid, wait for the installer PR to land first? I don't think we need two oc PRs to ratchet this in.

[wking]

Rebased with db7b417 -> d1b006a now that #167 has landed.

[soltysh]

/lgtm
/approve
/retest

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[sdodson]

/cherrypick release-4.3

[openshift-cherrypick-robot]

@sdodson: new pull request created: #249

Details

In response to this:

/cherrypick release-4.3

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.