Skip to content

Commit bd6e541

Browse files
jsafranejsafrane
committed
Allow CredentialsRequestController to check custom env. vars
CredentialsRequestController syncs / does not sync its CredentialsRequest based on presence of env. vars for AWS STS or GCE WIF. These env. vars are hardcoded. Allow the CredentialsRequest to set the env. vars it expects to exist in the CredentialsRequest annotations. So a CSI driver operator (AWS EFS) can have two CredentialsRequest, each synced / not synced based on different env. vars.
1 parent cd26fa5 commit bd6e541

File tree

2 files changed

+212
-19
lines changed

2 files changed

+212
-19
lines changed

‎pkg/operator/csi/credentialsrequestcontroller/credentials_request_controller.go‎

Lines changed: 83 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@ package credentialsrequestcontroller
33
import (
44
"context"
55
"fmt"
6-
"k8s.io/klog/v2"
76
"os"
87
"strings"
98
"time"
109

10+
"k8s.io/klog/v2"
11+
1112
apierrors "k8s.io/apimachinery/pkg/api/errors"
1213
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1314
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -26,6 +27,7 @@ import (
2627

2728
const (
2829
clusterCloudCredentialName = "cluster"
30+
EnvVarsAnnotationKey = "credentials.openshift.io/role-arns-vars"
2931
)
3032

3133
// CredentialsRequestController is a simple controller that maintains a CredentialsRequest static manifest.
@@ -34,6 +36,13 @@ const (
3436
// <name>Available: indicates that the secret was successfully provisioned by cloud-credential-operator.
3537
// <name>Progressing: indicates that the secret is yet to be provisioned by cloud-credential-operator.
3638
// <name>Degraded: produced when the sync() method returns an error.
39+
// The controller does not sync the CredentialsRequest if the cloud-credential-operator is in manual mode and
40+
// STS (or other short-term credentials) are not enabled, or STS is enabled, but the controller does not see
41+
// required env. vars set.
42+
// For AWS STS, the controller needs ROLEARN env. var set.
43+
// For GCP WIF, the controller needs POOL_ID, PROVIDER_ID, SERVICE_ACCOUNT_EMAIL, PROJECT_NUMBER env. vars set.
44+
// The controller also supports a custom annotation "credentials.openshift.io/role-arns-vars" on the CredentialsRequest
45+
// that allows to specify the comma-separated list of env. vars that should be used to set the role ARNs.
3746
type CredentialsRequestController struct {
3847
name string
3948
operatorClient v1helpers.OperatorClientWithFinalizers
@@ -93,15 +102,17 @@ func (c CredentialsRequestController) sync(ctx context.Context, syncContext fact
93102
return nil
94103
}
95104

96-
sync, err := shouldSync(c.operatorLister)
105+
cr := resourceread.ReadCredentialRequestsOrDie(c.manifest)
106+
107+
sync, err := shouldSync(c.operatorLister, cr)
97108
if err != nil {
98109
return err
99110
}
100111
if !sync {
101-
return nil
112+
return c.cleanConditions(ctx)
102113
}
103114

104-
cr, err := c.syncCredentialsRequest(ctx, spec, status, syncContext)
115+
cr, err = c.syncCredentialsRequest(ctx, spec, status, cr, syncContext)
105116
if err != nil {
106117
return err
107118
}
@@ -143,9 +154,9 @@ func (c CredentialsRequestController) syncCredentialsRequest(
143154
ctx context.Context,
144155
spec *opv1.OperatorSpec,
145156
status *opv1.OperatorStatus,
157+
cr *unstructured.Unstructured,
146158
syncContext factory.SyncContext,
147159
) (*unstructured.Unstructured, error) {
148-
cr := resourceread.ReadCredentialRequestsOrDie(c.manifest)
149160
err := unstructured.SetNestedField(cr.Object, c.targetNamespace, "spec", "secretRef", "namespace")
150161
if err != nil {
151162
return nil, err
@@ -174,6 +185,30 @@ func (c CredentialsRequestController) syncCredentialsRequest(
174185
return cr, err
175186
}
176187

188+
// cleanConditions cleans up the conditions when the controller does not sync any credentials request.
189+
func (c CredentialsRequestController) cleanConditions(ctx context.Context) error {
190+
availableCondition := opv1.OperatorCondition{
191+
Type: c.name + opv1.OperatorStatusTypeAvailable,
192+
Status: opv1.ConditionTrue,
193+
Message: "No role for short time token provided",
194+
Reason: "CredentialsDisabled",
195+
}
196+
197+
progressingCondition := opv1.OperatorCondition{
198+
Type: c.name + opv1.OperatorStatusTypeProgressing,
199+
Status: opv1.ConditionFalse,
200+
Message: "",
201+
Reason: "AsExpected",
202+
}
203+
_, _, err := v1helpers.UpdateStatus(
204+
ctx,
205+
c.operatorClient,
206+
v1helpers.UpdateConditionFn(availableCondition),
207+
v1helpers.UpdateConditionFn(progressingCondition),
208+
)
209+
return err
210+
}
211+
177212
func isProvisioned(cr *unstructured.Unstructured) (bool, error) {
178213
provisionedVal, found, err := unstructured.NestedFieldNoCopy(cr.Object, "status", "provisioned")
179214
if err != nil {
@@ -196,7 +231,7 @@ func isProvisioned(cr *unstructured.Unstructured) (bool, error) {
196231
return provisionedValBool, nil
197232
}
198233

199-
func shouldSync(cloudCredentialLister operatorv1lister.CloudCredentialLister) (bool, error) {
234+
func shouldSync(cloudCredentialLister operatorv1lister.CloudCredentialLister, cr *unstructured.Unstructured) (bool, error) {
200235
clusterCloudCredential, err := cloudCredentialLister.Get(clusterCloudCredentialName)
201236
if err != nil {
202237
klog.Errorf("Failed to get cluster cloud credential: %v", err)
@@ -205,19 +240,55 @@ func shouldSync(cloudCredentialLister operatorv1lister.CloudCredentialLister) (b
205240

206241
isManualMode := clusterCloudCredential.Spec.CredentialsMode == opv1.CloudCredentialsModeManual
207242

243+
if !isManualMode {
244+
// Return early, always sync in non-manual mode
245+
return true, nil
246+
}
247+
248+
// Manual mode. Sync only when env. vars with the role ARNs are present.
249+
250+
if envVars := getEnvVarsFromAnnotations(cr); envVars != nil {
251+
allEnvVarsFound := true
252+
for _, envVar := range envVars {
253+
if os.Getenv(envVar) == "" {
254+
allEnvVarsFound = false
255+
break
256+
}
257+
}
258+
// The CredentialsRequest has explicitly asked for some env. vars. via the annotation.
259+
// Don't check the default env. vars below, because they are not relevant for thiss CredentialsRequest.
260+
return allEnvVarsFound, nil
261+
}
262+
263+
// Default env. vars for AWS STS
208264
isAWSSTSEnabled := os.Getenv("ROLEARN") != ""
265+
if isAWSSTSEnabled {
266+
return true, nil
267+
}
209268

269+
// Default env. vars for GCE WIF
210270
poolID := os.Getenv("POOL_ID")
211271
providerID := os.Getenv("PROVIDER_ID")
212272
serviceAccountEmail := os.Getenv("SERVICE_ACCOUNT_EMAIL")
213273
projectNumber := os.Getenv("PROJECT_NUMBER")
214-
215274
isGCPWIFEnabled := poolID != "" && providerID != "" && serviceAccountEmail != "" && projectNumber != ""
216-
217-
// If cluster is in manual mode without short-term credentials enabled, do not sync cloud credentials.
218-
if isManualMode && !isAWSSTSEnabled && !isGCPWIFEnabled {
219-
return false, nil
275+
if isGCPWIFEnabled {
276+
return true, nil
220277
}
221278

222-
return true, nil
279+
// CCO is in manual mode, but some ARN env. vars are missing -> don't sync
280+
return false, nil
281+
}
282+
283+
func getEnvVarsFromAnnotations(cr *unstructured.Unstructured) []string {
284+
annotations := cr.GetAnnotations()
285+
if annotations == nil {
286+
return nil
287+
}
288+
envVars, found := annotations[EnvVarsAnnotationKey]
289+
if !found {
290+
return nil
291+
}
292+
envVarsList := strings.Split(envVars, ",")
293+
return envVarsList
223294
}

‎pkg/operator/csi/credentialsrequestcontroller/credentials_request_controller_test.go‎

Lines changed: 129 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,14 @@ import (
44
"context"
55
"errors"
66
"fmt"
7-
clocktesting "k8s.io/utils/clock/testing"
87
"os"
98
"reflect"
109
"strconv"
1110
"testing"
1211
"time"
1312

13+
clocktesting "k8s.io/utils/clock/testing"
14+
1415
apierrors "k8s.io/apimachinery/pkg/api/errors"
1516
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1617
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -325,15 +326,29 @@ func (fake *fakeDynamicClient) ApplyStatus(ctx context.Context, name string, obj
325326
}
326327

327328
func TestShouldSync(t *testing.T) {
329+
defaultCRManifest := makeFakeManifest(operandName, credentialRequestNamespace, operandNamespace)
330+
defaultCR := resourceread.ReadCredentialRequestsOrDie(defaultCRManifest)
331+
332+
emptyAnnotationCR := defaultCR.DeepCopy()
333+
emptyAnnotationCR.SetAnnotations(map[string]string{EnvVarsAnnotationKey: ""})
334+
335+
singleEnvVarAnnotationCR := emptyAnnotationCR.DeepCopy()
336+
singleEnvVarAnnotationCR.SetAnnotations(map[string]string{EnvVarsAnnotationKey: "NODE_ROLEARN"})
337+
338+
multipleEnvVarAnnotationCR := emptyAnnotationCR.DeepCopy()
339+
multipleEnvVarAnnotationCR.SetAnnotations(map[string]string{EnvVarsAnnotationKey: "NODE_POOL_ID,NODE_PROVIDER_ID,NODE_SERVICE_ACCOUNT_EMAIL,NODE_PROJECT_NUMBER"})
340+
328341
tests := []struct {
329342
name string
343+
credentialsRequest *unstructured.Unstructured
330344
cloudCredential *opv1.CloudCredential
331345
envVars map[string]string
332346
expectedShouldSync bool
333347
expectedError bool
334348
}{
335349
{
336-
name: "Default mode",
350+
name: "Default mode",
351+
credentialsRequest: defaultCR,
337352
cloudCredential: &opv1.CloudCredential{
338353
ObjectMeta: metav1.ObjectMeta{
339354
Name: clusterCloudCredentialName,
@@ -346,7 +361,8 @@ func TestShouldSync(t *testing.T) {
346361
expectedError: false,
347362
},
348363
{
349-
name: "Manual mode without short-term credentials",
364+
name: "Manual mode without short-term credentials",
365+
credentialsRequest: defaultCR,
350366
cloudCredential: &opv1.CloudCredential{
351367
ObjectMeta: metav1.ObjectMeta{
352368
Name: clusterCloudCredentialName,
@@ -359,7 +375,8 @@ func TestShouldSync(t *testing.T) {
359375
expectedError: false,
360376
},
361377
{
362-
name: "Manual mode with AWS STS enabled",
378+
name: "Manual mode with AWS STS enabled",
379+
credentialsRequest: defaultCR,
363380
cloudCredential: &opv1.CloudCredential{
364381
ObjectMeta: metav1.ObjectMeta{
365382
Name: clusterCloudCredentialName,
@@ -375,7 +392,8 @@ func TestShouldSync(t *testing.T) {
375392
expectedError: false,
376393
},
377394
{
378-
name: "Manual mode with GCP WIF enabled",
395+
name: "Manual mode with GCP WIF enabled",
396+
credentialsRequest: defaultCR,
379397
cloudCredential: &opv1.CloudCredential{
380398
ObjectMeta: metav1.ObjectMeta{
381399
Name: clusterCloudCredentialName,
@@ -394,7 +412,8 @@ func TestShouldSync(t *testing.T) {
394412
expectedError: false,
395413
},
396414
{
397-
name: "Manual mode with partial GCP WIF configuration",
415+
name: "Manual mode with partial GCP WIF configuration",
416+
credentialsRequest: defaultCR,
398417
cloudCredential: &opv1.CloudCredential{
399418
ObjectMeta: metav1.ObjectMeta{
400419
Name: clusterCloudCredentialName,
@@ -413,10 +432,113 @@ func TestShouldSync(t *testing.T) {
413432
},
414433
{
415434
name: "Error getting cloud credential",
435+
credentialsRequest: defaultCR,
416436
cloudCredential: nil,
417437
expectedShouldSync: false,
418438
expectedError: true,
419439
},
440+
{
441+
name: "Empty annotation",
442+
credentialsRequest: emptyAnnotationCR,
443+
cloudCredential: &opv1.CloudCredential{
444+
ObjectMeta: metav1.ObjectMeta{
445+
Name: clusterCloudCredentialName,
446+
},
447+
Spec: opv1.CloudCredentialSpec{
448+
CredentialsMode: opv1.CloudCredentialsModeManual,
449+
},
450+
},
451+
expectedShouldSync: false, // CredentialsRequest has the annotation, but it's empty
452+
expectedError: false,
453+
},
454+
{
455+
name: "Single annotation with env. var set",
456+
credentialsRequest: singleEnvVarAnnotationCR,
457+
cloudCredential: &opv1.CloudCredential{
458+
ObjectMeta: metav1.ObjectMeta{
459+
Name: clusterCloudCredentialName,
460+
},
461+
Spec: opv1.CloudCredentialSpec{
462+
CredentialsMode: opv1.CloudCredentialsModeManual,
463+
},
464+
},
465+
envVars: map[string]string{
466+
"NODE_ROLEARN": "arn:aws:iam::123456789012:role/test-role",
467+
},
468+
expectedShouldSync: true, // The env. var is set, so we should sync
469+
expectedError: false,
470+
},
471+
{
472+
name: "Single annotation with env. var unset",
473+
credentialsRequest: singleEnvVarAnnotationCR,
474+
cloudCredential: &opv1.CloudCredential{
475+
ObjectMeta: metav1.ObjectMeta{
476+
Name: clusterCloudCredentialName,
477+
},
478+
Spec: opv1.CloudCredentialSpec{
479+
CredentialsMode: opv1.CloudCredentialsModeManual,
480+
},
481+
},
482+
envVars: map[string]string{},
483+
expectedShouldSync: false,
484+
expectedError: false,
485+
},
486+
{
487+
name: "Single annotation with ROLEARN env. var set",
488+
credentialsRequest: singleEnvVarAnnotationCR,
489+
cloudCredential: &opv1.CloudCredential{
490+
ObjectMeta: metav1.ObjectMeta{
491+
Name: clusterCloudCredentialName,
492+
},
493+
Spec: opv1.CloudCredentialSpec{
494+
CredentialsMode: opv1.CloudCredentialsModeManual,
495+
},
496+
},
497+
envVars: map[string]string{
498+
"ROLEARN": "arn:aws:iam::123456789012:role/test-role",
499+
},
500+
expectedShouldSync: false, // The CredentialsRequests annotation asked for NODE_ROLEARN and that one is not set. It takes precedence over ROLEARN.
501+
expectedError: false,
502+
},
503+
{
504+
name: "Multiple annotations with env. var set",
505+
credentialsRequest: multipleEnvVarAnnotationCR,
506+
cloudCredential: &opv1.CloudCredential{
507+
ObjectMeta: metav1.ObjectMeta{
508+
Name: clusterCloudCredentialName,
509+
},
510+
Spec: opv1.CloudCredentialSpec{
511+
CredentialsMode: opv1.CloudCredentialsModeManual,
512+
},
513+
},
514+
envVars: map[string]string{
515+
"NODE_POOL_ID": "test-pool",
516+
"NODE_PROVIDER_ID": "test-provider",
517+
"NODE_SERVICE_ACCOUNT_EMAIL": "[email protected]",
518+
"NODE_PROJECT_NUMBER": "123456789",
519+
},
520+
expectedShouldSync: true, // All env. var are set
521+
expectedError: false,
522+
},
523+
{
524+
name: "Multiple annotations with some env. var unset",
525+
credentialsRequest: multipleEnvVarAnnotationCR,
526+
cloudCredential: &opv1.CloudCredential{
527+
ObjectMeta: metav1.ObjectMeta{
528+
Name: clusterCloudCredentialName,
529+
},
530+
Spec: opv1.CloudCredentialSpec{
531+
CredentialsMode: opv1.CloudCredentialsModeManual,
532+
},
533+
},
534+
envVars: map[string]string{
535+
"NODE_POOL_ID": "test-pool",
536+
"NODE_PROVIDER_ID": "test-provider",
537+
"NODE_SERVICE_ACCOUNT_EMAIL": "[email protected]",
538+
},
539+
expectedShouldSync: false, // NODE_PROJECT_NUMBER is not set
540+
expectedError: false,
541+
},
420542
}
421543

422544
for _, tc := range tests {
@@ -443,7 +565,7 @@ func TestShouldSync(t *testing.T) {
443565
}()
444566

445567
// Act
446-
shouldSync, err := shouldSync(cloudCredentialInformer.Operator().V1().CloudCredentials().Lister())
568+
shouldSync, err := shouldSync(cloudCredentialInformer.Operator().V1().CloudCredentials().Lister(), tc.credentialsRequest)
447569

448570
// Assert
449571
if tc.expectedError && err == nil {

0 commit comments

Comments
 (0)