Skip to content

Commit 6959fe2

Browse files
benluddybenluddy
committed
Remove component adoption labels before initial CSV copy.
Operator component labels were not being stripped before initially creating a CSV copy (they were only being stripped on subsequent updates to copies). Signed-off-by: Ben Luddy <[email protected]>
1 parent 4168648 commit 6959fe2

File tree

2 files changed

+143
-72
lines changed

2 files changed

+143
-72
lines changed

‎pkg/controller/operators/olm/operatorgroup.go

+37-55
Original file line numberDiff line numberDiff line change
@@ -705,79 +705,61 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
705705
}
706706

707707
logger := a.logger.WithField("operator-ns", csv.GetNamespace()).WithField("target-ns", namespace)
708+
708709
newCSV := csv.DeepCopy()
710+
newCSV.SetNamespace(namespace)
711+
newCSV.SetResourceVersion("")
712+
newCSV.SetLabels(utillabels.AddLabel(newCSV.GetLabels(), v1alpha1.CopiedLabelKey, csv.GetNamespace()))
713+
// remove Operator object component labels before copying so that copied CSVs do not show up in the component list
714+
for k := range newCSV.GetLabels() {
715+
if strings.HasPrefix(k, decorators.ComponentLabelKeyPrefix) {
716+
delete(newCSV.Labels, k)
717+
}
718+
}
709719
delete(newCSV.Annotations, v1.OperatorGroupTargetsAnnotationKey)
710-
711-
fetchedCSV, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(namespace).Get(newCSV.GetName())
712-
logger = logger.WithField("csv", csv.GetName())
713-
if fetchedCSV != nil {
714-
logger.Debug("checking annotations")
715-
716-
if !reflect.DeepEqual(a.copyOperatorGroupAnnotations(&fetchedCSV.ObjectMeta), a.copyOperatorGroupAnnotations(&newCSV.ObjectMeta)) ||
717-
len(decorators.OperatorNames(fetchedCSV.GetLabels())) > 0 {
720+
newCSV.Status.Reason = v1alpha1.CSVReasonCopied
721+
newCSV.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", csv.GetNamespace())
722+
723+
logger.Debug("copying CSV to target")
724+
copiedCSV, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Create(context.TODO(), newCSV, metav1.CreateOptions{})
725+
if k8serrors.IsAlreadyExists(err) {
726+
copiedCSV, err = a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(namespace).Get(newCSV.GetName())
727+
if !reflect.DeepEqual(a.copyOperatorGroupAnnotations(&copiedCSV.ObjectMeta), a.copyOperatorGroupAnnotations(&newCSV.ObjectMeta)) ||
728+
len(decorators.OperatorNames(copiedCSV.GetLabels())) > 0 {
718729
// TODO: only copy over the opgroup annotations, not _all_ annotations
719-
fetchedCSV.Annotations = newCSV.Annotations
720-
fetchedCSV.SetLabels(utillabels.AddLabel(fetchedCSV.GetLabels(), v1alpha1.CopiedLabelKey, csv.GetNamespace()))
730+
copiedCSV.Annotations = newCSV.Annotations
731+
copiedCSV.SetLabels(utillabels.AddLabel(copiedCSV.GetLabels(), v1alpha1.CopiedLabelKey, csv.GetNamespace()))
721732

722733
// remove Operator object component labels before copying so that copied CSVs do not show up in the component list
723-
for k := range fetchedCSV.GetLabels() {
734+
for k := range copiedCSV.GetLabels() {
724735
if strings.HasPrefix(k, decorators.ComponentLabelKeyPrefix) {
725-
delete(fetchedCSV.Labels, k)
736+
delete(copiedCSV.Labels, k)
726737
}
727738
}
728739

729740
// CRs don't support strategic merge patching, but in the future if they do this should be updated to patch
730-
logger.Debug("updating target CSV")
731-
if fetchedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Update(context.TODO(), fetchedCSV, metav1.UpdateOptions{}); err != nil {
741+
logger.Debug("updating existing target CSV")
742+
if copiedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Update(context.TODO(), copiedCSV, metav1.UpdateOptions{}); err != nil {
732743
logger.WithError(err).Error("update target CSV failed")
733744
return nil, err
734745
}
735746
}
736-
737-
logger.Debug("checking status")
738-
newCSV.Status = csv.Status
739-
newCSV.Status.Reason = v1alpha1.CSVReasonCopied
740-
newCSV.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", csv.GetNamespace())
741-
742-
if !reflect.DeepEqual(fetchedCSV.Status, newCSV.Status) {
743-
logger.Debug("updating status")
744-
// Must use fetchedCSV because UpdateStatus(...) checks resource UID.
745-
fetchedCSV.Status = newCSV.Status
746-
if fetchedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(context.TODO(), fetchedCSV, metav1.UpdateOptions{}); err != nil {
747-
logger.WithError(err).Error("status update for target CSV failed")
748-
return nil, err
749-
}
750-
}
751-
752-
return fetchedCSV, nil
753-
754-
} else if k8serrors.IsNotFound(err) {
755-
newCSV.SetNamespace(namespace)
756-
newCSV.SetResourceVersion("")
757-
newCSV.SetLabels(utillabels.AddLabel(newCSV.GetLabels(), v1alpha1.CopiedLabelKey, csv.GetNamespace()))
758-
759-
logger.Debug("copying CSV to target")
760-
createdCSV, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Create(context.TODO(), newCSV, metav1.CreateOptions{})
761-
if err != nil {
762-
a.logger.Errorf("Create for new CSV failed: %v", err)
763-
return nil, err
764-
}
765-
createdCSV.Status.Reason = v1alpha1.CSVReasonCopied
766-
createdCSV.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", csv.GetNamespace())
767-
if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(context.TODO(), createdCSV, metav1.UpdateOptions{}); err != nil {
768-
a.logger.Errorf("Status update for CSV failed: %v", err)
769-
return nil, err
770-
}
771-
772-
return createdCSV, nil
773-
774747
} else if err != nil {
775-
logger.WithError(err).Error("couldn't get CSV")
748+
a.logger.Errorf("Create for new CSV failed: %v", err)
776749
return nil, err
777750
}
778751

779-
// this return shouldn't be hit
780-
return nil, fmt.Errorf("unhandled code path")
752+
logger.Debug("checking status")
753+
if !reflect.DeepEqual(copiedCSV.Status, newCSV.Status) {
754+
logger.Debug("updating status")
755+
// Must use copiedCSV because UpdateStatus(...) checks resource UID.
756+
copiedCSV.Status = newCSV.Status
757+
if copiedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(context.TODO(), copiedCSV, metav1.UpdateOptions{}); err != nil {
758+
logger.WithError(err).Error("status update for target CSV failed")
759+
return nil, err
760+
}
761+
}
762+
return copiedCSV, nil
781763
}
782764

783765
func (a *Operator) pruneFromNamespace(operatorGroupName, namespace string) error {

‎pkg/controller/operators/olm/operatorgroup_test.go

+106-17
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
package olm
22

33
import (
4-
"io/ioutil"
54
"testing"
65
"time"
76

8-
"github.com/sirupsen/logrus"
7+
"github.com/sirupsen/logrus/hooks/test"
98
"github.com/stretchr/testify/require"
109

1110
"k8s.io/apimachinery/pkg/api/errors"
@@ -74,6 +73,20 @@ func TestCopyToNamespace(t *testing.T) {
7473
},
7574
},
7675
ExpectedActions: []ktesting.Action{
76+
ktesting.NewCreateAction(gvr, "bar", &v1alpha1.ClusterServiceVersion{
77+
ObjectMeta: metav1.ObjectMeta{
78+
Name: "name",
79+
Namespace: "bar",
80+
Labels: map[string]string{
81+
"olm.copiedFrom": "foo",
82+
},
83+
},
84+
Status: v1alpha1.ClusterServiceVersionStatus{
85+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
86+
Message: "The operator is running in foo but is managing this namespace",
87+
Reason: v1alpha1.CSVReasonCopied,
88+
},
89+
}),
7790
ktesting.NewUpdateSubresourceAction(gvr, "status", "bar", &v1alpha1.ClusterServiceVersion{
7891
ObjectMeta: metav1.ObjectMeta{
7992
Name: "name",
@@ -107,7 +120,8 @@ func TestCopyToNamespace(t *testing.T) {
107120
Status: v1alpha1.ClusterServiceVersionStatus{
108121
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
109122
Message: "The operator is running in foo but is managing this namespace",
110-
Reason: v1alpha1.CSVReasonCopied},
123+
Reason: v1alpha1.CSVReasonCopied,
124+
},
111125
},
112126
ExpectedResult: &v1alpha1.ClusterServiceVersion{
113127
ObjectMeta: metav1.ObjectMeta{
@@ -120,18 +134,34 @@ func TestCopyToNamespace(t *testing.T) {
120134
Reason: v1alpha1.CSVReasonCopied,
121135
},
122136
},
137+
ExpectedActions: []ktesting.Action{
138+
ktesting.NewCreateAction(gvr, "bar", &v1alpha1.ClusterServiceVersion{
139+
ObjectMeta: metav1.ObjectMeta{
140+
Name: "name",
141+
Namespace: "bar",
142+
Labels: map[string]string{
143+
"olm.copiedFrom": "foo",
144+
},
145+
},
146+
Status: v1alpha1.ClusterServiceVersionStatus{
147+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
148+
Message: "The operator is running in foo but is managing this namespace",
149+
Reason: v1alpha1.CSVReasonCopied,
150+
},
151+
}),
152+
},
123153
},
124154
{
125-
Name: "component labels are stripped before copy",
155+
Name: "component labels are stripped before updating copy",
126156
Namespace: "bar",
127157
Original: &v1alpha1.ClusterServiceVersion{
128158
ObjectMeta: metav1.ObjectMeta{
129-
Name: "name",
159+
Name: "name",
130160
Namespace: "foo",
131161
Labels: map[string]string{
132162
"operators.coreos.com/foo": "",
133163
"operators.coreos.com/bar": "",
134-
"untouched": "fine",
164+
"untouched": "fine",
135165
},
136166
},
137167
},
@@ -142,40 +172,100 @@ func TestCopyToNamespace(t *testing.T) {
142172
Labels: map[string]string{
143173
"operators.coreos.com/foo": "",
144174
"operators.coreos.com/bar": "",
145-
"untouched": "fine",
175+
"untouched": "fine",
146176
},
147177
},
148178
Status: v1alpha1.ClusterServiceVersionStatus{
149-
Message: "The operator is running in foo but is managing this namespace",
150-
Reason: v1alpha1.CSVReasonCopied},
179+
Message: "The operator is running in foo but is managing this namespace",
180+
Reason: v1alpha1.CSVReasonCopied},
151181
},
152182
ExpectedResult: &v1alpha1.ClusterServiceVersion{
153183
ObjectMeta: metav1.ObjectMeta{
154184
Name: "name",
155185
Namespace: "bar",
156186
Labels: map[string]string{
157-
"untouched": "fine",
187+
"untouched": "fine",
158188
"olm.copiedFrom": "foo",
159189
},
160190
},
161191
Status: v1alpha1.ClusterServiceVersionStatus{
162-
Message: "The operator is running in foo but is managing this namespace",
163-
Reason: v1alpha1.CSVReasonCopied,
192+
Message: "The operator is running in foo but is managing this namespace",
193+
Reason: v1alpha1.CSVReasonCopied,
164194
},
165195
},
166196
ExpectedActions: []ktesting.Action{
197+
ktesting.NewCreateAction(gvr, "bar", &v1alpha1.ClusterServiceVersion{
198+
ObjectMeta: metav1.ObjectMeta{
199+
Name: "name",
200+
Namespace: "bar",
201+
Labels: map[string]string{
202+
"untouched": "fine",
203+
"olm.copiedFrom": "foo",
204+
},
205+
},
206+
Status: v1alpha1.ClusterServiceVersionStatus{
207+
Message: "The operator is running in foo but is managing this namespace",
208+
Reason: v1alpha1.CSVReasonCopied,
209+
},
210+
}),
167211
ktesting.NewUpdateAction(gvr, "bar", &v1alpha1.ClusterServiceVersion{
168212
ObjectMeta: metav1.ObjectMeta{
169213
Name: "name",
170214
Namespace: "bar",
171215
Labels: map[string]string{
172-
"untouched": "fine",
216+
"untouched": "fine",
173217
"olm.copiedFrom": "foo",
174218
},
175219
},
176220
Status: v1alpha1.ClusterServiceVersionStatus{
177-
Message: "The operator is running in foo but is managing this namespace",
178-
Reason: v1alpha1.CSVReasonCopied,
221+
Message: "The operator is running in foo but is managing this namespace",
222+
Reason: v1alpha1.CSVReasonCopied,
223+
},
224+
}),
225+
},
226+
},
227+
{
228+
Name: "component labels are stripped before initial copy",
229+
Namespace: "bar",
230+
Original: &v1alpha1.ClusterServiceVersion{
231+
ObjectMeta: metav1.ObjectMeta{
232+
Name: "name",
233+
Namespace: "foo",
234+
Labels: map[string]string{
235+
"operators.coreos.com/foo": "",
236+
"operators.coreos.com/bar": "",
237+
"untouched": "fine",
238+
},
239+
},
240+
},
241+
ExistingCopy: nil,
242+
ExpectedResult: &v1alpha1.ClusterServiceVersion{
243+
ObjectMeta: metav1.ObjectMeta{
244+
Name: "name",
245+
Namespace: "bar",
246+
Labels: map[string]string{
247+
"untouched": "fine",
248+
"olm.copiedFrom": "foo",
249+
},
250+
},
251+
Status: v1alpha1.ClusterServiceVersionStatus{
252+
Message: "The operator is running in foo but is managing this namespace",
253+
Reason: v1alpha1.CSVReasonCopied,
254+
},
255+
},
256+
ExpectedActions: []ktesting.Action{
257+
ktesting.NewCreateAction(gvr, "bar", &v1alpha1.ClusterServiceVersion{
258+
ObjectMeta: metav1.ObjectMeta{
259+
Name: "name",
260+
Namespace: "bar",
261+
Labels: map[string]string{
262+
"untouched": "fine",
263+
"olm.copiedFrom": "foo",
264+
},
265+
},
266+
Status: v1alpha1.ClusterServiceVersionStatus{
267+
Message: "The operator is running in foo but is managing this namespace",
268+
Reason: v1alpha1.CSVReasonCopied,
179269
},
180270
}),
181271
},
@@ -192,8 +282,7 @@ func TestCopyToNamespace(t *testing.T) {
192282
v1alpha1lister.ClusterServiceVersionListerReturns(FakeClusterServiceVersionLister{tc.ExistingCopy})
193283
}
194284

195-
logger := logrus.New()
196-
logger.SetOutput(ioutil.Discard)
285+
logger, _ := test.NewNullLogger()
197286

198287
o := &Operator{
199288
lister: lister,

0 commit comments

Comments
 (0)