add ownership for unshared secrets and configmaps #220

deads2k · 2023-09-15T19:34:13Z

The openshift/api field will merge and be revendored here.

serving secrets and injected configmaps are not shared, we stomp all the keys. Add the service-ca as the owner of these configmaps since we control the content.

deads2k · 2023-09-19T18:54:03Z

now updated with vendored constant

liouk · 2023-10-19T08:42:58Z

pkg/controller/servingcert/controller/secret_creating_controller.go

-					api.ServiceNameAnnotation: service.Name,
+					api.ServiceUIDAnnotation:          string(service.UID),
+					api.ServiceNameAnnotation:         service.Name,
+					apiannotations.OpenShiftComponent: "service-ca",


Can we use api.OwningJiraComponent here as well?

liouk · 2023-10-19T08:43:06Z

pkg/controller/servingcert/controller/secret_creating_controller.go

-				api.AlphaServiceNameAnnotation: service.Name,
+				api.AlphaServiceUIDAnnotation:     string(service.UID),
+				api.AlphaServiceNameAnnotation:    service.Name,
+				apiannotations.OpenShiftComponent: "service-ca",


Can we use api.OwningJiraComponent here as well?

liouk · 2023-10-19T08:56:28Z

pkg/controller/servingcert/controller/secret_updating_controller.go

@@ -207,7 +208,17 @@ func (sc *serviceServingCertUpdateController) ensureSecretData(service *v1.Servi
 		}
 		return true, nil
 	}
