Skip to content
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.

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

feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch #187

Conversation

bearaujus
Copy link

@bearaujus bearaujus commented Nov 14, 2024

Steps for Alicloud IAM Plugin Implementation:

  • Continue develop on this branch bearaujus/implement-alicloudiam-plugin
  • After develop phase is done, next step I will create another PR to merge bearaujus/implement-alicloudiam-plugin into main branch (Will also request @rahmatrhd for this. For developing on branch bearaujus/implement-alicloudiam-plugin we just need internal review)

*PS: For this PR please review the main flow only and ignore another typos, todos.
That all will be fixed and tidy up on the next PR included with the onboarding of new proto variable for role type, unitests & all things related to implementation completion! :)

Thank you 🙏

@bearaujus bearaujus changed the title Feat(plugins,domain): Implement Alicloud IAM plugin feat(plugins,domain): Implement Alicloud IAM plugin Nov 14, 2024
@bearaujus bearaujus changed the title feat(plugins,domain): Implement Alicloud IAM plugin feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch Nov 14, 2024
bearaujus added a commit to goto/proton that referenced this pull request Nov 14, 2024
This field purposed due to AliCloudIAM plugin requirement.
MVP for the plugin PR: goto/guardian#187

After some research we need to add type to role message.
First you can read this docs: https://www.alibabacloud.com/help/en/ram/developer-reference/api-ram-2015-05-01-getpolicy.

On that docs there was field "PolicyType". Policy type has two constant which is:
- System
- Custom
anjali9791 pushed a commit to goto/proton that referenced this pull request Nov 15, 2024
This field purposed due to AliCloudIAM plugin requirement.
MVP for the plugin PR: goto/guardian#187

After some research we need to add type to role message.
First you can read this docs: https://www.alibabacloud.com/help/en/ram/developer-reference/api-ram-2015-05-01-getpolicy.

On that docs there was field "PolicyType". Policy type has two constant which is:
- System
- Custom
Makefile Outdated Show resolved Hide resolved
return c.EncryptCredentials()
}

func (p *Provider) GetResources(_ context.Context, pc *domain.ProviderConfig) ([]*domain.Resource, error) {

Choose a reason for hiding this comment

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

We can remove context as the param since it is not used.

Copy link
Author

Choose a reason for hiding this comment

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

We cannot remove it. It required to implement this interface:
https://github.com/goto/guardian/blob/main/plugins/providers/client.go

And that interface will be used on initializing new providers at:
https://github.com/goto/guardian/blob/main/internal/server/services.go#L114

Copy link
Author

Choose a reason for hiding this comment

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

This was likely added to encourage implementing requests with a context. By doing so, if a request is sent to a provider, it won’t block indefinitely and will automatically cancel all ongoing processes when the parent context expires or is canceled.

Copy link
Author

@bearaujus bearaujus Nov 16, 2024

Choose a reason for hiding this comment

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

But since Ali SDK has no support for it, I already raised the question here:
aliyun/darabonba-openapi#171

@bearaujus
Copy link
Author

bearaujus commented Nov 16, 2024

I’ve already raised a question about the parent context TODO here:
aliyun/darabonba-openapi#171

If the SDK doesn’t support it yet, I’ll leave it as-is for now.

@anjali9791 anjali9791 merged commit 7480d87 into goto:bearaujus/implement-alicloudiam-plugin Nov 18, 2024
3 checks passed
@bearaujus
Copy link
Author

Parent PR:
#190

rahmatrhd added a commit that referenced this pull request Dec 26, 2024
* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch (#187)

* feat(plugins,domain): Implement MVP for Alicloud IAM plugin

* feat(plugins,domain): Implement MVP for Alicloud IAM plugin patch

* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch patch 2

* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch patch 3

* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch patch 4

* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch patch 5

* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch patch 6

* feat(plugins,domain): Implement Alicloud IAM plugin to dummy branch patch 7

* feat: add support for cross account access management

* feat(plugins): refactor create iam client & fix unittests for alicloud_iam plugin

* fix(plugins): remove async process and un-implement list access

* feat(plugins): change permissions type & improve validation

* feat(plugins): fix alicloud sdk client race condition

This occur because AliCloud SDK is using builder pattern when receiving and sending request to their own API. Because of this, we need to create a new client each time we invoking a request.

* feat(plugins): cleanup go sum

* feat(plugins): simplify expression & fix error check

* feat(plugins): add more test cases

* feat(plugins): update alicloudiam plugin docs

* fix: resolve comments

* fix: resolve remaining conflict

* feat: adjust logic, unittests & add readme

* feat: fix readme

* feat: add more documentation and role validation

* feat: add support for region on client creation

* fix: add struct tag for region id

* Update ali-role-arn-structure.png

* fix: fix account types

* fix: client caching not updated

---------

Co-authored-by: anjali.agarwal <anjali.aggarwal@gojek.com>
Co-authored-by: Rahmat Hidayat <rahmat.hd@gojek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants