-
Notifications
You must be signed in to change notification settings - Fork 86
feat(identity): add support for IAM profile management #1845
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
Conversation
| optional: [ProfileFields.region], | ||
| disallowed: [ProfileFields.sso_account_id, ProfileFields.sso_role_name], | ||
| }, | ||
| IamUserProfile: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The terminology specified in the AWS CLI and SDK and Tools public docs is generally "short-term"/"temporary" credentials (when containing a aws_session_token) or "long-term" credentials (when not). Either way they're just "IAM credentials". The short-term credentials can only represent roles, not users. The long-term can represent both. I think storing both long-term and short-term in the same type with an optional aws_session_token is fine, but the name should probably be IamCredentialsProfile.
| ], | ||
| disallowed: [ProfileFields.credential_source], | ||
| }, | ||
| IamRoleInstanceProfile: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this type of profile can be used for containers as well and to be consistent with IamRoleSourceProfile naming, I would suggest renaming this to IamRoleCredentialSourceProfile (clearly indicating that the credential_source field is being used instead of the source_profile field.
I'd also suggest dropping the Role designation. While roles are more commonly used these days and users aren't supported with some types of profiles, profiles (aside from SSO token profiles) result in just IAM credentials, whether they were generated from a role or user is generally inconsequential.
| optional: [ProfileFields.external_id, ProfileFields.role_session_name, ProfileFields.region], | ||
| disallowed: [ProfileFields.source_profile], | ||
| }, | ||
| IamProcessProfile: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For conistent naming, use IamCredentialProcessProfile.
| this.throwOnInvalidProfile(!params.profile, 'Profile required.') | ||
| const profile = params.profile! | ||
|
|
||
| // Removing this check for profile deletion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this comment mean in this context? Is it supposed to be here?
Also, nothing to do with your PR, but seems like the original code here might've been written before the DuckTypers as this seems to be redundant with what it does. Consider adding a // TODO Can this be refactored and simplified using the existing DuckTypers? here. Don't worry about looking it any further for this project though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, that comment was supposed to be removed. It was in reference to using updateProfile to invalidate credential fields. The plan now is to allow language clients to invalidate credential fields by calling updateProfile with an Unknown profile kind. Note that they cannot invalidate using the IAM profile kinds because the credential fields must be non-null for their duck typers to pass.
| const relevantFields = [...fields.required, ...fields.optional] | ||
| for (const field of relevantFields) { | ||
| if (settings[field] !== undefined) { | ||
| profile.settings![field] = settings[field] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did TS required the ! after settings given it's explicitly defined in line 53 above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes TS still requires ! . I think it's because the settings field is optional in the Profile interface definition.
| accessKeyId: string | ||
| secretAccessKey: string | ||
| sessionToken?: string | ||
| expiration?: Date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this field used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* test: codecov integration * add coverage script for chat-client and lsp core * add c8 configs * use js files for coverage
* feat: add conversation compaction * fix: address comments * fix: add unit tests * fix: fix unit tests
…s#1912) * fix: use document change events for auto trigger classifier input * fix: improve readability
* fix: add active user tracker and emit metrics * fix: add time window state persistence across IDE restarts * fix: lint
* fix: align auto trigger classifier with documentChangeEvent * fix: add unit test
* fix(amazonq): replacing image's large binary in log * fix(amazonq): replacing image's large binary in log
* fix: treat `echo`/`find`/`grep` as mutating as discussed in internal channels * tst
* chore(release): release packages from branch main * build: add missing packagelock file (aws#1927) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Will Lo <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/flare-iam-base #1845 +/- ##
=========================================================
Coverage ? 65.96%
=========================================================
Files ? 238
Lines ? 50623
Branches ? 3385
=========================================================
Hits ? 33393
Misses ? 17170
Partials ? 60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
The identity LSP was changed to load and save IAM profile kinds.
Problem
The identity LSP does not support saving and loading IAM profiles. This forces IDE extensions to manage their own IAM profiles, which leads to code duplication and added complexity.
Solution
This is part of #1981.
This PR introduces 4 new IAM-related profile types that aim to be compliant with the AWS CLI: IamCredentialsProfile, IamSourceProfileProfile, IamCredentialSourceProfile, and IamCredentialProcessProfile. The identity LSP has been modified to save and load these new profile types, in addition to the SsoTokenProfile, from the shared config files. These changes lay the groundwork for retrieving IAM credentials in the next few PRs, since language clients can update profile fields to specify how to obtain IAM credentials.
Note: This PR currently fails the CI pipeline because it depends on changes from aws/language-server-runtimes#599.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.