ROX-14544 - Add CentralRequest field documentation #743

SimonBaeumer · 2023-01-20T09:38:14Z

Description

Added CentralRequest documentation.

Checklist (Definition of Done)

Unit and integration tests added
Added test description under Test manual
Evaluated and added CHANGELOG.md entry if required
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

openshift-ci · 2023-01-20T09:38:20Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

SimonBaeumer · 2023-01-20T12:14:32Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

+	Region string `json:"region"`
+	// ClusterID is the data-plane cluster ID
+	ClusterID string `json:"cluster_id" gorm:"index"`
+	// CloudProvider ...
 	CloudProvider  string `json:"cloud_provider"`


Double-check in production in production system

Cloud provider that runs the data plane instance. Also the cloud provider that owns the customer account we bill again.

SimonBaeumer · 2023-01-20T12:15:00Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

+	Region string `json:"region"`
+	// ClusterID is the data-plane cluster ID
+	ClusterID string `json:"cluster_id" gorm:"index"`
+	// CloudProvider ...
 	CloudProvider  string `json:"cloud_provider"`
 	CloudAccountID string `json:"cloud_account_id"`


Double-check in production in production system. Maybe remove it (cc @johannes94)

Maybe it is used by @kovayur, see:

rr.BillingMarketplaceAccount(dinosaur.CloudAccountID)

This is the billing cloud account, definitely don't remove or we won't get paid ;-).

SimonBaeumer · 2023-01-20T12:16:41Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	MultiAZ        bool   `json:"multi_az"`
+	// MultiAZ enables multi AZ support
+	MultiAZ bool `json:"multi_az"`
+	// Name of the ACS instance
 	Name           string `json:"name" gorm:"index"`
 	Status         string `json:"status" gorm:"index"`


Lifecycle status of the CentralRequest, link to constants

SimonBaeumer · 2023-01-20T12:17:30Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	MultiAZ        bool   `json:"multi_az"`
+	// MultiAZ enables multi AZ support
+	MultiAZ bool `json:"multi_az"`
+	// Name of the ACS instance
 	Name           string `json:"name" gorm:"index"`
 	Status         string `json:"status" gorm:"index"`
 	SubscriptionID string `json:"subscription_id"`


by @kovayur

SubscriptionID is returned by ams (ocm) when the user’s (owner) quota is reserved for Central

Owner = OCM owner who created the tenant (email), how is it populated? (Identify ID type, seems to be different for RH employees joined after a specific date, or based on the preferred username (OpenID feature, ocm whoami prints the active username))
OwnerAccountID and OrganisationID maybe the same fields.

Code snippet:

dinosaurRequest.Owner, _ = claims.GetUsername() dinosaurRequest.OrganisationID, _ = claims.GetOrgID() dinosaurRequest.OwnerAccountID, _ = claims.GetAccountID() dinosaurRequest.OwnerUserID, _ = claims.GetUserID() tenantUsernameClaim = "username" //-------------- // GetUsername ... func (c *ACSClaims) GetUsername() (string, error) { if idx, val := arrays.FindFirst(func(x interface{}) bool { return x != nil }, (*c)[tenantUsernameClaim], (*c)[alternateTenantUsernameClaim]); idx != -1 { if userName, ok := val.(string); ok { return userName, nil } } return "", fmt.Errorf("can't find neither %q or %q attribute in claims", tenantUsernameClaim, alternateTenantUsernameClaim) } //-------------- // sso.redhat.com token claim keys. alternateTenantUsernameClaim = "preferred_username" tenantUserIDClaim = "account_id" tenantSubClaim = "sub" // Only service accounts that have been created via the service_accounts API have this claim set. alternateTenantIDClaim = "rh-org-id

OwnerUserID, OwnerAccountID needs investigation. Is the same in @johannes94 database. Not equal to organisation ID.

organisation ID and organisation name are the Red Hat SSO organisation identifiers - it identifies a customer organisation, e.g. 16678382 and IBM. We need the id for authn/z, and the name for observability purposes. There can be many users that part of one customer organisation - for example, all Red Hat employee accounts are part of org_id=11009103.

