Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCM-2966 : Feat : Added subscription_type parameter to create cluster command #531

Merged
merged 11 commits into from
Sep 12, 2023
Merged
100 changes: 97 additions & 3 deletions cmd/ocm/create/cluster/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,23 @@ import (
"github.com/openshift-online/ocm-cli/pkg/provider"
"github.com/openshift-online/ocm-cli/pkg/utils"
sdk "github.com/openshift-online/ocm-sdk-go"
amv1 "github.com/openshift-online/ocm-sdk-go/accountsmgmt/v1"
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
"github.com/openshift/rosa/pkg/interactive"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

var ValidSubscriptionTypes = []string{StandardSubscriptionType, marketplaceRhmSubscriptionType, marketplaceGcpSubscriptionType}

const (
defaultIngressRouteSelectorFlag = "default-ingress-route-selector"
defaultIngressExcludedNamespacesFlag = "default-ingress-excluded-namespaces"
defaultIngressWildcardPolicyFlag = "default-ingress-wildcard-policy"
defaultIngressNamespaceOwnershipPolicyFlag = "default-ingress-namespace-ownership-policy"
StandardSubscriptionType = "standard"
marketplaceRhmSubscriptionType = "marketplace-rhm"
marketplaceGcpSubscriptionType = "marketplace-gcp"
)

var args struct {
Expand All @@ -68,6 +74,7 @@ var args struct {
clusterWideProxy c.ClusterWideProxy
gcpServiceAccountFile arguments.FilePath
etcdEncryption bool
subscriptionType string

// Scaling options
computeMachineType string
Expand Down Expand Up @@ -301,6 +308,15 @@ func init() {
fmt.Sprintf("Namespace Ownership Policy for ingress. Options are %s",
strings.Join(ingress.ValidNamespaceOwnershipPolicies, ",")),
)

fs.StringVar(
&args.subscriptionType,
"subscription-type",
StandardSubscriptionType,
fmt.Sprintf("The subscription billing model for the cluster."),
)
arguments.SetQuestion(fs, "subscription-type", "Subscription type:")
Cmd.RegisterFlagCompletionFunc("subscription-type", arguments.MakeCompleteFunc(getSubscriptionTypeOptions))
}

func osdProviderOptions(_ *sdk.Connection) ([]arguments.Option, error) {
Expand Down Expand Up @@ -377,6 +393,56 @@ func getVersionOptionsWithDefault(connection *sdk.Connection, channelGroup strin
return
}

func getSubscriptionTypeOptions(connection *sdk.Connection) ([]arguments.Option, error) {
options := []arguments.Option{}
billingModels, err := getBillingModels(connection)
if err != nil {
return options, err
}
for _, billingModel := range billingModels {
options = append(options, arguments.Option{
Value: fmt.Sprintf("%s (%s)", billingModel.Id(), billingModel.Description()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to add the description to the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it before since interactive mode has a ? option to show description, but seems it only shows descriptions of the argument, not the drop down. Removed the description

Description: billingModel.Description(),
})
}
return options, nil
}

func getSubscriptionTypeIdFromDescription(connection *sdk.Connection, value string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest using the same approach used for subnets, see https://github.com/openshift-online/ocm-cli/blob/main/cmd/ocm/create/cluster/cmd.go#L885

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used similar methods as subnets

billingModels, _ := getBillingModels(connection)
for _, billingModel := range billingModels {
if value == fmt.Sprintf("%s (%s)", billingModel.Id(), billingModel.Description()) {
return billingModel.Id()
}
}
return ""
}

func getBillingModel(connection *sdk.Connection, billingModelID string) (*amv1.BillingModelItem, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this function reusable (as well as getBillingModels) by moving them to its own package under pkg/billing, for example.

This logic doesn't really belong to cmd/ocm/create/cluster/cmd.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to the billing package

bilingModel, err := connection.AccountsMgmt().V1().BillingModels().BillingModel(billingModelID).Get().Send()
if err != nil {
return nil, err
}
return bilingModel.Body(), nil
}

func getBillingModels(connection *sdk.Connection) ([]*amv1.BillingModelItem, error) {
response, err := connection.AccountsMgmt().V1().BillingModels().List().Send()
if err != nil {
return nil, err
}
billingModels := response.Items().Slice()
var validBillingModel []*amv1.BillingModelItem
for _, billingModel := range billingModels {
for _, validSubscriptionTypeId := range ValidSubscriptionTypes {
if billingModel.Id() == validSubscriptionTypeId {
validBillingModel = append(validBillingModel, billingModel)
}
}
}
return validBillingModel, nil
}

func getMachineTypeOptions(connection *sdk.Connection) ([]arguments.Option, error) {
return provider.GetMachineTypeOptions(
connection.ClustersMgmt().V1(),
Expand Down Expand Up @@ -421,9 +487,27 @@ func preRun(cmd *cobra.Command, argv []string) error {
// Validate flags / ask for missing data.
fs := cmd.Flags()

// Get options for subscription type
subscriptionTypeOptions, _ := getSubscriptionTypeOptions(connection)
err = arguments.PromptOneOf(fs, "subscription-type", subscriptionTypeOptions)
if err != nil {
return err
}

// Only offer the 2 providers known to support OSD now;
// but don't validate if set, to not block `ocm` CLI from creating clusters on future providers.
providers, _ := osdProviderOptions(connection)

// If marketplace-gcp subscription type is used, provider can only be GCP
gcpBillingModel, _ := getBillingModel(connection, marketplaceGcpSubscriptionType)
isGcpSubscriptionType := args.subscriptionType == fmt.Sprintf("%s (%s)", gcpBillingModel.Id(), gcpBillingModel.Description())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isGcpSubscriptionType not all GCP subscriptions falls into this. I would suggest naming it isGcpMarketplace instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

if isGcpSubscriptionType {
providers = []arguments.Option{
{Value: c.ProviderGCP, Description: ""},
}
args.ccs.Enabled = true
}

err = arguments.PromptOneOf(fs, "provider", providers)
if err != nil {
return err
Expand All @@ -435,7 +519,7 @@ func preRun(cmd *cobra.Command, argv []string) error {
args.clusterWideProxy.Enabled = true
}

err = promptCCS(fs)
err = promptCCS(fs, isGcpSubscriptionType)
if err != nil {
return err
}
Expand Down Expand Up @@ -530,6 +614,7 @@ func preRun(cmd *cobra.Command, argv []string) error {
if err != nil {
return err
}

return nil
}

Expand All @@ -554,6 +639,10 @@ func run(cmd *cobra.Command, argv []string) error {
return err
}

if args.interactive {
args.subscriptionType = getSubscriptionTypeIdFromDescription(connection, args.subscriptionType)
}

clusterConfig := c.Spec{
Name: args.clusterName,
Region: args.region,
Expand All @@ -577,6 +666,7 @@ func run(cmd *cobra.Command, argv []string) error {
Private: &args.private,
EtcdEncryption: args.etcdEncryption,
DefaultIngress: defaultIngress,
SubscriptionType: args.subscriptionType,
}

cluster, err := c.CreateCluster(connection.ClustersMgmt().V1(), clusterConfig, args.dryRun)
Expand Down Expand Up @@ -1036,8 +1126,12 @@ func promptExistingVPC(fs *pflag.FlagSet, connection *sdk.Connection) error {
return err
}

func promptCCS(fs *pflag.FlagSet) error {
err := arguments.PromptBool(fs, "ccs")
func promptCCS(fs *pflag.FlagSet, isGcpSubscriptionType bool) error {
var err error
// If marketplace-gcp subscription type is used, ccs should by default be true
if !isGcpSubscriptionType {
err = arguments.PromptBool(fs, "ccs")
}
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/nwidger/jsoncolor v0.3.2
github.com/onsi/ginkgo/v2 v2.11.0
github.com/onsi/gomega v1.27.8
github.com/openshift-online/ocm-sdk-go v0.1.356
github.com/openshift-online/ocm-sdk-go v0.1.367
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to wait the next version as we are changing the BillingModelItem from a struct to a class.

See https://github.com/openshift-online/ocm-api-model/pull/831/files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used latest sdk version

github.com/openshift/rosa v1.2.24
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8
github.com/spf13/cobra v1.7.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,8 @@ github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAl
github.com/onsi/gomega v1.19.0/go.mod h1:LY+I3pBVzYsTBU1AnDwOSxaYi9WoWiqgwooUqq9yPro=
github.com/onsi/gomega v1.27.8 h1:gegWiwZjBsf2DgiSbf5hpokZ98JVDMcWkUiigk6/KXc=
github.com/onsi/gomega v1.27.8/go.mod h1:2J8vzI/s+2shY9XHRApDkdgPo1TKT7P2u6fXeJKFnNQ=
github.com/openshift-online/ocm-sdk-go v0.1.356 h1:FFUAi+mR5pdBsp3pisXp7M4SiFdxKQwNwKTQNY7QDY0=
github.com/openshift-online/ocm-sdk-go v0.1.356/go.mod h1:KYOw8kAKAHyPrJcQoVR82CneQ4ofC02Na4cXXaTq4Nw=
github.com/openshift-online/ocm-sdk-go v0.1.367 h1:17eJeoorPLhrHRte3vUGPxIKRDS8BEp1Uv+XhHPY87k=
github.com/openshift-online/ocm-sdk-go v0.1.367/go.mod h1:KYOw8kAKAHyPrJcQoVR82CneQ4ofC02Na4cXXaTq4Nw=
github.com/openshift/rosa v1.2.24 h1:vv0yYnWHx6CCPEAau/0rS54P2ksaf+uWXb1TQPWxiYE=
github.com/openshift/rosa v1.2.24/go.mod h1:MVXB27O3PF8WoOic23I03mmq6/9kVxpFx6FKyLMCyrQ=
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU=
Expand Down
6 changes: 4 additions & 2 deletions pkg/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
"time"

sdk "github.com/openshift-online/ocm-sdk-go"
amsv1 "github.com/openshift-online/ocm-sdk-go/accountsmgmt/v1"
amv1 "github.com/openshift-online/ocm-sdk-go/accountsmgmt/v1"

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed these to keep is consistent with CS (they have used cmv1 instead of cmsv1), but we can revert this back

cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
)

Expand Down Expand Up @@ -68,6 +68,7 @@ type Spec struct {
ChannelGroup string
Expiration time.Time
EtcdEncryption bool
SubscriptionType string

// Scaling config
ComputeMachineType string
Expand Down Expand Up @@ -312,6 +313,7 @@ func CreateCluster(cmv1Client *cmv1.Client, config Spec, dryRun bool) (*cmv1.Clu
ID(config.Flavour),
).
EtcdEncryption(config.EtcdEncryption).
BillingModel(cmv1.BillingModel(config.SubscriptionType)).
Properties(clusterProperties)

clusterBuilder = clusterBuilder.Version(
Expand Down Expand Up @@ -703,7 +705,7 @@ func GetClusterAddOns(connection *sdk.Connection, clusterID string) ([]*AddOnIte
}

// Only display add-ons for which the org has quota
quotaCosts.Each(func(quotaCost *amsv1.QuotaCost) bool {
quotaCosts.Each(func(quotaCost *amv1.QuotaCost) bool {
relatedResources := quotaCost.RelatedResources()
for _, relatedResource := range relatedResources {
if relatedResource.ResourceType() == "add-on" &&
Expand Down
2 changes: 2 additions & 0 deletions pkg/cluster/describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ func PrintClusterDescription(connection *sdk.Connection, cluster *cmv1.Cluster)
"Infra: %d\n"+
"Computes: %s\n"+
"Product: %s\n"+
"Subscription type: %s\n"+
"Provider: %s\n"+
"Version: %s\n"+
"Region: %s\n"+
Expand All @@ -221,6 +222,7 @@ func PrintClusterDescription(connection *sdk.Connection, cluster *cmv1.Cluster)
cluster.Nodes().Infra(),
computesStr,
cluster.Product().ID(),
cluster.BillingModel(),
cluster.CloudProvider().ID(),
cluster.OpenshiftVersion(),
cluster.Region().ID(),
Expand Down