OCPBUGS-24005: webhookcontroller: report when a webhook resource is missing a caBundle provided by the service-ca-operator #1587
Conversation
|
/hold |
openshift-ci
bot
added
do-not-merge/work-in-progress
openshift-ci
bot
added
the
approved
| // waitForServiceCABundle returns true if a resource has the service-ca | ||
| // annotation and the caBundle hasn't been yet provided. | ||
| func waitForServiceCABundle(annotations map[string]string, caBundle []byte) bool { | ||
| if hasServiceCaAnnotation(annotations) { |
There was a problem hiding this comment.
do we need to check for the annotation, shouldn't it be enough to just check for the length of the caBundle?
There was a problem hiding this comment.
I think we do, at least when the resource is annotated, in that case we rely on the service-ca to provide the bundle.
There was a problem hiding this comment.
In general, a caBundle is optional and when it is empty we use CAs provided by the underlying OS.
| if annotations == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
| if annotations == nil { | |
| return false | |
| } |
I haven't found any additional checks in The service-ca requires a non-empty |
the actuall values of the conditions were changed in 0b85b30
p0lyn0mial
force-pushed
the
webhook-controller-checks-service-ca-annotation
branch
from
090fc9e
to
686ff4b
Compare
p0lyn0mial
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
p0lyn0mial
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@p0lyn0mial: This pull request references Jira Issue OCPBUGS-24005, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
| if caBundleProvidedByServiceCA && len(caBundle) == 0 { | ||
| err = fmt.Errorf("%v. NOTE that the caBundle is provided by the service-ca-operator but was empty. Please check the service-ca's logs", err) | ||
| } |
There was a problem hiding this comment.
I think we should perform this check before attempting the connection. There is little chance that the connection would succeed anyway. This would allow for a simpler error message for the status and less noise in the webhook logs for failed connections that would never have succeeded without the CA bundle.
There was a problem hiding this comment.
OK, I've updated the PR, PTAL.
…le provided by the service-ca-operator
p0lyn0mial
force-pushed
the
webhook-controller-checks-service-ca-annotation
branch
from
686ff4b
to
aa5d5ed
Compare
|
/jira refresh |
|
@p0lyn0mial: This pull request references Jira Issue OCPBUGS-24005, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@p0lyn0mial: This pull request references Jira Issue OCPBUGS-24005, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/retest-required |
|
@p0lyn0mial: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, sanchezl 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 |
openshift-merge-bot
bot
merged commit d6f4bca
into
openshift:master
|
@p0lyn0mial: Jira Issue OCPBUGS-24005: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-24005 has been moved to the MODIFIED state. 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. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-kube-apiserver-operator-container-v4.16.0-202312191909.p0.gd6f4bca.assembly.stream for distgit ose-cluster-kube-apiserver-operator. |
|
/cherry-pick release-4.15 |
|
@p0lyn0mial: new pull request created: #1628 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-4.14 |
|
@wangke19: new pull request created: #1646 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. |
This is clearly a gap in the platform. Perhaps the best way would be to introduce a readiness condition
kaswould honour. On the other hand the issue is not that severe since most of the time it will appear during a webhook installation. I've checked and the cert-manager works exactly in the same way.Initially I was thinking about waiting for a caBundle to be populated but realised it would hide the issue. I've changed the PR to report the issue instead.