Skip to content

NO-JIRA: Isolate manifest handling in release-extraction #2048

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NO-JIRA: Isolate manifest handling in release-extraction #2048

Show file tree
Hide file tree
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
34 changes: 13 additions & 21 deletions pkg/cli/admin/release/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"fmt"
"io"
"os"
"path"
"path/filepath"
"sync"
"time"
Expand All @@ -20,6 +19,7 @@ import (

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/cli-runtime/pkg/genericiooptions"
"k8s.io/client-go/rest"
kcmdutil "k8s.io/kubectl/pkg/cmd/util"
Expand Down Expand Up @@ -349,7 +349,7 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
}
}

var manifestErrs []error
manifestReceiver := ManifestReceiver{skipNames: sets.New[string]("image-references", "release-metadata")}
// o.ExtractManifests implies o.File == ""
if o.ExtractManifests {
expectedProviderSpecKind := credRequestCloudProviderSpecKindMapping[o.Cloud]
Expand Down Expand Up @@ -379,8 +379,8 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
include = newIncluder(inclusionConfig)
}

opts.TarEntryCallback = func(hdr *tar.Header, _ extract.LayerInfo, r io.Reader) (bool, error) {
if hdr.Name == "image-references" && !o.CredentialsRequests {
manifestReceiver.manifestsCallback = func(filename string, ms []manifest.Manifest, r io.Reader) (bool, error) {
if filename == "image-references" && !o.CredentialsRequests {
buf := &bytes.Buffer{}
if _, err := io.Copy(buf, r); err != nil {
return false, fmt.Errorf("unable to load image-references from release payload: %w", err)
Expand All @@ -398,7 +398,7 @@ func (o *ExtractOptions) Run(ctx context.Context) error {

out := o.Out
if o.Directory != "" {
out, err = os.Create(filepath.Join(o.Directory, hdr.Name))
out, err = os.Create(filepath.Join(o.Directory, filename))
if err != nil {
return false, err
}
Expand All @@ -408,10 +408,10 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
return true, err
}
return true, nil
} else if hdr.Name == "release-metadata" && !o.CredentialsRequests {
} else if filename == "release-metadata" && !o.CredentialsRequests {
out := o.Out
if o.Directory != "" {
out, err = os.Create(filepath.Join(o.Directory, hdr.Name))
out, err = os.Create(filepath.Join(o.Directory, filename))
if err != nil {
return false, err
}
Expand All @@ -423,16 +423,6 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
return true, nil
}

if ext := path.Ext(hdr.Name); len(ext) == 0 || !(ext == ".yaml" || ext == ".yml" || ext == ".json") {
return true, nil
}
klog.V(4).Infof("Found manifest %s", hdr.Name)
ms, err := manifest.ParseManifests(r)
if err != nil {
manifestErrs = append(manifestErrs, errors.Wrapf(err, "error parsing %s", hdr.Name))
return true, nil
}

for i := len(ms) - 1; i >= 0; i-- {
if o.Included && o.CredentialsRequests && ms[i].GVK == credentialsRequestGVK && len(ms[i].Obj.GetAnnotations()) == 0 {
klog.V(4).Infof("Including %s for manual CredentialsRequests, despite lack of annotations", ms[i].String())
Expand Down Expand Up @@ -469,25 +459,26 @@ func (o *ExtractOptions) Run(ctx context.Context) error {

out := o.Out
if o.Directory != "" {
out, err = os.Create(filepath.Join(o.Directory, hdr.Name))
out, err = os.Create(filepath.Join(o.Directory, filename))
if err != nil {
return false, errors.Wrapf(err, "error creating manifest in %s", hdr.Name)
return false, errors.Wrapf(err, "error creating manifest in %s", filename)
}
}
if out != nil {
for _, m := range manifestsToWrite {
yamlBytes, err := yaml.JSONToYAML(m.Raw)
if err != nil {
return false, errors.Wrapf(err, "error serializing manifest in %s", hdr.Name)
return false, errors.Wrapf(err, "error serializing manifest in %s", filename)
}
fmt.Fprintf(out, "---\n")
if _, err := out.Write(yamlBytes); err != nil {
return false, errors.Wrapf(err, "error writing manifest in %s", hdr.Name)
return false, errors.Wrapf(err, "error writing manifest in %s", filename)
}
}
}
return true, nil
}
opts.TarEntryCallback = manifestReceiver.TarEntryCallback
}

fileFound := false
Expand All @@ -506,6 +497,7 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
return err
}

manifestErrs := manifestReceiver.ManifestErrs
if metadataVerifyMsg != "" {
if o.File == "" && o.Out != nil {
fmt.Fprintf(o.Out, "%s\n", metadataVerifyMsg)
Expand Down
40 changes: 35 additions & 5 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 @@ -13,6 +13,7 @@ import (
"hash"
"io"
"os"
"path"
"path/filepath"
"regexp"
"runtime"
Expand All @@ -21,25 +22,25 @@ import (
"sync"
"syscall"

"k8s.io/utils/ptr"

"github.com/MakeNowJust/heredoc"
"github.com/pkg/errors"
"golang.org/x/crypto/openpgp"
terminal "golang.org/x/term"

"k8s.io/klog/v2"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/cli-runtime/pkg/genericiooptions"
appsv1client "k8s.io/client-go/kubernetes/typed/apps/v1"
"k8s.io/client-go/rest"
"k8s.io/klog/v2"
"k8s.io/utils/ptr"
"sigs.k8s.io/yaml"

"github.com/MakeNowJust/heredoc"
configv1 "github.com/openshift/api/config/v1"
configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
imagereference "github.com/openshift/library-go/pkg/image/reference"
"github.com/openshift/library-go/pkg/manifest"

"github.com/openshift/oc/pkg/cli/admin/internal/codesign"
"github.com/openshift/oc/pkg/cli/image/extract"
"github.com/openshift/oc/pkg/cli/image/imagesource"
Expand Down Expand Up @@ -1286,3 +1287,32 @@ func newIncluder(config manifestInclusionConfiguration) includer {
return m.Include(config.ExcludeIdentifier, config.RequiredFeatureSet, config.Profile, config.Capabilities, config.Overrides)
}
}

// ManifestReceiver has a TarEntryCallback function which can be used to as a callback to ExtractOptions.TarEntryCallback.
// It feeds the downstream manifestsCallback
// * with the manifests from every file whose name is not in skipNames, OR
// * with the reader that contains the content of each file whose name is in skipNames.
// All the errors encountered when parsing the manifests are collected in ManifestErrs.
type ManifestReceiver struct {
manifestsCallback func(filename string, manifests []manifest.Manifest, reader io.Reader) (cont bool, err error)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just call it downstreamCallback? Even your Godoc talks about it that way. Calling it manifestCallback is a bit confusing to me because it kinda hints that it would be called with just manifests (esp. together with the other field called 'skipNames`)

skipNames sets.Set[string]
Copy link
Member

Choose a reason for hiding this comment

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

The skipNames naming is a bit confusing to me; "skip" makes me think they are skipped entirely, but they are actually just not parsed as manifests. And actually they are anti-skipped because they would otherwise be skipped because they are not json/yaml files.


ManifestErrs []error
}

func (mr *ManifestReceiver) TarEntryCallback(h *tar.Header, _ extract.LayerInfo, r io.Reader) (cont bool, err error) {
if mr.skipNames.Has(h.Name) {
return mr.manifestsCallback(h.Name, nil, r)
}

if ext := path.Ext(h.Name); len(ext) == 0 || !(ext == ".yaml" || ext == ".yml" || ext == ".json") {
return true, nil
}
klog.V(4).Infof("Found manifest %s", h.Name)
ms, err := manifest.ParseManifests(r)
if err != nil {
mr.ManifestErrs = append(mr.ManifestErrs, errors.Wrapf(err, "error parsing %s", h.Name))
Copy link
Member

Choose a reason for hiding this comment

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

This makes the callback concurrent-unsafe; that's fine (IIRC you already checked once that it is never called in parallel - but maybe it should?) but the GoDoc should mention it.

return true, nil
}
return mr.manifestsCallback(h.Name, ms, nil)
}