Skip to content
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-1674: Add ownership annotations to new and existing olm-managed secrets #626

Merged
merged 1 commit into from Dec 4, 2023

Conversation

dinhxuanvu
Copy link
Member

As a part of certificates ownership work, all of new and existing secrets that are created with certificate information need to have ownership annotations. New secrets that are created with OLM certresources controller will have new ownership annotations injected at creation/update time. The existing olm-managed secrets that don't have the required annotations will get reconciled at startup when olm operator is restarted/redeployed. This PR only add OpenShift owning component annotation and the description annotation can be added later.

Signed-off-by: Vu Dinh vudinh@outlook.com

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023
@dinhxuanvu dinhxuanvu force-pushed the cert-annotations branch 2 times, most recently from 43986c1 to 2c514bb Compare November 30, 2023 14:34
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023
@dinhxuanvu dinhxuanvu force-pushed the cert-annotations branch 3 times, most recently from 7f08f40 to 554becb Compare November 30, 2023 15:10
@tmshort
Copy link
Contributor

tmshort commented Nov 30, 2023

This contains non-upstreamed changes to the staging directory.

Because of that, it needs to be marked specially. I believe [carry] needs to be added to the git commit headline.

…rets

As a part of certificates ownership work, all of new and existing secrets that are created with
certificate information need to have ownership annotations. New secrets that are created with OLM
certresources controller will have new ownership annotations injected at creation/update time. The
existing olm-managed secrets that don't have the required annotations will get reconciled at startup
when olm operator is restarted/redeployed. This PR only add OpenShift owning component annotation
and the description annotation can be added later.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
@dinhxuanvu
Copy link
Member Author

@tmshort @joelanford PTAL

@dinhxuanvu
Copy link
Member Author

/test e2e-gcp-olm

}
for _, secret := range secrets {
if secret.Annotations[install.OpenShiftComponent] == "" {
secret.Annotations[install.OpenShiftComponent] = install.OLMOwnershipAnnotation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this question applies here and in the cert generation code: Should we really say that the openshift component for every secret that OLM manages on behalf of operators is an OLM component?

Or could we properly associate the secrets we manage for the Foo operator to the Foo component?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I see your point, it doesn't change the fact that OLM is creating these secrets (and the certificates as well). Until OLM can delegate that responsibility to someone else, OLM effectively owns those resources from certificate ownership perspective. Also, the operators don't control these secrets. They are the consumers from that perspective. It is hardly logical for them to claim ownership on these resources that they don't have control over.

@dinhxuanvu dinhxuanvu changed the title Add ownership annotations to new and existing olm-managed secrets API-1674: Add ownership annotations to new and existing olm-managed secrets Dec 1, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 1, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 1, 2023

@dinhxuanvu: This pull request references API-1674 which is a valid jira issue.

In response to this:

As a part of certificates ownership work, all of new and existing secrets that are created with certificate information need to have ownership annotations. New secrets that are created with OLM certresources controller will have new ownership annotations injected at creation/update time. The existing olm-managed secrets that don't have the required annotations will get reconciled at startup when olm operator is restarted/redeployed. This PR only add OpenShift owning component annotation and the description annotation can be added later.

Signed-off-by: Vu Dinh vudinh@outlook.com

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.

@dinhxuanvu
Copy link
Member Author

@joelanford @tmshort Seeking for a LGTM here unless there are any further feedbacks.

@tmshort
Copy link
Contributor

tmshort commented Dec 4, 2023

I can look after lunch EST

Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023
Copy link
Contributor

openshift-ci bot commented Dec 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, tmshort

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

openshift-ci bot commented Dec 4, 2023

@dinhxuanvu: 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.

@openshift-merge-bot openshift-merge-bot bot merged commit 85c579f into openshift:master Dec 4, 2023
12 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build operator-registry-container-v4.15.0-202312042232.p0.g85c579f.assembly.stream for distgit operator-registry.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build operator-lifecycle-manager-container-v4.15.0-202312042232.p0.g85c579f.assembly.stream for distgit operator-lifecycle-manager.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants