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: Update STS web identity credentials provider & add integration test #1327

Merged
merged 12 commits into from
Feb 9, 2024

Conversation

sichanyoo
Copy link
Contributor

@sichanyoo sichanyoo commented Feb 7, 2024

Issue #

#1189
#1194

Description of changes

Integration test structure overview:

  1. Create a temporary role with assume role policy that accepts unauthenticated OIDC tokens made by Cognito
  2. Create a Cognito identity pool, then fetch Cognito ID from it, then fetch OIDC token from it using Cognito ID
  3. Save the fetched OIDC token to a file
  4. Construct STS web identity credentials provider using the role and the file path to cached token
  5. Call STS::getCallerIdentity using a STS client that has just the STS web identity credentials provider configured
  6. Confirm request succeeded by checking response fields (not nil & not empty string)
  7. Clean up role policy, role, Cognito identity pool, and token file after the test

New/existing dependencies impact assessment, if applicable

Conventional Commits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


// Helper methods for saving & deleting token file
private func saveTokenIntoFile() throws {
tokenFilePath = NSHomeDirectory() + "/token.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to use FileManager.default.temporaryDirectory instead of NSHomeDirectory()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested.

import Foundation

/// Tests STS web identity credentials provider using STS::getCallerIdentity.
class STSWebIdentityCredentialsProviderTests: XCTestCase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move helpers and role information to a separate file so that this file is easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep stuff for this integration test confined to one file. I'll go ahead and refactor stuff into more helper methods and locate them all at bottom of file for better readability.

Comment on lines +92 to +95
// This wait is necessary for role creation to propagate everywhere
let seconds = 10
let NSEC_PER_SEC = 1_000_000_000
try await Task.sleep(nanoseconds: UInt64(seconds * NSEC_PER_SEC))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't introduce anything non-deterministic in our integration tests if we can help it. A better approach would be to poll IAM to see if the role was created successfully.

Example using getRole:

func isRoleReady(iamClient: IAMClient, roleName: String) async -> Bool {
    do {
        let response = try await iamClient.getRole(input: GetRoleInput(roleName: roleName))
        return response.role != nil
    } catch {
        return false
    }
}

Then you can setup a function that calls isRoleReady every X seconds using Task.sleep until a configured timeout (if after X seconds/minutes the role is still not there, throw an error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely would be ideal to have some polling mechanism like the one you suggested instead of a fixed wait behavior, but getRole doesn't serve our purpose here (nor does any other IAM API). That's because changes to IAM entities become available to IAM API immediately BUT it takes time for that info to propagate everywhere. With 7 second wait it reliably passed every time on local machine, so I made it 10 just to be safe. Here's a relevant stackoverflow discussion if you'd like to read more about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also see if there is a waiter defined on the service for the resource you are waiting on.

Check the Kinesis integration test: it uses a predefined waiter and manual waiting in two stages of the test to ensure conditions are met before proceeding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried the waiters and they don't serve the polling purpose in this instance for the same reasons explained above, as expected. Unless IAM provides some sort of API where it shows IAM identity's propagation status, I don't think it's possible to poll this info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, we can try this live in CI for a while, but we'll have to revisit this test if it causes spurious failures in practice.

With the size & complexity of our test suite, we can't afford even a small number of flaky tests.

Comment on lines 179 to 180
// Confirm STS web identity credentials provider gave valid credentials
// ...by checking response was returned successfully.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make use of Swift's multi-line comments instead of ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested.

Copy link
Collaborator

@dayaffe dayaffe left a comment

Choose a reason for hiding this comment

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

Overall looks good! just a comment to use getRole instead of sleeping to make the test deterministic and some clean up

/* /////////////////////////
* TEARDOWN HELPER FUNCTIONS
* /////////////////////////
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Xcode has a "mark" comment directive that can be used to delineate sections of code, and the marks you create show up in the list of symbols in the dropdown above the editor.

See: https://www.avanderlee.com/xcode/xcode-mark-line-comment/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested.

case TokenFileDeletionFailed(String)
}

enum IdentityPoolStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Swift convention is that enum cases should be lowerCamelCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested.

Comment on lines +92 to +95
// This wait is necessary for role creation to propagate everywhere
let seconds = 10
let NSEC_PER_SEC = 1_000_000_000
try await Task.sleep(nanoseconds: UInt64(seconds * NSEC_PER_SEC))
Copy link
Contributor

Choose a reason for hiding this comment

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

Also see if there is a waiter defined on the service for the resource you are waiting on.

Check the Kinesis integration test: it uses a predefined waiter and manual waiting in two stages of the test to ensure conditions are met before proceeding.

Copy link
Contributor

@jbelkins jbelkins left a comment

Choose a reason for hiding this comment

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

Approved, but note my comment on the time delay issue.

@sichanyoo sichanyoo merged commit 92b1e2a into main Feb 9, 2024
17 checks passed
@sichanyoo sichanyoo deleted the feat/sts-web-identity-cred-provider branch February 9, 2024 22:37
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.

3 participants