Skip to content

OCM-23781 | fix: Adding better logic for setting rosa_region tag #3235

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.

Sign up for GitHub

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

Draft
jerichokeyne wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

OCM-23781 | fix: Adding better logic for setting rosa_region tag #3235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pkg/aws/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,10 @@ const (

// Since CloudFormation stacks are region-dependent, we hard-code OCM's default region and
// then use it to ensure that the user always gets the stack from the same region.
DefaultRegion = "us-east-1"
Inline = "inline"
Attached = "attached"
DefaultRegion = "us-east-1"
DefaultGovcloudRegion = "us-gov-east-1"
Inline = "inline"
Attached = "attached"

LocalZone = "local-zone"
WavelengthZone = "wavelength-zone"
Expand Down
58 changes: 48 additions & 10 deletions pkg/aws/cloudformation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@ import (
"github.com/aws/smithy-go"

"github.com/openshift/rosa/assets"
"github.com/openshift/rosa/pkg/fedramp"
)

func readCloudFormationTemplate(path string) (string, error) {
cfTemplate, err := assets.Asset(path)
if err != nil {
return "", fmt.Errorf("Unable to read cloudformation template: %s", err)
return "", fmt.Errorf("unable to read cloudformation template: %s", err)
}

return string(cfTemplate), nil
Expand All @@ -43,7 +44,17 @@ func readCloudFormationTemplate(path string) (string, error) {
// Ensure osdCcsAdmin IAM user is created
func (c *awsClient) EnsureOsdCcsAdminUser(stackName string, adminUserName string, awsRegion string) (bool, error) {
userExists := true
regionForInit, err := c.GetClusterRegionTagForUser(adminUserName)
var region string
if awsRegion != "" {
region = awsRegion
} else {
if fedramp.Enabled() {
region = DefaultGovcloudRegion
} else {
region = DefaultRegion
}
}
adminRegionTag, err := c.GetClusterRegionTagForUser(adminUserName)
if err != nil {
//If user doesn't exists proceed normally
//If users exists and tag is not present then add the tag
Expand All @@ -56,8 +67,13 @@ func (c *awsClient) EnsureOsdCcsAdminUser(stackName string, adminUserName string
}
}
if userExists {
if regionForInit == "" {
err = c.TagUserRegion(adminUserName, DefaultRegion)
if adminRegionTag == "" {
err = c.TagUserRegion(adminUserName, region)
if err != nil {
return false, err
}
} else if fedramp.Enabled() && adminRegionTag == DefaultRegion {
err = c.TagUserRegion(adminUserName, region)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -106,7 +122,7 @@ func (c *awsClient) EnsureOsdCcsAdminUser(stackName string, adminUserName string
return false, err
}

err = c.TagUserRegion(adminUserName, awsRegion)
err = c.TagUserRegion(adminUserName, region)
if err != nil {
return false, err
}
Expand All @@ -115,7 +131,15 @@ func (c *awsClient) EnsureOsdCcsAdminUser(stackName string, adminUserName string

func (c *awsClient) CreateStack(cfTemplateBody, stackName string) (bool, error) {
// Create cloudformation stack
_, err := c.cfClient.CreateStack(context.Background(), buildCreateStackInput(cfTemplateBody, stackName, []cloudformationtypes.Parameter{}, []cloudformationtypes.Tag{}))
_, err := c.cfClient.CreateStack(
context.Background(),
buildCreateStackInput(
cfTemplateBody,
stackName,
[]cloudformationtypes.Parameter{},
[]cloudformationtypes.Tag{},
),
)
if err != nil {
return false, err
}
Expand All @@ -128,7 +152,13 @@ func (c *awsClient) CreateStack(cfTemplateBody, stackName string) (bool, error)
return true, nil
}

func (c *awsClient) CreateStackWithParamsTags(ctx context.Context, cfTemplateBody, stackName string, stackParams, stackTags map[string]string) (*string, error) {
func (c *awsClient) CreateStackWithParamsTags(
ctx context.Context,
cfTemplateBody,
stackName string,
stackParams,
stackTags map[string]string,
) (*string, error) {
// stack tags
var cfTags []cloudformationtypes.Tag
for k, v := range stackTags {
Expand Down Expand Up @@ -165,13 +195,16 @@ func (c *awsClient) GetCFStack(ctx context.Context, stackName string) (*cloudfor
}

if len(output.Stacks) == 0 {
return nil, fmt.Errorf("No CF stacks with name %s found", stackName)
return nil, fmt.Errorf("no CF stacks with name %s found", stackName)
}

return &output.Stacks[0], nil
}

func (c *awsClient) DescribeCFStackResources(ctx context.Context, stackName string) (*[]cloudformationtypes.StackResource, error) {
func (c *awsClient) DescribeCFStackResources(
ctx context.Context,
stackName string,
) (*[]cloudformationtypes.StackResource, error) {
output, err := c.cfClient.DescribeStackResources(ctx, &cloudformation.DescribeStackResourcesInput{
StackName: aws.String(stackName),
})
Expand Down Expand Up @@ -265,7 +298,12 @@ func (c *awsClient) DeleteOsdCcsAdminUser(stackName string) error {
}

// Build cloudformation create stack input
func buildCreateStackInput(cfTemplateBody, stackName string, cfParams []cloudformationtypes.Parameter, cfTags []cloudformationtypes.Tag) *cloudformation.CreateStackInput {
func buildCreateStackInput(
cfTemplateBody,
stackName string,
cfParams []cloudformationtypes.Parameter,
cfTags []cloudformationtypes.Tag,
) *cloudformation.CreateStackInput {
// Special cloudformation capabilities are required to create IAM resources in AWS
cfCapabilityIAM := cloudformationtypes.CapabilityCapabilityIam
cfCapabilityNamedIAM := cloudformationtypes.CapabilityCapabilityNamedIam
Expand Down
15 changes: 9 additions & 6 deletions pkg/ocm/regions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"github.com/openshift/rosa/pkg/arguments"
"github.com/openshift/rosa/pkg/aws"
"github.com/openshift/rosa/pkg/fedramp"
"github.com/openshift/rosa/pkg/helper"
"github.com/openshift/rosa/pkg/logging"
)
Expand Down Expand Up @@ -96,14 +97,14 @@ func (c *Client) GetRegions(roleARN string, externalID string) (regions []*cmv1.
Logger(logger).
Build()
if err != nil {
return nil, fmt.Errorf("Error creating AWS client: %v", err)
return nil, fmt.Errorf("error creating AWS client: %v", err)
}
Comment on lines +100 to 101

@coderabbitai coderabbitai Bot Apr 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify remaining non-wrapping fmt.Errorf(err) patterns in this file.
rg -nP --type=go 'fmt\.Errorf\(".*%(v|s)".*,\s*err\)' pkg/ocm/regions.go

Repository: openshift/rosa

Length of output: 375

Use %w for error formatting to enable errors.Is/As unwrapping.

Lines 100, 107, 117, and 155 use %v/%s with error values, preventing callers from using errors.Is/As to check the underlying error types. Change these to %w for proper error chain handling.

Suggested patch 
-			return nil, fmt.Errorf("error creating AWS client: %v", err)
+			return nil, fmt.Errorf("error creating AWS client: %w", err)
@@
-			return nil, fmt.Errorf("failed to get local AWS credentials: %v", err)
+			return nil, fmt.Errorf("failed to get local AWS credentials: %w", err)
@@
-		return nil, fmt.Errorf("failed to build AWS credentials for user '%s': %v", aws.AdminUserName, err)
+		return nil, fmt.Errorf("failed to build AWS credentials for user '%s': %w", aws.AdminUserName, err)
@@
-		err = fmt.Errorf("failed to retrieve AWS regions: %s", err)
+		err = fmt.Errorf("failed to retrieve AWS regions: %w", err)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, fmt.Errorf("error creating AWS client: %v", err)
}
return nil, fmt.Errorf("error creating AWS client: %w", err)
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/ocm/regions.go` around lines 100 - 101, In pkg/ocm/regions.go replace
fmt.Errorf calls that format wrapped errors with %v or %s to use %w so callers
can use errors.Is/As; specifically update the error returns whose messages start
with "error creating AWS client", and the other fmt.Errorf occurrences in the
same file (the ones that currently interpolate err with %v/%s around the AWS
client and related error messages) to use %w and pass the original err as the
last argument, leaving the rest of the message intact.


// Get AWS region
currentAWSCreds, err := awsClient.GetIAMCredentials()

if err != nil {
return nil, fmt.Errorf("Failed to get local AWS credentials: %v", err)
return nil, fmt.Errorf("failed to get local AWS credentials: %v", err)
}

awsBuilder = awsBuilder.
Expand All @@ -113,7 +114,7 @@ func (c *Client) GetRegions(roleARN string, externalID string) (regions []*cmv1.

awsCredentials, err := awsBuilder.Build()
if err != nil {
return nil, fmt.Errorf("Failed to build AWS credentials for user '%s': %v", aws.AdminUserName, err)
return nil, fmt.Errorf("failed to build AWS credentials for user '%s': %v", aws.AdminUserName, err)
}

collection := c.ocm.ClustersMgmt().V1().
Expand Down Expand Up @@ -151,7 +152,7 @@ func (c *Client) GetRegionList(multiAZ bool, roleARN string,
regionAZ map[string]bool, err error) {
regions, err := c.GetFilteredRegionsByVersion(roleARN, version, awsClient, externalID)
if err != nil {
err = fmt.Errorf("Failed to retrieve AWS regions: %s", err)
err = fmt.Errorf("failed to retrieve AWS regions: %s", err)
return
}

Expand All @@ -176,7 +177,9 @@ func (c *Client) GetRegionList(multiAZ bool, roleARN string,
}

func (c *Client) GetDatabaseRegionList() ([]string, error) {
response, err := c.ocm.ClustersMgmt().V1().CloudProviders().CloudProvider("aws").Regions().List().Send()
filter := fmt.Sprintf("govcloud is %t", fedramp.Enabled())
response, err :=
c.ocm.ClustersMgmt().V1().CloudProviders().CloudProvider("aws").Regions().List().Parameter("search", filter).Send()
if err != nil {
return []string{}, weberr.Errorf("Failed to get regions listing: %v", err)
}
Expand All @@ -202,7 +205,7 @@ func (c *Client) ValidateAwsClientRegion() error {
return err
}
if !helper.Contains(supportedRegions, awsRegionInUserConfig) {
return fmt.Errorf("Unsupported region '%s', available regions: %s",
return fmt.Errorf("unsupported region '%s', available regions: %s",
awsRegionInUserConfig, helper.SliceToSortedString(supportedRegions))
}
return nil
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/test_rosacli_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ var _ = Describe("Classic cluster creation validation",
"2": "Multi AZ cluster requires at least 3 compute nodes",
"0": "Multi AZ cluster requires at least 3 compute nodes",
"-3": "must be non-negative",
"5": "multi AZ clusters require that the replicas be a multiple of 3",
"5": "Multi AZ clusters require that the replicas be a multiple of 3",
}
for k, v := range invalidReplicasErrorMapMultiAZ {
rosalCommand.ReplaceFlagValue(map[string]string{
Expand Down Expand Up @@ -1402,7 +1402,7 @@ var _ = Describe("Classic cluster creation validation",
})
stdout, err = rosaClient.Runner.RunCMD(strings.Split(rosalCommand.GetFullCommand(), " "))
Expect(err).NotTo(BeNil())
Expect(stdout.String()).To(ContainSubstring("Unsupported region"))
Expect(stdout.String()).To(ContainSubstring("unsupported region"))

By("Check the validation for invalid billing-account for classic sts cluster")
rosalCommand.ReplaceFlagValue(map[string]string{
Expand Down
8 changes: 4 additions & 4 deletions tests/e2e/test_rosacli_region.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import (
"fmt"
"time"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
. "github.com/onsi/ginkgo/v2" //nolint:staticcheck
. "github.com/onsi/gomega" //nolint:staticcheck

"github.com/openshift/rosa/tests/ci/labels"
"github.com/openshift/rosa/tests/utils/exec/rosacli"
Expand All @@ -19,7 +19,7 @@ var _ = Describe("Region",
var (
rosaClient *rosacli.Client
ocmResourceService rosacli.OCMResourceService
permissionsBoundaryArn string = "arn:aws:iam::aws:policy/AdministratorAccess"
permissionsBoundaryArn = "arn:aws:iam::aws:policy/AdministratorAccess"
)

BeforeEach(func() {
Expand Down Expand Up @@ -132,6 +132,6 @@ var _ = Describe("Region",
availableMachineTypes, output, err := ocmResourceService.ListInstanceTypes(
"--region", "xxx", "--role-arn", classicInstallerRoleArn)
Expect(err).To(HaveOccurred())
Expect(output.String()).Should(ContainSubstring("ERR: Unsupported region 'xxx', available regions"))
Expect(output.String()).Should(ContainSubstring("ERR: unsupported region 'xxx', available regions"))
})
})