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: renewal & better errors #4

Closed
wants to merge 6 commits into from
Closed

Conversation

DanielHougaard
Copy link
Collaborator

This PR introduces renewal for all currently supported authentication methods. Additionally it also improves our error handling. Previously we were returning raw Axios errors, which isn't ideal. Now we are parsing the errors and throwing either InfisicalError or InfisicalRequestError. The InfisicalRequestError is only thrown when we're certain that the error is an API error, as it contains special formatting specifically for API errors.

Minor

Updated the Dynamic Secrets types.

@DanielHougaard DanielHougaard self-assigned this Oct 9, 2024
});

return res.data.accessToken;
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern seems very redundant. Maybe you can catch things at the instance level? That way, you catch everything in one spot. Worth doing a bit of research into

src/custom/errors.ts Outdated Show resolved Hide resolved
src/custom/errors.ts Show resolved Hide resolved

while (true) {
try {
await new Promise(resolve => setTimeout(resolve, 10_000));
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why wait 10 seconds before each loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To slow down the loop. There's no need to check it more frequently. We could reduce it to 5 seconds if we wanted though, but I don't see a specific reason to so

headers: {
Authorization: `Bearer ${accessToken}`
// If there's less than 15 seconds until the token expires, we reauthenticate early
if (firstFetchTimeInSeconds >= this.#tokenDetails.accessTokenMaxTTL - 15) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this would mean that if the ttl was 15 or less, it would keep renewing indefinitely? How about we do a percent, like 2/3rds of the way to end of life and then we renew

Copy link
Contributor

Choose a reason for hiding this comment

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

It might also be worth putting a warning if the max access token/access token ttl is shorter than how much we are subtracting because in those cases it will keep re logging in and firing tons of API calls

[AuthMethod.UniversalAuth]: async () =>
await this.#authClient.universalAuth.login(this.#authCredentials?.credentials as UniversalAuthCredentials),
[AuthMethod.AWSIam]: async () => await this.#authClient.awsIamAuth.login(this.#authCredentials?.credentials as AWSIamCredentials),
[AuthMethod.AccessToken]: async () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like you are accessing this one, are you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it's being accessed with await authMethodMap[this.#authCredentials.type]();

this.#secretsClient = new SecretsClient(this.#apiInstance, this.#requestOptions);
this.#dynamicSecretsClient = new DynamicSecretsClient(this.#apiInstance, this.#requestOptions);
this.#authClient = new AuthClient(this.authenticate.bind(this), this.#apiInstance, this.#basePath);
#authenticator = {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is there a reason why these methods are in a object like this instead of having them be standalone methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No specific reason, but I wanted to consolidate the auth-related helper functions

await authMethodMap[this.#authCredentials.type]();
this.#tokenDetails.firstFetchTime = new Date();
// If there's less than 10 seconds until the token expires, we renew it early
} else if (timeSinceFetchInSeconds >= this.#tokenDetails.expiresIn - 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make more sense to see if we can renew the token first before we resort to the last hope which is re login

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes sense though. If the max TTL has been reached, there's no point trying to renew the token because it will always fail if max TTL has been reached

await this.#authClient.accessToken((this.#authCredentials?.credentials as AccessTokenCredentials).accessToken)
};

while (true) {
Copy link
Contributor

@maidul98 maidul98 Oct 9, 2024

Choose a reason for hiding this comment

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

I'm worried about how memory safe this is. i know in go we have go routine but unsure how this compares to a ever running while loop

Copy link
Collaborator Author

@DanielHougaard DanielHougaard Oct 9, 2024

Choose a reason for hiding this comment

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

  1. Excessive CPU usage is limited by the sleep function
  2. The loop will never break due to the try-catch statement
  3. while(true) is necessary in this case, because intended behavior is to have this run in the background. Javascript isn't multi-threaded, and we don't want to spawn a sub-process for handling token lifecycle

I don't see any reason for this being unsafe

this.#authClient = new AuthClient(this.authenticate.bind(this), this.#apiInstance, this.#basePath);
#authenticator = {
authenticate: async (tokenDetails: Omit<TTokenDetails, "fetchedTime" | "firstFetchTime">) => {
this.#apiInstance = new InfisicalApi(
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of a new instance of the client here, maybe you can do it in the renew logic to keep all related code in one spot?


type AwsAuthLoginOptions = {
identityId?: string;
};

export const renewToken = async (apiClient: InfisicalApi, token?: string) => {

Choose a reason for hiding this comment

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

Suggestion: Add JSDoc comment to explain the purpose, parameters, return type, and possible errors of the function. This is helpful for maintainability and for other developers who might use this function.
'''
/**

  • Renews the access token using the provided Infisical API client.
  • @param apiClient - An instance of InfisicalApi to make the request.
  • @param token - The current access token to be renewed.
  • @returns The renewed token data.
  • @throws {InfisicalError} If the token is missing or the renewal request fails.
    */
    '''

@DanielHougaard
Copy link
Collaborator Author

Handled in a separate PR.

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