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.

Sign up for GitHub

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

Minor fixes and changes to warnings handling in option checkers #1086

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Minor fixes and changes to warnings handling in option checkers #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc1623a
distro/fedora: don't return early from FIPS check in checkOptions()
achilleas-k Dec 4, 2024
80adea9
distro/fedora: always return warnings from checkOptions()
achilleas-k Dec 4, 2024
811bc5f
distro/rhel: always return warnings from checkOptions()
achilleas-k Dec 4, 2024
db697f2
distro/rhel: don't print warnings in checkOptions()
achilleas-k Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 33 additions & 32 deletions pkg/distro/fedora/imagetype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,32 +290,34 @@ func (t *imageType) Manifest(bp *blueprint.Blueprint,
func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOptions) ([]string, error) {
customizations := bp.Customizations

var warnings []string

if !t.rpmOstree && options.OSTree != nil {
return nil, fmt.Errorf("OSTree is not supported for %q", t.Name())
return warnings, fmt.Errorf("OSTree is not supported for %q", t.Name())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow here (for this commit and for commit 811bc5f). The way I see it is that the go convention is that for cases like this return nil, err is the norm, adding the warnings here seems to be unnecessary. I read in the commit message that it is to guard against code-shuffling but looking at this case here the only way that warnings would get dropped is if we chnage the code from a) erroring in this case and b) still returning early. That seems quite unlikely (and would hopefully get caught in code reviews).

I guess there is a argument that it might be nice to show the user both the error and the warnings so that they can fix both but that interpretation does not matches the commit message. Maybe I'm missing something?

what exactly would this protect against?

Copy link
Member Author

@achilleas-k achilleas-k Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess there is a argument that it might be nice to show the user both the error and the warnings so that they can fix both

That's the idea, yeah.

but that interpretation does not matches the commit message. Maybe I'm missing something?

The commit message mentioning reshuffling is because I was expecting (or assuming) an argument that it doesn't make sense to return warnings before there's any reason for it to be set. Warnings only really get set pretty late in the function. I suppose a more complete message would say something like:

Always return warnings so the caller gets both warnings and error messages for
the customizations and options.  Return from the top even if no warnings could
possibly be generated to cover potential future additions or reordering.

Which makes me think, maybe we should be collecting all errors too and only returning them at the end, either as a list or Join()ed.

}

// we do not support embedding containers on ostree-derived images, only on commits themselves
if len(bp.Containers) > 0 && t.rpmOstree && (t.name != "iot-commit" && t.name != "iot-container") {
return nil, fmt.Errorf("embedding containers is not supported for %s on %s", t.name, t.arch.distro.name)
return warnings, fmt.Errorf("embedding containers is not supported for %s on %s", t.name, t.arch.distro.name)
}

if options.OSTree != nil {
if err := options.OSTree.Validate(); err != nil {
return nil, err
return warnings, err
}
}

if t.bootISO && t.rpmOstree {
// ostree-based ISOs require a URL from which to pull a payload commit
if options.OSTree == nil || options.OSTree.URL == "" {
return nil, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.name)
return warnings, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.name)
}
}

if t.name == "iot-raw-image" || t.name == "iot-qcow2-image" {
allowed := []string{"User", "Group", "Directories", "Files", "Services", "FIPS"}
if err := customizations.CheckAllowed(allowed...); err != nil {
return nil, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", "))
return warnings, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", "))
}
// TODO: consider additional checks, such as those in "edge-simplified-installer" in RHEL distros
}
Expand All @@ -326,16 +328,16 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp
if t.name == "iot-simplified-installer" {
allowed := []string{"InstallationDevice", "FDO", "Ignition", "Kernel", "User", "Group", "FIPS"}
if err := customizations.CheckAllowed(allowed...); err != nil {
return nil, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", "))
return warnings, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", "))
}
if customizations.GetInstallationDevice() == "" {
return nil, fmt.Errorf("boot ISO image type %q requires specifying an installation device to install to", t.name)
return warnings, fmt.Errorf("boot ISO image type %q requires specifying an installation device to install to", t.name)
}

// FDO is optional, but when specified has some restrictions
if customizations.GetFDO() != nil {
if customizations.GetFDO().ManufacturingServerURL == "" {
return nil, fmt.Errorf("boot ISO image type %q requires specifying FDO.ManufacturingServerURL configuration to install to when using FDO", t.name)
return warnings, fmt.Errorf("boot ISO image type %q requires specifying FDO.ManufacturingServerURL configuration to install to when using FDO", t.name)
}
var diunSet int
if customizations.GetFDO().DiunPubKeyHash != "" {
Expand All @@ -348,54 +350,54 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp
diunSet++
}
if diunSet != 1 {
return nil, fmt.Errorf("boot ISO image type %q requires specifying one of [FDO.DiunPubKeyHash,FDO.DiunPubKeyInsecure,FDO.DiunPubKeyRootCerts] configuration to install to when using FDO", t.name)
return warnings, fmt.Errorf("boot ISO image type %q requires specifying one of [FDO.DiunPubKeyHash,FDO.DiunPubKeyInsecure,FDO.DiunPubKeyRootCerts] configuration to install to when using FDO", t.name)
}
}

