Bug 2035315: fix passthrough test cases#438
Conversation
AWS passthrough test cases were not asserting that the CredentialsRequest .spec.secretRef contents matched the root secret creds. Fixing this revealed several places in the passthrough path where we could be doing a better job of bubbling up appropriate errors when the root credentials are simply not good enough to be passed through to satisfy the CredentialsRequest. Lastly, check for and return a potential error that was being ignored. Add test cases to cover root credentials rotation, and test cases for when insufficient root creds are found.
openshift-ci
Bot
added
the
approved
|
/assign @akhil-rane |
Codecov Report
@@ Coverage Diff @@
## master #438 +/- ##
==========================================
+ Coverage 46.17% 46.36% +0.18%
==========================================
Files 91 92 +1
Lines 9236 9204 -32
==========================================
+ Hits 4265 4267 +2
+ Misses 4455 4417 -38
- Partials 516 520 +4
|
joelddiaz
changed the title
openshift-ci
Bot
added
the
bugzilla/severity-medium
|
@joelddiaz: This pull request references Bugzilla bug 2035315, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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. |
openshift-ci
Bot
added
the
bugzilla/valid-bug
| mode, _, err := utils.GetOperatorConfiguration(a.Client, logger) | ||
| if err != nil { | ||
| msg := "error getting operator configuration" | ||
| logger.WithError(err).Error(msg) | ||
| return &actuatoriface.ActuatorError{ | ||
| ErrReason: minterv1.CredentialsProvisionFailure, | ||
| Message: fmt.Sprintf("%v: %v", msg, err), | ||
| } | ||
| } | ||
| if mode == operatorv1.CloudCredentialsModePassthrough { | ||
| logger.Debug("will not perform permissions simulation because operator in mode %q", mode) | ||
| } else { |
There was a problem hiding this comment.
why do we again need to confirm the mode here if we are already syncing for passthrough?
There was a problem hiding this comment.
So there's a difference between the Secret being annotated with "passthrough" and the CloudCredential Operator config saying that the mode is explicitly "Passthrough".
When the operator is explicitly configured with a non-default mode (eg not the empty string ""), then it is a way to tell CCO not to perform permissions validations. We came up with this behavior to address the case where a cluster is installed in an account where SCP policies would keep us from reliably being able to SimulatePolicy(). https://github.com/openshift/enhancements/blob/ce4d303db807622687159eb9d3248285a003fabb/enhancements/installer/aws-permissions-check-bypass.md
So in the mode = "" case (the default), CCO will see if the creds can do Mint, and use that if possible. If that fails, it will check for Passthrough, and use that if possible. But in either case where CCO dynamically determined Mint or Passthrough (ie CCO wasn't explicitly told), CCO will do the SimulatePolicy() calls to check whether the creds really are good enough.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akhil-rane, joelddiaz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@joelddiaz: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@joelddiaz: All pull requests linked via external trackers have merged: Bugzilla bug 2035315 has been moved to the MODIFIED state. DetailsIn response to this:
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. |
Bug 2035315: fix passthrough test cases
4 participants
AWS passthrough test cases were not asserting that the
CredentialsRequest .spec.secretRef contents matched the root
secret creds.
Fixing this revealed several places in the passthrough path where we
could be doing a better job of bubbling up appropriate errors when the
root credentials are simply not good enough to be passed through to
satisfy the CredentialsRequest.
Lastly, check for and return a potential error that was being ignored.
Add test cases to cover root credentials rotation, and test cases for
when insufficient root creds are found.