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

Rate limit errors when paginating using app authentication #738

Open
djrodgerspryor opened this issue Nov 27, 2024 · 4 comments
Open

Rate limit errors when paginating using app authentication #738

djrodgerspryor opened this issue Nov 27, 2024 · 4 comments

Comments

@djrodgerspryor
Copy link

I'm only part way through isolating this, but I'm making an issue in case anyone else is running into similar problems.

We recently upgraded to the most recent release (0.42.1) and we're hitting rate limits on one of our tools that lists a lot of github release pages.

There seem to be two problems:

  1. Only the first request of a pagination is authenticated. See the two logged requests below:
Request {
    method: GET,
    uri: https://api.github.com/repos/BurntSushi/ripgrep/releases?,
    version: HTTP/1.1,
    headers: {
        "content-length": "0",
        "authorization": Sensitive,
        "user-agent": "octocrab",
    },
    body: OctoBody(
        RwLock {
            data: BoxBody,
            poisoned: false,
            ..
        },
    ),
}
Request {
    method: GET,
    uri: https://api.github.com/repositories/53631945/releases?page=2,
    version: HTTP/1.1,
    headers: {
        "content-length": "0",
        "user-agent": "octocrab",
    },
    body: OctoBody(
        RwLock {
            data: BoxBody,
            poisoned: false,
            ..
        },
    ),
}

This behaviour seems identical when using either into_stream, or explicitly calling get_page with the next_page property from the previous request. The above logs were generated via this code:

use futures::stream::TryStreamExt;

let releases: Vec<_> = octocrab
        .repos(organisation, repository)
        .releases()
        .list()
        .send()
        .await?
        .into_stream(&octocrab)
        .try_collect()
        .await?;

This means that our paginated requests are being subjected to the public rate limit.

  1. There doesn't seem to be any automatic handling of the rate limit errors: I'm guessing that the relevant issue is Handle Rate Limits in Octocrab #349 ? I tried enabling the new retryfeature, but it doesn't handle 403 error or the rate limit header, so it's not very useful.

Handling 403s in the retry middleware would be easy to fix, but it's not the main problem, since the public rate limits are very tight for our use case.

@djrodgerspryor
Copy link
Author

I initially suspected #562 but it seems that the allowed_authorities check in auth_header.rs is passing, but self.auth_header is None when the request is sent, so it doesn't get sent.

@djrodgerspryor
Copy link
Author

For reference, this is our Octocrab initialization code:

octocrab::OctocrabBuilder::new()
        .add_retry_config(octocrab::service::middleware::retry::RetryConfig::Simple(
            20,
        ))
        .app(
            std::env::var("GITHUB_APP_ID")
                .expect("GITHUB_APP_ID env variable is missing")
                .parse::<u64>()
                .unwrap()
                .into(),
            jsonwebtoken::EncodingKey::from_rsa_pem(
                std::env::var("GITHUB_APP_KEY")
                    .expect("GITHUB_APP_KEY env variable is missing")
                    .as_bytes(),
            )
            .expect("Cannot convert contents of GITHUB_APP_KEY to an EncodingKey"),
        )
        .build()
        .unwrap()
        .installation(
            std::env::var("GITHUB_APP_INSTALLATION_ID")
                .expect("GITHUB_APP_INSTALLATION_ID env variable is missing")
                .parse::<u64>()
                .unwrap()
                .into(),
        )
        .unwrap()
        ```

@djrodgerspryor
Copy link
Author

djrodgerspryor commented Nov 27, 2024

Hmm, ripping out the AuthHeaderLayer middleware doesn't seem to fix problem 1.

Although now that I know what I'm looking for, there are other comments on similar issues which blame that middleware: #576

@djrodgerspryor
Copy link
Author

djrodgerspryor commented Nov 27, 2024

I can confirm that downgrading to 0.33.3 fixes the problem.

I haven't been able to pinpoint the issue, but I think it's in the way the auth header is generated and passed around when using app authentication. I'm out of time to dig into this tonight, but I'm interested in whether anyone else has any insights!

@djrodgerspryor djrodgerspryor changed the title Rate limit errors when paginating Rate limit errors when paginating using app authentication Nov 27, 2024
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

No branches or pull requests

1 participant