Adds CRUD service for Identity Center resources #47609

tcsc · 2024-10-16T04:52:26Z

This is just the pure CRUD read/write. Caching & watching support coming as
necessary in subsequent PRs.

lib/services/identitycenter.go

smallinsky · 2024-10-16T08:05:40Z

lib/services/identitycenter.go

+ CreateAccountAssignment(context.Context, IdentityCenterAccountAssignment) (IdentityCenterAccountAssignment, error)
+
+ // GetAccountAssignment fetches a specific Account Assignment record.
+ GetAccountAssignment(context.Context, IdentityCenterAccountAssignmentID) (IdentityCenterAccountAssignment, error)


nit: might be a personal preference but for me It it a bit overkill to hide and wrap only 1 id string parameter with the separate type IdentityCenterAccountAssignmentID.

I can see that. The rationale for doing this is all in the Identity Center integration code, which you can't see yet.

Rationale is that there are so many different resource types in flight in the IC integration, so I wanted to make using the wrong ID in the wrong place a compile-time error rather than a debugging session. The CRUD service seemed a reasonable boundary to pack/unpack the IDs, so that everything downstream is checked by the compiler.

But yeah, in isolation, it's very much overkill.

lib/services/local/identitycenter.go

smallinsky · 2024-10-16T08:09:18Z

lib/services/local/identitycenter.go

