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
OCPBUGS-24212: Add ownership annotation for certificates #2158
OCPBUGS-24212: Add ownership annotation for certificates #2158
Conversation
vrutkovs
commented
Nov 21, 2023
•
edited
edited
- I added CHANGELOG entry for this change.
- No user facing changes, so no entry in CHANGELOG was needed.
Skipping CI for Draft Pull Request. |
/test e2e-aws-ovn |
fa3fe09
to
8344fc0
Compare
/test e2e-aws-ovn |
8344fc0
to
27256e2
Compare
/test e2e-aws-ovn |
27256e2
to
d20f426
Compare
/test e2e-aws-ovn |
d20f426
to
2ee9e24
Compare
/test e2e-aws-ovn |
2ee9e24
to
0df7f1d
Compare
/test e2e-aws-ovn |
1 similar comment
/test e2e-aws-ovn |
cdd19be
to
1d1db14
Compare
/test e2e-aws-ovn |
1d1db14
to
5e5899e
Compare
How these new annotation will be used? |
This would help customers file correct bug reports. This case is fairly obvious, but its not the case for other components. Also it can help us find situations when a team adds a new unregistered TLS artifact. More metadata would be added to these secrets later. See openshift/enhancements#1502 for more information |
I see the changes introduced here are in-line with: "As an Openshift developer, I want to be able to find which team manages a particular certificate.". While this looks fine, we should hold off on merging this until the EP gets in. |
Ownership annotations are already in the openshif/api. The enhancement is about building an automatic report against those annotations, not the annotations themselves. IOW: the annotations are ready for merge now. |
We pre-generate static assets and I think the annotation can be added there already. See https://github.com/openshift/cluster-monitoring-operator/blob/master/jsonnet/utils/generate-certificate-injection.libsonnet. Adding the annotation there will probably do the trick. That'll keep things consistent for future ca-bundle ConfigMaps. |
@vrutkovs: This pull request references Jira Issue OCPBUGS-24212, 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: 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. |
IIUC these will not be applied to the upgraded clusters? If these would work too I can create a follow up PR |
They will/should be applied on upgrades. |
Ah, certs with Also, more cert-specific metadata would be added there (i.e. cert description) so we still need a place where its being set per-cert/configmap |
Adding the "openshift.io/owning-component" annotation in the jsonnet code is the right thing to do IMHO because the information is static and it's way easier for future ourselves. IIUC we don't have to do anything for certificates which are issued by the service-ca operator and the cluster-network-operator? |
@simonpasquier I don't mind adding the annotations to jsonnet code, but I can't seem to find where these are defined. Any pointers? |
The labels/annotations can be added here and it will be applied to all ca bundle config maps generated from this jsonnet function. Here is where the CA bundle is generate for alertmanager. We put static informations in jsonnet files and dynamic informations in the controller code. |
5e5899e
to
b8fd250
Compare
You now just need to run |
b8fd250
to
c8fe7cd
Compare
Done, now component annotation is set in jsonnet where possible |
/test e2e-aws-ovn-techpreview |
@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/test-infra repository. I understand the commands that are listed here. |
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 APPROVED This pull-request has been approved by: simonpasquier, vrutkovs 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 |
@vrutkovs: Jira Issue OCPBUGS-24212: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-24212 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. |
Fix included in accepted release 4.15.0-0.nightly-2023-12-08-080446 |