Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Adam0Brien
Copy link
Member

@Adam0Brien Adam0Brien commented Jul 3, 2023

Fix #275

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

@Adam0Brien Adam0Brien self-assigned this Jul 3, 2023
@Adam0Brien Adam0Brien force-pushed the issue-275 branch 3 times, most recently from 5c5e3d1 to 9e622f7 Compare July 3, 2023 16:09
@bgilbert
Copy link
Contributor

bgilbert commented Jul 3, 2023

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
Copy link
Member Author

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
Copy link
Contributor

bgilbert commented Jul 4, 2023

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.

@Adam0Brien 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 Adam0Brien force-pushed the issue-275 branch 5 times, most recently from 11a4472 to 7f78a08 Compare July 27, 2023 16:11
Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
base/v0_6_exp/validate.go Outdated Show resolved Hide resolved
base/v0_6_exp/validate.go Outdated Show resolved Hide resolved
base/v0_6_exp/validate.go Show resolved Hide resolved
base/v0_6_exp/validate.go Outdated Show resolved Hide resolved
base/v0_6_exp/validate.go Outdated Show resolved Hide resolved
@Adam0Brien Adam0Brien force-pushed the issue-275 branch 2 times, most recently from ec4e35c to 1198407 Compare August 10, 2023 12:46
@Adam0Brien Adam0Brien force-pushed the issue-275 branch 5 times, most recently from d1c334b to a6dcbda Compare August 23, 2023 13:05
@Adam0Brien Adam0Brien marked this pull request as ready for review August 23, 2023 13:40
@travier
Copy link
Member

travier commented Aug 23, 2023

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

@Adam0Brien
Copy link
Member Author

Adam0Brien commented Aug 23, 2023

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?

Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its getting really close, keep it up!

base/v0_6_exp/validate.go Outdated Show resolved Hide resolved
r.Merge(butaneReport)
}
if err != nil {
if err == common.ErrNoFilesDir {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return
}

func MapIgnitionReportToButane(ignitionReport report.Report) report.Report {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: wdyt about ConvertToButaneReport

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Adam0Brien Adam0Brien force-pushed the issue-275 branch 3 times, most recently from 986194b to bfdb07c Compare September 19, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate merged/replaced Ignition configs if they're local/inline
4 participants