Merge pull request #28 from zaneb/error-reporting

Bug 2038272: Improve error reporting

cmd/controller/main.go

@@ -127,13 +127,13 @@ func main() {
 		os.Exit(1)
 	}
 
-	_, err = url.Parse(imagesPublishAddr)
+	publishURL, err := url.Parse(imagesPublishAddr)
 	if err != nil {
 		setupLog.Error(err, "imagesPublishAddr is not parsable")
 		os.Exit(1)
 	}
 
-	imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), envInputs.DeployISO, envInputs.DeployInitrd, imagesPublishAddr)
+	imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), envInputs.DeployISO, envInputs.DeployInitrd, publishURL)
 	http.Handle("/", http.FileServer(imageServer.FileSystem()))
 
 	go func() {

cmd/static-server/main.go

@@ -58,12 +58,18 @@ func loadStaticNMState(env *env.EnvInputs, nmstateDir string, imageServer imageh
 		if err != nil {
 			return errors.WithMessagef(err, "problem reading %s", path.Join(nmstateDir, f.Name()))
 		}
-		igBuilder := ignition.New(b, registries,
+		igBuilder, err := ignition.New(b, registries,
 			env.IronicBaseURL,
 			env.IronicAgentImage,
 			env.IronicAgentPullSecret,
 			env.IronicRAMDiskSSHKey,
 		)
+		if err != nil {
+			return errors.WithMessage(err, "failed to configure ignition")
+		}
+		if err, _ := igBuilder.ProcessNetworkState(); err != nil {
+			return errors.WithMessage(err, "failed to convert nmstate data")
+		}
 		ign, err := igBuilder.Generate()
 		if err != nil {
 			return errors.WithMessagef(err, "problem generating ignition %s", f.Name())
@@ -107,7 +113,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	_, err = url.Parse(imagesPublishAddr)
+	publishURL, err := url.Parse(imagesPublishAddr)
 	if err != nil {
 		log.Error(err, "imagesPublishAddr is not parsable")
 		os.Exit(1)
@@ -118,7 +124,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), env.DeployISO, env.DeployInitrd, imagesPublishAddr)
+	imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), env.DeployISO, env.DeployInitrd, publishURL)
 	http.Handle("/", http.FileServer(imageServer.FileSystem()))
 
 	if err := loadStaticNMState(env, nmstateDir, imageServer); err != nil {

go.mod

@@ -24,7 +24,7 @@ require (
 )
 
 replace (
-	github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0
-	github.com/metal3-io/baremetal-operator/apis => github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0
-	github.com/metal3-io/baremetal-operator/pkg/hardwareutils => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0
+	github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3
+	github.com/metal3-io/baremetal-operator/apis => github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3
+	github.com/metal3-io/baremetal-operator/pkg/hardwareutils => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3
 )

go.sum

@@ -513,12 +513,12 @@ github.com/onsi/gomega v1.15.0 h1:WjP/FQ/sk43MRmnEcT+MlDw2TFvkrXlprrPST/IudjU=
 github.com/onsi/gomega v1.15.0/go.mod h1:cIuvLEne0aoVhAgh/O6ac0Op8WWw9H6eYCriF+tEHG0=
 github.com/openshift/assisted-image-service v0.0.0-20211122133112-1552361c0458 h1:TDYrQaxV3PObc4pmcft15BUCbuDe0UNhzXTDZGnvzZ0=
 github.com/openshift/assisted-image-service v0.0.0-20211122133112-1552361c0458/go.mod h1:DSFZEsQZpIHqnV9saugs7meqdYmGKlI5FKKkDe421/g=
-github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0 h1:ZYUtfAnTsU9EntVmflF1y6i8y05h3DI2XmkRIDy9IWg=
-github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0/go.mod h1:p32F1DBUxfgd0JjM4rCuhJomFJokEoWR1Z/LZNL2LM8=
-github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0 h1:V1jCAv73CBOhuM/MGOoj08S7uVpdQnY0JMTqkiZMoTU=
-github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0/go.mod h1:CVSU+wS3oYrFJooMeiyDtTpatoXoKyXPE2YS5vT26vE=
-github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0 h1:9J4ZFKJvsQYg1StHmMl1M0kITHwnl2tQTLC+PeN/Wg4=
-github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0/go.mod h1:Q+r+xTc1jDcx/y61bVspJ9ANiAjJlsx/j+sL44mCB8w=
+github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3 h1:rZNvtqFsyOnxLAHcYnPM7/BB1gRWHxJ2PHsRv0HZOeI=
+github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3/go.mod h1:Nm29FDSH26opS7wK6ykg4pAjNFNlbJGUUUcB1rsUSLg=
+github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3 h1:yqTZ+2P0+tslKx+KRt7eiH9XDQ5DP1GgSaV8A/Cyjq8=
+github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3/go.mod h1:+60ass2P7bP6t6f1WUojQ38hi7cKHuJ3tphaYzimPlA=
+github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3 h1:8TnG8Q2LZXKpoIvOg938PC68k/VpLNyhTQYV+ftrmhA=
+github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3/go.mod h1:/PSTQInIZmfuOmAp/pSgZAs4txs6T49woC0MYIa4QzE=
 github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc=
 github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k=
 github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181zc=

pkg/env/env.go

@@ -10,7 +10,7 @@ import (
 type EnvInputs struct {
 	DeployISO             string `envconfig:"DEPLOY_ISO" required:"true"`
 	DeployInitrd          string `envconfig:"DEPLOY_INITRD" required:"true"`
-	IronicBaseURL         string `envconfig:"IRONIC_BASE_URL" required:"true"`
+	IronicBaseURL         string `envconfig:"IRONIC_BASE_URL"`
 	IronicAgentImage      string `envconfig:"IRONIC_AGENT_IMAGE" required:"true"`
 	IronicAgentPullSecret string `envconfig:"IRONIC_AGENT_PULL_SECRET"`
 	IronicRAMDiskSSHKey   string `envconfig:"IRONIC_RAMDISK_SSH_KEY"`

pkg/ignition/builder.go

@@ -25,26 +25,44 @@ type ignitionBuilder struct {
 	ironicAgentImage      string
 	ironicAgentPullSecret string
 	ironicRAMDiskSSHKey   string
+	networkKeyFiles       []byte
 }
 
-func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey string) *ignitionBuilder {
+func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey string) (*ignitionBuilder, error) {
+	if ironicBaseURL == "" {
+		return nil, errors.New("ironicBaseURL is required")
+	}
+	if ironicAgentImage == "" {
+		return nil, errors.New("ironicAgentImage is required")
+	}
+
 	return &ignitionBuilder{
 		nmStateData:           nmStateData,
 		registriesConf:        registriesConf,
 		ironicBaseURL:         ironicBaseURL,
 		ironicAgentImage:      ironicAgentImage,
 		ironicAgentPullSecret: ironicAgentPullSecret,
 		ironicRAMDiskSSHKey:   ironicRAMDiskSSHKey,
+	}, nil
+}
+
+func (b *ignitionBuilder) ProcessNetworkState() (error, string) {
+	if len(b.nmStateData) > 0 {
+		nmstatectl := exec.Command("nmstatectl", "gc", "-")
+		nmstatectl.Stdin = strings.NewReader(string(b.nmStateData))
+		out, err := nmstatectl.Output()
+		if err != nil {
+			if ee, ok := err.(*exec.ExitError); ok {
+				return err, string(ee.Stderr)
+			}
+			return err, ""
+		}
+		b.networkKeyFiles = out
 	}
+	return nil, ""
 }
 
 func (b *ignitionBuilder) Generate() ([]byte, error) {
-	if b.ironicAgentImage == "" {
-		return nil, errors.New("ironicAgentImage is required")
-	}
-	if b.ironicBaseURL == "" {
-		return nil, errors.New("ironicBaseURL is required")
-	}
 	config := ignition_config_types_32.Config{
 		Ignition: ignition_config_types_32.Ignition{
 			Version: "3.2.0",
@@ -86,15 +104,8 @@ func (b *ignitionBuilder) Generate() ([]byte, error) {
 		config.Storage.Files = append(config.Storage.Files, registriesFile)
 	}
 
-	if len(b.nmStateData) > 0 {
-		nmstatectl := exec.Command("nmstatectl", "gc", "-")
-		nmstatectl.Stdin = strings.NewReader(string(b.nmStateData))
-		out, err := nmstatectl.Output()
-		if err != nil {
-			return nil, err
-		}
-
-		files, err := nmstateOutputToFiles(out)
+	if len(b.networkKeyFiles) > 0 {
+		files, err := nmstateOutputToFiles(b.networkKeyFiles)
 		if err != nil {
 			return nil, err
 		}

pkg/ignition/builder_test.go

@@ -15,10 +15,13 @@ func TestGenerateRegistries(t *testing.T) {
   [[registry.mirror]]
     location = "virthost.ostest.test.metalkube.org:5000/localimages/local-release-image"
 `
-	builder := New([]byte{}, []byte(registries),
+	builder, err := New([]byte{}, []byte(registries),
 		"http://ironic.example.com",
 		"quay.io/openshift-release-dev/ironic-ipa-image",
 		"", "")
+	if err != nil {
+		t.Fatalf("Unexpected error %v", err)
+	}
 
 	ignition, err := builder.Generate()
 	if err != nil {

pkg/imagehandler/imagehandler.go

@@ -14,6 +14,7 @@ limitations under the License.
 package imagehandler
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"sync"
@@ -22,12 +23,24 @@ import (
 	"github.com/google/uuid"
 )
 
+type InvalidBaseImageError struct {
+	cause error
+}
+
+func (ie InvalidBaseImageError) Error() string {
+	return "Base Image not available"
+}
+
+func (ie InvalidBaseImageError) Unwrap() error {
+	return ie.cause
+}
+
 // imageFileSystem is an http.FileSystem that creates a virtual filesystem of
 // host images.
 type imageFileSystem struct {
 	isoFile       *baseIso
 	initramfsFile *baseInitramfs
-	baseURL       string
+	baseURL       *url.URL
 	keys          map[string]string
 	images        map[string]*imageFile
 	mu            *sync.Mutex
@@ -43,7 +56,7 @@ type ImageHandler interface {
 	RemoveImage(key string)
 }
 
-func NewImageHandler(logger logr.Logger, isoFile, initramfsFile, baseURL string) ImageHandler {
+func NewImageHandler(logger logr.Logger, isoFile, initramfsFile string, baseURL *url.URL) ImageHandler {
 	return &imageFileSystem{
 		log:           logger,
 		isoFile:       newBaseIso(isoFile),
@@ -81,7 +94,7 @@ func (f *imageFileSystem) getNameForKey(key string) (name string, err error) {
 func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initramfs, static bool) (string, error) {
 	size, err := f.getBaseImage(initramfs).Size()
 	if err != nil {
-		return "", err
+		return "", InvalidBaseImageError{cause: err}
 	}
 
 	f.mu.Lock()
@@ -94,6 +107,10 @@ func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initram
 			return "", err
 		}
 	}
+	p, err := url.Parse(fmt.Sprintf("/%s", name))
+	if err != nil {
+		return "", err
+	}
 
 	if _, exists := f.images[key]; !exists {
 		f.keys[name] = key
@@ -105,12 +122,7 @@ func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initram
 		}
 	}
 
-	u, err := url.Parse(f.baseURL)
-	if err != nil {
-		return "", err
-	}
-	u.Path = name
-	return u.String(), nil
+	return f.baseURL.ResolveReference(p).String(), nil
 }
 
 func (f *imageFileSystem) imageFileByName(name string) *imageFile {

pkg/imagehandler/imagehandler_test.go

@@ -17,6 +17,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"sync"
 	"testing"
@@ -42,11 +43,13 @@ func TestImageHandler(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	baseURL, _ := url.Parse("http://localhost:8080")
+
 	rr := httptest.NewRecorder()
 	imageServer := &imageFileSystem{
 		log:     zap.New(zap.UseDevMode(true)),
 		isoFile: &baseIso{baseFileData{filename: "dummyfile.iso", size: 12345}},
-		baseURL: "http://localhost:8080",
+		baseURL: baseURL,
 		keys: map[string]string{
 			"host-xyz-45-uuid": "host-xyz-45.iso",
 		},
@@ -79,10 +82,14 @@ func TestImageHandler(t *testing.T) {
 }
 
 func TestNewImageHandler(t *testing.T) {
+	baseUrl, err := url.Parse("http://base.test:1234")
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
 	handler := NewImageHandler(zap.New(zap.UseDevMode(true)),
 		"dummyfile.iso",
 		"dummyfile.initramfs",
-		"http://base.test:1234")
+		baseUrl)
 
 	ifs := handler.(*imageFileSystem)
 	ifs.isoFile.size = 12345
@@ -122,10 +129,14 @@ func TestNewImageHandler(t *testing.T) {
 }
 
 func TestNewImageHandlerStatic(t *testing.T) {
+	baseUrl, err := url.Parse("http://base.test:1234")
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
 	handler := NewImageHandler(zap.New(zap.UseDevMode(true)),
 		"dummyfile.iso",
 		"dummyfile.initramfs",
-		"http://base.test:1234")
+		baseUrl)
 
 	ifs := handler.(*imageFileSystem)
 	ifs.isoFile.size = 12345

pkg/imageprovider/rhcos.go

@@ -1,6 +1,7 @@
 package imageprovider
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/go-logr/logr"
@@ -47,12 +48,25 @@ func (ip *rhcosImageProvider) SupportsFormat(format metal3.ImageFormat) bool {
 func (ip *rhcosImageProvider) buildIgnitionConfig(networkData imageprovider.NetworkData) ([]byte, error) {
 	nmstateData := networkData["nmstate"]
 
-	return ignition.New(nmstateData, ip.RegistriesConf,
+	builder, err := ignition.New(nmstateData, ip.RegistriesConf,
 		ip.EnvInputs.IronicBaseURL,
 		ip.EnvInputs.IronicAgentImage,
 		ip.EnvInputs.IronicAgentPullSecret,
 		ip.EnvInputs.IronicRAMDiskSSHKey,
-	).Generate()
+	)
+	if err != nil {
+		return nil, imageprovider.BuildInvalidError(err)
+	}
+
+	err, message := builder.ProcessNetworkState()
+	if message != "" {
+		return nil, imageprovider.BuildInvalidError(errors.New(message))
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	return builder.Generate()
 }
 
 func imageKey(data imageprovider.ImageData) string {
@@ -71,8 +85,12 @@ func (ip *rhcosImageProvider) BuildImage(data imageprovider.ImageData, networkDa
 		return "", err
 	}
 
-	return ip.ImageHandler.ServeImage(imageKey(data), ignitionConfig,
+	url, err := ip.ImageHandler.ServeImage(imageKey(data), ignitionConfig,
 		data.Format == metal3.ImageFormatInitRD, false)
+	if errors.As(err, &imagehandler.InvalidBaseImageError{}) {
+		return "", imageprovider.BuildInvalidError(err)
+	}
+	return url, err
 }
 
 func (ip *rhcosImageProvider) DiscardImage(data imageprovider.ImageData) error {