-	return update, nil
+
+	if update {


Minor refactoring suggestion; drop if update{} and:

if !update && len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 { secretCopy... update = true } return update, nil

I personally dislike it because you have to remember more state, but I will yield to the reviewer.

liouk · 2023-10-27T07:42:07Z

pkg/controller/cabundleinjector/configmap.go

@@ -109,6 +109,11 @@ func (bi *configMapCABundleInjector) Sync(ctx context.Context, syncCtx factory.S
 	// make a copy to avoid mutating cache state
 	configMapCopy := configMap.DeepCopy()
 	configMapCopy.Data = map[string]string{api.InjectionDataKey: bi.caBundle}
+	// set the owning-component unless someone else has claimed it.
+	if len(configMapCopy.Annotations[api.OwningJiraComponent]) == 0 {
+		configMapCopy.Annotations[api.OwningJiraComponent] = api.OwningJiraComponent


Shouldn't it be configMapCopy.Annotations[apiannotations.OpenShiftComponent] instead?

liouk · 2023-10-27T07:43:22Z

pkg/controller/servingcert/controller/secret_creating_controller.go

@@ -329,6 +329,7 @@ func toBaseSecret(service *corev1.Service) *corev1.Secret {
 				Annotations: map[string]string{
 					api.ServiceUIDAnnotation:  string(service.UID),
 					api.ServiceNameAnnotation: service.Name,
+					api.OwningJiraComponent:   "service-ca",


I actually meant to replace "service-ca" with api.OwningJiraComponent:

apiannotations.OpenShiftComponent: api.OwningJiraComponent,

liouk · 2023-10-27T07:44:03Z

pkg/controller/servingcert/controller/secret_creating_controller.go

@@ -342,6 +343,7 @@ func toBaseSecret(service *corev1.Service) *corev1.Secret {
 			Annotations: map[string]string{
 				api.AlphaServiceUIDAnnotation:  string(service.UID),
 				api.AlphaServiceNameAnnotation: service.Name,
+				api.OwningJiraComponent:        "service-ca",


apiannotations.OpenShiftComponent: api.OwningJiraComponent,

liouk · 2023-10-27T07:45:20Z

pkg/controller/servingcert/controller/secret_creating_controller.go

@@ -407,6 +409,7 @@ func toRequiredSecret(dnsSuffix string, ca *crypto.CA, intermediateCACert *x509.

 	secretCopy.Annotations[api.AlphaServingCertExpiryAnnotation] = servingCert.Certs[0].NotAfter.Format(time.RFC3339)
 	secretCopy.Annotations[api.ServingCertExpiryAnnotation] = servingCert.Certs[0].NotAfter.Format(time.RFC3339)
+	secretCopy.Annotations[api.OwningJiraComponent] = api.OwningJiraComponent


Shouldn't it be secretCopy.Annotations[apiannotations.OpenShiftComponent] instead?

liouk · 2023-10-27T07:46:36Z

pkg/controller/servingcert/controller/secret_creating_controller_test.go

@@ -147,6 +147,7 @@ func TestServiceServingCertControllerSync(t *testing.T) {
 			expectedSecretAnnotations: map[string]string{
 				api.AlphaServiceUIDAnnotation:  testServiceUID,
 				api.AlphaServiceNameAnnotation: testServiceName,
+				api.OwningJiraComponent:        api.OwningJiraComponent,


Wrong annotation name here as well; apiannotations.OpenShiftComponent. Several occurences in this file.

liouk · 2023-10-27T07:47:00Z

pkg/controller/servingcert/controller/secret_updating_controller.go

+
+	// set the owning-component unless someone else has claimed it.
+	if !update && len(secretCopy.Annotations[api.OwningJiraComponent]) == 0 {
+		secretCopy.Annotations[api.OwningJiraComponent] = api.OwningJiraComponent


Shouldn't it be secretCopy.Annotations[apiannotations.OpenShiftComponent] instead?

deads2k · 2023-10-27T12:54:48Z

I hate constants that cannot change for constants. tried again.

deads2k · 2023-10-27T19:18:12Z

huh, wonder if that's a flake or I broke it

/retest

deads2k · 2023-10-30T18:54:56Z

installer failure
/retest

deads2k · 2023-11-01T14:46:54Z

install failures

/retest

openshift-ci · 2023-11-02T20:03:48Z

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

liouk · 2023-11-03T08:17:54Z

/lgtm

openshift-ci · 2023-11-03T08:18:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liouk

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:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2023-11-21T01:49:15Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-service-ca-operator-container-v4.15.0-202311202349.p0.gfe88761.assembly.stream for distgit ose-service-ca-operator.
All builds following this will include this PR.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2023

openshift-ci bot requested a review from stlaz September 15, 2023 19:35

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2023

deads2k force-pushed the ownership branch from fd8e001 to 8605020 Compare September 19, 2023 18:52

deads2k changed the title ~~[wip] add ownership for unshared secrets and configmaps~~ add ownership for unshared secrets and configmaps Sep 19, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

deads2k force-pushed the ownership branch from 8605020 to 64645c3 Compare September 19, 2023 18:53

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

deads2k force-pushed the ownership branch from 64645c3 to bd2085a Compare September 27, 2023 21:06

liouk suggested changes Oct 19, 2023

View reviewed changes

deads2k force-pushed the ownership branch from bd2085a to 153d34d Compare October 26, 2023 20:15

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2023

deads2k force-pushed the ownership branch from 565dbaa to bfd68cc Compare October 26, 2023 20:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2023

liouk suggested changes Oct 27, 2023

View reviewed changes

deads2k force-pushed the ownership branch from bfd68cc to 7cc7d77 Compare October 27, 2023 12:54

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2023

deads2k force-pushed the ownership branch from 7cc7d77 to 41b9b86 Compare October 27, 2023 12:54

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2023

deads2k added 2 commits November 2, 2023 14:16

update openshift/api for const

5d43ef6

add ownership for unshared secrets and configmaps

565b605

deads2k force-pushed the ownership branch from d95d0d0 to 565b605 Compare November 2, 2023 18:16

openshift-ci bot assigned liouk Nov 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2023

deads2k added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 3, 2023

openshift-ci bot merged commit fe88761 into openshift:master Nov 3, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ownership for unshared secrets and configmaps #220

add ownership for unshared secrets and configmaps #220

deads2k commented Sep 15, 2023

deads2k commented Sep 19, 2023

liouk Oct 19, 2023

liouk Oct 19, 2023

liouk Oct 19, 2023

deads2k Oct 26, 2023

liouk Oct 27, 2023

liouk Oct 27, 2023

liouk Oct 27, 2023

liouk Oct 27, 2023

liouk Oct 27, 2023

liouk Oct 27, 2023

deads2k commented Oct 27, 2023

deads2k commented Oct 27, 2023

deads2k commented Oct 30, 2023

deads2k commented Nov 1, 2023

openshift-ci bot commented Nov 2, 2023

liouk commented Nov 3, 2023

openshift-ci bot commented Nov 3, 2023

openshift-bot commented Nov 21, 2023

add ownership for unshared secrets and configmaps #220

add ownership for unshared secrets and configmaps #220

Conversation

deads2k commented Sep 15, 2023

deads2k commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 27, 2023

deads2k commented Oct 27, 2023

deads2k commented Oct 30, 2023

deads2k commented Nov 1, 2023

openshift-ci bot commented Nov 2, 2023

liouk commented Nov 3, 2023

openshift-ci bot commented Nov 3, 2023

openshift-bot commented Nov 21, 2023