feat(aws)!: Remove aws subcommand

[simar7]

Description

See discussion: #6818

Checklist

• I've read the guidelines for contributing to this repository.
• I've followed the conventions in the PR title.
• I've added tests that prove my fix is effective or that my feature works.
• I've updated the documentation with the relevant information (if needed).
• I've added usage information (if the PR introduces new options)
• I've included a "before" and "after" example to the description (if the PR is a user interface change).

[knqyf263]

Are you going to just remove the subcommand? Don't we show a hint to existing users somehow? (e.g. aquasecurity/trivy-plugin-index#8 (comment))

[simar7]

Are you going to just remove the subcommand? Don't we show a hint to existing users somehow? (e.g. aquasecurity/trivy-plugin-index#8 (comment))

Added!

[k-le]

Hey, sorry for reviving this old PR.

I noticed that, with the removal of the subcommand and the flags, if/when the trivy-aws module/plugin upgrades its dependency on trivy to >=0.53.0, there's going to be an error as it can no longer reference the appropriate CloudOptions flags.

As a workaround, we've added the flags back in and have kept the aws subcommand removed.

However, I wanted to bring this up as it's an issue in its current state. Do we have any objections to adding back the CloudOptions options back?

[simar7]

Hey, sorry for reviving this old PR.

I noticed that, with the removal of the subcommand and the flags, if/when the trivy-aws module/plugin upgrades its dependency on trivy to >=0.53.0, there's going to be an error as it can no longer reference the appropriate CloudOptions flags.

As a workaround, we've added the flags back in and have kept the aws subcommand removed.

However, I wanted to bring this up as it's an issue in its current state. Do we have any objections to adding back the CloudOptions options back?

hi @k-le - good point! I just did a quick search and didn't find any other usages of the CloudOptions option in Trivy. I think in this case, it might be better to add this to the trivy-aws repo itself as it's the only user and update the version of Trivy. Would you mind opening a PR in the trivy-aws repo in this case? Thanks!

[k-le]

Can do!

[k-le]

@simar7 I'm taking a look at adding CloudOptions to just trivy-aws and (granted, I'm inexperienced with this codebase) I'm struggling to see a clean way of adding the option without embedding the Flags and Options structs from trivy into a new Flags and Options structs in trivy-aws and having things be a little more convoluted.

Here's an example of the approach that I'm taking:

Reintroduce the CloudFlagGroup and CloudOptions structs:

// trivy-aws/pkg/flag/cloud_flags.go

package flag

import (
	trivyFlag "github.com/aquasecurity/trivy/pkg/flag"
	"golang.org/x/xerrors"
	"time"
)

var (
	cloudUpdateCacheFlag = trivyFlag.Flag[bool]{
		Name:       "update-cache",
		ConfigName: "cloud.update-cache",
		Usage:      "Update the cache for the applicable cloud provider instead of using cached results.",
	}
	cloudMaxCacheAgeFlag = trivyFlag.Flag[time.Duration]{
		Name:       "max-cache-age",
		ConfigName: "cloud.max-cache-age",
		Default:    time.Hour * 24,
		Usage:      "The maximum age of the cloud cache. Cached data will be required from the cloud provider if it is older than this.",
	}
)

type CloudFlagGroup struct {
	UpdateCache *trivyFlag.Flag[bool]
	MaxCacheAge *trivyFlag.Flag[time.Duration]
}

type CloudOptions struct {
	MaxCacheAge time.Duration
	UpdateCache bool
}

func NewCloudFlagGroup() *CloudFlagGroup {
	return &CloudFlagGroup{
		UpdateCache: cloudUpdateCacheFlag.Clone(),
		MaxCacheAge: cloudMaxCacheAgeFlag.Clone(),
	}
}

func (f *CloudFlagGroup) Name() string {
	return "Cloud"
}

func (f *CloudFlagGroup) Flags() []trivyFlag.Flagger {
	return []trivyFlag.Flagger{
		f.UpdateCache,
		f.MaxCacheAge,
	}
}

func (f *CloudFlagGroup) ToOptions() (CloudOptions, error) {
	if err := parseFlags(f); err != nil {
		return CloudOptions{}, err
	}
	return CloudOptions{
		UpdateCache: f.UpdateCache.Value(),
		MaxCacheAge: f.MaxCacheAge.Value(),
	}, nil
}

func parseFlags(fg trivyFlag.FlagGroup) error {
	for _, flag := range fg.Flags() {
		if err := flag.Parse(); err != nil {
			return xerrors.Errorf("unable to parse flag: %w", err)
		}
	}
	return nil
}

Create new Flags and Options structs that embed the trivy variant:

// trivy-aws/pkg/flag/options.go

package flag

import (
	trivyFlag "github.com/aquasecurity/trivy/pkg/flag"
	"github.com/spf13/cobra"
	"golang.org/x/xerrors"
)

type Flags struct {
	BaseFlags      trivyFlag.Flags
	CloudFlagGroup *CloudFlagGroup
}

type Options struct {
	BaseOptions trivyFlag.Options
	CloudOptions
}

func (f *Flags) Bind(cmd *cobra.Command) error {
	err := f.BaseFlags.Bind(cmd)
	if err != nil {
		return xerrors.Errorf("%w", err)
	}

	return nil
}

func (f *Flags) ToOptions(args []string) (Options, error) {
	baseOptions, err := f.BaseFlags.ToOptions(args)
	if err != nil {
		return Options{}, xerrors.Errorf("%w", err)
	}

	opts := Options{
		BaseOptions: baseOptions,
	}
	if f.CloudFlagGroup != nil {
		opts.CloudOptions, err = f.CloudFlagGroup.ToOptions()
		if err != nil {
			return Options{}, xerrors.Errorf("cloud flag error: %w", err)
		}
	}

	return opts, nil
}

Now, this approach leads to a handful of changes where, in trivy-aws, to reference any flag group from trivy, it'd be:

package commands

import (
	"github.com/aquasecurity/trivy-aws/pkg/flag"

	trivyFlag "github.com/aquasecurity/trivy/pkg/flag"
)

func example() {
    globalFlags := trivyFlag.NewGlobalFlagGroup()
    awsFlags := &flag.Flags{
        BaseFlags: trivyFlag.Flags{
            GlobalFlagGroup: globalFlags,
        },
        CloudFlagGroup: flag.NewCloudFlagGroup(),
    }

    opts, err := awsFlags.ToOptions(args)
    if err != nil { ... }
    if opts.BaseOptions.Timeout < time.Hour {
        // some logic...
    }
}

I agree with you that, conceptually, since CloudOptions are only used by trivy-aws that it could belong there, but to maintain simplicity and the current design, it might be better to have CloudOptions reintroduced to trivy.

I'm open to other ideas and suggestions, please let me know! I'd love to learn.

[simar7]

hi @k-le - to me that your approach looks reasonable, what was your concern? In essence, all plugins are free to do extend their functionality the way it's appropriate for them and in this case it sounds like adding a few new options within the plugin itself should be reasonable.

@nikpivkin do you have any ideas on how to approach this besides what @k-le already mentioned?