ROX-12151: cloud accounts endpoint #517

ivan-degtiarenko · 2022-10-25T02:16:40Z

Description

Introduces the call to AMS client as well as new endpoint. The organization ID is taken from the caller's claims.

Checklist (Definition of Done)

Unit and integration tests added
Added test description under Test manual
Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
CI and all relevant tests are passing
Add the ticket number to the PR title if available, i.e. ROX-12345: ...
Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.

Test manual

TODO: Add manual testing efforts

./fleet-manager serve --ocm-base-url=https://api.openshift.com --ocm-client-id-file=secrets/ocm-service.clientId --ocm-client-secret-file=secrets/ocm-service.clientSecret
GET http://localhost:8000/api/rhacs/v1/cloud_accounts with rhacs-test-customer Bearer token
Returned result:

{
    "cloudAccounts": [
        {
            "cloudAccountId": "<redacted>",
            "cloudProviderId": "aws"
        }
    ]
}

stehessel · 2022-10-25T09:37:43Z

pkg/client/ocm/client.go

+		return nil, fmt.Errorf("error getting cloud accounts: %w", err)
+	}
+
+	quotaCostList.Items().Each(func(qc *amsv1.QuotaCost) bool {


This will list all cloud accounts, right? We should filter here on the cloud accounts that are attached to our marketplace SKU, i.e. "quota_id": "cluster|rhinfra|rhacs|marketplace". Otherwise we'll also return cloud accounts that have purchased different products.

stehessel · 2022-10-25T09:39:10Z

pkg/client/ocm/client.go

@@ -600,3 +602,26 @@ func (c client) GetQuotaCostsForProduct(organizationID, resourceName, product st

 	return res, nil
 }
+
+func (c *client) GetCustomerCloudAccounts(externalID string) ([]*amsv1.CloudAccount, error) {


Can you add a unit test for this function using the generated mocks?

Do you mean cloud_accounts_test.go for the endpoint handler? There's no way to mock sdkClient.Connection used in client.go

Hm yes. I think you could write a unit test with the cloud_accounts_test.mock, and you could also take a look at https://github.com/stackrox/acs-fleet-manager/blob/main/internal/dinosaur/pkg/services/quota/ams_quota_service_test.go for how to mock the OCM client. With the latter you could test the SKU filter logic.

sachaudh · 2022-10-25T20:22:47Z

internal/dinosaur/pkg/api/public/api/openapi.yaml

@@ -608,6 +608,37 @@ paths:
      security:
      - Bearer: []
      summary: Returns the list of supported regions of the supported cloud provider
+  /api/rhacs/v1/cloud_accounts:
+    get:
+      operationId: getCloudAccounts


The endpoint looks good to me. The only thing I'm wondering about is this operationId. @stehessel mentioned in his comment how to access the organization.id using ocm command. Is operationId supposed to be that? Also, accessing that id using the ocm command is helpful, but I can't do that through the browser so I'll need to find another way to do that. If you or @stehessel might know how that's possible, let me know. I am searching through https://api.openshift.com/ to see if there might be some API endpoint that might give me that information

Is /api/accounts_mgmt/v1/organizations the API i'm looking for? Looks like that gives me a list of organizations (there seems to be just one though). I see fields like id and external_id which seem relevant

@sachaudh organization.id will be extracted by fleet-manager from AMS using orgId provided in the Bearer token. The endpoint doesn't take any parameters.

operationId is swagger parameter that is used to uniquely identify the operation within the API spec:
https://swagger.io/docs/specification/paths-and-operations/

Ok, then I think that's fine with me. Thanks for adding the endpoint @ivan-degtiarenko 👍🏼

stehessel · 2022-10-26T21:02:56Z

pkg/client/ocm/client.go

+
+func (c *client) GetCustomerCloudAccounts(externalID string, quotaIDs []string) ([]*amsv1.CloudAccount, error) {
+	var res []*amsv1.CloudAccount
+	organizationClient := c.connection.AccountsMgmt().V1().Organizations()


There is already GetQuotaCostsForProduct - seems like it's what we want, it's only missing a fetchCloudAccounts=true flag. Maybe we could reuse this here?

Not really - GetQuotaCostsForProduct does filtration by resourceName and product

In principle that is what we also want. But I think you are right, we would want to filter for billing_model==marketplace as well. Easier to go the quota_id route then.

stehessel · 2022-10-26T21:05:29Z

internal/dinosaur/pkg/handlers/cloud_accounts_test.go

+	JwtCAFile  = "test/support/jwt_ca.pem"
+)
+
+func TestGetSuccess(t *testing.T) {


It would be good to test GetCustomerCloudAccounts as well, in particular testing that the filtering by resource id works. I think this is possible by mocking the other OCM client methods like QuotaCost (or GetQuotaCostsForProduct).

GetQuotaCostsForProduct can't be reused for the reasons mentioned above. I'm not sure how one can mock QuotaCost method on the organizationClient - it is created by Connection:
https://github.com/stackrox/acs-fleet-manager/blob/ivan%2FROX-12151-aws-accounts-endpoint/pkg/client/ocm/client.go#L608-L608

Hm yes that is impractical, I thought I saw it in the method list of the client, but you are right it's not. That would leave us with mocks on http level, which maybe are supported via the "mock modes" (https://github.com/stackrox/acs-fleet-manager/blob/ivan/ROX-12151-aws-accounts-endpoint/pkg/client/ocm/config.go#L12). But probably a lot of effort for a simple feature...

stehessel · 2022-10-27T00:08:13Z

internal/dinosaur/pkg/handlers/cloud_accounts.go

+func (h *cloudAccountsHandler) Get(w http.ResponseWriter, r *http.Request) {
+	cfg := &handlers.HandlerConfig{
+		Action: func() (i interface{}, serviceError *errors.ServiceError) {
+			ctx := r.Context()


If you extract inside of this function (func() (i interface{}, serviceError *errors.ServiceError) { ... }, you could test it separately from the http related parts of Get. This would save you some http setup in the cloud_accounts_test.go.

openshift-ci · 2022-10-27T23:15:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivan-degtiarenko, stehessel

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~~ [ivan-degtiarenko]

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

openshift-ci · 2022-10-28T00:17:19Z

New changes are detected. LGTM label has been removed.

openshift-ci bot added do-not-merge/work-in-progress approved labels Oct 25, 2022

stehessel reviewed Oct 25, 2022

View reviewed changes

sachaudh reviewed Oct 25, 2022

View reviewed changes

ivan-degtiarenko changed the title ~~[WIP] Ivan/rox 12151 aws accounts endpoint~~ ROX-12151: cloud accounts endpoint Oct 25, 2022

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 25, 2022

ivan-degtiarenko requested review from stehessel and sachaudh October 25, 2022 23:11

sachaudh mentioned this pull request Oct 26, 2022

ROX-12152: added field for aws account number in central instance creation modal RedHatInsights/acs-ui#31

Merged

2 tasks

stehessel reviewed Oct 26, 2022

View reviewed changes

stehessel reviewed Oct 27, 2022

View reviewed changes

ivan-degtiarenko added 4 commits October 27, 2022 23:51

GET cloud accounts endpoint

66e9885

wrap organization id error

d95da4e

filter by quota id

f083ec1

add unit test; pass quota ids as list

617a45a

stehessel approved these changes Oct 27, 2022

View reviewed changes

openshift-ci bot assigned stehessel Oct 27, 2022

openshift-ci bot added the lgtm label Oct 27, 2022

address feedback

a5c79f5

ivan-degtiarenko force-pushed the ivan/ROX-12151-aws-accounts-endpoint branch from 886031e to a5c79f5 Compare October 28, 2022 00:17

openshift-ci bot removed the lgtm label Oct 28, 2022

ivan-degtiarenko merged commit faa8dc3 into main Oct 28, 2022

ivan-degtiarenko deleted the ivan/ROX-12151-aws-accounts-endpoint branch October 28, 2022 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-12151: cloud accounts endpoint #517

ROX-12151: cloud accounts endpoint #517

ivan-degtiarenko commented Oct 25, 2022 •

edited

Loading

stehessel Oct 25, 2022

stehessel Oct 25, 2022

ivan-degtiarenko Oct 25, 2022

stehessel Oct 25, 2022

sachaudh Oct 25, 2022

sachaudh Oct 25, 2022 •

edited

Loading

ivan-degtiarenko Oct 25, 2022

sachaudh Oct 26, 2022

stehessel Oct 26, 2022

ivan-degtiarenko Oct 27, 2022

stehessel Oct 27, 2022

stehessel Oct 26, 2022

ivan-degtiarenko Oct 27, 2022

stehessel Oct 27, 2022

stehessel Oct 27, 2022

openshift-ci bot commented Oct 27, 2022

openshift-ci bot commented Oct 28, 2022

ROX-12151: cloud accounts endpoint #517

ROX-12151: cloud accounts endpoint #517

Conversation

ivan-degtiarenko commented Oct 25, 2022 • edited Loading

Description

Checklist (Definition of Done)

Test manual

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachaudh Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 27, 2022

openshift-ci bot commented Oct 28, 2022

ivan-degtiarenko commented Oct 25, 2022 •

edited

Loading

sachaudh Oct 25, 2022 •

edited

Loading