OCPBUGS-16684: Set cr.status.provisioned=false on syncErr path #583

bentito · 2023-07-26T18:09:43Z

I think this cures the bug as long as I tracked from the correct part of the code that was setting cr.status.provisioned to true despite a failure to actually provision the Secret.

openshift-ci-robot · 2023-07-26T18:09:51Z

@bentito: This pull request references Jira Issue OCPBUGS-16684, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @fxierh

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

I think this cures the bug as long as I tracked from the correct part of the code that was setting cr.status.provisioned to true despite a failure to actually provision the Secret.

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.

bentito · 2023-07-26T18:15:21Z

@fxierh I think I could have fixed it with this change, but I'm not totally sure. If you can run this PR, set conditions again, and we'll see if it is fixed. If it's not fixed, I'll see how I can debug on this PR, perhaps with the STS workflow with clusterbot.

codecov · 2023-07-26T19:36:47Z

Codecov Report

Merging #583 (eb6ef9b) into master (b4d1c3d) will increase coverage by 0.23%.
Report is 19 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
+ Coverage   48.15%   48.38%   +0.23%     
==========================================
  Files          94       94              
  Lines       11543    11641      +98     
==========================================
+ Hits         5558     5633      +75     
- Misses       5377     5398      +21     
- Partials      608      610       +2

Files Changed	Coverage Δ
pkg/aws/actuator/actuator.go	`63.90% <0.00%> (-0.16%)`	⬇️
...redentialsrequest/credentialsrequest_controller.go	`53.25% <0.00%> (+4.32%)`	⬆️

... and 5 files with indirect coverage changes

fxierh · 2023-07-27T09:56:48Z

pkg/operator/credentialsrequest/credentialsrequest_controller.go

@@ -688,20 +688,17 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec

 		var syncErr error
 		syncErr = r.CreateOrUpdateOnCredsExist(ctx, credsExists, syncErr, cr)


When CR.spec.providerSpec.stsIAMRoleARN is unset, syncErr is still nil

cloud-credential-operator/pkg/aws/actuator/actuator.go

Lines 342 to 345 in 859de53

if awsSTSIAMRoleARN == "" {

logger.Debug("CredentialsRequest has no awsSTSIAMRoleARN, no reason to sync")

return nil

}

So we fall into the else branch below where we call

cloud-credential-operator/pkg/operator/credentialsrequest/credentialsrequest_controller.go

Line 724 in 859de53

updateErr := r.UpdateProvisionedStatus(cr, true)

In other words CR.status.provisioned is still set to true when CR.spec.providerSpec.stsIAMRoleARN is unset and there is no Secret provisioned.

The comment above aligns with what I observed today in a test (on this PR).

Thanks! I think my next commit fixes it then.

fxierh · 2023-07-27T09:59:43Z

For debugging IMO you can try to install an STS cluster, scale down CVO & CCO and then run the controllers locally with the latest commit.

fxierh · 2023-07-27T10:04:53Z

Btw just wanted to check if you're planning to address the comment I left under https://issues.redhat.com/browse/OCPBUGS-16684 in this PR (or I can open another card for it if you want them to be separated) ?

bentito · 2023-07-27T15:09:46Z

For debugging IMO you can try to install an STS cluster, scale down CVO & CCO and then run the controllers locally with the latest commit.

Yeah, that's basically what's happening with clusterbot, but with automation, but it's way slow for a quick test to see if I got something wrong or right.

bentito · 2023-07-27T15:11:50Z

Btw just wanted to check if you're planning to address the comment I left under https://issues.redhat.com/browse/OCPBUGS-16684 in this PR (or I can open another card for it if you want them to be separated) ?

If you could make a new card about the lastSyncGeneration problems that would be good. Thanks.

fxierh · 2023-07-27T15:25:03Z

