Skip to content

Commit 94fc682

Browse files
varshaprasad96varshaprasad96
authored
Address review comments for operator conditions helpers (#42)
* Address review comments for opertor conditions helpers * Move getOperatorNamespace() to a seprate utils package. * Allow namespace to be set locally with an env variable. * Modify the order of tests and add godoc to env variables. * Remove redundant code. * Update GetOperatorNamespace
1 parent 973850e commit 94fc682

File tree

7 files changed

+216
-184
lines changed

7 files changed

+216
-184
lines changed

‎conditions/operator_conditions.go

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,11 @@
1515
package conditions
1616

1717
import (
18-
"errors"
1918
"fmt"
20-
"io/ioutil"
2119
"os"
22-
"strings"
2320

2421
api "github.com/operator-framework/api/pkg/operators/v1"
22+
"github.com/operator-framework/operator-lib/internal/utils"
2523
"k8s.io/apimachinery/pkg/api/meta"
2624
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2725
"k8s.io/apimachinery/pkg/types"
@@ -32,26 +30,29 @@ var (
3230
ErrNoOperatorCondition = fmt.Errorf("operator Condition CRD is nil")
3331
)
3432

35-
// TODO: verify from OLM if this will be the name of the environment variable
36-
// which is set for the Condition resource owned by the operator.
37-
const operatorCondEnvVar = "OPERATOR_CONDITION_NAME"
33+
const (
34+
// operatorCondEnvVar is the env variable which
35+
// contains the name of the Condition CR associated to the operator,
36+
// set by OLM.
37+
operatorCondEnvVar = "OPERATOR_CONDITION_NAME"
38+
)
3839

39-
var readNamespace = func() ([]byte, error) {
40-
return ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")
41-
}
40+
var readNamespace = utils.GetOperatorNamespace
4241

4342
// GetNamespacedName returns the NamespacedName of the CR. It returns an error
4443
// when the name of the CR cannot be found from the environment variable set by
45-
// OLM, or when the namespace cannot be found from the associated service account
46-
// secret.
44+
// OLM. Hence, GetNamespacedName() can provide the NamespacedName when the operator
45+
// is running on cluster and is being managed by OLM. If running locally, operator
46+
// writers are encouraged to skip this method or gracefully handle the errors by logging
47+
// a message.
4748
func GetNamespacedName() (*types.NamespacedName, error) {
4849
conditionName := os.Getenv(operatorCondEnvVar)
4950
if conditionName == "" {
50-
return nil, fmt.Errorf("required env %s not set, cannot find operator condition CR for the operator", operatorCondEnvVar)
51+
return nil, fmt.Errorf("could not determine operator condition name: environment variable %s not set", operatorCondEnvVar)
5152
}
52-
operatorNs, err := getOperatorNamespace()
53+
operatorNs, err := readNamespace()
5354
if err != nil {
54-
return nil, err
55+
return nil, fmt.Errorf("could not determine operator namespace: %v", err)
5556
}
5657
return &types.NamespacedName{Name: conditionName, Namespace: operatorNs}, nil
5758
}
@@ -118,16 +119,3 @@ func IsConditionStatusPresentAndEqual(operatorCondition *api.OperatorCondition,
118119
}
119120
return false, nil
120121
}
121-
122-
// getOperatorNamespace returns the namespace the operator should be running in.
123-
func getOperatorNamespace() (string, error) {
124-
nsBytes, err := readNamespace()
125-
if err != nil {
126-
if os.IsNotExist(err) {
127-
return "", errors.New("cannot find namespace of the operator")
128-
}
129-
return "", err
130-
}
131-
ns := strings.TrimSpace(string(nsBytes))
132-
return ns, nil
133-
}

‎conditions/operator_conditions_test.go

Lines changed: 66 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,47 @@ var _ = Describe("Conditions helpers", func() {
5757
},
5858
}
5959
})
60+
61+
Describe("GetNamespacedName", func() {
62+
It("should error when name of the operator condition cannot be found", func() {
63+
err := os.Unsetenv(operatorCondEnvVar)
64+
Expect(err).NotTo(HaveOccurred())
65+
66+
objKey, err := GetNamespacedName()
67+
Expect(err).To(HaveOccurred())
68+
Expect(objKey).To(BeNil())
69+
Expect(err.Error()).To(ContainSubstring("could not determine operator condition name"))
70+
})
71+
72+
It("should error when object namespace cannot be found", func() {
73+
err := os.Setenv(operatorCondEnvVar, "test")
74+
Expect(err).NotTo(HaveOccurred())
75+
76+
readNamespace = func() (string, error) {
77+
return "", os.ErrNotExist
78+
}
79+
80+
objKey, err := GetNamespacedName()
81+
Expect(err).To(HaveOccurred())
82+
Expect(objKey).To(BeNil())
83+
Expect(err.Error()).To(ContainSubstring("could not determine operator namespace"))
84+
})
85+
86+
It("should return the right namespaced name from SA namespace file", func() {
87+
err := os.Setenv(operatorCondEnvVar, "test")
88+
Expect(err).NotTo(HaveOccurred())
89+
90+
readNamespace = func() (string, error) {
91+
return "testns", nil
92+
}
93+
objKey, err := GetNamespacedName()
94+
Expect(err).NotTo(HaveOccurred())
95+
Expect(objKey).NotTo(BeNil())
96+
Expect(objKey.Name).To(BeEquivalentTo("test"))
97+
Expect(objKey.Namespace).To(BeEquivalentTo("testns"))
98+
})
99+
})
100+
60101
Describe("SetOperatorCondition", func() {
61102
It("should set condition status", func() {
62103
newCond := metav1.Condition{
@@ -110,222 +151,145 @@ var _ = Describe("Conditions helpers", func() {
110151
})
111152

112153
Describe("RemoveOperatorCondition", func() {
113-
var (
114-
rmvConditionType string
115-
)
116154
It("should remove the condition", func() {
117-
rmvConditionType = conditionFoo
118-
Expect(RemoveOperatorCondition(operatorCondition, rmvConditionType)).NotTo(HaveOccurred())
155+
Expect(RemoveOperatorCondition(operatorCondition, conditionFoo)).NotTo(HaveOccurred())
119156
Expect(len(operatorCondition.Status.Conditions)).To(BeEquivalentTo(0))
120157
})
121158
It("should not error when condition to be removed is not available", func() {
122-
rmvConditionType = conditionBar
123-
Expect(RemoveOperatorCondition(operatorCondition, rmvConditionType)).NotTo(HaveOccurred())
159+
Expect(RemoveOperatorCondition(operatorCondition, conditionBar)).NotTo(HaveOccurred())
124160
Expect(len(operatorCondition.Status.Conditions)).To(BeEquivalentTo(1))
125161
})
126162
It("should error when operatorCondition is nil", func() {
127-
rmvConditionType = conditionFoo
128-
err := RemoveOperatorCondition(nil, rmvConditionType)
163+
err := RemoveOperatorCondition(nil, conditionFoo)
129164
Expect(err).To(HaveOccurred())
130165
Expect(err).Should(MatchError(ErrNoOperatorCondition))
131166
})
132167
})
133168

134169
Describe("FindOperatorCondition", func() {
135-
var (
136-
findConditionType string
137-
)
138-
139170
It("should return the condition if it exists", func() {
140-
findConditionType = conditionFoo
141171
conditionToFind := &metav1.Condition{
142172
Type: conditionFoo,
143173
Status: metav1.ConditionTrue,
144174
Reason: "foo",
145175
Message: "The operator is in foo condition",
146176
LastTransitionTime: transitionTime,
147177
}
148-
c, err := FindOperatorCondition(operatorCondition, findConditionType)
178+
c, err := FindOperatorCondition(operatorCondition, conditionFoo)
149179
Expect(err).NotTo(HaveOccurred())
150180
Expect(reflect.DeepEqual(c, conditionToFind)).To(BeTrue())
151181
})
152182
It("should return error when condition does not exist", func() {
153-
findConditionType = conditionBar
154-
c, err := FindOperatorCondition(operatorCondition, findConditionType)
183+
c, err := FindOperatorCondition(operatorCondition, conditionBar)
155184
Expect(err).To(HaveOccurred())
156185
Expect(c).To(BeNil())
157-
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", findConditionType)))
186+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
158187
})
159188
It("should error when operatorCondition is nil", func() {
160-
findConditionType = conditionFoo
161-
c, err := FindOperatorCondition(nil, findConditionType)
189+
c, err := FindOperatorCondition(nil, conditionFoo)
162190
Expect(err).To(HaveOccurred())
163191
Expect(c).To(BeNil())
164192
Expect(err).Should(MatchError(ErrNoOperatorCondition))
165193
})
166194
})
167195

168196
Describe("Verfiy status of the condition", func() {
169-
var (
170-
conditionStatusFor string
171-
)
172-
173197
It("should return correct value when condition exists", func() {
174-
conditionStatusFor = conditionFoo
175-
176198
// IsConditionStatusTrue should return true
177-
val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
199+
val, err := IsConditionStatusTrue(operatorCondition, conditionFoo)
178200
Expect(err).NotTo(HaveOccurred())
179201
Expect(val).To(BeTrue())
180202

181203
// IsConditionStatusFalse should return false
182-
val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
204+
val, err = IsConditionStatusFalse(operatorCondition, conditionFoo)
183205
Expect(err).NotTo(HaveOccurred())
184206
Expect(val).To(BeFalse())
185207

186208
// IsConditionStatusUnknown should return false
187-
val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
209+
val, err = IsConditionStatusUnknown(operatorCondition, conditionFoo)
188210
Expect(err).NotTo(HaveOccurred())
189211
Expect(val).To(BeFalse())
190212
})
191213

192214
It("should return false if condition status is not set to true", func() {
193-
conditionStatusFor = conditionFoo
194215
operatorCondition.Status.Conditions[0].Status = metav1.ConditionFalse
195216

196217
// IsConditionStatusTrue should return false
197-
val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
218+
val, err := IsConditionStatusTrue(operatorCondition, conditionFoo)
198219
Expect(err).NotTo(HaveOccurred())
199220
Expect(val).To(BeFalse())
200221

201222
// IsConditionStatusFalse should return true
202-
val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
223+
val, err = IsConditionStatusFalse(operatorCondition, conditionFoo)
203224
Expect(err).NotTo(HaveOccurred())
204225
Expect(val).To(BeTrue())
205226

206227
// IsConditionStatusUnknown should return false
207-
val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
228+
val, err = IsConditionStatusUnknown(operatorCondition, conditionFoo)
208229
Expect(err).NotTo(HaveOccurred())
209230
Expect(val).To(BeFalse())
210231
})
211232
It("should return false if condition status is unknown", func() {
212-
conditionStatusFor = conditionFoo
213233
operatorCondition.Status.Conditions[0].Status = metav1.ConditionUnknown
214234

215235
// IsConditionStatusTrue should return false
216-
val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
236+
val, err := IsConditionStatusTrue(operatorCondition, conditionFoo)
217237
Expect(err).NotTo(HaveOccurred())
218238
Expect(val).To(BeFalse())
219239

220240
// IsConditionStatusFalse should return false
221-
val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
241+
val, err = IsConditionStatusFalse(operatorCondition, conditionFoo)
222242
Expect(err).NotTo(HaveOccurred())
223243
Expect(val).To(BeFalse())
224244

225245
// IsConditionStatusUnknown should return true
226-
val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
246+
val, err = IsConditionStatusUnknown(operatorCondition, conditionFoo)
227247
Expect(err).NotTo(HaveOccurred())
228248
Expect(val).To(BeTrue())
229249

230250
})
231251
It("should error when condition cannot be found", func() {
232-
conditionStatusFor = conditionBar
233-
234252
// IsConditionStatusTrue should return error
235-
val, err := IsConditionStatusTrue(operatorCondition, conditionStatusFor)
253+
val, err := IsConditionStatusTrue(operatorCondition, conditionBar)
236254
Expect(err).To(HaveOccurred())
237-
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionStatusFor)))
255+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
238256
Expect(val).To(BeFalse())
239257

240258
// IsConditionStatusFalse should return error
241-
val, err = IsConditionStatusFalse(operatorCondition, conditionStatusFor)
259+
val, err = IsConditionStatusFalse(operatorCondition, conditionBar)
242260
Expect(err).To(HaveOccurred())
243-
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionStatusFor)))
261+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
244262
Expect(val).To(BeFalse())
245263

246264
// IsConditionStatusUnknown should return error
247-
val, err = IsConditionStatusUnknown(operatorCondition, conditionStatusFor)
265+
val, err = IsConditionStatusUnknown(operatorCondition, conditionBar)
248266
Expect(err).To(HaveOccurred())
249-
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionStatusFor)))
267+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s not found", conditionBar)))
250268
Expect(val).To(BeFalse())
251269
})
252270
})
253271

