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

validate image: Allow custom Rego variables with new --extra flag #1474

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

mbestavros
Copy link
Contributor

@mbestavros mbestavros commented Mar 26, 2024

This PR adds a new option, --extra, to the ec validate image command. This argument allows users to inject additional variables into the Rego rule data per source. Syntax is simple: --extra key=value. The flag can also be used multiple times.

This is an evolution of the idea that @lcarva proposed in #1277.

Here's an example output from ec validate image with --extra var=value:

policy:
  description: Includes rules for levels 1, 2 & 3 of SLSA v0.1. This is the default
    config used for new Konflux applications. Available collections are defined in
    https://redhat-appstudio.github.io/docs.stonesoup.io/ec-policies/release_policy.html#_available_rule_collections.
    If a different policy configuration is desired, this resource can serve as a starting
    point. See the docs on how to include and exclude rules https://redhat-appstudio.github.io/docs.stonesoup.io/ec-policies/policy_configuration.html#_including_and_excluding_rules.
  name: Default
  publicKey: /dev/fd/63
  sources:
  - config:
      include:
      - '@slsa3'
    data:
    - oci::quay.io/redhat-appstudio-tekton-catalog/data-acceptable-bundles:latest
    - github.com/release-engineering/rhtap-ec-policy//data
    name: Default
    policy:
    - github.com/enterprise-contract/ec-policies//policy/lib
    - github.com/enterprise-contract/ec-policies//policy/release
    ruleData:
      var: value
success: true

This still needs tests added, but it appears to be functional, as seen above.

@mbestavros mbestavros changed the title WIP: validate image: Allow custom Rego variables with new --extra flag WIP: validate image: Allow custom Rego variables with new --extra flag Mar 26, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 80.59%. Comparing base (934c6e6) to head (61499d0).
Report is 19 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1474      +/-   ##
==========================================
+ Coverage   80.46%   80.59%   +0.12%     
==========================================
  Files          67       66       -1     
  Lines        4782     4761      -21     
==========================================
- Hits         3848     3837      -11     
+ Misses        934      924      -10     
Flag Coverage Δ
generative 80.59% <86.66%> (+0.12%) ⬆️
integration 80.59% <86.66%> (+0.12%) ⬆️
unit 80.59% <86.66%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/validate/image.go 96.26% <86.66%> (ø)

... and 1 file with indirect coverage changes

@@ -393,6 +444,10 @@ func validateImageCmd(validate imageValidationFunc) *cobra.Command {
a RFC3339 formatted value, e.g. 2022-11-18T00:00:00Z.
`))

cmd.Flags().StringSliceVar(&data.extra, "extra", data.extra, hd.Doc(`
Copy link
Member

Choose a reason for hiding this comment

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

The name "extra" feels not quite specific enough. Something like this perhaps:

Suggested change
cmd.Flags().StringSliceVar(&data.extra, "extra", data.extra, hd.Doc(`
cmd.Flags().StringSliceVarP(&data.extraRuleData, "extra-rule-data", "e", data.extraRuleData, hd.Doc(`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to your suggestion, but open to other ideas as well. I know we floated override or override-rule-data as well.

@simonbaird
Copy link
Member

Let's get some test coverage.

@simonbaird simonbaird closed this Mar 26, 2024
@simonbaird
Copy link
Member

(Wrong button sorry..)

@simonbaird simonbaird reopened this Mar 26, 2024
@mbestavros mbestavros force-pushed the cli-data branch 3 times, most recently from 75cb590 to 3956707 Compare March 28, 2024 14:26
@mbestavros mbestavros force-pushed the cli-data branch 2 times, most recently from 06702c1 to 634af83 Compare April 5, 2024 17:10
@mbestavros
Copy link
Contributor Author

@simonbaird I added a unit and acceptance test for the positive case. Had to get a bit hacky for the unit test to work correctly.

@mbestavros mbestavros force-pushed the cli-data branch 8 times, most recently from 2f0a2f8 to ab1c07e Compare April 9, 2024 18:26
@@ -673,6 +674,104 @@ configuration:
assert.NoError(t, err)
}

func Test_ValidateImageCommandExtraData(t *testing.T) {
validate := func(_ context.Context, component app.SnapshotComponent, _ policy.Policy, _ bool) (*output.Output, error) {
Copy link
Member

Choose a reason for hiding this comment

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

There is a compile error on this line:

Suggested change
validate := func(_ context.Context, component app.SnapshotComponent, _ policy.Policy, _ bool) (*output.Output, error) {
validate := func(_ context.Context, component app.SnapshotComponent, _ policy.Policy, _ []evaluator.Evaluator, _ bool) (*output.Output, error) {

Copy link
Member

Choose a reason for hiding this comment

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

It looks like @mbestavros did a rebase that brought in the change in the signature for the validate function. I have questions...

  • The only CI check failing right now is the linter. How did "Checks / Test" pass?!
  • Why was the linter complaining about this line before when it was obviously correct at the time?

Signed-off-by: Mark Bestavros <[email protected]>
@mbestavros mbestavros changed the title WIP: validate image: Allow custom Rego variables with new --extra flag validate image: Allow custom Rego variables with new --extra flag Apr 25, 2024
@mbestavros mbestavros marked this pull request as ready for review April 25, 2024 14:36
@zregvart zregvart merged commit d534c4c into enterprise-contract:main Apr 26, 2024
12 checks passed
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.

4 participants