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

@@ -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 {

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

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)
+		}
+	}
+}