pkg/aws/actuator/actuator.go

 		if awsSTSIAMRoleARN == "" {
 			logger.Debug("CredentialsRequest has no awsSTSIAMRoleARN, no reason to sync")
-			return nil


I haven't tested your latest commit in action yet but it appears to me that:
if the stsIAMRoleARN field is unset, this awsSTSIAMRoleARN string would be empty and err would be nil. So returning err would be equivalent to returning nil.

Yep, new commit, didn't think that through very well. Thanks!

fxierh · 2023-07-28T16:27:56Z

@bentito With the current implementation the .status field of CRs without .stsIAMRoleARN (e.g. the ones extracted via oc adm release extract $IMG --cloud=aws --credentials-requests) looks like the following:

status:
  conditions:
  - lastProbeTime: "2023-07-28T16:08:52Z"
    lastTransitionTime: "2023-07-28T16:08:52Z"
    message: 'failed to grant creds: an empty roleARN was found so no Secret was created'
    reason: CredentialsProvisionFailure
    status: "True"
    type: CredentialsProvisionFailure
  lastSyncGeneration: 0
  lastSyncTimestamp: "2023-07-28T16:16:09Z"
  provisioned: false

Just wanted to make sure this is expected behavior.

fxierh · 2023-07-28T16:40:01Z

On the other hand, the aforementioned failing condition (CredentialsProvisionFailure) is not reflected at the CO level (i.e. we won't see "%d of %d credentials requests are failing to sync" when calling oc get co cloud-credential -o yaml:

cloud-credential-operator/pkg/operator/credentialsrequest/status.go

Lines 107 to 108 in 859de53

    
           if operatorIsDisabled { 
        
           	return conditions

This makes me wonder if we should display Secret provision failures on CO/cloud-credential in STS mode just like in mint.
cc. @abutcher

fxierh · 2023-07-28T16:59:49Z

If you could make a new card about the lastSyncGeneration problems that would be good. Thanks.

Done.

bentito · 2023-08-01T04:14:02Z

/test e2e-aws-ovn

bentito · 2023-08-01T06:18:13Z

Just wanted to make sure this is expected behavior.
@fxierh Yes, I think this is as desired...
And if so, @abutcher , I think this can get an LGTM?

pkg/aws/actuator/actuator.go

Co-authored-by: Andrew Butcher <abutcher@redhat.com>

abutcher · 2023-08-09T14:12:22Z

I'll make a card for the CO level status.
/lgtm

openshift-ci · 2023-08-09T14:18:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abutcher, bentito

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~~ [abutcher]

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

openshift-ci-robot · 2023-08-09T14:55:02Z

/retest-required

Remaining retests: 0 against base HEAD 4031723 and 2 for PR HEAD eb6ef9b in total

bentito · 2023-08-09T15:27:56Z

@abutcher ci/prow/e2e-azure-manual-oidc is showing up as a required test here and it definitely shouldn't be here on an AWS related PR I'd think. Or at least not as a required...

abutcher · 2023-08-09T17:21:47Z

Related to a recent change we made to the tests which should have made e2e-azure-manual-oidc required for changes which impact specific pathing but are instead making the test required.

/override ci/prow/e2e-azure-manual-oidc

openshift-ci · 2023-08-09T17:26:06Z

@abutcher: Overrode contexts on behalf of abutcher: ci/prow/e2e-azure-manual-oidc

In response to this:

Related to a recent change we made to the tests which should have made e2e-azure-manual-oidc required for changes which impact specific pathing but are instead making the test required.

/override ci/prow/e2e-azure-manual-oidc

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.

bentito · 2023-08-10T04:58:30Z

/test e2e-hypershift

bentito · 2023-08-10T08:49:38Z

/test e2e-hypershift

bentito · 2023-08-10T12:20:30Z

/test e2e-hypershift

bentito · 2023-08-10T18:21:06Z

/test e2e-hypershift

openshift-ci · 2023-08-10T20:31:12Z

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

openshift-ci-robot · 2023-08-10T20:36:03Z

@bentito: Jira Issue OCPBUGS-16684: All pull requests linked via external trackers have merged:

openshift/cloud-credential-operator#583

Jira Issue OCPBUGS-16684 has been moved to the MODIFIED state.

In response to this:

I think this cures the bug as long as I tracked from the correct part of the code that was setting cr.status.provisioned to true despite a failure to actually provision the Secret.

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.

Set cr.status.provisioned=false on syncErr path

f32ce70

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Jul 26, 2023

openshift-ci bot requested a review from fxierh July 26, 2023 18:10

openshift-ci bot requested review from 2uasimojo and suhanime July 26, 2023 18:16

fxierh reviewed Jul 27, 2023

View reviewed changes

Change AWS STS syncErr outcome for no ARN set

e21bd86

fxierh reviewed Jul 27, 2023

View reviewed changes

bentito added 2 commits July 27, 2023 22:54

Create error for empty roleARN

82b40ef

Now correctly bounces empty roleARN so update test

efb0ea6

abutcher reviewed Aug 4, 2023

View reviewed changes

pkg/aws/actuator/actuator.go Outdated Show resolved Hide resolved

Use the name of the field rather than roleARN

eb6ef9b

Co-authored-by: Andrew Butcher <abutcher@redhat.com>

openshift-ci bot assigned abutcher Aug 9, 2023

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

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

openshift-merge-robot merged commit f5b6c10 into openshift:master Aug 10, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-16684: Set cr.status.provisioned=false on syncErr path #583

OCPBUGS-16684: Set cr.status.provisioned=false on syncErr path #583

bentito commented Jul 26, 2023

openshift-ci-robot commented Jul 26, 2023

bentito commented Jul 26, 2023

codecov bot commented Jul 26, 2023 •

edited

fxierh Jul 27, 2023

fxierh Jul 27, 2023

bentito Jul 27, 2023

fxierh commented Jul 27, 2023 •

edited

fxierh commented Jul 27, 2023 •

edited

bentito commented Jul 27, 2023

bentito commented Jul 27, 2023

fxierh Jul 27, 2023

bentito Jul 27, 2023

fxierh commented Jul 28, 2023 •

edited

fxierh commented Jul 28, 2023

fxierh commented Jul 28, 2023

bentito commented Aug 1, 2023

bentito commented Aug 1, 2023

abutcher commented Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

openshift-ci-robot commented Aug 9, 2023

bentito commented Aug 9, 2023

abutcher commented Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

bentito commented Aug 10, 2023

bentito commented Aug 10, 2023

bentito commented Aug 10, 2023

bentito commented Aug 10, 2023

openshift-ci bot commented Aug 10, 2023

openshift-ci-robot commented Aug 10, 2023

		@@ -688,20 +688,17 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec

		var syncErr error
		syncErr = r.CreateOrUpdateOnCredsExist(ctx, credsExists, syncErr, cr)

	if awsSTSIAMRoleARN == "" {
	logger.Debug("CredentialsRequest has no awsSTSIAMRoleARN, no reason to sync")
	return nil
	}

OCPBUGS-16684: Set cr.status.provisioned=false on syncErr path #583

OCPBUGS-16684: Set cr.status.provisioned=false on syncErr path #583

Conversation

bentito commented Jul 26, 2023

openshift-ci-robot commented Jul 26, 2023

bentito commented Jul 26, 2023

codecov bot commented Jul 26, 2023 • edited

Codecov Report

fxierh Jul 27, 2023

Choose a reason for hiding this comment

fxierh Jul 27, 2023

Choose a reason for hiding this comment

bentito Jul 27, 2023

Choose a reason for hiding this comment

fxierh commented Jul 27, 2023 • edited

fxierh commented Jul 27, 2023 • edited

bentito commented Jul 27, 2023

bentito commented Jul 27, 2023

fxierh Jul 27, 2023

Choose a reason for hiding this comment

bentito Jul 27, 2023

Choose a reason for hiding this comment

fxierh commented Jul 28, 2023 • edited

fxierh commented Jul 28, 2023

fxierh commented Jul 28, 2023

bentito commented Aug 1, 2023

bentito commented Aug 1, 2023

abutcher commented Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

openshift-ci-robot commented Aug 9, 2023

bentito commented Aug 9, 2023

abutcher commented Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

bentito commented Aug 10, 2023

bentito commented Aug 10, 2023

bentito commented Aug 10, 2023

bentito commented Aug 10, 2023

openshift-ci bot commented Aug 10, 2023

openshift-ci-robot commented Aug 10, 2023

codecov bot commented Jul 26, 2023 •

edited

fxierh commented Jul 27, 2023 •

edited

fxierh commented Jul 27, 2023 •

edited

fxierh commented Jul 28, 2023 •

edited