OCPBUGS-35231: cert-inspection: parse secret/configmap/files as kubeconfigs #1746
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vrutkovs 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 |
vrutkovs
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-35231, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
vrutkovs
force-pushed
the
tls-registry-kubeconfigs
branch
2 times, most recently
from
d98911a
to
d2c7ae1
Compare
|
non-binding lgtm |
| if obj == nil { | ||
| return nil, fmt.Errorf("empty object") | ||
| } | ||
| for _, v := range obj.Data { |
There was a problem hiding this comment.
this appears to be parsing every key of potentially every configmap to see if it's a kubeconfig? Instead of doing this, could we choose specific keys we find valid?
There was a problem hiding this comment.
Yeah, I can modify this to a hardcoded list of keys, but I'm worried that it might miss some new kubeconfigs.
| return caBundleDetail, nil | ||
| } | ||
|
|
||
| func extractKubeConfigFromConfigMap(obj *corev1.ConfigMap) (*rest.Config, error) { |
There was a problem hiding this comment.
might be more useful to return the actual kubeconfig type since it can contain multiple stanzas and rest.Config only holds one.
There was a problem hiding this comment.
Good idea, I'll implement that
There was a problem hiding this comment.
This would require reworking entire InspectSecret/InspectConfigMap as they are expected to return just one secret. I'll implement this in a separate PR
| "k8s.io/client-go/util/cert" | ||
| ) | ||
|
|
||
| func InspectSecret(obj *corev1.Secret) (*certgraphapi.CertKeyPair, error) { | ||
| resourceString := fmt.Sprintf("secrets/%s[%s]", obj.Name, obj.Namespace) | ||
| tlsCrt, isTLS := obj.Data["tls.crt"] | ||
| if !isTLS { | ||
| if detail, err := InspectSecretAsKubeConfig(obj); err == nil { | ||
| return detail, nil | ||
| } |
There was a problem hiding this comment.
Would it be useful to somehow log that this has been attempted and an error occurred?
There was a problem hiding this comment.
Any error is treated as critical here, so "collect TLS artifacts" test would fail if errors found. Also, any valid certificate would be treated as invalid kubeconfig, so we should continue instead of returning error.
Extract CA and certs used in kubeconfigs as TLS artifacts too
vrutkovs
force-pushed
the
tls-registry-kubeconfigs
branch
from
d2c7ae1
to
40ee799
Compare
|
@vrutkovs: all tests passed! 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-sigs/prow repository. I understand the commands that are listed here. |
Extract CA and certs used in kubeconfigs as TLS artifacts too.
Tested in openshift/origin#28864