base/v0_6_exp: Validate merged/replaced Ignition configs #476

Adam0Brien · 2023-07-03T14:47:28Z

This PR introduces validation for local/inline fields when using the merge/replace feature in butane.

For this PR ignitions Parse function needs to be used so the vendor folder is has been updated to accommodate this

bgilbert · 2023-07-03T18:53:00Z

The file extension doesn't matter. There's no requirement that Ignition configs have an .ign extension. And, in the case of the inline directive, there isn't any file extension; the file contents are included directly in the parent config.

As #275 says, what we should do instead is validate the contents of the specified config using Ignition config validation.

Adam0Brien · 2023-07-04T12:58:15Z

As #275 says, what we should do instead is validate the contents of the specified config using Ignition config validation.

by ignition config validation do you mean https://github.com/coreos/butane/blob/main/vendor/github.com/coreos/ignition/v2/config/v3_5_experimental/types/resource.go#L34 ?

bgilbert · 2023-07-04T20:48:31Z

That's a validator that's invoked by the validation process. We need to run the validation process.

But thinking about it some more, we need to do more than just validation. The embedded Ignition config is in the form of text, not Go structs, so we also need to parse it. Ignition's config parsing function in config handles both parsing and validation, so we can just call that, and merge its report into ours.

A few notes:

The parse function sometimes returns an error alongside an empty report. We should probably AddOnError the returned error to our validation report in addition to merging Ignition's report.
Merging Ignition's report is a bit tricky. We'd need to replace all the path fields with the path to the Butane field holding the config, which will be something like $.ignition.config.merge.0.inline. That loses information but I'm not sure there's a good way around it. We could try replacing the Ignition error in the log entry with a new one that includes the original path and the original message.
The user is allowed to use older versions of Butane that don't understand the current Ignition spec, and is allowed to embed Ignition configs with versions newer than Butane understands. We should have special handling for that case: Validate merged/replaced Ignition configs if they're local/inline #275 (comment)
This validation isn't FCOS-specific; it applies to all variants. So, it should be in base. And since we're only going to be producing errors for configs that were already invalid (but would be rejected when the machine boots rather than when Butane runs) we should backport it to stable base specs.

prestist

Hey, thank you for working on this I just took a first pass at this.

I found some surface level questions for you.

I am planning on taking another deeper pass so see why your CI is failing.

prestist · 2023-07-27T18:35:28Z

base/v0_6_exp/validate.go

