From e29e373b46455b7891059d014c0cb33aa1456e1a Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 10 Jun 2021 01:15:22 -0400 Subject: [PATCH 1/2] config/openshift/*: fail if file compression is specified The MCO ignores the compression field and writes out the file without decompressing it first. https://bugzilla.redhat.com/show_bug.cgi?id=1970218 --- config/common/errors.go | 23 +++++++++++---------- config/openshift/v4_8/translate.go | 9 ++++++++ config/openshift/v4_8/translate_test.go | 5 +++++ config/openshift/v4_9_exp/translate.go | 9 ++++++++ config/openshift/v4_9_exp/translate_test.go | 5 +++++ 5 files changed, 40 insertions(+), 11 deletions(-) diff --git a/config/common/errors.go b/config/common/errors.go index a0d11618..1d2defb7 100644 --- a/config/common/errors.go +++ b/config/common/errors.go @@ -48,15 +48,16 @@ var ( ErrTooFewMirrorDevices = errors.New("mirroring requires at least two devices") // MachineConfigs - ErrFieldElided = errors.New("field ignored in raw mode") - ErrNameRequired = errors.New("metadata.name is required") - ErrRoleRequired = errors.New("machineconfiguration.openshift.io/role label is required") - ErrInvalidKernelType = errors.New("must be empty, \"default\", or \"realtime\"") - ErrBtrfsSupport = errors.New("btrfs is not supported in this spec version") - ErrDirectorySupport = errors.New("directories are not supported in this spec version") - ErrFileAppendSupport = errors.New("appending to files is not supported in this spec version") - ErrLinkSupport = errors.New("links are not supported in this spec version") - ErrGroupSupport = errors.New("groups are not supported in this spec version") - ErrUserFieldSupport = errors.New("fields other than \"name\" and \"ssh_authorized_keys\" are not supported in this spec version") - ErrUserNameSupport = errors.New("users other than \"core\" are not supported in this spec version") + ErrFieldElided = errors.New("field ignored in raw mode") + ErrNameRequired = errors.New("metadata.name is required") + ErrRoleRequired = errors.New("machineconfiguration.openshift.io/role label is required") + ErrInvalidKernelType = errors.New("must be empty, \"default\", or \"realtime\"") + ErrBtrfsSupport = errors.New("btrfs is not supported in this spec version") + ErrDirectorySupport = errors.New("directories are not supported in this spec version") + ErrFileAppendSupport = errors.New("appending to files is not supported in this spec version") + ErrFileCompressionSupport = errors.New("file compression is not supported in this spec version") + ErrLinkSupport = errors.New("links are not supported in this spec version") + ErrGroupSupport = errors.New("groups are not supported in this spec version") + ErrUserFieldSupport = errors.New("fields other than \"name\" and \"ssh_authorized_keys\" are not supported in this spec version") + ErrUserNameSupport = errors.New("users other than \"core\" are not supported in this spec version") ) diff --git a/config/openshift/v4_8/translate.go b/config/openshift/v4_8/translate.go index 05155dab..8cbd9f28 100644 --- a/config/openshift/v4_8/translate.go +++ b/config/openshift/v4_8/translate.go @@ -212,6 +212,10 @@ func validateMCOSupport(mc result.MachineConfig, ts translate.TranslationSet) re // is provisioned, and the struct contains unsupported fields, MCD // will mark the node degraded, even if the change only affects // supported fields. We reject these. + // + // BUGGED - Ignored by the MCD but not by Ignition. Ignition + // correctly applies the setting, but the MCD doesn't, and writes + // incorrect state to the node. var r report.Report for i := range mc.Spec.Config.Storage.Directories { @@ -223,6 +227,11 @@ func validateMCOSupport(mc result.MachineConfig, ts translate.TranslationSet) re // FORBIDDEN r.AddOnError(path.New("json", "spec", "config", "storage", "files", i, "append"), common.ErrFileAppendSupport) } + if util.NotEmpty(file.Contents.Compression) { + // BUGGED + // https://bugzilla.redhat.com/show_bug.cgi?id=1970218 + r.AddOnError(path.New("json", "spec", "config", "storage", "files", i, "contents", "compression"), common.ErrFileCompressionSupport) + } } for i := range mc.Spec.Config.Storage.Links { // IMMUTABLE diff --git a/config/openshift/v4_8/translate_test.go b/config/openshift/v4_8/translate_test.go index 1c0c0a6e..ce4b1027 100644 --- a/config/openshift/v4_8/translate_test.go +++ b/config/openshift/v4_8/translate_test.go @@ -351,6 +351,10 @@ func TestValidateSupport(t *testing.T) { Inline: util.StrToPtr("z"), }, }, + Contents: base.Resource{ + Inline: util.StrToPtr("z"), + Compression: util.StrToPtr("gzip"), + }, }, }, Filesystems: []base.Filesystem{ @@ -408,6 +412,7 @@ func TestValidateSupport(t *testing.T) { {report.Error, common.ErrBtrfsSupport, path.New("yaml", "storage", "filesystems", 0, "format")}, {report.Error, common.ErrDirectorySupport, path.New("yaml", "storage", "directories", 0)}, {report.Error, common.ErrFileAppendSupport, path.New("yaml", "storage", "files", 1, "append")}, + {report.Error, common.ErrFileCompressionSupport, path.New("yaml", "storage", "files", 1, "contents", "compression")}, {report.Error, common.ErrLinkSupport, path.New("yaml", "storage", "links", 0)}, {report.Error, common.ErrGroupSupport, path.New("yaml", "passwd", "groups", 0)}, {report.Error, common.ErrUserFieldSupport, path.New("yaml", "passwd", "users", 0, "gecos")}, diff --git a/config/openshift/v4_9_exp/translate.go b/config/openshift/v4_9_exp/translate.go index af3428f8..11dcee83 100644 --- a/config/openshift/v4_9_exp/translate.go +++ b/config/openshift/v4_9_exp/translate.go @@ -212,6 +212,10 @@ func validateMCOSupport(mc result.MachineConfig, ts translate.TranslationSet) re // is provisioned, and the struct contains unsupported fields, MCD // will mark the node degraded, even if the change only affects // supported fields. We reject these. + // + // BUGGED - Ignored by the MCD but not by Ignition. Ignition + // correctly applies the setting, but the MCD doesn't, and writes + // incorrect state to the node. var r report.Report for i := range mc.Spec.Config.Storage.Directories { @@ -223,6 +227,11 @@ func validateMCOSupport(mc result.MachineConfig, ts translate.TranslationSet) re // FORBIDDEN r.AddOnError(path.New("json", "spec", "config", "storage", "files", i, "append"), common.ErrFileAppendSupport) } + if util.NotEmpty(file.Contents.Compression) { + // BUGGED + // https://bugzilla.redhat.com/show_bug.cgi?id=1970218 + r.AddOnError(path.New("json", "spec", "config", "storage", "files", i, "contents", "compression"), common.ErrFileCompressionSupport) + } } for i := range mc.Spec.Config.Storage.Links { // IMMUTABLE diff --git a/config/openshift/v4_9_exp/translate_test.go b/config/openshift/v4_9_exp/translate_test.go index 9f090d04..3e732ff7 100644 --- a/config/openshift/v4_9_exp/translate_test.go +++ b/config/openshift/v4_9_exp/translate_test.go @@ -351,6 +351,10 @@ func TestValidateSupport(t *testing.T) { Inline: util.StrToPtr("z"), }, }, + Contents: base.Resource{ + Inline: util.StrToPtr("z"), + Compression: util.StrToPtr("gzip"), + }, }, }, Filesystems: []base.Filesystem{ @@ -408,6 +412,7 @@ func TestValidateSupport(t *testing.T) { {report.Error, common.ErrBtrfsSupport, path.New("yaml", "storage", "filesystems", 0, "format")}, {report.Error, common.ErrDirectorySupport, path.New("yaml", "storage", "directories", 0)}, {report.Error, common.ErrFileAppendSupport, path.New("yaml", "storage", "files", 1, "append")}, + {report.Error, common.ErrFileCompressionSupport, path.New("yaml", "storage", "files", 1, "contents", "compression")}, {report.Error, common.ErrLinkSupport, path.New("yaml", "storage", "links", 0)}, {report.Error, common.ErrGroupSupport, path.New("yaml", "passwd", "groups", 0)}, {report.Error, common.ErrUserFieldSupport, path.New("yaml", "passwd", "users", 0, "gecos")}, From 023fdd41adbb4be52583fef6a4355ec96a6bfa5a Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 10 Jun 2021 02:10:56 -0400 Subject: [PATCH 2/2] config/openshift/*: disable automatic file compression The MCO ignores the compression field and writes out the file without decompressing it first. https://bugzilla.redhat.com/show_bug.cgi?id=1970218 --- config/openshift/v4_8/translate.go | 4 ++ config/openshift/v4_8/translate_test.go | 71 +++++++++++++++++++++ config/openshift/v4_9_exp/translate.go | 4 ++ config/openshift/v4_9_exp/translate_test.go | 71 +++++++++++++++++++++ 4 files changed, 150 insertions(+) diff --git a/config/openshift/v4_8/translate.go b/config/openshift/v4_8/translate.go index 8cbd9f28..86d57363 100644 --- a/config/openshift/v4_8/translate.go +++ b/config/openshift/v4_8/translate.go @@ -41,6 +41,10 @@ const ( // can be tracked back to their source in the source config. No config // validation is performed on input or output. func (c Config) ToMachineConfig4_8Unvalidated(options common.TranslateOptions) (result.MachineConfig, translate.TranslationSet, report.Report) { + // disable inline resource compression since the MCO doesn't support it + // https://bugzilla.redhat.com/show_bug.cgi?id=1970218 + options.NoResourceAutoCompression = true + cfg, ts, r := c.Config.ToIgn3_2Unvalidated(options) if r.IsFatal() { return result.MachineConfig{}, ts, r diff --git a/config/openshift/v4_8/translate_test.go b/config/openshift/v4_8/translate_test.go index ce4b1027..ac7b2ede 100644 --- a/config/openshift/v4_8/translate_test.go +++ b/config/openshift/v4_8/translate_test.go @@ -55,6 +55,7 @@ func TestElidedFieldWarning(t *testing.T) { } func TestTranslateConfig(t *testing.T) { + zzz := "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" tests := []struct { in Config out result.MachineConfig @@ -96,6 +97,76 @@ func TestTranslateConfig(t *testing.T) { {path.New("yaml", "version"), path.New("json", "spec", "config", "ignition", "version")}, }, }, + // ensure automatic compression is disabled + { + Config{ + Metadata: Metadata{ + Name: "z", + Labels: map[string]string{ + ROLE_LABEL_KEY: "z", + }, + }, + Config: fcos.Config{ + Config: base.Config{ + Storage: base.Storage{ + Files: []base.File{ + { + Path: "/z", + Contents: base.Resource{ + Inline: util.StrToPtr(zzz), + }, + }, + }, + }, + }, + }, + }, + result.MachineConfig{ + ApiVersion: result.MC_API_VERSION, + Kind: result.MC_KIND, + Metadata: result.Metadata{ + Name: "z", + Labels: map[string]string{ + ROLE_LABEL_KEY: "z", + }, + }, + Spec: result.Spec{ + Config: types.Config{ + Ignition: types.Ignition{ + Version: "3.2.0", + }, + Storage: types.Storage{ + Files: []types.File{ + { + Node: types.Node{ + Path: "/z", + }, + FileEmbedded1: types.FileEmbedded1{ + Contents: types.Resource{ + Source: util.StrToPtr("data:," + zzz), + }, + }, + }, + }, + }, + }, + }, + }, + []translate.Translation{ + {path.New("yaml", "version"), path.New("json", "apiVersion")}, + {path.New("yaml", "version"), path.New("json", "kind")}, + {path.New("yaml", "version"), path.New("json", "spec")}, + {path.New("yaml"), path.New("json", "spec", "config")}, + {path.New("yaml", "ignition"), path.New("json", "spec", "config", "ignition")}, + {path.New("yaml", "version"), path.New("json", "spec", "config", "ignition", "version")}, + {path.New("yaml", "storage"), path.New("json", "spec", "config", "storage")}, + {path.New("yaml", "storage", "files"), path.New("json", "spec", "config", "storage", "files")}, + {path.New("yaml", "storage", "files", 0), path.New("json", "spec", "config", "storage", "files", 0)}, + {path.New("yaml", "storage", "files", 0, "path"), path.New("json", "spec", "config", "storage", "files", 0, "path")}, + {path.New("yaml", "storage", "files", 0, "contents"), path.New("json", "spec", "config", "storage", "files", 0, "contents")}, + {path.New("yaml", "storage", "files", 0, "contents", "inline"), path.New("json", "spec", "config", "storage", "files", 0, "contents", "source")}, + }, + }, // FIPS { Config{ diff --git a/config/openshift/v4_9_exp/translate.go b/config/openshift/v4_9_exp/translate.go index 11dcee83..3f2825e4 100644 --- a/config/openshift/v4_9_exp/translate.go +++ b/config/openshift/v4_9_exp/translate.go @@ -41,6 +41,10 @@ const ( // can be tracked back to their source in the source config. No config // validation is performed on input or output. func (c Config) ToMachineConfig4_9Unvalidated(options common.TranslateOptions) (result.MachineConfig, translate.TranslationSet, report.Report) { + // disable inline resource compression since the MCO doesn't support it + // https://bugzilla.redhat.com/show_bug.cgi?id=1970218 + options.NoResourceAutoCompression = true + cfg, ts, r := c.Config.ToIgn3_3Unvalidated(options) if r.IsFatal() { return result.MachineConfig{}, ts, r diff --git a/config/openshift/v4_9_exp/translate_test.go b/config/openshift/v4_9_exp/translate_test.go index 3e732ff7..57e992b9 100644 --- a/config/openshift/v4_9_exp/translate_test.go +++ b/config/openshift/v4_9_exp/translate_test.go @@ -55,6 +55,7 @@ func TestElidedFieldWarning(t *testing.T) { } func TestTranslateConfig(t *testing.T) { + zzz := "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" tests := []struct { in Config out result.MachineConfig @@ -96,6 +97,76 @@ func TestTranslateConfig(t *testing.T) { {path.New("yaml", "version"), path.New("json", "spec", "config", "ignition", "version")}, }, }, + // ensure automatic compression is disabled + { + Config{ + Metadata: Metadata{ + Name: "z", + Labels: map[string]string{ + ROLE_LABEL_KEY: "z", + }, + }, + Config: fcos.Config{ + Config: base.Config{ + Storage: base.Storage{ + Files: []base.File{ + { + Path: "/z", + Contents: base.Resource{ + Inline: util.StrToPtr(zzz), + }, + }, + }, + }, + }, + }, + }, + result.MachineConfig{ + ApiVersion: result.MC_API_VERSION, + Kind: result.MC_KIND, + Metadata: result.Metadata{ + Name: "z", + Labels: map[string]string{ + ROLE_LABEL_KEY: "z", + }, + }, + Spec: result.Spec{ + Config: types.Config{ + Ignition: types.Ignition{ + Version: "3.3.0-experimental", + }, + Storage: types.Storage{ + Files: []types.File{ + { + Node: types.Node{ + Path: "/z", + }, + FileEmbedded1: types.FileEmbedded1{ + Contents: types.Resource{ + Source: util.StrToPtr("data:," + zzz), + }, + }, + }, + }, + }, + }, + }, + }, + []translate.Translation{ + {path.New("yaml", "version"), path.New("json", "apiVersion")}, + {path.New("yaml", "version"), path.New("json", "kind")}, + {path.New("yaml", "version"), path.New("json", "spec")}, + {path.New("yaml"), path.New("json", "spec", "config")}, + {path.New("yaml", "ignition"), path.New("json", "spec", "config", "ignition")}, + {path.New("yaml", "version"), path.New("json", "spec", "config", "ignition", "version")}, + {path.New("yaml", "storage"), path.New("json", "spec", "config", "storage")}, + {path.New("yaml", "storage", "files"), path.New("json", "spec", "config", "storage", "files")}, + {path.New("yaml", "storage", "files", 0), path.New("json", "spec", "config", "storage", "files", 0)}, + {path.New("yaml", "storage", "files", 0, "path"), path.New("json", "spec", "config", "storage", "files", 0, "path")}, + {path.New("yaml", "storage", "files", 0, "contents"), path.New("json", "spec", "config", "storage", "files", 0, "contents")}, + {path.New("yaml", "storage", "files", 0, "contents", "inline"), path.New("json", "spec", "config", "storage", "files", 0, "contents", "source")}, + }, + }, // FIPS { Config{