+
+// UpdateIdentityCenterAccount performs a conditional update on an Identity
+// Center Account record, returning the updated record on success.
+func (svc *IdentityCenterService) UpdateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {


Suggested change

func (svc *IdentityCenterService) UpdateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {

func (svc *IdentityCenterService) UpdateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (*services.IdentityCenterAccount, error) {

smallinsky · 2024-10-16T08:09:26Z

lib/services/local/identitycenter.go

+// Identity Center Account record, returning the updated record on success.
+// Be careful when mixing UpsertIdentityCenterAccount() with resources
+// protected by optimistic locking
+func (svc *IdentityCenterService) UpsertIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {


Suggested change

func (svc *IdentityCenterService) UpsertIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {

func (svc *IdentityCenterService) UpsertIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (*services.IdentityCenterAccount, error) {

smallinsky · 2024-10-16T08:09:50Z

lib/services/local/identitycenter.go

+}
+
+// GetIdentityCenterAccount fetches a specific Identity Center Account
+func (svc *IdentityCenterService) GetIdentityCenterAccount(ctx context.Context, name services.IdentityCenterAccountID) (services.IdentityCenterAccount, error) {


Suggested change

func (svc *IdentityCenterService) GetIdentityCenterAccount(ctx context.Context, name services.IdentityCenterAccountID) (services.IdentityCenterAccount, error) {

func (svc *IdentityCenterService) GetIdentityCenterAccount(ctx context.Context, name services.IdentityCenterAccountID) (*services.IdentityCenterAccount, error) {

smallinsky · 2024-10-16T08:09:56Z

lib/services/local/identitycenter.go

+}
+
+// CreateIdentityCenterAccount creates a new Identity Center Account record
+func (svc *IdentityCenterService) CreateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {


Suggested change

func (svc *IdentityCenterService) CreateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {

func (svc *IdentityCenterService) CreateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (*services.IdentityCenterAccount, error) {

The services.IdentityCenterAccount is already just a pointer wrapped in a struct.

I can see making the IdentityCenterService take/return pointers for the sake of consistency, but I didn't think the extra indirection was worth it.

smallinsky · 2024-10-16T08:17:10Z

lib/services/identitycenter.go

+// allow it to implement the interfaces required for use with the Unified
+// Resource listing. IdentityCenterAccount simply wraps a pointer to the


Just to confirm. Does the IdentityCenterAccount will be handled in united resource view ?
I thought that in UI the AWS IC account will be represented by Teleport Application not by IdentityCenterAccount resource ?

Does the IdentityCenterAccount will be handled in united resource view ?

Not in the proxy but in the auth united resource service. E.g. from the poc implementation we transform IdentityCenterAccount to App Server https://github.com/gravitational/teleport/blob/sshah/idc-dev/lib/services/unified_resource.go#L925

kopiczko

LGTM after proto.Clone is used as @smallinsky suggested (if possible)

kopiczko · 2024-10-16T12:27:01Z

lib/services/identitycenter.go

+// IdentityCenterAccountAssignment wraps a raw identitycenterv1.AccountAssignment
+// record in a new type to allow it to implement the interfaces required for use
+// with the Unified Resource listing. IdentityCenterAccountAssignment simply
+// wraps a pointer to the underlying account record, and can be treated as a
+// reference-like type.
+//
+// Copies of an IdentityCenterAccountAssignment will point to the same record.
+type IdentityCenterAccountAssignment struct {
+ *identitycenterv1.AccountAssignment
+}
+
+// CloneResource creates a deep copy of the underlying account resource
+func (a IdentityCenterAccountAssignment) CloneResource() IdentityCenterAccountAssignment {
+ var expiry *timestamppb.Timestamp
+ if a.Metadata.Expires != nil {
+ expiry = &timestamppb.Timestamp{
+ Seconds: a.Metadata.Expires.Seconds,
+ Nanos: a.Metadata.Expires.Nanos,
+ }
+ }
+
+ return IdentityCenterAccountAssignment{
+ AccountAssignment: &identitycenterv1.AccountAssignment{
+ Kind: a.Kind,
+ SubKind: a.SubKind,
+ Version: a.Version,
+ Metadata: &headerv1.Metadata{
+ Name: a.Metadata.Name,
+ Namespace: a.Metadata.Namespace,
+ Description: a.Metadata.Description,
+ Labels: maps.Clone(a.Metadata.Labels),
+ Expires: expiry,
+ Revision: a.Metadata.Revision,
+ },
+ Spec: &identitycenterv1.AccountAssignmentSpec{
+ Display: a.Spec.Display,
+ AccountId: a.Spec.AccountId,
+ AccountName: a.Spec.AccountName,
+ PermissionSet: &identitycenterv1.PermissionSetInfo{
+ Arn: a.Spec.PermissionSet.Arn,
+ Name: a.Spec.PermissionSet.Name,
+ },
+ },
+ },
+ }
+}


Maybe then it makes sense to:

type IdentityCenterAccount identitycenterv1.Account // CloneResource creates a deep copy of the underlying account resource func (a *IdentityCenterAccount) CloneResource() *IdentityCenterAccount { ... }

I got triggered by those services.IdentityCenterAccount{Account: acct} statements in the implementation

I tried that originally - unfortunately it hides all of the other interfaces I need on the Account. I've added some commentary to explain the odd choice of embedding the pointer in a struct.

lib/services/local/identitycenter_test.go

lib/services/local/events_test.go

api/types/constants.go

lib/auth/auth.go

flyinghermit · 2024-10-16T14:52:34Z

lib/services/identitycenter.go

+// allow it to implement the interfaces required for use with the Unified
+// Resource listing. IdentityCenterAccount simply wraps a pointer to the


Does the IdentityCenterAccount will be handled in united resource view ?

Not in the proxy but in the auth united resource service. E.g. from the poc implementation we transform IdentityCenterAccount to App Server https://github.com/gravitational/teleport/blob/sshah/idc-dev/lib/services/unified_resource.go#L925

flyinghermit

Looks good once existing suggestions by other reviewers are addressed.

lib/services/identitycenter.go

lib/services/local/identitycenter_test.go

This is just the pure CRUD read/write. Caching & watching support coming as necessary in subsequent PRs. Co-authored-by: Pawel Kopiczko <[email protected]> Co-authored-by: Sakshyam Shah <[email protected]>

kopiczko · 2024-10-17T08:29:09Z

lib/services/identitycenter.go

+}
+
+// CloneResource creates a deep copy of the underlying account resource
+func (a *IdentityCenterAccount) CloneResource() IdentityCenterAccount {


Just spotted this one is a pointer receiver and the other one is a value receiver func (a IdentityCenterAccountAssignment) CloneResource()

tcsc added the no-changelog Indicates that a PR does not require a changelog entry label Oct 16, 2024

tcsc requested review from kopiczko, flyinghermit and smallinsky October 16, 2024 04:53

github-actions bot requested review from kimlisa and marcoandredinis October 16, 2024 04:53

github-actions bot added the size/lg label Oct 16, 2024

smallinsky reviewed Oct 16, 2024

View reviewed changes

kopiczko approved these changes Oct 16, 2024

View reviewed changes

flyinghermit reviewed Oct 16, 2024

View reviewed changes

marcoandredinis removed their request for review October 16, 2024 15:09

flyinghermit approved these changes Oct 16, 2024

View reviewed changes

Adds CRUD service for Identity Center resources

c547c76

This is just the pure CRUD read/write. Caching & watching support coming as necessary in subsequent PRs. Co-authored-by: Pawel Kopiczko <[email protected]> Co-authored-by: Sakshyam Shah <[email protected]>

tcsc force-pushed the tcsc/identitycenter-crud branch from 1162cae to c547c76 Compare October 17, 2024 03:09

smallinsky approved these changes Oct 17, 2024

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from kimlisa October 17, 2024 07:03

kopiczko reviewed Oct 17, 2024

View reviewed changes

Merge branch 'master' into tcsc/identitycenter-crud

086a371

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds CRUD service for Identity Center resources #47609

Adds CRUD service for Identity Center resources #47609

tcsc commented Oct 16, 2024

smallinsky Oct 16, 2024

tcsc Oct 17, 2024

smallinsky Oct 16, 2024

smallinsky Oct 16, 2024

smallinsky Oct 16, 2024

smallinsky Oct 16, 2024

tcsc Oct 17, 2024

smallinsky Oct 16, 2024

flyinghermit Oct 16, 2024

kopiczko left a comment

kopiczko Oct 16, 2024

tcsc Oct 17, 2024

flyinghermit Oct 16, 2024

flyinghermit left a comment

kopiczko Oct 17, 2024

	func (svc *IdentityCenterService) UpdateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {
	func (svc IdentityCenterService) UpdateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {

	func (svc *IdentityCenterService) UpsertIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {
	func (svc IdentityCenterService) UpsertIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {

	func (svc *IdentityCenterService) GetIdentityCenterAccount(ctx context.Context, name services.IdentityCenterAccountID) (services.IdentityCenterAccount, error) {
	func (svc IdentityCenterService) GetIdentityCenterAccount(ctx context.Context, name services.IdentityCenterAccountID) (services.IdentityCenterAccount, error) {

	func (svc *IdentityCenterService) CreateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {
	func (svc IdentityCenterService) CreateIdentityCenterAccount(ctx context.Context, acct services.IdentityCenterAccount) (services.IdentityCenterAccount, error) {

		// allow it to implement the interfaces required for use with the Unified
		// Resource listing. IdentityCenterAccount simply wraps a pointer to the

Adds CRUD service for Identity Center resources #47609

Are you sure you want to change the base?

Adds CRUD service for Identity Center resources #47609

Conversation

tcsc commented Oct 16, 2024

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

kopiczko left a 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

flyinghermit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment