Skip to content

: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Jul 8, 2024
Merged

: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 51 additions & 33 deletions pkg/cli/admin/release/extract_tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@ type extractTarget struct {
Optional bool
NewArch bool

InjectReleaseImage bool
InjectReleaseVersion bool
SignMachOBinary bool
InjectReleaseImage bool
InjectReleaseVersion bool
InjectReleaseArchitecture bool
SignMachOBinary bool

ArchiveFormat string
AsArchive bool
Expand Down Expand Up @@ -409,10 +410,11 @@ func (o *ExtractOptions) extractCommand(command string) error {
Command: "openshift-install",
Mapping: extract.Mapping{Image: "installer-artifacts", From: "usr/share/openshift/mac/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
ArchiveFormat: "openshift-install-mac-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
ArchiveFormat: "openshift-install-mac-%s.tar.gz",
},
{
OS: "darwin",
Expand All @@ -421,22 +423,24 @@ func (o *ExtractOptions) extractCommand(command string) error {
NewArch: true,
Mapping: extract.Mapping{Image: "installer-artifacts", From: "usr/share/openshift/mac_arm64/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
SignMachOBinary: true,
ArchiveFormat: "openshift-install-mac-arm64-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
SignMachOBinary: true,
ArchiveFormat: "openshift-install-mac-arm64-%s.tar.gz",
},
{
OS: "linux",
Arch: targetReleaseArch,
Command: "openshift-install",
Mapping: extract.Mapping{Image: "installer", From: "usr/bin/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
ArchiveFormat: "openshift-install-linux-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
ArchiveFormat: "openshift-install-linux-%s.tar.gz",
},
{
OS: "linux",
Expand All @@ -445,10 +449,11 @@ func (o *ExtractOptions) extractCommand(command string) error {
NewArch: true,
Mapping: extract.Mapping{Image: "installer-artifacts", From: "usr/share/openshift/linux_amd64/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
ArchiveFormat: "openshift-install-linux-amd64-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
Copy link
Member

@ardaguclu ardaguclu Jun 3, 2024

Choose a reason for hiding this comment

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

There are various usage patterns of this command, have you had a chance to check all of them;

  • oc adm release info --tools
  • oc adm release --command=installer --command=darwing/arm64
    ...

Copy link
Contributor Author

@jeffdyoung jeffdyoung Jun 18, 2024

Choose a reason for hiding this comment

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

yes, they will print a warning if the openshift-install doesn't have the right marker, but it will exit 0.

ArchiveFormat: "openshift-install-linux-amd64-%s.tar.gz",
},
{
OS: "linux",
Expand All @@ -457,10 +462,11 @@ func (o *ExtractOptions) extractCommand(command string) error {
NewArch: true,
Mapping: extract.Mapping{Image: "installer-artifacts", From: "usr/share/openshift/linux_arm64/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
ArchiveFormat: "openshift-install-linux-arm64-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
ArchiveFormat: "openshift-install-linux-arm64-%s.tar.gz",
},
{
OS: "linux",
Expand All @@ -469,10 +475,11 @@ func (o *ExtractOptions) extractCommand(command string) error {
Optional: true,
Mapping: extract.Mapping{Image: "baremetal-installer", From: "usr/bin/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
ArchiveFormat: "openshift-baremetal-install-linux-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
ArchiveFormat: "openshift-baremetal-install-linux-%s.tar.gz",
},
{
OS: "linux",
Expand All @@ -481,10 +488,11 @@ func (o *ExtractOptions) extractCommand(command string) error {
Optional: true,
Mapping: extract.Mapping{Image: "baremetal-installer", From: "usr/bin/openshift-install"},

Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
ArchiveFormat: "openshift-install-rhel-%s.tar.gz",
Readme: readmeInstallUnix,
InjectReleaseImage: true,
InjectReleaseVersion: true,
InjectReleaseArchitecture: true,
ArchiveFormat: "openshift-install-rhel-%s.tar.gz",
},
{
OS: "linux",
Expand Down Expand Up @@ -637,6 +645,7 @@ func (o *ExtractOptions) extractCommand(command string) error {
releaseArch := release.Config.Architecture
releaseName := release.PreferredName()
refExact := release.ImageRef
releaseArchitecture := release.Platform()
refExact.Ref.Tag = ""
// if the release image is manifestlist image, we'll not change digest with
// arch based sha. Because we want that the extracted tool can be used for all archs.
Expand Down Expand Up @@ -887,6 +896,13 @@ func (o *ExtractOptions) extractCommand(command string) error {
value: releaseName,
})
}
if target.InjectReleaseArchitecture {
replacements = append(replacements, replacement{
name: "release architecture",
marker: append([]byte{0}, []byte(releaseArchitectureMarker[1:])...),
value: releaseArchitecture,
})
}
err = copyAndReplace(o.ErrOut, w, r, 4*1024, replacements, target.Command)
if err != nil {
closeFn()
Expand All @@ -909,7 +925,7 @@ func (o *ExtractOptions) extractCommand(command string) error {
klog.V(2).Infof("Unable to set extracted file modification time: %v", err)
}

if (target.InjectReleaseVersion || target.InjectReleaseImage) && target.SignMachOBinary {
if (target.InjectReleaseVersion || target.InjectReleaseImage || target.InjectReleaseArchitecture) && target.SignMachOBinary {
if err = codesign.ResignMacho(layer.Mapping.To, target.AsArchive, target.Command, target.LinkTo); err != nil {
klog.Infof("Unable to resign macho binaries: %v", err)
} else {
Expand Down Expand Up @@ -1041,6 +1057,8 @@ const (
releaseImageMarker = "!_RELEASE_IMAGE_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00"
// releaseVersionMarker is the placeholder within a binary for the release image version name string.
releaseVersionMarker = "!_RELEASE_VERSION_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00"
// releaseArchitectureMarker is the placeholder within a binary for the list of architectures the release image supports.
releaseArchitectureMarker = "!_RELEASE_ARCHITECTURE_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00"
Copy link
Member

@ardaguclu ardaguclu Jun 3, 2024

Choose a reason for hiding this comment

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

We are modifying binaries that are already built and that inevitably causes checksum value changes which is not good with respect to sanity checks. I'm against this change as it additionally modifies installer binary with more data and I believe that this is not the correct way to pass information.

But as far as I understand you can't find any alternative solution and I won't block this PR. I can only suggest that please ensure that the checksum values of the modified binary must be same with the data in the sha256sum file.

)

type replacement struct {
Expand Down