Merge pull request #2170 from hongkailiu/accept-cmd

OTA-1548: set up accepted risks

Author: openshift-merge-bot[bot]

pkg/cli/admin/upgrade/accept/accept.go

@@ -0,0 +1,213 @@
+package accept
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"strings"
+
+	"github.com/google/go-cmp/cmp"
+	configv1 "github.com/openshift/api/config/v1"
+	configv1client "github.com/openshift/client-go/config/clientset/versioned"
+	"github.com/spf13/cobra"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/cli-runtime/pkg/genericiooptions"
+	kcmdutil "k8s.io/kubectl/pkg/cmd/util"
+	"k8s.io/kubectl/pkg/util/templates"
+)
+
+func newOptions(streams genericiooptions.IOStreams) *options {
+	return &options{
+		IOStreams: streams,
+	}
+}
+
+var (
+	acceptExample = templates.Examples(`
+		# Accept RiskA and RiskB and stop accepting RiskC if accepted
+		oc adm upgrade accept RiskA,RiskB,RiskC-
+
+		# Accept RiskA and RiskB and nothing else
+		oc adm upgrade accept --replace RiskA,RiskB
+
+		# Accept no risks
+		oc adm upgrade accept --clear
+	`)
+
+	acceptLong = templates.LongDesc(`
+		Manage update risk acceptance.
+
+		Multiple risks are concatenated with comma. By default, the command appends the provided accepted risks into the existing
+		list. If --replace is specified, the existing accepted risks will be replaced with the provided
+		ones instead of appending. Placing "-" as suffix to an accepted risk will lead to
+		removal if it exists and no-ops otherwise. If --replace is specified, the suffix "-" on the risks
+		is not allowed.
+
+		Passing --clear removes all existing accepted risks.
+		`)
+)
+
+func New(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Command {
+	o := newOptions(streams)
+	cmd := &cobra.Command{
+		Use:     "accept",
+		Short:   "Accept risks exposed to conditional updates.",
+		Long:    acceptLong,
+		Example: acceptExample,
+		Run: func(cmd *cobra.Command, args []string) {
+			kcmdutil.CheckErr(o.Complete(f, cmd, args))
+			kcmdutil.CheckErr(o.Run())
+		},
+	}
+
+	flags := cmd.Flags()
+	flags.BoolVar(&o.replace, "replace", false, "Replace existing accepted risks with new ones")
+	flags.BoolVar(&o.clear, "clear", false, "Remove all existing accepted risks")
+	return cmd
+}
+
+// clusterVersionInterface is the subset of configv1client.ClusterVersionInterface
+// that we need, for easier mocking in unit tests.
+type clusterVersionInterface interface {
+	Get(ctx context.Context, name string, opts metav1.GetOptions) (*configv1.ClusterVersion, error)
+	Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (result *configv1.ClusterVersion, err error)
+}
+
+type options struct {
+	genericiooptions.IOStreams
+
+	Client  configv1client.Interface
+	replace bool
+	clear   bool
+	add     sets.Set[string]
+	remove  sets.Set[string]
+}
+
+func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string) error {
+	if o.clear && o.replace {
+		return kcmdutil.UsageErrorf(cmd, "--clear and --replace are mutually exclusive")
+	}
+
+	if o.clear {
+		kcmdutil.RequireNoArguments(cmd, args)
+	} else if len(args) == 0 {
+		return kcmdutil.UsageErrorf(cmd, "no positional arguments given")
+	}
+
+	if len(args) > 1 {
+		return kcmdutil.UsageErrorf(cmd, "multiple positional arguments given")
+	} else if len(args) == 1 {
+		o.add = sets.New[string]()
+		o.remove = sets.New[string]()
+		for _, s := range strings.Split(args[0], ",") {
+			trimmed := strings.TrimSpace(s)
+			if trimmed == "-" || trimmed == "" {
+				return kcmdutil.UsageErrorf(cmd, "illegal risk %q", trimmed)
+			}
+			if strings.HasSuffix(trimmed, "-") {
+				o.remove.Insert(strings.TrimSuffix(trimmed, "-"))
+			} else {
+				o.add.Insert(trimmed)
+			}
+		}
+	}
+
+	if conflict := o.add.Intersection(o.remove); conflict.Len() > 0 {
+		return kcmdutil.UsageErrorf(cmd, "requested risks with both Risk and Risk-: %s", strings.Join(sets.List(conflict), ","))
+	}
+
+	if o.replace && o.remove.Len() > 0 {
+		return kcmdutil.UsageErrorf(cmd, "The suffix '-' on risks is not allowed if --replace is specified")
+	}
+
+	cfg, err := f.ToRESTConfig()
+	if err != nil {
+		return err
+	}
+	client, err := configv1client.NewForConfig(cfg)
+	if err != nil {
+		return err
+	}
+	o.Client = client
+	return nil
+}
+
+func (o *options) Run() error {
+	ctx := context.TODO()
+	cv, err := o.Client.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return fmt.Errorf("no cluster version information available - you must be connected to an OpenShift server to fetch the current version")
+		}
+		return err
+	}
+
+	var existing []configv1.AcceptRisk
+	if cv.Spec.DesiredUpdate != nil {
+		existing = cv.Spec.DesiredUpdate.AcceptRisks
+	}
+	acceptRisks := getAcceptRisks(existing, o.replace, o.clear, o.add, o.remove)
+
+	if diff := cmp.Diff(acceptRisks, existing); diff != "" {
+		if err := patchDesiredUpdate(ctx, acceptRisks, o.Client.ConfigV1().ClusterVersions(), "version"); err != nil {
+			return err
+		}
+		var names []string
+		for _, risk := range acceptRisks {
+			names = append(names, risk.Name)
+		}
+		fmt.Fprintf(o.Out, "info: Accept risks are [%s]\n", strings.Join(names, ", ")) //nolint:errcheck
+	} else {
+		fmt.Fprintf(o.Out, "info: Accept risks are not changed\n") //nolint:errcheck
+	}
+
+	return nil
+}
+
+func getAcceptRisks(existing []configv1.AcceptRisk, replace, clear bool, add sets.Set[string], remove sets.Set[string]) []configv1.AcceptRisk {
+	var acceptRisks []configv1.AcceptRisk
+
+	if clear {
+		return acceptRisks
+	}
+
+	if !replace {
+		for _, risk := range existing {
+			if !remove.Has(risk.Name) {
+				acceptRisks = append(acceptRisks, *risk.DeepCopy())
+			}
+		}
+	}
+
+	riskNames := sets.New[string]()
+	for _, risk := range acceptRisks {
+		riskNames.Insert(risk.Name)
+	}
+
+	for _, name := range sets.List[string](add) {
+		if !riskNames.Has(name) && !remove.Has(name) {
+			acceptRisks = append(acceptRisks, configv1.AcceptRisk{
+				Name: name,
+			})
+		}
+	}
+
+	return acceptRisks
+}
+
+func patchDesiredUpdate(ctx context.Context, acceptRisks []configv1.AcceptRisk, client clusterVersionInterface,
+	clusterVersionName string) error {
+	acceptRisksJSON, err := json.Marshal(acceptRisks)
+	if err != nil {
+		return fmt.Errorf("marshal ClusterVersion patch: %v", err)
+	}
+	patch := []byte(fmt.Sprintf(`{"spec": {"desiredUpdate": {"acceptRisks": %s}}}`, acceptRisksJSON))
+	if _, err := client.Patch(ctx, clusterVersionName, types.MergePatchType, patch,
+		metav1.PatchOptions{}); err != nil {
+		return fmt.Errorf("unable to accept risks: %v", err)
+	}
+	return nil
+}