254272
Describe("IsConditionStatusPresentAndEqual", func() {
255-
var (
256-
conditionType string
257-
conditionStatus metav1.ConditionStatus
258-
)
259273

260274
It("should return true when condition is in the specified status", func() {
261-
conditionType = conditionFoo
262-
conditionStatus = metav1.ConditionTrue
263-
264-
val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionType, conditionStatus)
275+
val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionFoo, metav1.ConditionTrue)
265276
Expect(err).NotTo(HaveOccurred())
266277
Expect(val).To(BeTrue())
267278
})
268279

269280
It("should return false when condition is not present in the specified status", func() {
270-
conditionType = conditionFoo
271-
conditionStatus = metav1.ConditionUnknown
272-
273-
val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionType, conditionStatus)
281+
val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionFoo, metav1.ConditionUnknown)
274282
Expect(err).NotTo(HaveOccurred())
275283
Expect(val).To(BeFalse())
276284
})
277285

278286
It("should return error when condition is not present", func() {
279-
conditionType = conditionBar
280-
conditionStatus = metav1.ConditionTrue
281-
282-
val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionType, conditionStatus)
287+
val, err := IsConditionStatusPresentAndEqual(operatorCondition, conditionBar, metav1.ConditionTrue)
283288
Expect(err).To(HaveOccurred())
284289
Expect(val).To(BeFalse())
285290
})
286291

