test-infra
test-infra copied to clipboard
kubernetes
Allow App Auth related Post Requests in Dry Run
When in dry run mode allow obtaining an app access token to be able to perform authenticated get requests.
We we're trying to run a dry run// confirm=false with app auth and were not getting to the "skipping changes..." log lines that would've helped us evaluating which changes would be done.
It would error out early within the check we removed here: https://github.com/kubernetes/test-infra/compare/master...nouseforaname:feature/dry-run-authenticated-get-requests-app-auth?expand=1#diff-43bdff30b56d33fef7db34c315dd186dab4fa8295ef72f8796fdf33984e959aeL4652-L4654
instead we added a regex check here: https://github.com/kubernetes/test-infra/compare/master...nouseforaname:feature/dry-run-authenticated-get-requests-app-auth?expand=1#diff-43bdff30b56d33fef7db34c315dd186dab4fa8295ef72f8796fdf33984e959aeR991
to make sure that we only post to the token endpoint. CC @rkoster
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.
It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
- If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
- If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
- If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
- Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
The committers are authorized under a signed CLA.
- :white_check_mark: Konstantin (bed05e85222343cec2f7026134ea80ab53dcfc28)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
Welcome @nouseforaname!
It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes/test-infra has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @nouseforaname. Thanks for your PR.
I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Is it still a dry-run if you can get the token?
IMO: Authenticated dry-runs are quite common. In most cases creating a diff for the changes requires to be authenticated and authorized. Not quite sure how this can be achieved within this context without having access to some kind of authentication.
Basically (at least from our perspective) it would be nice to have the same experience for peribolos whether you use
-github-token-path vs -github-app-id + -github-app-private-key-path for auth
Any update on this? Not sure if this is stuck on me missing something
/ok-to-test The change seems fine, could you add a test for this please?
/ok-to-test The change seems fine, could you add a test for this please?
Sorry for replying so late. Somehow overlooked the notification around the request.
I could add a test, but looking at the proposed change I'm not quite sure what to test for?
I could add a test, but looking at the proposed change I'm not quite sure what to test for?
Well, that requests from the apps auth roundtripper get executed even when in dry run mode? I also think this is incomplete, the flow is to first get a token using the RSA key and app id and then getting an installation token. It doesn't seem like the former is allowlisted.
RSA key and app id
But that isn't a request it's just creating a JWT using the PrivKey and AppID (https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps) which you then use to get the actual Token or did I misunderstand that?
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: nouseforaname
To complete the pull request process, please assign fejta after the PR has been reviewed.
You can assign the PR to them by writing /assign @fejta in a comment when ready.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
/ok-to-test The change seems fine, could you add a test for this please?
I tried adding a test but I'm really bad at it (still learning :/ ). Let me know if that is what you expected and if not what needs to be changed
But that isn't a request it's just creating a JWT using the PrivKey and AppID (https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps) which you then use to get the actual Token or did I misunderstand that
Yes, but that actual token is needed to do anything useful. Effectively you will need to allowlist all the paths used in any of the methods of the apps auths roundtrippers github client here: https://github.com/kubernetes/test-infra/blob/5c3cd7ce2f5f442364c924863aafd6dd89da5efb/prow/github/app_auth_roundtripper.go#L40-L44
Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.
The list of commits with invalid commit messages:
- 7504591 Update prow/github/client.go
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
@alvaroaleman I removed the additional slice and updated the struct to take a dry run param. But I had to revert the suggestion about the POST path check as it did not work as intended.
Yes, but that actual token is needed to do anything useful. Effectively you will need to allowlist all the paths used in any of the methods of the apps auths roundtrippers github client here:
Isn't that what the regex takes care of? I'm not sure which additional post paths to add there as the path where we post the JWT (that is created locally) is in there already.
@nouseforaname this still isn't addressed so this will not actually work: https://github.com/kubernetes/test-infra/pull/24882#issuecomment-1111337997
/label tide/merge-method-squash
@nouseforaname: PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@nouseforaname: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-test-infra-verify-cri-o | 2238a903ca5a187e3793d31318e2f8965c74dd56 | link | true | /test pull-test-infra-verify-cri-o |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the PR is closed
You can:
- Mark this PR as fresh with
/remove-lifecycle stale - Close this PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