pkg/cli/admin/upgrade/accept/accept_test.go

@@ -0,0 +1,60 @@
+package accept
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	configv1 "github.com/openshift/api/config/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+func Test_getAcceptRisks(t *testing.T) {
+	for _, testCase := range []struct {
+		name     string
+		existing []configv1.AcceptRisk
+		replace  bool
+		clear    bool
+		plus     sets.Set[string]
+		minus    sets.Set[string]
+		expected []configv1.AcceptRisk
+	}{
+		{
+			name: "all zeros",
+		},
+		{
+			name:     "riskA, riskB + riskB + riskC - riskA - riskD",
+			existing: []configv1.AcceptRisk{{Name: "riskA"}, {Name: "riskB"}},
+			plus:     sets.New[string]("riskB", "riskC"),
+			minus:    sets.New[string]("riskA", "riskD"),
+			expected: []configv1.AcceptRisk{
+				{Name: "riskB"},
+				{Name: "riskC"},
+			},
+		},
+		{
+			name:     "replace",
+			existing: []configv1.AcceptRisk{{Name: "riskA"}, {Name: "riskB"}},
+			plus:     sets.New[string]("riskB", "riskC"),
+			minus:    sets.New[string]("does not matter"),
+			replace:  true,
+			expected: []configv1.AcceptRisk{
+				{Name: "riskB"},
+				{Name: "riskC"},
+			},
+		},
+		{
+			name:     "clear",
+			existing: []configv1.AcceptRisk{{Name: "riskA"}, {Name: "riskB"}},
+			plus:     sets.New[string]("not important"),
+			minus:    sets.New[string]("does not matter"),
+			clear:    true,
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			actual := getAcceptRisks(testCase.existing, testCase.replace, testCase.clear, testCase.plus, testCase.minus)
+			if diff := cmp.Diff(testCase.expected, actual); diff != "" {
+				t.Errorf("getAcceptRisks() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

pkg/cli/admin/upgrade/upgrade.go

@@ -25,6 +25,7 @@ import (
 	configv1client "github.com/openshift/client-go/config/clientset/versioned"
 	imagereference "github.com/openshift/library-go/pkg/image/reference"
 
+	"github.com/openshift/oc/pkg/cli/admin/upgrade/accept"
 	"github.com/openshift/oc/pkg/cli/admin/upgrade/channel"
 	"github.com/openshift/oc/pkg/cli/admin/upgrade/recommend"
 	"github.com/openshift/oc/pkg/cli/admin/upgrade/rollback"
@@ -105,7 +106,7 @@ func New(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Command
 		`),
 		Run: func(cmd *cobra.Command, args []string) {
 			kcmdutil.CheckErr(o.Complete(f, cmd, args))
-			kcmdutil.CheckErr(o.Run())
+			kcmdutil.CheckErr(o.Run(cmd.Context()))
 		},
 	}
 	flags := cmd.Flags()
@@ -126,6 +127,9 @@ func New(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Command
 	if kcmdutil.FeatureGate("OC_ENABLE_CMD_UPGRADE_ROLLBACK").IsEnabled() {
 		cmd.AddCommand(rollback.New(f, streams))
 	}
+	if kcmdutil.FeatureGate("OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS").IsEnabled() {
+		cmd.AddCommand(accept.New(f, streams))
+	}
 	cmd.AddCommand(recommend.New(f, streams))
 
 	return cmd
@@ -200,26 +204,34 @@ func (o *Options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string
 	return nil
 }
 
-func (o *Options) Run() error {
-	cv, err := o.Client.ConfigV1().ClusterVersions().Get(context.TODO(), "version", metav1.GetOptions{})
+func (o *Options) Run(ctx context.Context) error {
+	cv, err := o.Client.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{})
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			return fmt.Errorf("No cluster version information available - you must be connected to an OpenShift version 4 server to fetch the current version")
 		}
 		return err
 	}
 
-	ctx := context.TODO()
-
 	switch {
 	case o.Clear:
 		if cv.Spec.DesiredUpdate == nil {
 			fmt.Fprintf(o.Out, "info: No update in progress\n")
 			return nil
 		}
 		original := cv.Spec.DesiredUpdate
-		cv.Spec.DesiredUpdate = nil
-		updated, err := o.Client.ConfigV1().ClusterVersions().Patch(context.TODO(), cv.Name, types.MergePatchType, []byte(`{"spec":{"desiredUpdate":null}}`), metav1.PatchOptions{})
+		if original != nil && original.AcceptRisks != nil {
+			cv.Spec.DesiredUpdate = &configv1.Update{
+				AcceptRisks: original.AcceptRisks,
+			}
+		} else {
+			cv.Spec.DesiredUpdate = nil
+		}
+		bytes, err := json.Marshal(cv.Spec.DesiredUpdate)
+		if err != nil {
+			return fmt.Errorf("failed to marshal update: %v", err)
+		}
+		updated, err := o.Client.ConfigV1().ClusterVersions().Patch(ctx, cv.Name, types.MergePatchType, []byte(fmt.Sprintf(`{"spec":{"desiredUpdate":%s}}`, bytes)), metav1.PatchOptions{})
 		if err != nil {
 			return fmt.Errorf("Unable to cancel current rollout: %v", err)
 		}
@@ -252,6 +264,9 @@ func (o *Options) Run() error {
 			fmt.Fprintln(o.ErrOut, "warning: --force overrides cluster verification of your supplied release image and waives any update precondition failures. Only use this if you are testing unsigned release images or you are working around a known bug in the cluster-version operator and you have verified the authenticity of the provided image yourself.")
 		}
 
+		if cv.Spec.DesiredUpdate != nil {
+			update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks
+		}
 		if err := patchDesiredUpdate(ctx, update, o.Client, cv.Name); err != nil {
 
 			return err
@@ -391,6 +406,9 @@ func (o *Options) Run() error {
 			fmt.Fprintf(o.ErrOut, "warning: --allow-upgrade-with-warnings is bypassing: %s\n", err)
 		}
 
+		if cv.Spec.DesiredUpdate != nil {
+			update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks
+		}
 		if err := patchDesiredUpdate(ctx, update, o.Client, cv.Name); err != nil {
 			return err
 		}