Closed simar7 closed 3 months ago
Are you going to just remove the subcommand? Don't we show a hint to existing users somehow? (e.g. https://github.com/aquasecurity/trivy-plugin-index/pull/8#issuecomment-2177850484)
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!
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?
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 ontrivy
to>=0.53.0
, there's going to be an error as it can no longer reference the appropriateCloudOptions
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!
Can do!
@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.
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?
Description
See discussion: https://github.com/aquasecurity/trivy/discussions/6818
Checklist