-
Notifications
You must be signed in to change notification settings - Fork 50
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
OpenSCAP tailoring: add key/value rule overrides #300
Conversation
3e15780
to
43959a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I added a few comments / ideas inline.
Refactor the way in which the default datastream is obtained. This will make it easier to generalise and simplify the creation of the OpenSCAP remediation & tailoring stage options. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Extract the creation of the required OpenSCAP directories into the OpenSCAP package. Simplify the directory creation into a single function which returns all the required directories for both the remediation and tailoring stages. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Generalise the tailoring stage options so it can be re-used by each of the distros. Before this, there was a lot of repetition for each distro when creating the stage options. This simplification also allowed some decoupling from the remediation stage options. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Generalise the creation of the remediation stage options and extract the function to the `oscap` package. This function also slightly reduces the tight coupling with the `tailoring` stage options. However, this stage still depends on some of the config from the `tailoring` stage. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Expose a field in the blueprints to be able to override rule values using the OpenSCAP autotailor feature. The customization exposes an array with two values, the name of the var to overwrite and the value to write to the var. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Expose the overrides field in the OpenSCAP autotailor stage. This parses the input and adds the relevant fields to the osbuild json schema. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Enable override rules for the `tailoring` stage. These rules allow users to specify a key/value pair for rule values that they would like to override. Jira: https://issues.redhat.com/browse/COMPOSER-1994
Update the OpenSCAP tests with the autotailor overrides Jira: https://issues.redhat.com/browse/COMPOSER-1994
43959a9
to
3b6b310
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice rework (plus of course the added support for override values), I like it!
I added a few comments, mostly of a cosmetic nature. On top of that, I think that moving all functions that generate osbuild stage options should be moved to the osbuild
package.
} | ||
return defaultCentos8Datastream | ||
|
||
return defaultRHELDatastream(d.Releasever()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, not sure if of any value: Would it make sense to check also rhel
as the prefix and maybe panic()
in the default case when the distro is not explicitly supported?
// although the osbuild stage will create this directory, | ||
// it's probably better to ensure that it is created here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to double-check. Because the directory is technically not created here, but in the OS pipeline implementation which generates the osbuild pipeline. Directories are created before the OSCAP stage is added to the pipeline, so this is all fine in the end.
return fmt.Sprintf("%s_osbuild_tailoring", profileID) | ||
} | ||
|
||
func CreateTailoringStageOptions(oscapConfig *blueprint.OpenSCAPCustomization, d distro.Distro) *osbuild.OscapAutotailorStageOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it would make more sense to move this to the osbuild
package, instead of the oscap
. We usually generate osbuild stage options from our internal abstractions in the osbuild
package.
Lastly, a constructor function for stage options would IMHO feel more natural in the osbuild
package.
pkg/distro/fedora/images.go
Outdated
t.arch.distro, | ||
) | ||
|
||
if tailorConfig := osc.OpenSCAPTailorConfig; tailorConfig != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only to make the following two lines shorter? I must admit that I had to stop for a second to carefully read the code and understand that you are assigning a variable to another variable just to check if it is nil
...
@@ -64,3 +64,38 @@ func CreateTailoringStageOptions(oscapConfig *blueprint.OpenSCAPCustomization, d | |||
}, | |||
) | |||
} | |||
|
|||
func CreateRemediationStageOptions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this should IMO go into the osbuild
package.
if isOSTree { | ||
return nil, nil, fmt.Errorf("unexpected oscap options for ostree image type") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is IMO not the place to check if the image type is ostree or not. This is not the job if a function which generates an osbuild stage options. Instead, this should be checked on a higher-lervel in the distro implementation and we should return an error there in case one wants to use BP customizations with an image type that does not support them.
if len(directories) > 0 { | ||
osc.Directories = append(osc.Directories, directories...) | ||
} | ||
var directories []*fsnode.Directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: consider renaming this to something like oscapDirectories
to make it obvious what the variable is used for.
case string: | ||
ot.Var = d["var"].(string) | ||
default: | ||
return fmt.Errorf("TOML unmarshal: override var must be string, got %v of type %T", d["var"], d["var"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: you could use this instead of specifying the same value twice:
return fmt.Errorf("TOML unmarshal: override var must be string, got %[1]v of type %[1]T", d["var"])
and basically in all similar cases in this file.
continue | ||
} | ||
|
||
return fmt.Errorf("override 'value' must be an integere or a string") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to also include the name of the override value in the error?
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 30+7 days with no activity. |
Expose a field in the blueprints to be able to override rule values
using the OpenSCAP autotailor feature. The customization exposes an
array with two values, the name of the var to overwrite and the value
to write to the var.
This depends on osbuild/osbuild#1407
Jira: https://issues.redhat.com/browse/COMPOSER-1994