Merge pull request #3144 from hunterkepley/ocm-21402

OCM-21402 | fix: Edit/logforwarder always use CW/S3 yaml fields

Author: openshift-merge-bot[bot]

cmd/edit/logforwarder/cmd.go

@@ -66,7 +66,7 @@ func NewEditLogForwarderCommand() *cobra.Command {
 	return cmd
 }
 
-func EditLogForwarderRunner(ctx context.Context, r *rosa.Runtime, command *cobra.Command, argv []string) error {
+func EditLogForwarderRunner(_ context.Context, r *rosa.Runtime, _ *cobra.Command, argv []string) error {
 	if len(argv) != 1 {
 		return fmt.Errorf("expected exactly one argument: log-forwarder ID")
 	}
@@ -90,9 +90,18 @@ func EditLogForwarderRunner(ctx context.Context, r *rosa.Runtime, command *cobra
 
 	var logForwarderYaml logforwarding.LogForwarderYaml
 
+	currentLogForwarder, err := r.OCMClient.FetchLogForwarder(cluster.ID(), logFwdID)
+	if err != nil {
+		return err
+	}
+	if currentLogForwarder == nil {
+		return errors.UserErrorf("Unable to fetch log forwarder with id '%s' from cluster '%s'",
+			logFwdID, cluster.ID())
+	}
+
 	if len(configData) == 0 {
-		interactiveObject, err := interactiveLogForwarding.InteractiveLogForwardingConfig(
-			r.OCMClient)
+		interactiveObject, err := interactiveLogForwarding.InteractiveLogForwardingConfigWithDefaults(
+			r.OCMClient, currentLogForwarder)
 		if err != nil {
 			return errors.UserErrorf("failed to create log fowarder config: %s", err)
 		}
@@ -110,7 +119,7 @@ func EditLogForwarderRunner(ctx context.Context, r *rosa.Runtime, command *cobra
 		}
 	}
 
-	err = r.OCMClient.EditLogForwarder(cluster.ID(), logFwdID, logForwarderYaml)
+	err = r.OCMClient.EditLogForwarder(cluster.ID(), logFwdID, logForwarderYaml, currentLogForwarder)
 	if err != nil {
 		return fmt.Errorf("failed to edit log forwarder '%s' for cluster '%s': %s", logFwdID, cluster.ID(), err)
 	}

pkg/interactive/logforwarding/logforwarding.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"strings"
 
+	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
+
 	"github.com/openshift/rosa/pkg/interactive"
 	"github.com/openshift/rosa/pkg/logforwarding"
 	"github.com/openshift/rosa/pkg/ocm"
@@ -36,13 +38,13 @@ func InteractiveLogForwardingConfig(ocmClient *ocm.Client) (
 	cloudWatchResult := &logforwarding.CloudWatchLogForwarderConfig{}
 
 	if con == cloudWatch || con == both {
-		cloudWatchResult, err = interactiveCloudWatch(ocmClient)
+		cloudWatchResult, err = interactiveCloudWatch(ocmClient, "", "")
 		if err != nil {
 			return nil, err
 		}
 	}
 	if con == s3 || con == both {
-		s3Result, err = interactiveS3(ocmClient)
+		s3Result, err = interactiveS3(ocmClient, "", "")
 		if err != nil {
 			return nil, err
 		}
@@ -55,12 +57,43 @@ func InteractiveLogForwardingConfig(ocmClient *ocm.Client) (
 	return &result, nil
 }
 
-func interactiveCloudWatch(ocmClient *ocm.Client) (
+func InteractiveLogForwardingConfigWithDefaults(ocmClient *ocm.Client, logForwarder *cmv1.LogForwarder) (
+	*logforwarding.LogForwarderYaml, error) {
+
+	s3Result := &logforwarding.S3LogForwarderConfig{}
+	cloudWatchResult := &logforwarding.CloudWatchLogForwarderConfig{}
+
+	var err error
+
+	if logForwarder.Cloudwatch() != nil && logForwarder.Cloudwatch().LogDistributionRoleArn() != "" {
+		cloudWatchResult, err = interactiveCloudWatch(ocmClient,
+			logForwarder.Cloudwatch().LogGroupName(), logForwarder.Cloudwatch().LogDistributionRoleArn())
+		if err != nil {
+			return nil, err
+		}
+	}
+	if logForwarder.S3() != nil && logForwarder.S3().BucketName() != "" {
+		s3Result, err = interactiveS3(ocmClient, logForwarder.S3().BucketName(), logForwarder.S3().BucketPrefix())
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	result := logforwarding.LogForwarderYaml{}
+	result.S3 = s3Result
+	result.CloudWatch = cloudWatchResult
+
+	return &result, nil
+}
+
+func interactiveCloudWatch(ocmClient *ocm.Client, defaultLogGroupName string, defaultLogRoleArn string) (
 	*logforwarding.CloudWatchLogForwarderConfig, error) {
+
 	cloudWatchConfig := logforwarding.CloudWatchLogForwarderConfig{}
 	roleArn, err := interactive.GetString(interactive.Input{
 		Question: "CloudWatch Log forwarding role ARN",
 		Help:     "The role ARN which forwards logs to CloudWatch",
+		Default:  defaultLogRoleArn,
 		Required: true,
 	})
 	if err != nil {
@@ -71,6 +104,7 @@ func interactiveCloudWatch(ocmClient *ocm.Client) (
 	groupName, err := interactive.GetString(interactive.Input{
 		Question: "CloudWatch log group name",
 		Help:     "The name of the group on CloudWatch which will contain the logs",
+		Default:  defaultLogGroupName,
 		Required: true,
 	})
 	if err != nil {
@@ -96,11 +130,14 @@ func interactiveCloudWatch(ocmClient *ocm.Client) (
 	return &cloudWatchConfig, nil
 }
 
-func interactiveS3(ocmClient *ocm.Client) (*logforwarding.S3LogForwarderConfig, error) {
+func interactiveS3(ocmClient *ocm.Client, defaultBucketName string, defaultBucketPrefix string) (
+	*logforwarding.S3LogForwarderConfig, error) {
+
 	s3Config := logforwarding.S3LogForwarderConfig{}
 	bucketPrefix, err := interactive.GetString(interactive.Input{
 		Question: "S3 Bucket prefix",
 		Help:     "The identifiable prefix to prepend to the S3 Bucket used for log forwarding",
+		Default:  defaultBucketPrefix,
 		Required: false,
 	})
 	if err != nil {
@@ -111,6 +148,7 @@ func interactiveS3(ocmClient *ocm.Client) (*logforwarding.S3LogForwarderConfig,
 	bucketName, err := interactive.GetString(interactive.Input{
 		Question: "S3 Bucket name",
 		Help:     "The identifiable name to append to the S3 Bucket used for log forwarding",
+		Default:  defaultBucketName,
 		Required: true,
 	})
 	if err != nil {

pkg/ocm/log_forwards.go

@@ -183,32 +183,21 @@ func (c *Client) FetchLogForwarder(clusterId string, logForwarderId string) (*cm
 }
 
 func (c *Client) EditLogForwarder(clusterId string, logForwarderId string,
-	logForwarderYaml logforwarding.LogForwarderYaml) error {
+	logForwarderYaml logforwarding.LogForwarderYaml, currentLogForwarder *cmv1.LogForwarder) error {
 
 	if logForwarderYaml.S3 == nil && logForwarderYaml.CloudWatch == nil {
 		return errors.UserErrorf("log forwarding config provided contained no valid log forwarders")
 	}
 
 	var logForwarderConfig *cmv1.LogForwarderBuilder
 
-	currentLogForwarder, err := c.FetchLogForwarder(clusterId, logForwarderId)
-	if err != nil {
-		return err
-	}
-	if currentLogForwarder == nil {
-		return errors.UserErrorf("Unable to fetch log forwarder with id '%s' from cluster '%s'",
-			logForwarderId, clusterId)
-	}
-
 	if logForwarderYaml.S3 != nil {
 		logForwarderConfigBuilder := cmv1.NewLogForwarder()
 		logForwarderS3ConfigBuilder := cmv1.NewLogForwarderS3Config()
-		if logForwarderYaml.S3.S3ConfigBucketPrefix != currentLogForwarder.S3().BucketPrefix() {
-			logForwarderS3ConfigBuilder.BucketPrefix(logForwarderYaml.S3.S3ConfigBucketPrefix)
-		}
-		if logForwarderYaml.S3.S3ConfigBucketName != currentLogForwarder.S3().BucketName() {
-			logForwarderS3ConfigBuilder.BucketName(logForwarderYaml.S3.S3ConfigBucketName)
-		}
+
+		logForwarderS3ConfigBuilder.BucketPrefix(logForwarderYaml.S3.S3ConfigBucketPrefix)
+
+		logForwarderS3ConfigBuilder.BucketName(logForwarderYaml.S3.S3ConfigBucketName)
 
 		if !reflect.DeepEqual(currentLogForwarder.Applications(), logForwarderYaml.S3.Applications) {
 			logForwarderConfigBuilder.Applications(logForwarderYaml.S3.Applications...)
@@ -222,17 +211,15 @@ func (c *Client) EditLogForwarder(clusterId string, logForwarderId string,
 			logForwarderConfigBuilder.Groups(groupBuilder...)
 		}
 
+		logForwarderConfigBuilder.S3(logForwarderS3ConfigBuilder)
 		logForwarderConfig = logForwarderConfigBuilder
 	} else if logForwarderYaml.CloudWatch != nil {
 		logForwarderConfigBuilder := cmv1.NewLogForwarder()
 		logForwarderCWConfigBuilder := cmv1.NewLogForwarderCloudWatchConfig()
-		if logForwarderYaml.CloudWatch.CloudWatchLogGroupName != currentLogForwarder.Cloudwatch().LogGroupName() {
-			logForwarderCWConfigBuilder.LogGroupName(logForwarderYaml.CloudWatch.CloudWatchLogGroupName)
-		}
-		if logForwarderYaml.CloudWatch.CloudWatchLogRoleArn !=
-			currentLogForwarder.Cloudwatch().LogDistributionRoleArn() {
-			logForwarderCWConfigBuilder.LogDistributionRoleArn(logForwarderYaml.CloudWatch.CloudWatchLogRoleArn)
-		}
+
+		logForwarderCWConfigBuilder.LogGroupName(logForwarderYaml.CloudWatch.CloudWatchLogGroupName)
+
+		logForwarderCWConfigBuilder.LogDistributionRoleArn(logForwarderYaml.CloudWatch.CloudWatchLogRoleArn)
 
 		if !reflect.DeepEqual(currentLogForwarder.Applications(), logForwarderYaml.CloudWatch.Applications) {
 			logForwarderConfigBuilder.Applications(logForwarderYaml.CloudWatch.Applications...)
@@ -246,6 +233,7 @@ func (c *Client) EditLogForwarder(clusterId string, logForwarderId string,
 			logForwarderConfigBuilder.Groups(groupBuilder...)
 		}
 
+		logForwarderConfigBuilder.Cloudwatch(logForwarderCWConfigBuilder)
 		logForwarderConfig = logForwarderConfigBuilder
 	}
 

pkg/ocm/log_forwards_test.go

@@ -199,7 +199,7 @@ var _ = Describe("EditLogForwarder", func() {
 		It("Should handle empty config", func() {
 			client := Client{}
 			emptyConfig := logforwarding.LogForwarderYaml{}
-			err := client.EditLogForwarder("cluster-123", "log-fwd-123", emptyConfig)
+			err := client.EditLogForwarder("cluster-123", "log-fwd-123", emptyConfig, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring(
 				"log forwarding config provided contained no valid log forwarders"))