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

Fix bug with enterpriseURL for multi-tenancy #1781

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

bm1216
Copy link
Contributor

@bm1216 bm1216 commented Sep 6, 2022

Addresses and fixes #1764.

Changes:

  • Add IsEnterprise bool to the GitHub client struct. It is set when the client is an enterprise client.
  • Check if the main client is an enterprise client before setting conf.EnterpriseURL for clients created using githubAPICredentialsFrom.
  • Add command line argument for setting EnterpriseURL in GitHub config.

Reasoning

Currently, ARC creates a GitHub client (let's call it client A) when the controller is created - this client is passed to the RunnerReconciler. As part of the multi-tenancy feature, a new client (let's call it client B) is created when a RunnerDeployment is added with the field githubAPICredentialsFrom.

The GitHub wrapper creates a client from a config struct. When creating client B, it sets the enterpriseURL in its config to the GithubBaseURL of client A1. This causes issues when client A is not an enterprise client, as client B now has its enterpriseURL defined. The GitHub wrapper behaves differently when an enterpriseURL defined in the config struct, leading to the newly created client B calling the wrong API endpoint for tasks such as runner registration.

To fix this, client A needs a basic idea of whether it is using an enterpriseURL. We have decided to use a bool (as opposed to doing checks on the baseURL) as it's just cleaner and less likely to go wrong.

Also, added command line support for enterpriseURL as it was the only thing that was missing.

1 This was introduced as part of #1725 and issue #1714. The concern was valid because if the config for client B has no enterpriseURL defined it should be using the enterpriseURL of the original client (A). Nevertheless, the above bug was introduced.

* Add cmd line arg for enterprise url. Fix enterprise bug.

* Fix package import order

* Fix comment
@bm1216
Copy link
Contributor Author

bm1216 commented Sep 6, 2022

@Jalmeida1994 for visibility. I tested it out and it should still work for your use case.

@bm1216
Copy link
Contributor Author

bm1216 commented Sep 8, 2022

@mumoshu @toast-gear thoughts?

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

Thank you very much for diagnosing and fixing it! I love your detailed explanation of the issue and the fix too-it helped me quickly understand and confirm this is the right fix!

Definitely LGTM 👍

@mumoshu mumoshu merged commit 546b525 into actions:master Sep 9, 2022
@Jalmeida1994
Copy link
Contributor

@Jalmeida1994 for visibility. I tested it out and it should still work for your use case.

Hey @bm1216 nice work! When I get the time I'll test it also on an Enterprise Environment but the code and the reasoning makes sense! Cheers, and sorry again for introducing this bug @mumoshu @toast-gear and @bm1216!

@bm1216
Copy link
Contributor Author

bm1216 commented Sep 9, 2022

Thanks @mumoshu @Jalmeida1994 for taking a look. Appreciate all the comments

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.

Multi-tenancy doesn't work with Github Apps with no explicit Enterprise URL defined.
3 participants