API-1789: update-tls-artifacts: ondisk metadata updates, techpreview data and required tests #28868
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/payload-job periodic-ci-openshift-release-master-ci-4.17-e2e-aws-ovn periodic-ci-openshift-release-master-ci-4.17-e2e-azure-ovn periodic-ci-openshift-release-master-ci-4.17-e2e-gcp-ovn periodic-ci-openshift-release-master-nightly-4.17-e2e-metal-ipi-ovn-bm periodic-ci-openshift-release-master-nightly-4.17-e2e-vsphere-ovn-serial periodic-ci-openshift-release-master-nightly-4.17-e2e-aws-ovn-single-node |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@vrutkovs: trigger 6 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4af16a50-275a-11ef-81fb-0bb21a10eda8-0 |
|
[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
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
db67ecf
to
ffd18d4
Compare
|
/payload-job periodic-ci-openshift-release-master-ci-4.17-e2e-aws-ovn periodic-ci-openshift-release-master-ci-4.17-e2e-azure-ovn periodic-ci-openshift-release-master-ci-4.17-e2e-gcp-ovn periodic-ci-openshift-release-master-nightly-4.17-e2e-metal-ipi-ovn-bm periodic-ci-openshift-release-master-nightly-4.17-e2e-vsphere-ovn-serial periodic-ci-openshift-release-master-nightly-4.17-e2e-aws-ovn-single-node |
|
@vrutkovs: trigger 6 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ff288a00-27b7-11ef-992a-c878d15d4ad3-0 |
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
1d7ad2e
to
c2f208d
Compare
1 task
vrutkovs
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
2 times, most recently
from
fe80539
to
a61b9db
Compare
vrutkovs
changed the title
|
@vrutkovs: This pull request explicitly references no jira issue. 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-reference
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
a61b9db
to
bebb467
Compare
|
Job Failure Risk Analysis for sha: bebb467
|
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
bebb467
to
936d5f6
Compare
|
Job Failure Risk Analysis for sha: 936d5f6
|
benluddy
reviewed
Aug 9, 2024
| ) | ||
| } | ||
| } | ||
| if currCertKeyPair.OnDiskLocation != nil { |
There was a problem hiding this comment.
I don't know how stable the API is yet, but would it make sense to have a discriminated union for artifact locations? If both or neither location field is nil, the interpretation is ambiguous and probably indicates an error. This looks like if both location fields are nil, the entry is ignored, and if both are non-nil, the presence of an on-disk location is ignored.
The certKeyInfo and certificateAuthorityBundleInfo fields seem to be independent of location, why are they fields of the location types instead of sibling fields to location?
There was a problem hiding this comment.
I don't know how stable the API is yet
Feel free to suggest changes - these annotations are not widely used outside of control group, so now is a good time to improve them.
This looks like if both location fields are nil, the entry is ignored
We'll need to throw an error when this happens - TLS artifact is useless if we can't trace it back to an object or file.
and if both are non-nil, the presence of an on-disk location is ignored.
Correct - cluster objects are preferred as they can be annotated. Ondisk only certificates need to have their metadata set in origin repo here
| @@ -73,12 +73,12 @@ func (o annotationRequirement) generateInspectionMarkdown(pkiInfo *certs.PKIRegi | |||
|
|
|||
| for i := range pkiInfo.CertKeyPairs { | |||
| curr := pkiInfo.CertKeyPairs[i] | |||
| if curr.InClusterLocation == nil { | |||
| certKeyInfo := GetCertKeyPairInfo(curr) | |||
There was a problem hiding this comment.
Similar comment about the API shape here. This looks like it's working around the fact that "info" lives inside "location". Does it need to be that way?
There was a problem hiding this comment.
API can be changed, yes. TLS artifact locations are being backfilled with matching info here to build reports easier but it could as well be the other way around
| owner := curr.InClusterLocation.CertKeyInfo.OwningJiraComponent | ||
| regenerates, _ := AnnotationValue(curr.InClusterLocation.CertKeyInfo.SelectedCertMetadataAnnotations, o.GetAnnotationName()) | ||
| owner := certKeyInfo.OwningJiraComponent | ||
| regenerates, _ := AnnotationValue(certKeyInfo.SelectedCertMetadataAnnotations, o.GetAnnotationName()) |
There was a problem hiding this comment.
Does the regenerates identifier here and below pre-date making this check parameterized by o.annotationName?
Could there ever be a case where the empty string is a valid value for some required annotation? Why is this checking for the empty string value instead of annotation presence?
There was a problem hiding this comment.
Good idea, in description case we want to check both - mark it as a violation when its either not set or empty (currently we have a mix of both).
Other requirements may want to consider empty string as valid
| continue | ||
| } | ||
| owner := curr.InClusterLocation.CABundleInfo.OwningJiraComponent | ||
| regenerates, _ := AnnotationValue(curr.InClusterLocation.CABundleInfo.SelectedCertMetadataAnnotations, o.GetAnnotationName()) | ||
| owner := caBundleInfo.OwningJiraComponent |
There was a problem hiding this comment.
Note to me: Empty owner is validated separately.
Comment on lines
24
to
26
| - [Unknown (9)](#Unknown-9) | ||
| - [Certificates (3)](#Certificates-3) | ||
| - [Certificate Authority Bundles (6)](#Certificate-Authority-Bundles-6) |
There was a problem hiding this comment.
New generated markdown for file-based certs and CA bundles here.
tls/descriptions/descriptions.md
Outdated
Comment on lines
24
to
26
| - [Unknown (9)](#Unknown-9) | ||
| - [Certificates (3)](#Certificates-3) | ||
| - [Certificate Authority Bundles (6)](#Certificate-Authority-Bundles-6) |
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
2 times, most recently
from
9978da3
to
0db71c2
Compare
|
Job Failure Risk Analysis for sha: 0db71c2
|
| @@ -14,17 +14,17 @@ const UnknownOwner = "Unknown" | |||
|
|
|||
| var ( | |||
There was a problem hiding this comment.
I am afraid that I am missing the point.
Isn't /var/lib/kubelet/pki/kubelet-client-current.pem covered as a on-disk cert?
There was a problem hiding this comment.
Ah, good catch - we don't scan this directory. So far its limited to this list
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
0db71c2
to
4f1b3c5
Compare
|
Job Failure Risk Analysis for sha: 4f1b3c5
|
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
4 times, most recently
from
ccdf266
to
9391486
Compare
|
Job Failure Risk Analysis for sha: 9391486
|
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
2 times, most recently
from
b60b6bc
to
abdb681
Compare
|
Job Failure Risk Analysis for sha: abdb681
|
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
abdb681
to
af1abdc
Compare
|
Job Failure Risk Analysis for sha: af1abdc
|
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
3 times, most recently
from
0564ca6
to
4b43858
Compare
vrutkovs
changed the title
|
@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. |
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
4b43858
to
02d2902
Compare
vrutkovs
force-pushed
the
tls-artifacts-ondisk-todo
branch
from
02d2902
to
11b058c
Compare
|
@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. |
|
Looking at https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/28868/pull-ci-openshift-origin-master-e2e-agnostic-ovn-cmd/1828381245009039360, do we already know why "all tls artifacts must be registered" and "all registered tls artifacts must have no metadata violation regressions" flaked? Presumably something broke during the BeforeAll node. |
OK, seems like this is just what the test report looks like when we make a Ginkgo spec flake directly, and the spec isn't actually being run twice. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
make TLS artifacts tests requiredmoving this to new PR so that it would be easier to revert/reapplyTODO:
/etc/kubernetes/ca.crt?