Reimplement a way to retrieve an aws-sdk-go-v2 config from backplane #257

mjlshen · 2023-11-18T22:09:32Z

What type of PR is this?

bug/feature

What this PR does / Why we need it?

This functionality was originally added in #66, but was mistakenly removed in #237

Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file.

Which Jira/Github issue(s) does this PR fix?

OSD-19711

Special notes for your reviewer

This is semi-urgent to merge+release because osdctl builds an aws-sdk-go-v2 client using this function https://github.com/openshift/osdctl/blob/1b906de67017e15057a6312d698ac28565250941/pkg/osdCloud/aws.go#L182-L190 pinned on an older version where it still exists: https://github.com/openshift/osdctl/blob/1b906de67017e15057a6312d698ac28565250941/go.mod#L38 so SRE's using commands like osdctl network verify-egress, osdctl cluster resize infra, etc. are not going through the proxy configured in backplane.

Pre-checks (if applicable)

Ran unit tests locally
Validated the changes in a cluster
Included documentation changes with PR

mjlshen · 2023-11-18T22:12:37Z

On the testing side, I modified osdctl to build off my local copy of backplane-cli and verified that osdctl network verify-egress now uses the proxy with these fixes. openshift/osdctl#482

codecov-commenter · 2023-11-18T22:56:03Z

Codecov Report

Merging #257 (b75e6ce) into main (23570ca) will decrease coverage by 3.22%.
The diff coverage is 14.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #257      +/-   ##
==========================================
- Coverage   51.87%   48.66%   -3.22%     
==========================================
  Files          52       52              
  Lines        3433     3438       +5     
==========================================
- Hits         1781     1673     -108     
- Misses       1357     1504     +147     
+ Partials      295      261      -34

Files	Coverage Δ
cmd/ocm-backplane/cloud/common.go	`59.64% <100.00%> (-33.58%)`	⬇️
pkg/utils/cluster.go	`88.23% <ø> (+3.32%)`	⬆️
pkg/utils/ocmWrapper.go	`0.00% <0.00%> (ø)`
pkg/credentials/aws.go	`21.05% <0.00%> (-78.95%)`	⬇️
cmd/ocm-backplane/cloud/credentials.go	`40.31% <0.00%> (-22.19%)`	⬇️

... and 3 files with indirect coverage changes

This was originally added in #66, but was mistakenly removed in #237 Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file. Signed-off-by: Michael Shen <mshen@redhat.com>

samanthajayasinghe · 2023-11-19T20:30:05Z

cmd/ocm-backplane/cloud/credentials_test.go