@@ -29,10 +30,22 @@ func (rs Resource) Validate(c path.ContextPath) (r report.Report) {
 	if rs.Local != nil {
 		sources++
 		field = "local"
+		_, report, err := exp.ParseCompatibleVersion([]byte(*rs.Local))
+		if err != nil {
+			r.AddOnError(c.Append("ignition", "config", "merge", "local"), common.ErrTooManyResourceSources)


So is ErrTooManyResourceSources the only error that is relevant to ParseCompatibleVersion? I ask because if not, we losing valuable debug info for the user.

Additionally, in the comment #275 (comment)
there was talk about catching ErrUnknownVersion and instead of returning the error just warning in this case to work around ratcheting when butane / Ignition are undergoing stabilization.

base/v0_6_exp/validate.go

travier · 2023-08-23T14:37:15Z

Let's split the vendor update into its own commit to make reviewing the change easier.

Adam0Brien · 2023-08-23T15:03:16Z

Let's split the vendor update into its own commit to make reviewing the change easier.

That's no problem!, but wont that break CI since validate.go wont be able to compile the parse function?

prestist

Its getting really close, keep it up!

base/v0_6_exp/validate.go

prestist · 2023-08-24T19:41:05Z

base/v0_6_exp/validate.go

+				r.Merge(butaneReport)
+			}
+			if err != nil {
+				if err == common.ErrNoFilesDir {


So I think we want to be returning any errors other than the special case. The special case in this instance is ErrUnknownVersion => #275 (comment)

prestist · 2023-08-24T19:45:24Z

base/v0_6_exp/validate.go

 	return
 }

+func MapIgnitionReportToButane(ignitionReport report.Report) report.Report {


Nit: wdyt about ConvertToButaneReport

I think in a perfect world we might make this a helper function ..

i.e func(report.report) toButane() report.Report {}

However def not scope for this PR as I am not sure where I would want to put that type of a utility.

prestist · 2023-08-24T20:36:12Z

base/v0_6_exp/validate.go

+func MapIgnitionReportToButane(ignitionReport report.Report) report.Report {
+	var butaneRep report.Report
+	for _, entry := range ignitionReport.Entries {
+		butaneEntry := report.Entry{


Im not sure this is everything we need.
Looping through the entries and expanding them into a new report object with those entries is good.
The issue comes with the the entry.Context path the paths are different between the ignition package and the butane package. this is mostly because we are converting from json to yaml butane is human readable so their names change.

internal to butane it looks like we already have a similar issue, we have to translate to json from yaml the direction is the inverse of what we need now though.

It has a good test to explain this here => https://github.com/coreos/butane/blob/0244d31c627890b9af64b82e2a2224ac39bbb4d4/base/v0_6_exp/translate_test.go#L573C12-L573C12

…ources

Adam0Brien self-assigned this Jul 3, 2023

Adam0Brien force-pushed the issue-275 branch 3 times, most recently from 5c5e3d1 to 9e622f7 Compare July 3, 2023 16:09

Adam0Brien changed the title ~~fcos/v1_6_exp: Fail if user provides invalid file type~~ base/v0_6_exp: Validate merged/replaced Ignition configs Jul 27, 2023

Adam0Brien force-pushed the issue-275 branch 5 times, most recently from 11a4472 to 7f78a08 Compare July 27, 2023 16:11

Adam0Brien force-pushed the issue-275 branch from 7f78a08 to 15ef46c Compare August 8, 2023 12:57

prestist reviewed Aug 9, 2023

View reviewed changes

Adam0Brien force-pushed the issue-275 branch 2 times, most recently from ec4e35c to 1198407 Compare August 10, 2023 12:46

Adam0Brien force-pushed the issue-275 branch 5 times, most recently from d1c334b to a6dcbda Compare August 23, 2023 13:05

Adam0Brien marked this pull request as ready for review August 23, 2023 13:40

Adam0Brien force-pushed the issue-275 branch from a6dcbda to c6358d1 Compare August 23, 2023 13:44

prestist reviewed Aug 24, 2023

View reviewed changes

Adam0Brien force-pushed the issue-275 branch 3 times, most recently from 986194b to bfdb07c Compare September 19, 2023 07:01

Adam0Brien force-pushed the issue-275 branch from bfdb07c to 8ee89ab Compare September 19, 2023 08:32

base/v0_6_exp/validate.go: Add Validation when merging local/inline s…

77a0c18

…ources

Adam0Brien force-pushed the issue-275 branch from 8ee89ab to 77a0c18 Compare June 19, 2024 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

base/v0_6_exp: Validate merged/replaced Ignition configs #476

base/v0_6_exp: Validate merged/replaced Ignition configs #476

Adam0Brien commented Jul 3, 2023 •

edited

Loading

bgilbert commented Jul 3, 2023

Adam0Brien commented Jul 4, 2023

bgilbert commented Jul 4, 2023

prestist left a comment

prestist Jul 27, 2023

travier commented Aug 23, 2023 •

edited

Loading

Adam0Brien commented Aug 23, 2023 •

edited

Loading

prestist left a comment

prestist Aug 24, 2023

prestist Aug 24, 2023

prestist Aug 24, 2023

prestist Aug 24, 2023

base/v0_6_exp: Validate merged/replaced Ignition configs #476

Are you sure you want to change the base?

base/v0_6_exp: Validate merged/replaced Ignition configs #476

Conversation

Adam0Brien commented Jul 3, 2023 • edited Loading

bgilbert commented Jul 3, 2023

Adam0Brien commented Jul 4, 2023

bgilbert commented Jul 4, 2023

prestist left a comment

Choose a reason for hiding this comment

prestist Jul 27, 2023

Choose a reason for hiding this comment

travier commented Aug 23, 2023 • edited Loading

Adam0Brien commented Aug 23, 2023 • edited Loading

prestist left a comment

Choose a reason for hiding this comment

prestist Aug 24, 2023

Choose a reason for hiding this comment

prestist Aug 24, 2023

Choose a reason for hiding this comment

prestist Aug 24, 2023

Choose a reason for hiding this comment

prestist Aug 24, 2023

Choose a reason for hiding this comment

Adam0Brien commented Jul 3, 2023 •

edited

Loading

travier commented Aug 23, 2023 •

edited

Loading

Adam0Brien commented Aug 23, 2023 •

edited

Loading