[WIP][DoNotReview]✨ clusterctl: verify cert-manager did inject the CA certificates into the objects before proceeding #10433
Conversation
k8s-ci-robot
added
area/e2e-testing
k8s-ci-robot
added
the
size/S
|
[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 |
chrischdi
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
/test help |
|
@chrischdi: The specified target(s) for
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-e2e-main |
4 similar comments
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-main |
k8s-ci-robot
added
size/L
|
/test help |
|
@chrischdi: The specified target(s) for
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-e2e-main |
chrischdi
force-pushed
the
pr-e2e-clusterctl-upgrade-x509
branch
from
50b8cc5 to
501da90
Compare
|
/test pull-cluster-api-e2e-main |
chrischdi
force-pushed
the
pr-e2e-clusterctl-upgrade-x509
branch
from
501da90 to
d577747
Compare
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-mink8s-main |
|
/test pull-cluster-api-e2e-main |
|
Unrelated flake:
/test pull-cluster-api-e2e-main |
|
Unrelated flake:
/test pull-cluster-api-e2e-main |
chrischdi
force-pushed
the
pr-e2e-clusterctl-upgrade-x509
branch
from
d577747 to
64f6eab
Compare
chrischdi
force-pushed
the
pr-e2e-clusterctl-upgrade-x509
branch
from
dcca4c9 to
1b0ac8b
Compare
|
and some logging |
chrischdi
force-pushed
the
pr-e2e-clusterctl-upgrade-x509
branch
from
1b0ac8b to
9d0017e
Compare
|
Improved duration printing /test pull-cluster-api-e2e-main |
chrischdi
changed the title
chrischdi
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
I would suggest to wait with merging until after the releases today |
kind of ack, there won't be any cherry-picking until the releases are out, merging to main is okay but it needs reviews anyway :-) |
|
/assign @sbueringer :-) |
|
/cherry-pick release-1.7 |
|
@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you. 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. |
|
/cherry-pick release-1.6 |
|
@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you. 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. |
| for _, obj := range components.Objs() { | ||
| obj := obj | ||
| if value, ok := obj.GetAnnotations()[certManagerCAAnnotation]; ok { | ||
| _ = value |
| secretObjKey := client.ObjectKey{Namespace: certificate.GetNamespace(), Name: secretName} | ||
| certificateSecret := &corev1.Secret{} | ||
| if err := c.Client.Get(ctx, secretObjKey, certificateSecret); err != nil { | ||
| return "", errors.Wrapf(err, "getting secret %s", &certificateObjKey) |
There was a problem hiding this comment.
Should this be secretObjKey? (or maybe mention both)
| return client.ObjectKey{Namespace: nameStr[:splitPoint], Name: nameStr[splitPoint+1:]} | ||
| } | ||
|
|
||
| // checkObject gets the desired CA certificate and compares it relevant field on |
There was a problem hiding this comment.
| // checkObject gets the desired CA certificate and compares it relevant field on | |
| // checkObject gets the desired CA certificate and compares it with the relevant field on |
| return err | ||
| } | ||
|
|
||
| if crd.Spec.Conversion.Webhook == nil || crd.Spec.Conversion.Webhook.ClientConfig == nil || string(crd.Spec.Conversion.Webhook.ClientConfig.CABundle) != ca { |
There was a problem hiding this comment.
Should we instead skip the validation if Conversion.Webhook.ClientConfig doesn't exist?
It's a strange config for a CRD to have the annotation and no client config, but I think it's not on us to fail in that case.
I think if the conversion webhook is configured and the caBundle supposed to be used ClientConfig has to be configured before the cert-manger becomes active
| } | ||
| } | ||
| default: | ||
| return fmt.Errorf("unknown object type %s", obj.GetKind()) |
There was a problem hiding this comment.
Maybe we shouldn't error in this case. This way we stick to best-effort validation vs failing in whatever cases folks might have (e.g. we implemented something very similar for ExtensionConfig, although we picked a different name for the annotation)
| @@ -103,7 +103,11 @@ func (i *providerInstaller) Install(ctx context.Context, opts InstallOptions) ([ | |||
| ret = append(ret, components) | |||
| } | |||
|
|
|||
| return ret, waitForProvidersReady(ctx, opts, i.installQueue, i.proxy) | |||
| if err := waitForProvidersReady(ctx, opts, i.installQueue, i.proxy); err != nil { | |||
There was a problem hiding this comment.
We intentionally added a flag to make wait for providers opt-in. It feels wrong to add waitForCAInjection for all cases.
Does it make sense to waitForCAInjection when waitForProviders is disabled? (this is the default)
Maybe waitForCAInjection should simplyi be part of waitForProvidersReady
If not, we problably would want to add another flag and disable per default (we can still had-code it to true in the tests / test framework)
cmd/clusterctl/internal/util/objs.go
Outdated
| @@ -192,3 +193,8 @@ func IsDeploymentWithManager(obj unstructured.Unstructured) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // IsCertificate return true if obj is a certificate. | |||
| func IsCertificate(obj unstructured.Unstructured) bool { | |||
| @@ -116,9 +115,6 @@ func AssertOwnerReferences(namespace, kubeconfigPath string, assertFuncs ...map[ | |||
| // Sometimes the conversion-webhooks are not ready yet / cert-managers ca-injector | |||
| // may not yet have injected the new ca bundle after the upgrade. | |||
| // If this is the case we return an error to retry. | |||
There was a problem hiding this comment.
Let's also drop the corresponding comment
|
/tide merge-method-squash |
|
/test pull-cluster-api-e2e-main |
|
@chrischdi: The following test failed, say
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. |
|
/hold This seems to be broken now. |
k8s-ci-robot
added
the
do-not-merge/hold
|
Discussed with Christian. We have an alternative idea on how to fix this. Please don't review the current PR for now |
chrischdi
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
/close |
|
@chrischdi: Closed this PR. 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. |
What this PR does / why we need it:
Ensures that we do not fail the e2e tests for intermittent x509 errors.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part of #9688
/area e2e-testing