WIP: oc adm release mirror code clean-up

Modify to use proper k8s encoding/decoding and other minor cleanup.
Reference open comments from openshift#343.

pkg/cli/admin/release/extract.go

@@ -10,7 +10,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"strings"
+	//"strings"
 	"sync"
 	"time"
 
@@ -31,6 +31,7 @@ import (
 	"github.com/openshift/oc/pkg/cli/image/imagesource"
 	imagemanifest "github.com/openshift/oc/pkg/cli/image/manifest"
 	"github.com/openshift/oc/pkg/cli/image/workqueue"
+	helpers "github.com/openshift/oc/pkg/helpers/release"
 	"github.com/pkg/errors"
 )
 
@@ -152,8 +153,9 @@ type ExtractOptions struct {
 	File      string
 	FileDir   string
 
-	ExtractManifests bool
-	Manifests        []manifest.Manifest
+	ExtractManifests          bool
+	SignatureVerifierManifest manifest.Manifest
+	SignatureVerifierData     map[string]string
 
 	ImageMetadataCallback func(m *extract.Mapping, dgst, contentDigest digest.Digest, config *dockerv1client.DockerImageConfig)
 }
@@ -278,36 +280,34 @@ func (o *ExtractOptions) Run() error {
 				case "release-metadata":
 					return true, nil
 				default:
-					if ext := path.Ext(hdr.Name); len(ext) > 0 && (ext == ".yaml" || ext == ".yml" || ext == ".json") {
-						klog.V(4).Infof("Found manifest %s", hdr.Name)
-						raw, err := ioutil.ReadAll(r)
-						if err != nil {
-							manifestErrs = append(manifestErrs, errors.Wrapf(err, "error reading file %s", hdr.Name))
-							return true, nil
-						}
-						ms, err := manifest.ParseManifests(bytes.NewReader(raw))
-						if err != nil {
-							manifestErrs = append(manifestErrs, errors.Wrapf(err, "error parsing %s", hdr.Name))
-							return true, nil
-						}
-						for i := range ms {
-							ms[i].OriginalFilename = filepath.Base(hdr.Name)
-							src := fmt.Sprintf("the config map %s/%s", ms[i].Obj.GetNamespace(), ms[i].Obj.GetName())
-							data, _, err := unstructured.NestedStringMap(ms[i].Obj.Object, "data")
+					// done once signature verification data has been found in a manifest
+					if o.SignatureVerifierData == nil {
+						if ext := path.Ext(hdr.Name); len(ext) > 0 && (ext == ".yaml" || ext == ".yml" || ext == ".json") {
+							klog.V(4).Infof("Found manifest %s", hdr.Name)
+							raw, err := ioutil.ReadAll(r)
+							if err != nil {
+								manifestErrs = append(manifestErrs, errors.Wrapf(err, "error reading file %s", hdr.Name))
+								return true, nil
+							}
+							ms, err := manifest.ParseManifests(bytes.NewReader(raw))
 							if err != nil {
-								manifestErrs = append(manifestErrs, errors.Wrapf(err, "%s is not valid", src))
-								continue
+								manifestErrs = append(manifestErrs, errors.Wrapf(err, "error parsing %s", hdr.Name))
+								return true, nil
 							}
-							for k, v := range data {
-								switch {
-								case strings.HasPrefix(k, "verifier-public-key-"):
-									klog.V(2).Infof("Found in %s:\n%s %s", hdr.Name, k, v)
-								case strings.HasPrefix(k, "store-"):
-									klog.V(2).Infof("Found in %s:\n%s\n%s", hdr.Name, k, v)
+							for i := range ms {
+								verifierData := helpers.LoadConfigMapVerifierDataFromManifest(ms[i].Raw)
+								if len(verifierData) > 0 && o.SignatureVerifierData == nil {
+									klog.V(4).Infof("Found signature verifier data in %s", hdr.Name)
+									o.SignatureVerifierData = make(map[string]string)
+									for k, v := range verifierData {
+										o.SignatureVerifierData[k] = v
+									}
+									o.SignatureVerifierManifest = ms[i]
+									o.SignatureVerifierManifest.OriginalFilename = filepath.Base(hdr.Name)
 								}
+								return true, nil
 							}
 						}
-						o.Manifests = append(o.Manifests, ms...)
 					}
 				}
 				return true, nil
@@ -321,18 +321,18 @@ func (o *ExtractOptions) Run() error {
 		}
 
 		// Only output manifest errors if manifests were being extracted and we didn't find the expected signature
-		// manifests. We don't care about errors in other manifests and they will only confuse/alarm the user.
+		// verification data. We don't care about errors in other manifests and they will only confuse/alarm the user.
 		// Do not return an error so current operation, e.g. mirroring, continues.
 		if len(manifestErrs) > 0 {
-			if o.ExtractManifests && len(o.Manifests) == 0 {
+			if o.ExtractManifests && o.SignatureVerifierData == nil {
 				fmt.Fprintf(o.ErrOut, "Errors: %s\n", errorList(manifestErrs))
 			}
 		}
 
 		// Output an error if manifests were being extracted and we didn't find the expected signature
-		// manifests. Do not return an error so current operation, e.g. mirroring, continues.
-		if o.ExtractManifests && len(o.Manifests) == 0 {
-			fmt.Fprintf(o.ErrOut, "No manifests found\n")
+		// verification data. Do not return an error so current operation, e.g. mirroring, continues.
+		if o.ExtractManifests && o.SignatureVerifierData == nil {
+			fmt.Fprintf(o.ErrOut, "No signature verification data found\n")
 		}
 		return nil
 

pkg/cli/admin/release/mirror.go

@@ -44,6 +44,7 @@ import (
 	imagemanifest "github.com/openshift/oc/pkg/cli/image/manifest"
 	"github.com/openshift/oc/pkg/cli/image/mirror"
 	"github.com/openshift/oc/pkg/helpers/release"
+	helpers "github.com/openshift/oc/pkg/helpers/release"
 )
 
 // configFilesBaseDir is created under '--to-dir', when specified, to contain release image
@@ -360,7 +361,8 @@ func (o *MirrorOptions) handleSignatures(context context.Context, signaturesByDi
 			if o.DryRun {
 				fmt.Fprintf(o.Out, "info: Write configmap signature file %s\n", fullName)
 			} else {
-				cmDataBytes, err := yaml.Marshal(cmData)
+				cmDataBytes, err := helpers.ConfigMapAsBytes(cmData)
+				//cmDataBytes, err := yaml.Marshal(cmData)
 				if err != nil {
 					return fmt.Errorf("marshaling configmap YAML: %v", err)
 				}
@@ -371,6 +373,14 @@ func (o *MirrorOptions) handleSignatures(context context.Context, signaturesByDi
 					return err
 				}
 				fmt.Fprintf(o.Out, "Configmap signature file %s created\n", fullName)
+				newCm, err := helpers.ReadConfigMap(cmDataBytes)
+				if err != nil {
+					klog.Infof("error %v", err)
+				} else if newCm != nil {
+					klog.Infof("newCm %#v", newCm)
+				} else {
+					klog.Infof("newCm is nil")
+				}
 			}
 		}
 	}
@@ -455,7 +465,8 @@ func (o *MirrorOptions) Run() error {
 	}
 
 	var releaseDigest string
-	var manifests []manifest.Manifest
+	var data map[string]string
+	var manifest manifest.Manifest
 	verifier := imagemanifest.NewVerifier()
 	is := o.ImageStream
 	if is == nil {
@@ -489,7 +500,8 @@ func (o *MirrorOptions) Run() error {
 			}
 			fmt.Fprintf(o.ErrOut, "warning: %v\n", err)
 		}
-		manifests = extractOpts.Manifests
+		data = extractOpts.SignatureVerifierData
+		manifest = extractOpts.SignatureVerifierManifest
 	}
 	version = is.Name
 
@@ -501,7 +513,7 @@ func (o *MirrorOptions) Run() error {
 	clientBuilder := &verifyClientBuilder{builder: o.HTTPClient}
 
 	// Attempt to load a verifier as defined by the release being mirrored
-	imageVerifier, err := release.LoadConfigMapVerifierDataFromUpdate(manifests, clientBuilder, nil)
+	imageVerifier, err := release.ConfigureVerifier(data, manifest, clientBuilder, nil)
 	if err != nil {
 		return fmt.Errorf("Unable to load configmap verifier: %v", err)
 	}

pkg/helpers/release/configmap.go

@@ -15,21 +15,27 @@ import (
 	"k8s.io/klog"
 )
 
-// ReleaseAnnotationConfigMapVerifier is an annotation set on a config map in the
-// release payload to indicate that this config map controls signing for the payload.
-// Only the first config map within the payload should be used, regardless of whether
-// it has data. See NewFromConfigMapData for more.
-const ReleaseAnnotationConfigMapVerifier = "release.openshift.io/verification-config-map"
+const (
+	// ReleaseAnnotationConfigMapVerifier is an annotation set on a config map in the
+	// release payload to indicate that this config map controls signing for the payload.
+	// Only the first config map within the payload should be used, regardless of whether
+	// it has data. See NewFromConfigMapData for more.
+	ReleaseAnnotationConfigMapVerifier = "release.openshift.io/verification-config-map"
 
-// NamespaceLabelConfigMap is the Namespace label applied to a configmap
-// containing signatures.
-const NamespaceLabelConfigMap = "openshift-config-managed"
+	// NamespaceLabelConfigMap is the Namespace label applied to a configmap
+	// containing signatures.
+	NamespaceLabelConfigMap = "openshift-config-managed"
 
-// ReleaseLabelConfigMap is a label applied to a configmap inside the
-// NamespaceLabelConfigMap namespace that indicates it contains signatures
-// for release image digests. Any binaryData key that starts with the digest
-// is added to the list of signatures checked.
-const ReleaseLabelConfigMap = "release.openshift.io/verification-signatures"
+	// ReleaseLabelConfigMap is a label applied to a configmap inside the
+	// NamespaceLabelConfigMap namespace that indicates it contains signatures
+	// for release image digests. Any binaryData key that starts with the digest
+	// is added to the list of signatures checked.
+	ReleaseLabelConfigMap = "release.openshift.io/verification-signatures"
+
+	VerifierPublicKeyPrefix = "verifier-public-key-"
+
+	StorePrefix = "store-"
+)
 
 // digestToKeyPrefix changes digest to use '-' in place of ':',
 // {algo}-{hash} instead of {algo}:{hash}, because colons are not
@@ -104,13 +110,13 @@ func NewFromConfigMapData(src string, data map[string]string, clientBuilder Clie
 	var stores []*url.URL
 	for k, v := range data {
 		switch {
-		case strings.HasPrefix(k, "verifier-public-key-"):
+		case strings.HasPrefix(k, VerifierPublicKeyPrefix):
 			keyring, err := loadArmoredOrUnarmoredGPGKeyRing([]byte(v))
 			if err != nil {
 				return nil, errors.Wrapf(err, "%s has an invalid key %q that must be a GPG public key: %v", src, k, err)
 			}
 			verifiers[k] = keyring
-		case strings.HasPrefix(k, "store-"):
+		case strings.HasPrefix(k, StorePrefix):
 			v = strings.TrimSpace(v)
 			u, err := url.Parse(v)
 			if err != nil || (u.Scheme != "http" && u.Scheme != "https" && u.Scheme != "file") {

pkg/helpers/release/util.go

@@ -0,0 +1,39 @@
+package release
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
+)
+
+var (
+	coreScheme  = runtime.NewScheme()
+	coreCodecs  = serializer.NewCodecFactory(coreScheme)
+	coreEncoder runtime.Encoder
+)
+
+func init() {
+	if err := corev1.AddToScheme(coreScheme); err != nil {
+		panic(err)
+	}
+	coreEncoderCodecFactory := serializer.NewCodecFactory(coreScheme)
+	coreEncoder = coreEncoderCodecFactory.LegacyCodec(corev1.SchemeGroupVersion)
+}
+
+// ReadConfigMap reads configmap object from bytes.
+func ReadConfigMap(objBytes []byte) (*corev1.ConfigMap, error) {
+	requiredObj, err := runtime.Decode(coreCodecs.UniversalDecoder(corev1.SchemeGroupVersion), objBytes)
+	if err != nil {
+		return nil, err
+	}
+	cm, ok := requiredObj.(*corev1.ConfigMap)
+	if ok {
+		return cm, nil
+	}
+	return nil, nil
+}
+
+// ConfigMapAsBytes returns given config map as bytes.
+func ConfigMapAsBytes(cm *corev1.ConfigMap) ([]byte, error) {
+	return runtime.Encode(coreEncoder, cm)
+}

pkg/helpers/release/verify.go

@@ -19,10 +19,7 @@ import (
 	"sync"
 	"time"
 
-	"github.com/pkg/errors"
 	"golang.org/x/crypto/openpgp"
-	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	coreclientsetv1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/klog"
 
@@ -333,31 +330,38 @@ func (v *ReleaseVerifier) Signatures() map[string][][]byte {
 	return copied
 }
 
-// LoadConfigMapVerifierDataFromUpdate fetches the first config map in the payload with the correct annotation.
-// It returns an error if the data is not valid, or no verifier if no config map is found. See the verify
-// package for more details on the algorithm for verification. If the annotation is set, a verifier or error
-// is always returned.
-func LoadConfigMapVerifierDataFromUpdate(manifests []manifest.Manifest, clientBuilder ClientBuilder, configMapClient coreclientsetv1.ConfigMapsGetter) (Interface /**StorePersister,*/, error) {
-	configMapGVK := corev1.SchemeGroupVersion.WithKind("ConfigMap")
-	for _, manifest := range manifests {
-		if manifest.GVK != configMapGVK {
-			continue
-		}
-		if _, ok := manifest.Obj.GetAnnotations()[ReleaseAnnotationConfigMapVerifier]; !ok {
-			continue
-		}
-		src := fmt.Sprintf("the config map %s/%s", manifest.Obj.GetNamespace(), manifest.Obj.GetName())
-		data, _, err := unstructured.NestedStringMap(manifest.Obj.Object, "data")
-		if err != nil {
-			return nil, errors.Wrapf(err, "%s is not valid: %v", src, err)
-		}
-		verifier, err := NewFromConfigMapData(src, data, clientBuilder)
-		if err != nil {
-			return nil, err
+// LoadConfigMapVerifierDataFromManifest decodes the manifest as a config map and checks it for the correct
+// verifier annotations. If found returns data as a map otherwise returns an empty map.
+func LoadConfigMapVerifierDataFromManifest(manifestBytes []byte) map[string]string {
+	verifierData := make(map[string]string)
+	configMap, err := ReadConfigMap(manifestBytes)
+
+	// configMap will be nil if this was not a config map
+	if err == nil && configMap != nil {
+		for k, v := range configMap.Data {
+			switch {
+			case strings.HasPrefix(k, VerifierPublicKeyPrefix):
+				klog.Infof("%s prefix found", VerifierPublicKeyPrefix)
+				verifierData[k] = v
+			case strings.HasPrefix(k, StorePrefix):
+				klog.Infof("%s prefix found", StorePrefix)
+				verifierData[k] = v
+			}
 		}
-		return verifier, nil
 	}
-	return nil, nil
+	return verifierData
+}
+
+// ConfigureVerifier configures a verifier using the given verifier data and the manifest from which it was fetched.
+// It returns an error and no verifier if the data is not valid. See the verify package for more details on the algorithm
+// for verification.
+func ConfigureVerifier(verifierData map[string]string, sourceManifest manifest.Manifest, clientBuilder ClientBuilder, configMapClient coreclientsetv1.ConfigMapsGetter) (Interface, error) {
+	src := fmt.Sprintf("the config map %s/%s", sourceManifest.Obj.GetNamespace(), sourceManifest.Obj.GetName())
+	verifier, err := NewFromConfigMapData(src, verifierData, clientBuilder)
+	if err != nil {
+		return nil, err
+	}
+	return verifier, nil
 }
 
 // hasVerified returns true if the digest has already been verified.