-		It("should fail when AWS Unavailable", func() {
-			fakeAWSResp.StatusCode = http.StatusInternalServerError
-
-			mockClientUtil.EXPECT().GetBackplaneClient(proxyURI).Return(mockClient, nil).AnyTimes()
-			mockClient.EXPECT().GetCloudCredentials(gomock.Any(), trueClusterID).Return(fakeAWSResp, nil)
-			_, err := getCloudCredential(proxyURI, trueClusterID)
-			Expect(err).NotTo(BeNil())
-
-			Expect(err.Error()).To(ContainSubstring("error from backplane: \n Status Code: 500\n"))
-
-		})
-
-		It("should fail when GCP Unavailable", func() {
-			fakeGCloudResp.StatusCode = http.StatusInternalServerError
-
-			mockClientUtil.EXPECT().GetBackplaneClient(proxyURI).Return(mockClient, nil).AnyTimes()
-			mockClient.EXPECT().GetCloudCredentials(gomock.Any(), trueClusterID).Return(fakeGCloudResp, nil)
-			_, err := getCloudCredential(proxyURI, trueClusterID)
-			Expect(err).NotTo(BeNil())
-
-			Expect(err.Error()).To(ContainSubstring("error from backplane: \n Status Code: 500\n"))
-


Any reson to remove existing tests with AWS v2 approch ?

I'm ok with receiving pushback on this, but to share my reasoning mostly because they aren't passing anymore and it's critical to make sure that osdctl AWS calls are going through the proxy haha.

My personal preference is to refactor/add some tests back in a future PR, but I can add it into this PR if that's best. My functional "changes" are in the first commit in the PR and you can see all the refactored/mostly removed tests in the second commit.

Good point, @mjlshen .. Let's add/refactor unit-test in future PR's .. ( we are on a journey to reach 80% code coverage for bp-cli :) )

samanthajayasinghe · 2023-11-19T20:45:24Z

pkg/credentials/aws.go

+	proxyURL, err := url.Parse(bpConfig.ProxyURL)
+	if err != nil {
+		return aws.Config{}, fmt.Errorf("failed to parse proxy_url from backplane config file: %w", err)
+	}


This may already checked when login to the cluster

backplane-cli/pkg/cli/config/config.go

Line 99 in 23570ca

if config.ProxyURL != "" {

Yeah, but this function will not be called after ocm backplane login - e.g. https://github.com/openshift/osdctl/blob/1b906de67017e15057a6312d698ac28565250941/pkg/osdCloud/aws.go#L182-L190 so we cannot depend on that check

Though in general, ocm backplane cloud * can be called without needing to ocm backplane login

ahh got your point, We should move all config validation to pkg/cli/config rather than individual sub-commands .. I'll create a card for moving that. I'm fine to keep it here for now.

thanks @samanthajayasinghe , let's create a card for tracking this.

Signed-off-by: Michael Shen <mshen@redhat.com>

openshift-ci · 2023-11-19T21:59:42Z

@mjlshen: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wanghaoran1988 · 2023-11-19T22:44:36Z

/lgtm
/approve

openshift-ci · 2023-11-19T22:45:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjlshen, wanghaoran1988

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wanghaoran1988]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from MitaliBhalla and samanthajayasinghe November 18, 2023 22:09

mjlshen mentioned this pull request Nov 18, 2023

Update backplane-cli dependency for AWSV2Config openshift/osdctl#482

Merged

mjlshen force-pushed the OSD-19711 branch 2 times, most recently from 4d2a30c to a644f77 Compare November 19, 2023 20:25

mjlshen force-pushed the OSD-19711 branch from a644f77 to 2142d25 Compare November 19, 2023 20:27

samanthajayasinghe reviewed Nov 19, 2023

View reviewed changes

mjlshen force-pushed the OSD-19711 branch from 2142d25 to 728b8fd Compare November 19, 2023 21:45

Remove leftover mocks and misleading tests

b75e6ce

Signed-off-by: Michael Shen <mshen@redhat.com>

mjlshen force-pushed the OSD-19711 branch from 728b8fd to b75e6ce Compare November 19, 2023 21:54

openshift-ci bot assigned wanghaoran1988 Nov 19, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2023

openshift-merge-bot bot merged commit aa6130d into openshift:main Nov 19, 2023
7 checks passed

mjlshen deleted the OSD-19711 branch November 19, 2023 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement a way to retrieve an aws-sdk-go-v2 config from backplane #257

Reimplement a way to retrieve an aws-sdk-go-v2 config from backplane #257

mjlshen commented Nov 18, 2023 •

edited by openshift-ci bot

Loading

mjlshen commented Nov 18, 2023

codecov-commenter commented Nov 18, 2023 •

edited

Loading

samanthajayasinghe Nov 19, 2023

mjlshen Nov 19, 2023

samanthajayasinghe Nov 19, 2023

samanthajayasinghe Nov 19, 2023

mjlshen Nov 19, 2023 •

edited

Loading

samanthajayasinghe Nov 19, 2023

wanghaoran1988 Nov 19, 2023

openshift-ci bot commented Nov 19, 2023

wanghaoran1988 commented Nov 19, 2023

openshift-ci bot commented Nov 19, 2023

Reimplement a way to retrieve an aws-sdk-go-v2 config from backplane #257

Reimplement a way to retrieve an aws-sdk-go-v2 config from backplane #257

Conversation

mjlshen commented Nov 18, 2023 • edited by openshift-ci bot Loading

What type of PR is this?

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Special notes for your reviewer

Pre-checks (if applicable)

mjlshen commented Nov 18, 2023

codecov-commenter commented Nov 18, 2023 • edited Loading

Codecov Report

samanthajayasinghe Nov 19, 2023

Choose a reason for hiding this comment

mjlshen Nov 19, 2023

Choose a reason for hiding this comment

samanthajayasinghe Nov 19, 2023

Choose a reason for hiding this comment

samanthajayasinghe Nov 19, 2023

Choose a reason for hiding this comment

mjlshen Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

samanthajayasinghe Nov 19, 2023

Choose a reason for hiding this comment

wanghaoran1988 Nov 19, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Nov 19, 2023

wanghaoran1988 commented Nov 19, 2023

openshift-ci bot commented Nov 19, 2023

mjlshen commented Nov 18, 2023 •

edited by openshift-ci bot

Loading

codecov-commenter commented Nov 18, 2023 •

edited

Loading

mjlshen Nov 19, 2023 •

edited

Loading