// ignition is optional, we might be using FDO
if customizations.GetIgnition() != nil {
if customizations.GetIgnition().Embedded != nil && customizations.GetIgnition().FirstBoot != nil {
return nil, fmt.Errorf("both ignition embedded and firstboot configurations found")
return warnings, fmt.Errorf("both ignition embedded and firstboot configurations found")
}
if customizations.GetIgnition().FirstBoot != nil && customizations.GetIgnition().FirstBoot.ProvisioningURL == "" {
return nil, fmt.Errorf("ignition.firstboot requires a provisioning url")
return warnings, fmt.Errorf("ignition.firstboot requires a provisioning url")
}
}
} else if t.name == "iot-installer" || t.name == "image-installer" {
// "Installer" is actually not allowed for image-installer right now, but this is checked at the end
allowed := []string{"User", "Group", "FIPS", "Installer", "Timezone", "Locale"}
if err := customizations.CheckAllowed(allowed...); err != nil {
return nil, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", "))
return warnings, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", "))
}
} else if t.name == "live-installer" {
allowed := []string{"Installer"}
if err := customizations.CheckAllowed(allowed...); err != nil {
return nil, fmt.Errorf(distro.NoCustomizationsAllowedError, t.name)
return warnings, fmt.Errorf(distro.NoCustomizationsAllowedError, t.name)
}
}
}

if kernelOpts := customizations.GetKernel(); kernelOpts.Append != "" && t.rpmOstree {
return nil, fmt.Errorf("kernel boot parameter customizations are not supported for ostree types")
return warnings, fmt.Errorf("kernel boot parameter customizations are not supported for ostree types")
}

mountpoints := customizations.GetFilesystems()
partitioning, err := customizations.GetPartitioning()
if err != nil {
return nil, err
return warnings, err
}
if (len(mountpoints) > 0 || partitioning != nil) && t.rpmOstree {
return nil, fmt.Errorf("Custom mountpoints and partitioning are not supported for ostree types")
return warnings, fmt.Errorf("Custom mountpoints and partitioning are not supported for ostree types")
}
if len(mountpoints) > 0 && partitioning != nil {
return nil, fmt.Errorf("partitioning customizations cannot be used with custom filesystems (mountpoints)")
return warnings, fmt.Errorf("partitioning customizations cannot be used with custom filesystems (mountpoints)")
}

if err := blueprint.CheckMountpointsPolicy(mountpoints, policies.MountpointPolicies); err != nil {
return nil, err
return warnings, err
}
if err := blueprint.CheckDiskMountpointsPolicy(partitioning, policies.MountpointPolicies); err != nil {
return nil, err
return warnings, err
}
if err := partitioning.ValidateLayoutConstraints(); err != nil {
return nil, err
Expand All @@ -404,13 +406,13 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp
if osc := customizations.GetOpenSCAP(); osc != nil {
supported := oscap.IsProfileAllowed(osc.ProfileID, oscapProfileAllowList)
if !supported {
return nil, fmt.Errorf("OpenSCAP unsupported profile: %s", osc.ProfileID)
return warnings, fmt.Errorf("OpenSCAP unsupported profile: %s", osc.ProfileID)
}
if t.rpmOstree {
return nil, fmt.Errorf("OpenSCAP customizations are not supported for ostree types")
return warnings, fmt.Errorf("OpenSCAP customizations are not supported for ostree types")
}
if osc.ProfileID == "" {
return nil, fmt.Errorf("OpenSCAP profile cannot be empty")
return warnings, fmt.Errorf("OpenSCAP profile cannot be empty")
}
}

Expand All @@ -420,7 +422,7 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp

err = blueprint.ValidateDirFileCustomizations(dc, fc)
if err != nil {
return nil, err
return warnings, err
}

dcp := policies.CustomDirectoriesPolicies
Expand All @@ -433,33 +435,32 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp

err = blueprint.CheckDirectoryCustomizationsPolicy(dc, dcp)
if err != nil {
return nil, err
return warnings, err
}

err = blueprint.CheckFileCustomizationsPolicy(fc, fcp)
if err != nil {
return nil, err
return warnings, err
}

// check if repository customizations are valid
_, err = customizations.GetRepositories()
if err != nil {
return nil, err
return warnings, err
}

if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() {
w := fmt.Sprintln(common.FIPSEnabledImageWarning)
return []string{w}, nil
warnings = append(warnings, fmt.Sprintln(common.FIPSEnabledImageWarning))
}

instCust, err := customizations.GetInstaller()
if err != nil {
return nil, err
return warnings, err
}
if instCust != nil {
// only supported by the Anaconda installer
if slices.Index([]string{"iot-installer"}, t.name) == -1 {
return nil, fmt.Errorf("installer customizations are not supported for %q", t.Name())
return warnings, fmt.Errorf("installer customizations are not supported for %q", t.Name())
}

// NOTE: the image type check is redundant with the check above, but
Expand All @@ -470,9 +471,9 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp
instCust.Kickstart != nil &&
len(instCust.Kickstart.Contents) > 0 &&
(customizations.GetUsers() != nil || customizations.GetGroups() != nil) {
return nil, fmt.Errorf("iot-installer installer.kickstart.contents are not supported in combination with users or groups")
return warnings, fmt.Errorf("iot-installer installer.kickstart.contents are not supported in combination with users or groups")
}
}

return nil, nil
return warnings, nil
}
5 changes: 1 addition & 4 deletions pkg/distro/rhel/rhel10/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package rhel10

