diff --git a/pkg/cli/admin/upgrade/accept/accept.go b/pkg/cli/admin/upgrade/accept/accept.go new file mode 100644 index 0000000000..5307985396 --- /dev/null +++ b/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 +} diff --git a/pkg/cli/admin/upgrade/accept/accept_test.go b/pkg/cli/admin/upgrade/accept/accept_test.go new file mode 100644 index 0000000000..face3650b2 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/cli/admin/upgrade/upgrade.go b/pkg/cli/admin/upgrade/upgrade.go index 2550381190..37e1f1654f 100644 --- a/pkg/cli/admin/upgrade/upgrade.go +++ b/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,8 +204,8 @@ 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") @@ -209,8 +213,6 @@ func (o *Options) Run() error { return err } - ctx := context.TODO() - switch { case o.Clear: if cv.Spec.DesiredUpdate == nil { @@ -218,8 +220,18 @@ func (o *Options) Run() error { 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 } diff --git a/test/e2e/accept.go b/test/e2e/accept.go new file mode 100644 index 0000000000..91f3d9a7f7 --- /dev/null +++ b/test/e2e/accept.go @@ -0,0 +1,96 @@ +package e2e + +import ( + "context" + + g "github.com/onsi/ginkgo/v2" + "github.com/onsi/ginkgo/v2/types" + o "github.com/onsi/gomega" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/clientcmd" + + oteginkgo "github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo" + configv1 "github.com/openshift/api/config/v1" + configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" +) + +var _ = g.Describe("[sig-cli][OCPFeatureGate:ClusterUpdateAcceptRisks] oc", g.Label("cluster-version-operator"), func() { + + var ( + ctx = context.TODO() + kubeConfigPath = KubeConfigPath() + oc = NewCLI("oc", kubeConfigPath).EnvVar("OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS", "true") + configClient *configv1client.ConfigV1Client + ) + + g.BeforeEach(func() { + config, err := clientcmd.BuildConfigFromFlags("", kubeConfigPath) + o.Expect(err).NotTo(o.HaveOccurred()) + configClient, err = configv1client.NewForConfig(config) + o.Expect(err).NotTo(o.HaveOccurred()) + + skipIfMicroShift(oc) + SkipIfNotTechPreviewNoUpgrade(ctx, configClient) + + cv, err := configClient.ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + if cv.Spec.DesiredUpdate != nil { + o.Expect(cv.Spec.DesiredUpdate.AcceptRisks).To(o.BeEmpty()) + } + }) + + g.AfterEach(func() { + // No need to recover if the test is skipped. + // In case it is skipped up to disabled TP, we must not recover as the API might not be available. + // We could use patch instead but this guard should work too. + report := g.CurrentSpecReport() + if report.State.Is(types.SpecStateSkipped) { + return + } + cv, err := configClient.ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + if cv.Spec.DesiredUpdate != nil && len(cv.Spec.DesiredUpdate.AcceptRisks) > 0 { + backup := cv.DeepCopy() + backup.Spec.DesiredUpdate.AcceptRisks = nil + backup, err = configClient.ClusterVersions().Update(ctx, backup, metav1.UpdateOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + } + }) + + g.It("can operate accept risks [Serial]", g.Label("tech-preview"), oteginkgo.Informing(), func() { + + g.By("accepting some risks") + out, err := oc.Run("adm").Args("upgrade", "accept", "RiskA,RiskB").WithoutNamespace().Output() + o.Expect(err).NotTo(o.HaveOccurred(), "output: %s", out) + verifyAcceptRisks(ctx, configClient, []configv1.AcceptRisk{{Name: "RiskA"}, {Name: "RiskB"}}) + + g.By("accepting more risks") + out, err = oc.Run("adm").Args("upgrade", "accept", "RiskA,RiskB,RiskC").Output() + o.Expect(err).NotTo(o.HaveOccurred(), "output: %s", out) + verifyAcceptRisks(ctx, configClient, []configv1.AcceptRisk{{Name: "RiskA"}, {Name: "RiskB"}, {Name: "RiskC"}}) + + g.By("replacing some risks") + out, err = oc.Run("adm").Args("upgrade", "accept", "--replace", "RiskB,RiskD").Output() + o.Expect(err).NotTo(o.HaveOccurred(), "output: %s", out) + verifyAcceptRisks(ctx, configClient, []configv1.AcceptRisk{{Name: "RiskB"}, {Name: "RiskD"}}) + + g.By("removing some risks") + out, err = oc.Run("adm").Args("upgrade", "accept", "RiskB,RiskD-").Output() + o.Expect(err).NotTo(o.HaveOccurred(), "output: %s", out) + verifyAcceptRisks(ctx, configClient, []configv1.AcceptRisk{{Name: "RiskB"}}) + + g.By("removing all risks") + out, err = oc.Run("adm").Args("upgrade", "accept", "--clear").Output() + o.Expect(err).NotTo(o.HaveOccurred(), "output: %s", out) + verifyAcceptRisks(ctx, configClient, nil) + }) + +}) + +func verifyAcceptRisks(ctx context.Context, client *configv1client.ConfigV1Client, risks []configv1.AcceptRisk) { + cv, err := client.ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + o.Expect(cv.Spec.DesiredUpdate).NotTo(o.BeNil()) + o.Expect(cv.Spec.DesiredUpdate.AcceptRisks).To(o.Equal(risks)) +} diff --git a/test/e2e/util.go b/test/e2e/util.go index e107dcf46a..bce3092ce7 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -9,27 +9,29 @@ import ( "fmt" "io" "io/ioutil" - "regexp" - - o "github.com/onsi/gomega" - "math/rand" "net/http" - "os" "os/exec" "path/filepath" "reflect" + "regexp" "sort" "strconv" "strings" "time" g "github.com/onsi/ginkgo/v2" + o "github.com/onsi/gomega" + + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" + configv1 "github.com/openshift/api/config/v1" + configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" + "github.com/openshift/oc/test/testdata" ) @@ -72,6 +74,8 @@ type CLI struct { namespace string asAdmin bool withNamespace bool + + addEnvVars map[string]string } // NewCLI creates a new CLI instance @@ -139,17 +143,19 @@ func (c *CLI) AsGuestKubeconf(path string) *CLI { return c } // CLICommand represents a command to be executed type CLICommand struct { - cli *CLI - verb string - args []string + cli *CLI + verb string + args []string + addEnvVars map[string]string } // Run sets the verb for the CLI command func (c *CLI) Run(verb string) *CLICommand { return &CLICommand{ - cli: c, - verb: verb, - args: []string{}, + cli: c, + verb: verb, + args: []string{}, + addEnvVars: c.addEnvVars, } } @@ -210,6 +216,9 @@ func (cmd *CLICommand) Output() (string, error) { if cmd.cli.kubeconfig != "" { execCmd.Env = append(os.Environ(), "KUBECONFIG="+cmd.cli.kubeconfig) } + for k, v := range cmd.addEnvVars { + execCmd.Env = append(execCmd.Env, fmt.Sprintf("%s=%s", k, v)) + } output, err := execCmd.CombinedOutput() if err != nil { @@ -255,6 +264,9 @@ func (cmd *CLICommand) Outputs() (string, string, error) { if cmd.cli.kubeconfig != "" { execCmd.Env = append(os.Environ(), "KUBECONFIG="+cmd.cli.kubeconfig) } + for k, v := range cmd.addEnvVars { + execCmd.Env = append(execCmd.Env, fmt.Sprintf("%s=%s", k, v)) + } var stdout, stderr strings.Builder execCmd.Stdout = &stdout @@ -312,6 +324,9 @@ func (cmd *CLICommand) Background() (*exec.Cmd, *strings.Builder, *strings.Build if cmd.cli.kubeconfig != "" { execCmd.Env = append(os.Environ(), "KUBECONFIG="+cmd.cli.kubeconfig) } + for k, v := range cmd.addEnvVars { + execCmd.Env = append(execCmd.Env, fmt.Sprintf("%s=%s", k, v)) + } var stdout, stderr strings.Builder execCmd.Stdout = &stdout @@ -1462,3 +1477,32 @@ func (c *CLI) DeleteSpecifiedNamespaceAsAdmin(namespace string) { err := c.AsAdmin().WithoutNamespace().Run("delete").Args("namespace", namespace).Execute() o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to delete namespace/%s", namespace)) } + +// EnvVar sets an environment variable for the command, appended to whatever Env is set on the CLI +// when eventually executed. +func (c *CLI) EnvVar(name, value string) *CLI { + if c.addEnvVars == nil { + c.addEnvVars = make(map[string]string) + } + c.addEnvVars[name] = value + return c +} + +// IsTechPreviewNoUpgrade checks if a cluster is a TechPreviewNoUpgrade cluster +func IsTechPreviewNoUpgrade(ctx context.Context, client *configv1client.ConfigV1Client) bool { + featureGate, err := client.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return false + } + o.Expect(err).NotTo(o.HaveOccurred(), "could not retrieve feature-gate: %v", err) + } + return featureGate.Spec.FeatureSet == configv1.TechPreviewNoUpgrade +} + +// SkipIfNotTechPreviewNoUpgrade skips the test if a cluster is not a TechPreviewNoUpgrade cluster +func SkipIfNotTechPreviewNoUpgrade(ctx context.Context, client *configv1client.ConfigV1Client) { + if !IsTechPreviewNoUpgrade(ctx, client) { + g.Skip("This test is skipped because the Tech Preview NoUpgrade is not enabled") + } +}