OwnerAccountID will be used in telemetry, it is the account_id claim of the Red Hat SSO token. OwnerUserID is the subject claim (confusingly it is NOT the user_id claim) of the Red Hat SSO token - this one I don't know if we use it anywhere.

Owner is the Red Hat SSO login name. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.

@stehessel Where do you see the OrganisationName? I could find the field in the CentralRequest struct.

Rebased and added it

stehessel · 2023-01-21T15:46:32Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	MultiAZ        bool   `json:"multi_az"`
+	// MultiAZ enables multi AZ support
+	MultiAZ bool `json:"multi_az"`
+	// Name of the ACS instance
 	Name           string `json:"name" gorm:"index"`
 	Status         string `json:"status" gorm:"index"`
 	SubscriptionID string `json:"subscription_id"`


SubscriptionID is returned by AMS and identifies a Central instance in their system. We need it to deregister instances again from AMS.

stehessel · 2023-02-15T11:16:06Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	Region         string `json:"region"`
-	ClusterID      string `json:"cluster_id" gorm:"index"`
-	CloudProvider  string `json:"cloud_provider"`
+	// Region is the cloud region the service is deployed in, i.e. us-east-1


Can you end sentence comments with a "." everywhere?

stehessel · 2023-02-15T11:17:21Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	MultiAZ        bool   `json:"multi_az"`
-	Name           string `json:"name" gorm:"index"`
-	Status         string `json:"status" gorm:"index"`
+	// MultiAZ enables multi AZ support


I'd spell out availability zone in the explanation.

stehessel · 2023-02-15T11:18:26Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

 	SubscriptionID string `json:"subscription_id"`
-	Owner          string `json:"owner" gorm:"index"` // TODO: ocm owner?
+	// Owner is the Red Hat SSO login name. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.


Suggested change

// Owner is the Red Hat SSO login name. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.

// Owner is the Red Hat SSO login name of the user who created the instance. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.

stehessel · 2023-02-15T11:19:09Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	Owner          string `json:"owner" gorm:"index"` // TODO: ocm owner?
+	// Owner is the Red Hat SSO login name. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.
+	Owner string `json:"owner" gorm:"index"`
+	// OwnerAccountID is used in telemetry, it is the account_id claim of the Red Hat SSO token.


We switched to user_id claim in telemetry. owner_account_id is no longer used as far as I can tell.

It is never read anywhere, only set once in fleet-manager. Is it safe to assume that we can remove it?

How could I test that the change worked?

How could I test that the change worked?

You can take a look at Telemetry events in https://app.segment.com/redhat-devtools/sources/acs_instances_backend_dev/debugger.

Is it safe to assume that we can remove it?

I think we can remove this field, yes. Just need to be careful with backwards/forwards compatibility.

Would it be okay for you to mark it here as deprecated and removing the field in a follow-up ticket?

Yeah that makes sense.

stehessel · 2023-02-15T11:21:18Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

-	OrganisationID   string `json:"organisation_id" gorm:"index"`
+	Host string `json:"host"`
+	// OrganisationID is a Red Hat SSO organisation identifier to identify a customer. It is needed as an id for authn/z, and the name for observability purposes.
+	OrganisationID string `json:"organisation_id" gorm:"index"`


This is not 100% true (depends on definition of customer), it is just one customer organisation. For example IBM has many Red Hat orgs.

Changed it to:
// OrganisationID identifies a customer's organisation. It is needed as an id for authn/z, and the name for observability purposes.

stehessel · 2023-02-15T11:22:21Z

internal/dinosaur/pkg/api/dbapi/central_request_types.go

+	Host string `json:"host"`
+	// OrganisationID is a Red Hat SSO organisation identifier to identify a customer. It is needed as an id for authn/z, and the name for observability purposes.
+	OrganisationID string `json:"organisation_id" gorm:"index"`
+	// OrganisationName is a Red Hat SSO organisation identifier to identify a customer. It is needed as an id for authn/z, and the name for observability purposes.


