coreos / ignition

First boot installer and configuration tool
https://coreos.github.io/ignition/
Apache License 2.0
827 stars 244 forks source link

enhance error for unsupported versions #1134

Open cgwalters opened 3 years ago

cgwalters commented 3 years ago

For RHCOS we're going to have a long tail of dealing with people trying to boot 4.6+ with spec2x Ignition. Further we have the opposite case here: https://github.com/openshift/oc/pull/628#discussion_r524524863

Basically it'd be nice if in the API when we fail to parse a config because the version is unsupported we say which version was found. This might be kind of an API break if we use e.g. https://blog.golang.org/go1.13-errors to wrap...I was looking at

diff --git a/config/config.go b/config/config.go
index 8106e8c8..efa1b313 100644
--- a/config/config.go
+++ b/config/config.go
@@ -15,6 +15,8 @@
 package config

 import (
+   "fmt"
+
    "github.com/coreos/ignition/v2/config/shared/errors"
    "github.com/coreos/ignition/v2/config/util"
    "github.com/coreos/ignition/v2/config/v3_0"
@@ -67,7 +69,7 @@ func Parse(raw []byte) (types_exp.Config, report.Report, error) {
    case types_3_0.MaxVersion:
        return exp_from_3_2(v3_2_from_3_1(v3_1_from_3_0(v3_0.Parse(raw))))
    default:
-       return types_exp.Config{}, report.Report{}, errors.ErrUnknownVersion
+       return types_exp.Config{}, report.Report{}, fmt.Errorf("%w: %s", errors.ErrUnknownVersion, *version)
    }
 }

diff --git a/config/v3_2/config.go b/config/v3_2/config.go
index 8d0abbcc..eb2494f6 100644
--- a/config/v3_2/config.go
+++ b/config/v3_2/config.go
@@ -15,6 +15,8 @@
 package v3_2

 import (
+   "fmt"
+
    "github.com/coreos/ignition/v2/config/merge"
    "github.com/coreos/ignition/v2/config/shared/errors"
    "github.com/coreos/ignition/v2/config/util"
@@ -45,7 +47,7 @@ func Parse(rawConfig []byte) (types.Config, report.Report, error) {
    version, err := semver.NewVersion(config.Ignition.Version)

    if err != nil || *version != types.MaxVersion {
-       return types.Config{}, report.Report{}, errors.ErrUnknownVersion
+       return types.Config{}, report.Report{}, fmt.Errorf("%w: %s", errors.ErrUnknownVersion, *version)
    }

    rpt := validate.ValidateWithContext(config, rawConfig)
diff --git a/config/v3_3_experimental/config.go b/config/v3_3_experimental/config.go
index 969d3c56..7b9063f6 100644
--- a/config/v3_3_experimental/config.go
+++ b/config/v3_3_experimental/config.go
@@ -15,6 +15,8 @@
 package v3_3_experimental

 import (
+   "fmt"
+
    "github.com/coreos/ignition/v2/config/merge"
    "github.com/coreos/ignition/v2/config/shared/errors"
    "github.com/coreos/ignition/v2/config/util"
@@ -45,7 +47,7 @@ func Parse(rawConfig []byte) (types.Config, report.Report, error) {
    version, err := semver.NewVersion(config.Ignition.Version)

    if err != nil || *version != types.MaxVersion {
-       return types.Config{}, report.Report{}, errors.ErrUnknownVersion
+       return types.Config{}, report.Report{}, fmt.Errorf("%w: %s", errors.ErrUnknownVersion, config.Ignition.Version)
    }

    rpt := validate.ValidateWithContext(config, rawConfig)
bgilbert commented 3 years ago

The approach proposed above would break the ability of callers to programmatically detect the error. All Ignition errors use pre-created error objects for that reason, which unfortunately means that we can't use format strings.

We could perhaps change the error message to specify the range of currently supported versions, and/or have the engine detect the error and log an additional line that reports the detected config version.

cgwalters commented 3 years ago

Maybe add a new Parse2 method (naming is hard) that has the same signature but requires callers to use errors.Is() instead?