-
Notifications
You must be signed in to change notification settings - Fork 560
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
Try to find and verify existing OIDC providers before we try to create a new one #2901
Try to find and verify existing OIDC providers before we try to create a new one #2901
Conversation
@codablock: This issue is currently awaiting triage. If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the The 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. |
Hi @codablock. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
5ba0dfb
to
24d7c42
Compare
Shame we stored this in the status in the original implementation. @codablock - do you see any potential issues (i.e. edge cases) with trying to to automatically find the oidc provider? |
@richardcase I can't imagine any. I was first thinking that I could also try to update an existing OIDC provider's thumbprint ind client IDs, but then decided that I should not touch them as we can't know for sure WHY they would differ from the expected values...if they do, there is probably a good reason (e.g. manually created provider, for whatever reason). |
@richardcase Any update on how to proceed with this PR? |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/lifecycle frozen This looks good to me. I can't imagine any issues with this myself. @sedefsavas - what do you think? |
@richardcase: The In response to this:
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. |
@codablock - this looks good to me. Do you think there is anyway we can add a test around this functionality? |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
Unfortunately I'm a bit overloaded atm and won't find time to add tests. |
/help |
This is a nightmare to test. :D After I finally managed to get the HTTPS test server working with proper certs, now I'm in trouble with |
Yesss!! Almost there. :D now trying to reconcile the LAST HURDLE. :D This part: if err := s.reconcileTrustPolicy(); err != nil {
return errors.Wrap(err, "failed to reconcile trust policy in workload cluster")
} :D |
OH. MY. GODS.
Edit: NOPE. My Kind test cluster was running and it connected to that. :( |
Hah! I found a bug! :D :D Testing++++++ |
@richardcase @codablock This was DIFFICULT not gonna lie. :D But I added a test. :) And I found a bug where this was being done: if fmt.Sprintf("https://%s", *provider.Url) != issuerURL.String() {
continue
} But the provider url MUST contain |
959c82d
to
adfe1b9
Compare
adfe1b9
to
e870085
Compare
Sh*t. I was afraid of that. :/ The certificate is not installed/recognised by the local environment. So it doesn't like it. |
…f another bug in Go
Dang it. This won't work. :( I have to find a different approach and mock the http client completely. |
type ServiceOpts func(s *Service) | ||
|
||
// WithIAMClient creates an access spec with a custom http client. | ||
func WithIAMClient(client *http.Client) ServiceOpts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, I can't create a legit https service because the certificate is not trusted. So I had to fall back to making the client mockable.
If anyone has a better idea, I'm all ears.
Also, I made this an option to minimise the impact on changing NewService
.
d03bea6
to
019a51a
Compare
} | ||
|
||
func fetchRootCAThumbprint(issuerURL string, client *http.Client) (string, error) { | ||
// needed to appease noctx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Funny" how noctx never noticed "http.Get" but noticed client.Get immediately. Tsk, tsk, tsk.
019a51a
to
f2381df
Compare
This is now ready for review. :) |
ping @Ankitasw as Richard is OOO. :)) |
/test ? |
@Skarlso: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
/test pull-cluster-api-provider-aws-e2e-eks Hmm, hmm, unsure if this passes oidc part. Probably not. But at least it will test that EKS didn't break. :) |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Skarlso The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When moving clusters between management clusters, ControlPlane.Status.OIDCProvider.ARN
is lost. The new management cluster must then pickup the already existing
cluster, as otherwise it tries to create the same provider again and then
fails.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist:
Release note: