ci: Add authentication for github domains in check_url

[bookinabox]

Add github user authentication to check_url to increase rate limit from 60 to 5000.

Only if GITHUB_USER is defined and GITHUB_TOKEN is defined.

Changed main.yaml to export secrets.GITHUB_USER and secrets.GITHUB_TOKEN into environment variables.

Fixes #4304

Signed-off-by: Derek Lee [email protected]

[wainersm]

Hi @jodh-intel ! mind to review this?

[jodh-intel]

Thanks @bookinabox - I agree with @wainersm's comments so feel free to ping me once you've updated this PR.

[wainersm]

I am also wondering if we shouldn't be using OAuth2 authentication in header as suggested in https://docs.github.com/en/rest/overview/resources-in-the-rest-api#oauth2-token-sent-in-a-header , instead the user/secret approach. Then we create a token with with restricted permissions just like we have been using in other actions, as for example, .github/workflows/add-pr-sizing-label.yaml uses the KATA_GITHUB_ACTIONS_PR_SIZE_TOKEN

[bookinabox]

@jodh-intel @wainersm I fixed those two small changes, but looking at this ticket again, I'm actually confused about how we were hitting the rate limit. My understanding is that the rate limit that we've been referring to is for calls to the GitHub REST API (e.g. https://api.github.com/zen, https://api.github.com/rate_limit) rather than just looking at various github urls as we are in check_url. Does it somehow implicitly call the REST API? Or is it somehow because we are running it through github runner that this occurs (like there's a different limit for the number of urls a runner can call). We do use the github api for stuff that requires it (e.g. move-issues-to-in-progress), but I don't think that applies to check_url.

This is something I should have brought up before starting this ticket, but I guess better late than never. I'll try to investigate this on my own more. Hopefully I'm not just misunderstanding something super obvious.

[wainersm]

@jodh-intel @wainersm I fixed those two small changes, but looking at this ticket again, I'm actually confused about how we were hitting the rate limit. My understanding is that the rate limit that we've been referring to is for calls to the GitHub REST API (e.g. https://api.github.com/zen, https://api.github.com/rate_limit) rather than just looking at various github urls as we are in check_url. Does it somehow implicitly call the REST API? Or is it somehow because we are running it through github runner that this occurs (like there's a different limit for the number of urls a runner can call). We do use the github api for stuff that requires it (e.g. move-issues-to-in-progress), but I don't think that applies to check_url.

I've the same doubts.

Initially I thought the calls get redirected through the REST API, as it is curl'ing files from a pull request; however I did some investigation and it seems not the case. So it might be some security feature for the github runner.

This is something I should have brought up before starting this ticket, but I guess better late than never. I'll try to investigate this on my own more. Hopefully I'm not just misunderstanding something super obvious.

Conceptually your changes aren't wrong, getting authenticated https GET requests seems not a bad thing at all.

[bookinabox]

Not sure why the add-pr-sizing-label fails, but I noticed that man-db was removed in kata-containers/kata-containers, so I made a small PR to mirror that change over to here (https://github.com/kata-containers/tests/pull/5058)

[bookinabox]

Not sure why PR sizing label is failing. I cannot reproduce it on my fork or locally using nekto/act. Going to push some commits here for testing purposes, then I will revert them.

[bookinabox]

For anyone else looking at this ticket, I added sudo apt update and an echo statement and it worked. sudo apt update does not appear in the logs though.

[bookinabox]

And... reverting it breaks it again :( despite the logs saying that I was running:

pr=4883

sudo apt -y remove --purge man-db sudo apt -y install diffstat patchutils

pr-add-size-label.sh -p "$pr" shell: /usr/bin/bash -e {0} env: GITHUB_TOKEN: ***

both times

[bookinabox]

In an ironic twist, the action has now passed, but is actually erroring out with

could not find pull request diff: HTTP 403: API rate limit exceeded for user ID 36305720. (https://api.github.com/repos/kata-containers/tests/pulls/4883)

This is odd and suggests two things:

• sudo apt update is needed here
• There is a rate limiting issue using the gh command used in pr-add-sizing-label

It is unclear to me why the first one is true given that other PRs have and continue to pass fine without it. The second one I may want to investigate given that it is tangentially related to this PRs issue anyways.

[bookinabox]

Sorry for the spam for anyone following this PR. The behavior only seems to be here and not on any forks I make :/ .

~~I was under the impression that I should be able to edit the workflows. I did it when working with cargo-deny. There seems to be changes that occur when I edit the workflow, but the logs make it seem like I don't.~~ (Edit: difference was pull_request vs pull_request_target) I will get rid of my testing commit.

[bookinabox]

I going to try removing the add "sudo apt update" commit one more time. In theory, that commit should really not do anything. I do notice the rate limit being hit in other PRs. It's a bit hard for me to investigate it without making a PR and pinging someone else to run the test every time, so I think I'll leave that to someone else for now.

[bookinabox]

It looks like it passes, but also the rate limit was hit again for secrets.KATA_GITHUB_ACTIONS_PR_SIZE_TOKEN :/. The rate limit should be 1000 an hour per repo and I doubt we're hitting that.

We know we are authenticating because it does allow us to view the commit diff ( not having a token/a token without enough permissions would result in a gh no auth error)

But for now I think this can be merged in.

[wainersm]

/test

[wainersm]

/test-ubuntu-metrics

[stevenhorsman]

@bookinabox - Hey Derek, it looks like -A "$user_agent" was removed from the curl request in this PR and we're seeing failures now which I think might be related to this:

$ curl -sIL -H "Accept-Encoding: zstd, br, gzip, deflate" https://www.intel.com
HTTP/2 403

vs

$ curl -sIL -A $user_agent -H "Accept-Encoding: zstd, br, gzip, deflate" https://www.intel.com
HTTP/2 301

I'm not sure why we have only hit the problem now, but I wanted to check if this change was deliberate to fix something?

[stevenhorsman]

FYI - I've created #5186 to try and revert the user agent change to try and fix the failures we are seeing.

[bookinabox]

@bookinabox - Hey Derek, it looks like -A "$user_agent" was removed from the curl request in this PR and we're seeing failures now which I think might be related to this:

$ curl -sIL -H "Accept-Encoding: zstd, br, gzip, deflate" https://www.intel.com
HTTP/2 403

vs

$ curl -sIL -A $user_agent -H "Accept-Encoding: zstd, br, gzip, deflate" https://www.intel.com
HTTP/2 301

I'm not sure why we have only hit the problem now, but I wanted to check if this change was deliberate to fix something?

I don't think it was deliberate. That user agent was added from a different ticket of mine. Most likely, I just didn't look too carefully after merging it. I'm actually curious which urls are failing after the user agent was removed, because the only one I remember was AMD iirc.