-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
API-1789: tls artifacts: list ondisk locations alongside secret/configmaps #29090
base: master
Are you sure you want to change the base?
Conversation
@vrutkovs: This pull request references API-1789 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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. |
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.
/lgtm
tls/ownership/ownership.md
Outdated
Other locations: | ||
|
||
* file /etc/docker/certs.d/image-registry.openshift-image-registry.svc.cluster.local:5000/ca.crt | ||
* file /etc/docker/certs.d/image-registry.openshift-image-registry.svc:5000/ca.crt | ||
|
||
|
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 bet we'll end up finding disputes of "who actually owns this ca bundle" because while the content matches, the actual owner seems more likely to be the image registry. This is likely a common problem and suggests that even though the content is the same, the thing we're actually providing ownership for is the file itself.
/hold Alongside "this matches the content" we should also match by purpose - i.e. /etc/kubernetes/static-pod-resources/kube-apiserver-certs/configmaps/trusted-ca-bundle/ca-bundle.crt belongs to kube-apiserver and so on |
TLS artifact reports prefer in-cluster locations, so that they can be assigned metadata via annotations. This change prints other ondisk locations so that secret/configmap could be linked to the file on disk
3bacd6a
to
0607865
Compare
|
||
## Image Registry (2) | ||
### Certificate Authority Bundles (2) | ||
1. file /etc/docker/certs.d/image-registry.openshift-image-registry.svc.cluster.local:5000/ca.crt |
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.
These files both have the same owner and the same content, so they are printed twice here. Not sure if its worth fixing tbh
/hold cancel |
@vrutkovs: 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-sigs/prow repository. I understand the commands that are listed here. |
Job Failure Risk Analysis for sha: 0607865
|
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.
/lgtm
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dinhxuanvu, sanchezl, 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 |
TLS artifact reports prefer in-cluster locations, so that they can be assigned metadata via annotations. This change prints other ondisk locations so that secret/configmap could be linked to the file on disk