287292
})
288-
289-
Describe("GetNamespacedName", func() {
290-
It("should error when name of the operator condition cannot be found", func() {
291-
err := os.Unsetenv(operatorCondEnvVar)
292-
Expect(err).NotTo(HaveOccurred())
293-
294-
objKey, err := GetNamespacedName()
295-
Expect(err).To(HaveOccurred())
296-
Expect(objKey).To(BeNil())
297-
Expect(err.Error()).To(ContainSubstring("cannot find operator condition CR for the operator"))
298-
})
299-
300-
It("should error when object namespace cannot be found", func() {
301-
err := os.Setenv(operatorCondEnvVar, "test")
302-
Expect(err).NotTo(HaveOccurred())
303-
304-
readNamespace = func() ([]byte, error) {
305-
return nil, os.ErrNotExist
306-
}
307-
308-
objKey, err := GetNamespacedName()
309-
Expect(err).To(HaveOccurred())
310-
Expect(objKey).To(BeNil())
311-
Expect(err.Error()).To(ContainSubstring("cannot find namespace"))
312-
})
313-
314-
It("should return the right namespaced name", func() {
315-
err := os.Setenv(operatorCondEnvVar, "test")
316-
Expect(err).NotTo(HaveOccurred())
317-
318-
readNamespace = func() ([]byte, error) {
319-
return []byte("testns"), nil
320-
}
321-
objKey, err := GetNamespacedName()
322-
Expect(err).NotTo(HaveOccurred())
323-
Expect(objKey).NotTo(BeNil())
324-
Expect(objKey.Name).To(BeEquivalentTo("test"))
325-
Expect(objKey.Namespace).To(BeEquivalentTo("testns"))
326-
})
327-
})
328-
329293
})
330294

331295
func isConditionPresent(arr []metav1.Condition, con metav1.Condition) bool {

0 commit comments

Comments
 (0)