-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Test BYO certificates #10681
base: main
Are you sure you want to change the base?
🌱 Test BYO certificates #10681
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Finally got around to add the CA verfification now. |
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.
overal lgtm. I will just wait for #10633 to merge first
@@ -408,6 +409,9 @@ func fakeCASecret(namespace, name string, caData []byte) *corev1.Secret { | |||
ObjectMeta: metav1.ObjectMeta{ | |||
Name: name, | |||
Namespace: namespace, | |||
Labels: map[string]string{ |
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.
I think this is not necessary anymore as soon as we merge #10633
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.
Let's definitely revert (after #10633 merges it should work without). This secret should not require the cluster-name label (also the secret should not correspond to any cluster)
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.
Makes sense! I will wait for it to merge, then rebase and remove this. 🙂
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.
@lentzi90 PR is merged
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.
Thanks!
@adilGhaffarDev or @Sunnatillo does anyone of you want to take this over? It may be some time until I can get back to it since I'm on parental leave 😇
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.
No worries! Enjoy!
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.
I can address the comments and push the changes.
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.
pushed the changes, please check.
3c9cc4a
to
652b346
Compare
/retest |
I think this also requires changes to another test, see: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10681/pull-cluster-api-test-main/1812731035935313920 |
This adds an integration test for BYO CA certificate. It is practically a copy of TestReconcileInitializeControlPlane but with a custom CA and a check to verify that the generated kubeconfig is really using this CA. Signed-off-by: Lennart Jern <[email protected]>
Signed-off-by: Muhammad Adil Ghaffar <[email protected]>
Signed-off-by: Lennart Jern <[email protected]>
652b346
to
57fcab2
Compare
The extension config controller test made me realize that my naive solution to add the cache directly to the test environment was probably not the best idea. Somehow I didn't realize that different controllers have different cache behavior. To improve the situation I now made it so that each test suite can have its own cache options. I took inspiration from #11201 to implement it. It is in a separate commit for now so it can be easily reverted if there is some issue with it that I didn't realize. |
What this PR does / why we need it:
This adds an integration test for BYO CA certificate. It is practically a copy of TestReconcileInitializeControlPlane but with a custom CA and a check to verify that the generated kubeconfig is really using this CA.
To make the test environment behave similar to the real world, it now also has a cache that picks up objects based on labels. For this reason it was also necessary to add labels in a few places.
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 #9568 (it was closed already when we added the docs)
/area testing