Organisation names not unique. Its purpose is mostly human readability and observability purposes (e.g. display in dashboards).

stehessel

Thanks for going over these 🚀

openshift-ci · 2023-02-15T13:48:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SimonBaeumer, 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~~ [SimonBaeumer]

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

openshift-ci · 2023-02-15T14:31:28Z

New changes are detected. LGTM label has been removed.

openshift-ci bot added the do-not-merge/work-in-progress label Jan 20, 2023

SimonBaeumer temporarily deployed to development January 20, 2023 09:38 — with GitHub Actions Inactive

openshift-ci bot added the approved label Jan 20, 2023

SimonBaeumer commented Jan 20, 2023

View reviewed changes

rukletsov self-requested a review January 20, 2023 17:13

stehessel reviewed Jan 21, 2023

View reviewed changes

ivan-degtiarenko mentioned this pull request Jan 26, 2023

Add documentation to AuthConfig fields. #766

Merged

2 tasks

openshift-merge-robot added the needs-rebase label Feb 15, 2023

SimonBaeumer force-pushed the sb/add-doc branch from 55a38f3 to e48a8a3 Compare February 15, 2023 10:28

add more comments

aa9d806

SimonBaeumer force-pushed the sb/add-doc branch from e48a8a3 to aa9d806 Compare February 15, 2023 10:30

SimonBaeumer temporarily deployed to development February 15, 2023 10:31 — with GitHub Actions Inactive

openshift-merge-robot removed the needs-rebase label Feb 15, 2023

SimonBaeumer requested a review from stehessel February 15, 2023 10:31

SimonBaeumer marked this pull request as ready for review February 15, 2023 10:31

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 15, 2023

SimonBaeumer temporarily deployed to development February 15, 2023 10:31 — with GitHub Actions Inactive

SimonBaeumer changed the title ~~Add CentralRequest field documentation~~ ROX-14544 - Add CentralRequest field documentation Feb 15, 2023

stehessel reviewed Feb 15, 2023

View reviewed changes

CR comments

94daa80

SimonBaeumer temporarily deployed to development February 15, 2023 12:48 — with GitHub Actions Inactive

SimonBaeumer requested a review from stehessel February 15, 2023 12:58

stehessel approved these changes Feb 15, 2023

View reviewed changes

openshift-ci bot assigned stehessel Feb 15, 2023

openshift-ci bot added the lgtm label Feb 15, 2023

add deprecation message

54b2ce9

openshift-ci bot removed the lgtm label Feb 15, 2023

SimonBaeumer temporarily deployed to development February 15, 2023 14:31 — with GitHub Actions Inactive

SimonBaeumer enabled auto-merge (squash) February 15, 2023 14:34

SimonBaeumer merged commit dc1c855 into main Feb 15, 2023

SimonBaeumer deleted the sb/add-doc branch February 15, 2023 15:06

	// Owner is the Red Hat SSO login name. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.
	// Owner is the Red Hat SSO login name of the user who created the instance. It is either the email, or the user name, depending on what the user chose to login with. It's displayed in the console UI.

ROX-14544 - Add CentralRequest field documentation #743

ROX-14544 - Add CentralRequest field documentation #743

Conversation

SimonBaeumer commented Jan 20, 2023 • edited Loading

Description

Checklist (Definition of Done)

Test manual

openshift-ci bot commented Jan 20, 2023

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

Choose a reason for hiding this comment

SimonBaeumer Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

stehessel Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

stehessel Jan 21, 2023 • 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

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

SimonBaeumer Feb 15, 2023 • 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

stehessel left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 15, 2023

openshift-ci bot commented Feb 15, 2023

SimonBaeumer commented Jan 20, 2023 •

edited

Loading

SimonBaeumer Jan 20, 2023 •

edited

Loading

stehessel Jan 21, 2023 •

edited

Loading

stehessel Jan 21, 2023 •

edited

Loading

SimonBaeumer Feb 15, 2023 •

edited

Loading