import (
"fmt"
"log"

"slices"

Expand Down Expand Up @@ -91,9 +90,7 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima
}

if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() {
w := fmt.Sprintln(common.FIPSEnabledImageWarning)
log.Print(w)
warnings = append(warnings, w)
warnings = append(warnings, fmt.Sprintln(common.FIPSEnabledImageWarning))
}

instCust, err := customizations.GetInstaller()
Expand Down
14 changes: 4 additions & 10 deletions pkg/distro/rhel/rhel8/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package rhel8

import (
"fmt"
"log"
"strings"

"slices"
Expand All @@ -29,14 +28,14 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima

if options.OSTree != nil {
if err := options.OSTree.Validate(); err != nil {
return nil, err
return warnings, err
}
}

if t.BootISO && t.RPMOSTree {
// ostree-based ISOs require a URL from which to pull a payload commit
if options.OSTree == nil || options.OSTree.URL == "" {
return nil, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name())
return warnings, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name())
}

if t.Name() == "edge-simplified-installer" {
Expand Down Expand Up @@ -91,14 +90,10 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima
// TODO(edge): directly error if these options are provided when rhel-9.5's time arrives
if t.Name() == "edge-commit" || t.Name() == "edge-container" {
if customizations.GetUsers() != nil {
w := fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())
log.Print(w)
warnings = append(warnings, w)
warnings = append(warnings, fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()))
}
if customizations.GetGroups() != nil {
w := fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())
log.Print(w)
warnings = append(warnings, w)
warnings = append(warnings, fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()))
}
}

Expand Down Expand Up @@ -185,7 +180,6 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima

if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() {
w := fmt.Sprintln(common.FIPSEnabledImageWarning)
log.Print(w)
warnings = append(warnings, w)
}

Expand Down
17 changes: 5 additions & 12 deletions pkg/distro/rhel/rhel9/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package rhel9

import (
"fmt"
"log"
"strings"

"slices"
Expand Down Expand Up @@ -31,14 +30,14 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima

if options.OSTree != nil {
if err := options.OSTree.Validate(); err != nil {
return nil, err
return warnings, err
}
}

if t.BootISO && t.RPMOSTree {
// ostree-based ISOs require a URL from which to pull a payload commit
if options.OSTree == nil || options.OSTree.URL == "" {
return nil, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name())
return warnings, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name())
}

if t.Name() == "edge-simplified-installer" {
Expand Down Expand Up @@ -104,14 +103,10 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima
// TODO(edge): directly error if these options are provided when rhel-9.5's time arrives
if t.Name() == "edge-commit" || t.Name() == "edge-container" {
if customizations.GetUsers() != nil {
w := fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())
log.Print(w)
warnings = append(warnings, w)
warnings = append(warnings, fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()))
}
if customizations.GetGroups() != nil {
w := fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())
log.Print(w)
warnings = append(warnings, w)
warnings = append(warnings, fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()))
}
}

Expand Down Expand Up @@ -203,9 +198,7 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima
}

if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() {
w := fmt.Sprintln(common.FIPSEnabledImageWarning)
log.Print(w)
warnings = append(warnings, w)
warnings = append(warnings, fmt.Sprintln(common.FIPSEnabledImageWarning))
}

instCust, err := customizations.GetInstaller()
Expand Down
Loading