CFE-387: Added support for sts clusters #51

thejasn · 2022-04-18T09:54:53Z

No description provided.

openshift-ci · 2022-04-18T09:54:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

arjunrn

Still needs some work.

arjunrn · 2022-04-19T11:04:24Z

pkg/controllers/awsloadbalancercontroller/controller.go

@@ -50,6 +51,8 @@ const (
 	controllerWebhookPort = 9443
 	// common prefix for all resource of an operand
 	controllerResourcePrefix = "aws-load-balancer-controller"
+	// controllerReEnqueueDuration is the delay to re-enqueue.
+	controllerReEnqueueDuration = time.Second * 30


To be precise this value is used when the secret is missing, for some other retry condition it could be something else. So a better name would be secretMissingReEnqueueDuration.

arjunrn · 2022-04-19T11:09:15Z

pkg/controllers/awsloadbalancercontroller/controller.go

@@ -135,6 +138,20 @@ func (r *AWSLoadBalancerControllerReconciler) Reconcile(ctx context.Context, req
 		return ctrl.Result{}, fmt.Errorf("failed to ensure CredentialsRequest for AWSLoadBalancerController %q: %w", req.Name, err)
 	}

+	secretProvisioned, err := r.ensureCredentialsRequestSecret(ctx, credentialsRequest)


So we are not ensuring the credentials secret here. We are just checking if it is available. So something like provisioned, err := r.credentialsSecretProvisioned() would be a better fit. Also if you get a IsNotFound error then just return false. It keeps the outer calling function cleaner.

Also if you get a IsNotFound error then just return false. It keeps the outer calling function cleaner.

It doesn't help much, since we still need to handle other types of errors (unless we suppress those errors)

if err == nil && !secretProvisioned { // update status and re-enqueue } else if err != nil { // return non-NotFound errors }

All other errors we simply return an error even if secretProvisioned is true(because we never expect secretProvisioned to be true and for there to be an error).

provisioned, err := r.credentialsSecretProvisioned() if err!=nil { // this is only possible when there is some error other than not found // and we want to immediately retry because it's probably transient return err } r.updateStatus(..provisioned)

arjunrn · 2022-04-19T11:09:25Z

pkg/controllers/awsloadbalancercontroller/controller.go

+		// retrying after delay to ensure secret provisioning.
+		return ctrl.Result{RequeueAfter: controllerReEnqueueDuration}, nil
+	} else if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to ensure AWSLoadBalancerController %q service account: %w", req.Name, err)


This error message is wrong.

arjunrn · 2022-04-19T11:12:31Z

pkg/controllers/awsloadbalancercontroller/deployment.go

+							Name: boundSATokenVolumeName,
+							VolumeSource: corev1.VolumeSource{
+								Projected: &corev1.ProjectedVolumeSource{
+									DefaultMode: aws.Int32(420),


Make this a constant. Also you shouldn't be using the aws pointer helper function. Kubernetes has it's own pointer helper called pointer.Int32().

arjunrn · 2022-04-19T11:12:48Z

pkg/controllers/awsloadbalancercontroller/deployment.go

+									Sources: []corev1.VolumeProjection{{
+										ServiceAccountToken: &corev1.ServiceAccountTokenProjection{
+											Audience:          "openshift",
+											ExpirationSeconds: aws.Int64(3600),


Same goes for this.

arjunrn · 2022-04-19T11:14:03Z

pkg/controllers/awsloadbalancercontroller/deployment_test.go

+					Sources: []corev1.VolumeProjection{{
+						ServiceAccountToken: &corev1.ServiceAccountTokenProjection{
+							Audience:          "openshift",
+							ExpirationSeconds: aws.Int64(3600),


Change this please. Also do we have tests covering any change we make to the bound service account token?

arjunrn · 2022-04-19T11:15:02Z

pkg/controllers/awsloadbalancercontroller/serviceaccount.go

@@ -63,6 +64,7 @@ func desiredAWSLoadBalancerServiceAccount(namespace string, controller *albo.AWS
 			Namespace: namespace,
 			Name:      fmt.Sprintf("%s-%s", controllerResourcePrefix, controller.Name),
 		},
+		AutomountServiceAccountToken: aws.Bool(true),


Now that we are using a non default value we have to update the functions which verifies if the service account needs an update.

Yeah, my bad, I forgot this. Added.

arjunrn · 2022-04-19T11:16:12Z

pkg/controllers/awsloadbalancercontroller/status.go

-	CredentialsRequestAvailableCondition = "CredentialsRequestAvailable"
+	DeploymentAvailableCondition = "DeploymentAvailable"
+	DeploymentUpgradingCondition = "DeploymentUpgrading"
+	CredentialsRequestAvailable  = "CredentialsRequestAvailable"


This constant should still be called CredentialsRequestAvailableCondition.

arjunrn · 2022-04-19T11:17:38Z

pkg/controllers/awsloadbalancercontroller/status.go

@@ -34,42 +39,25 @@ func (r *AWSLoadBalancerControllerReconciler) updateControllerStatus(ctx context
 	return nil
 }

-func credentialRequestsConditions(cr *cco.CredentialsRequest, generation int64) []metav1.Condition {
+func credentialRequestsConditions(cr *cco.CredentialsRequest, secretProvisioned bool, generation int64) []metav1.Condition {
 	var conditions []metav1.Condition


I don't see the credentials request being used anywhere. Do we still have to pass it to this function.

Also the condition name now doesn't make sense. We should call it something like CredentialsSecretAvailable.

I don't see the credentials request being used anywhere. Do we still have to pass it to this function.

Changed argument to be just the secret name.

Also the condition name now doesn't make sense. We should call it something like CredentialsSecretAvailable.

Fixed

docs/sts/sts.md

arjunrn · 2022-04-19T16:18:42Z

docs/sts/sts.md

+### Extract and prepare the `ccoctl` binary
+
+1. Obtain the OpenShift Container Platform release image:
+
+    ```bash
+    RELEASE_IMAGE=$(./openshift-install version | awk '/release image/ {print $3}')
+    ```
+
+2. Get the CCO container image from the OpenShift Container Platform release image:
+
+    ```bash
+    CCO_IMAGE=$(oc adm release info --image-for='cloud-credential-operator' $RELEASE_IMAGE)
+    ```
+
+3. Extract the ccoctl binary from the CCO container image within the OpenShift
+Container Platform release image:
+
+    ```bash
+    oc image extract $CCO_IMAGE --file="/usr/bin/ccoctl" -a ~/.pull-secret
+    ```
+
+4. Change the permissions to make ccoctl executable:
+
+    ```bash
+    chmod 775 ccoctl
+    ```
+


Let's just link to the existing documentation where they describe how to get ccoctl.

arjunrn · 2022-04-19T16:20:50Z

docs/sts/sts.md

+3. Apply the secrets to your cluster:
+
+    ```bash
+    ls <path_to_ccoctl_output_dir>/manifests/*-credentials.yaml | xargs -I{} oc apply -f {}


<path_to_ccoctl_output_dir> is not defined anywhere.

arjunrn · 2022-04-19T16:22:07Z

pkg/controllers/awsloadbalancercontroller/credentials_request.go

@@ -78,6 +78,19 @@ func (r *AWSLoadBalancerControllerReconciler) ensureCredentialsRequest(ctx conte
 	return current, nil
 }

+func (r *AWSLoadBalancerControllerReconciler) credentialsSecretProvisioned(ctx context.Context, cr *cco.CredentialsRequest) (bool, error) {


This function should return a false, nil when the error is IsNotFound so that the caller doesn't have to handle it.

arjunrn · 2022-04-19T16:22:35Z

pkg/controllers/awsloadbalancercontroller/deployment.go

@@ -24,6 +24,9 @@ const (
 	appLabelName    = "app.kubernetes.io/name"
 	appName         = "aws-load-balancer-operator"
 	appInstanceName = "app.kubernetes.io/instance"
+	// awsSDKLoadConfigName is the name of the environment variable which enables shared configs.
+	// Without which certain fields in the config aren't set. Eg: role_arn.
+	awsSDKLoadConfigName = "AWS_SDK_LOAD_CONFIG"


Did you verify that this is actually required?

Yes, https://github.com/aws/aws-sdk-go/blob/v1.43.42/aws/session/shared_config.go#L332 doesn't bind role_arn without setting exOpts and AWS_SDK_LOAD_CONFIG sets that value.

arjunrn · 2022-04-21T08:25:56Z

pkg/controllers/awsloadbalancercontroller/controller.go

@@ -135,6 +138,23 @@ func (r *AWSLoadBalancerControllerReconciler) Reconcile(ctx context.Context, req
 		return ctrl.Result{}, fmt.Errorf("failed to ensure CredentialsRequest for AWSLoadBalancerController %q: %w", req.Name, err)
 	}

+	secretProvisioned, err := r.credentialsSecretProvisioned(ctx, credentialsRequest)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to ensure credentialsrequest secret %q for AWSLoadBalancerController %q: %w", credentialsRequest.Spec.SecretRef.Name, req.Name, err)


The error message should be different. Probably failed to verify if credentials secret %q for AWSLoadBalancerController %q has been provisioned.

arjunrn · 2022-04-21T08:25:56Z

pkg/controllers/awsloadbalancercontroller/controller.go

@@ -135,6 +138,23 @@ func (r *AWSLoadBalancerControllerReconciler) Reconcile(ctx context.Context, req
 		return ctrl.Result{}, fmt.Errorf("failed to ensure CredentialsRequest for AWSLoadBalancerController %q: %w", req.Name, err)
 	}

+	secretProvisioned, err := r.credentialsSecretProvisioned(ctx, credentialsRequest)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to ensure credentialsrequest secret %q for AWSLoadBalancerController %q: %w", credentialsRequest.Spec.SecretRef.Name, req.Name, err)


The error message should be different. Probably failed to verify if credentials secret %q for AWSLoadBalancerController %q has been provisioned.

arjunrn · 2022-04-21T08:39:55Z

/lgtm
/approve

openshift-ci · 2022-04-21T08:40:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arjunrn, thejasn

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~~ [arjunrn,thejasn]

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

thejasn · 2022-04-21T08:51:05Z

/assign @lihongan

lihongan · 2022-04-22T01:06:44Z

/label qe-approved

arjunrn · 2022-04-22T07:35:36Z

/label docs-approved
/label px-approved

openshift-ci · 2022-04-22T08:42:49Z

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

CFE-387: Added support for sts clusters

3d40130

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 Apr 18, 2022

openshift-ci bot requested review from alebedev87 and tjungblu April 18, 2022 09:56

added sa token mounting

f273dca

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2022

fixed re-enqueue logic

154dff1

thejasn marked this pull request as ready for review April 19, 2022 09:53

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 Apr 19, 2022

openshift-ci bot requested review from arjunrn and Miciah April 19, 2022 09:53

arjunrn suggested changes Apr 19, 2022

View reviewed changes

openshift-ci bot assigned arjunrn Apr 19, 2022

code review changes

83ef3cd

thejasn force-pushed the feature/sts-mode-creds branch from d889770 to 18743f0 Compare April 19, 2022 13:56

arjunrn reviewed Apr 19, 2022

View reviewed changes

docs/sts/sts.md Outdated Show resolved Hide resolved

arjunrn reviewed Apr 19, 2022

View reviewed changes

docs/sts/sts.md Outdated Show resolved Hide resolved

arjunrn reviewed Apr 19, 2022

View reviewed changes

thejasn force-pushed the feature/sts-mode-creds branch from 43dcee0 to c716a9a Compare April 20, 2022 05:42

arjunrn reviewed Apr 21, 2022

View reviewed changes

added docs for manual mode

c632a75

thejasn force-pushed the feature/sts-mode-creds branch from c716a9a to c632a75 Compare April 21, 2022 08:35

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2022

openshift-ci bot assigned lihongan Apr 21, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 22, 2022

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Apr 22, 2022

openshift-merge-robot merged commit 03659f5 into openshift:main Apr 22, 2022

CFE-387: Added support for sts clusters #51

CFE-387: Added support for sts clusters #51

Conversation

thejasn commented Apr 18, 2022

openshift-ci bot commented Apr 18, 2022

arjunrn left a 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

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

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

arjunrn commented Apr 21, 2022

openshift-ci bot commented Apr 21, 2022

thejasn commented Apr 21, 2022

lihongan commented Apr 22, 2022

arjunrn commented Apr 22, 2022

openshift-ci bot commented Apr 22, 2022