From c6126273ae90ca6a10c0998261685ce79a5f3cc4 Mon Sep 17 00:00:00 2001 From: Steven Rhodes Date: Thu, 14 Sep 2023 00:25:52 -0700 Subject: [PATCH] Improve some handling around packages - Add a mode for `packages list` that gives a format closer to what we use elsewhere in sansshell - Validate that the package we wanted to install was actually installed in both `InstallPackage` and `UpdatePackage` - Print out flags on some usage errors - Automatically use `yum downgrade` instead of `update-to` when `UpdatePackage` is called to update a package to an older version Interestingly, the behavior of `yum install-nevra` semms to differ between centos7 and fedora. On centos7 I see it refusing to downgrade and on fedora I see it installing the exact specified version. --- services/packages/client/client.go | 62 ++++++++++----- services/packages/server/packages.go | 97 ++++++++++++++++++++++- services/packages/server/packages_test.go | 63 +++++++++++++-- 3 files changed, 189 insertions(+), 33 deletions(-) diff --git a/services/packages/client/client.go b/services/packages/client/client.go index 2a8af747..a8cc5833 100644 --- a/services/packages/client/client.go +++ b/services/packages/client/client.go @@ -118,7 +118,8 @@ func (i *installCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...inter if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, i) + return subcommands.ExitUsageError } if i.name == "" || i.version == "" { fmt.Fprintln(os.Stderr, "Both --name and --version must be filled in") @@ -186,7 +187,8 @@ func (r *removeCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, r) + return subcommands.ExitUsageError } if r.name == "" || r.version == "" { fmt.Fprintln(os.Stderr, "Both --name and --version must be filled in") @@ -253,7 +255,8 @@ func (u *updateCmd) SetFlags(f *flag.FlagSet) { func (u *updateCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus { if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, u) + return subcommands.ExitUsageError } if u.name == "" || u.oldVersion == "" || u.newVersion == "" { fmt.Fprintln(os.Stderr, "--name, --old_version and --new_version must be supplied") @@ -301,24 +304,27 @@ func (u *updateCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf type listCmd struct { packageSystem string + nevra bool } func (*listCmd) Name() string { return "list" } func (*listCmd) Synopsis() string { return "List installed packages" } func (*listCmd) Usage() string { - return `list [--package_system=P]: + return `list [--package_system=P] [--nevra]: List the installed packages on the remote machine. ` } func (l *listCmd) SetFlags(f *flag.FlagSet) { f.StringVar(&l.packageSystem, "package-system", "YUM", fmt.Sprintf("Package system to use(one of: [%s])", strings.Join(shortPackageSystemNames(), ","))) + f.BoolVar(&l.nevra, "nevra", false, "For YUM, print output in NEVRA format instead of trying to imitate `yum list-installed`") } func (l *listCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus { if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, l) + return subcommands.ExitUsageError } ps, err := flagToType(l.packageSystem) if err != nil { @@ -347,21 +353,32 @@ func (l *listCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interfac retCode = subcommands.ExitFailure continue } - fmt.Fprint(state.Out[r.Index], "Installed Packages\n") - for _, pkg := range r.Resp.Packages { - // Print the package name.arch, [epoch]:version-release and repo with some reasonable spacing. - na := pkg.Name - if pkg.Architecture != "" { - na = na + "." + pkg.Architecture - } - evr := pkg.Version - if pkg.Release != "" { - evr = evr + "-" + pkg.Release + if l.nevra { + for _, pkg := range r.Resp.Packages { + ne := pkg.Name + if pkg.Epoch != 0 { + ne = fmt.Sprintf("%s-%d", pkg.Name, pkg.Epoch) + } + nevra := fmt.Sprintf("%v:%v-%v.%v", ne, pkg.Version, pkg.Release, pkg.Architecture) + fmt.Fprintln(state.Out[r.Index], nevra) } - if pkg.Epoch != 0 { - evr = fmt.Sprintf("%d:%s", pkg.Epoch, evr) + } else { + fmt.Fprint(state.Out[r.Index], "Installed Packages\n") + for _, pkg := range r.Resp.Packages { + // Print the package name.arch, [epoch]:version-release and repo with some reasonable spacing. + na := pkg.Name + if pkg.Architecture != "" { + na = na + "." + pkg.Architecture + } + evr := pkg.Version + if pkg.Release != "" { + evr = evr + "-" + pkg.Release + } + if pkg.Epoch != 0 { + evr = fmt.Sprintf("%d:%s", pkg.Epoch, evr) + } + fmt.Fprintf(state.Out[r.Index], "%40s %16s %32s\n", na, evr, pkg.Repo) } - fmt.Fprintf(state.Out[r.Index], "%40s %16s %32s\n", na, evr, pkg.Repo) } } return retCode @@ -393,7 +410,8 @@ func (l *searchCmd) SetFlags(f *flag.FlagSet) { func (l *searchCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus { if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, l) + return subcommands.ExitUsageError } if l.name == "" { fmt.Fprintln(os.Stderr, "--name must be supplied") @@ -481,7 +499,8 @@ func (r *repoListCmd) SetFlags(f *flag.FlagSet) { func (r *repoListCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus { if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, r) + return subcommands.ExitUsageError } ps, err := flagToType(r.packageSystem) if err != nil { @@ -562,7 +581,8 @@ func (r *cleanupCmd) SetFlags(f *flag.FlagSet) { func (r *cleanupCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus { if f.NArg() != 0 { fmt.Fprintln(os.Stderr, "All options are set via flags") - return subcommands.ExitFailure + subcommands.DefaultCommander.ExplainCommand(os.Stderr, r) + return subcommands.ExitUsageError } ps, err := flagToType(r.packageSystem) if err != nil { diff --git a/services/packages/server/packages.go b/services/packages/server/packages.go index db09795e..e858a214 100644 --- a/services/packages/server/packages.go +++ b/services/packages/server/packages.go @@ -122,18 +122,18 @@ var ( return genCmd(p.PackageSystem, removeOpts) } - generateValidate = func(p *pb.UpdateRequest) ([]string, error) { + generateValidate = func(packageSystem pb.PackageSystem, name, version string) ([]string, error) { validateOpts := map[pb.PackageSystem][]string{ pb.PackageSystem_PACKAGE_SYSTEM_YUM: { "list", "installed", }, } - out, err := genCmd(p.PackageSystem, validateOpts) + out, err := genCmd(packageSystem, validateOpts) if err != nil { return nil, err } - return addRepoAndPackage(out, p.PackageSystem, p.Name, p.OldVersion, &repoData{}), nil + return addRepoAndPackage(out, packageSystem, name, version, &repoData{}), nil } generateUpdate = func(p *pb.UpdateRequest) ([]string, error) { @@ -143,6 +143,9 @@ var ( "-y", }, } + if isOlderVersion(p.NewVersion, p.OldVersion) { + updateOpts[pb.PackageSystem_PACKAGE_SYSTEM_YUM] = []string{"downgrade", "-y"} + } out, err := genCmd(p.PackageSystem, updateOpts) if err != nil { return nil, err @@ -225,6 +228,68 @@ var ( } ) +var versionExtractorRe = regexp.MustCompile("") + +func parseToNumbers(version string) []int { + out := make([]int, 5) + + // Grab epoch + split := strings.SplitN(version, ":", 2) + if len(split) == 2 { + epoch, err := strconv.Atoi(split[0]) + if err != nil { + return out + } + out[0] = epoch + version = split[1] + } + + // Grab version numbers + split = strings.SplitN(version, "-", 2) + if len(split) < 2 { + return out + } + for i, n := range strings.SplitN(split[0], ".", 3) { + v, err := strconv.Atoi(n) + if err != nil { + return out + } + out[1+i] = v + } + remainder := split[1] + + // Grab revision + split = strings.SplitN(remainder, ".", 2) + if len(split) < 2 { + return out + } + revision, err := strconv.Atoi(split[0]) + if err != nil { + return out + } + out[4] = revision + + return out +} + +// isOlderVersion returns true if "a" is an older version than "b". +// Input should be the same as version in the RPC, like +// 14:4.99.3-2.fc38.x86_64 or 3.43.1-1.centos7.arm64. +// Results are indeterminate if either string isn't a proper version. +func isOlderVersion(a, b string) bool { + parsedA := parseToNumbers(a) + parsedB := parseToNumbers(b) + for i := 0; i < 5; i++ { + if parsedA[i] < parsedB[i] { + return true + } + if parsedA[i] > parsedB[i] { + return false + } + } + return false +} + // server is used to implement the gRPC server type server struct{} @@ -281,6 +346,18 @@ func (s *server) Install(ctx context.Context, req *pb.InstallRequest) (*pb.Insta } } + // Make sure we actually installed the package. + postValidateCommand, err := generateValidate(req.PackageSystem, req.Name, req.Version) + validation, err := util.RunCommand(ctx, postValidateCommand[0], postValidateCommand[1:]) + if err != nil { + recorder.CounterOrLog(ctx, packagesInstallFailureCounter, 1, attribute.String("reason", "post_validate_err")) + return nil, err + } + if err := validation.Error; validation.ExitCode != 0 || err != nil { + recorder.CounterOrLog(ctx, packagesInstallFailureCounter, 1, attribute.String("reason", "post_validate_err")) + return nil, status.Errorf(codes.Internal, "failed to validate change\nstdout:\n%s\nstderr:\n%s\ninstall stdout:\n%s\ninstall stderr:\n%s", util.TrimString(validation.Stdout.String()), util.TrimString(validation.Stderr.String()), util.TrimString(run.Stdout.String()), util.TrimString(run.Stderr.String())) + } + return &pb.InstallReply{ DebugOutput: fmt.Sprintf("%s%s", run.Stdout.String(), run.Stderr.String()), RebootRequired: isRebootRequired, @@ -362,7 +439,7 @@ func (s *server) Update(ctx context.Context, req *pb.UpdateRequest) (*pb.UpdateR } // We can generate both commands since errors duplicate here. - validateCommand, valErr := generateValidate(req) + validateCommand, valErr := generateValidate(req.PackageSystem, req.Name, req.OldVersion) updateCommand, upErr := generateUpdate(req) if valErr != nil || upErr != nil { recorder.CounterOrLog(ctx, packagesUpdateFailureCounter, 1, attribute.String("reason", "generate_cmd_err")) @@ -391,6 +468,18 @@ func (s *server) Update(ctx context.Context, req *pb.UpdateRequest) (*pb.UpdateR return nil, status.Errorf(codes.Internal, "error from running - %v\nstdout:\n%s\nstderr:\n%s", err, util.TrimString(run.Stdout.String()), util.TrimString(run.Stderr.String())) } + // Make sure we actually installed the package. + postValidateCommand, err := generateValidate(req.PackageSystem, req.Name, req.NewVersion) + validation, err := util.RunCommand(ctx, postValidateCommand[0], postValidateCommand[1:]) + if err != nil { + recorder.CounterOrLog(ctx, packagesUpdateFailureCounter, 1, attribute.String("reason", "post_validate_err")) + return nil, err + } + if err := validation.Error; validation.ExitCode != 0 || err != nil { + recorder.CounterOrLog(ctx, packagesUpdateFailureCounter, 1, attribute.String("reason", "post_validate_err")) + return nil, status.Errorf(codes.Internal, "failed to validate change\nstdout:\n%s\nstderr:\n%s\ninstall stdout:\n%s\ninstall stderr:\n%s", util.TrimString(validation.Stdout.String()), util.TrimString(validation.Stderr.String()), util.TrimString(run.Stdout.String()), util.TrimString(run.Stderr.String())) + } + return &pb.UpdateReply{ DebugOutput: fmt.Sprintf("%s%s", run.Stdout.String(), run.Stderr.String()), }, nil diff --git a/services/packages/server/packages_test.go b/services/packages/server/packages_test.go index 76ff7116..b9a1a318 100644 --- a/services/packages/server/packages_test.go +++ b/services/packages/server/packages_test.go @@ -177,6 +177,11 @@ func TestInstall(t *testing.T) { return []string{testutil.ResolvePath(t, "echo"), "-n", testdataInput}, nil } t.Cleanup(func() { generateInstall = savedGenerateInstall }) + savedGenerateValidate := generateValidate + generateValidate = func(packageSystem pb.PackageSystem, name, version string) ([]string, error) { + return []string{testutil.ResolvePath(t, "echo"), "-n", testdataInput}, nil + } + t.Cleanup(func() { generateValidate = savedGenerateValidate }) // Test 0: Bunch of permutations for invalid input. for _, tc := range []struct { @@ -399,22 +404,26 @@ func TestUpdate(t *testing.T) { savedGenerateValidate := generateValidate savedGenerateUpdate := generateUpdate var cmdLine, validateCmdLine string - generateValidate = func(u *pb.UpdateRequest) ([]string, error) { + generateValidate = func(packageSystem pb.PackageSystem, name, version string) ([]string, error) { // Capture what was generated so we can validate it. - out, err := savedGenerateValidate(u) + out, err := savedGenerateValidate(packageSystem, name, version) if err != nil { return nil, err } - validateCmdLine = strings.Join(out, " ") + if validateCmdLine == "" { + validateCmdLine = strings.Join(out, " ") + } return []string{testutil.ResolvePath(t, "echo"), "-n", testdataInput}, nil } - badValidate := func(u *pb.UpdateRequest) ([]string, error) { + badValidate := func(packageSystem pb.PackageSystem, name, version string) ([]string, error) { // Capture what was generated so we can validate it. - out, err := savedGenerateValidate(u) + out, err := savedGenerateValidate(packageSystem, name, version) if err != nil { return nil, err } - validateCmdLine = strings.Join(out, " ") + if validateCmdLine == "" { + validateCmdLine = strings.Join(out, " ") + } return []string{testutil.ResolvePath(t, "false")}, nil } @@ -550,6 +559,7 @@ func TestUpdate(t *testing.T) { DisableRepo: "otherrepo", } + validateCmdLine = "" // Clear on each test run savedYumBin := YumBin t.Cleanup(func() { YumBin = savedYumBin @@ -586,7 +596,7 @@ func TestUpdate(t *testing.T) { for _, tc := range []struct { name string generate func(*pb.UpdateRequest) ([]string, error) - validate func(*pb.UpdateRequest) ([]string, error) + validate func(packageSystem pb.PackageSystem, name, version string) ([]string, error) }{ { name: "bad command", @@ -596,7 +606,7 @@ func TestUpdate(t *testing.T) { }, { name: "bad path - validate", - validate: func(*pb.UpdateRequest) ([]string, error) { + validate: func(packageSystem pb.PackageSystem, name, version string) ([]string, error) { return []string{"bad path"}, nil }, }, @@ -1180,3 +1190,40 @@ func TestCleanup(t *testing.T) { }) } } + +func TestIsOlder(t *testing.T) { + for _, tc := range []struct { + first, second string + wantIsOlder bool + }{ + { + first: "14:4.99.4-1.fc38.x86_64", + second: "14:4.99.3-2.fc38.x86_64", + wantIsOlder: false, + }, + { + first: "14:4.99.3-2.fc38.x86_64", + second: "14:4.99.4-1.fc38.x86_64", + wantIsOlder: true, + }, + { + first: "13:4.99.3-2.fc38.x86_64", + second: "14:4.99.4-1.fc38.x86_64", + wantIsOlder: true, + }, + { + first: "4.99.3-2.fc38.x86_64", + second: "14:4.99.4-1.fc38.x86_64", + wantIsOlder: true, + }, + { + first: "14:4.99.4-1.fc38.x86_64", + second: "4.99.3-2.fc38.x86_64", + wantIsOlder: false, + }, + } { + if older := isOlderVersion(tc.first, tc.second); older != tc.wantIsOlder { + t.Errorf("%q vs %q got %v for isOlderVersion, want %v", tc.first, tc.second, older, tc.wantIsOlder) + } + } +}