Bug 1896918: add new credentials field for AWS Secrets#264
Conversation
openshift-ci-robot
added
the
approved
joelddiaz
force-pushed
the
new-credentials-field
branch
from
99cebea to
7dca54d
Compare
dgoodwin
left a comment
dgoodwin
left a comment
There was a problem hiding this comment.
Looking great just a few nits and questions.
|
|
||
| // Make sure we update old Secrets that don't have the new "credentials" field | ||
| if credentialsKey == "" || credentialsKey != string(generateAWSCredentialsConfig(accessKey, secretKey)) { | ||
| logger.Debugf("Secret %s key needs updating, will update Secret contents", secretDataCredentialsKey) |
There was a problem hiding this comment.
Infof as I think this is low occurrence high value.
| require.NoError(t, err, "unexpected error creating/updating Secret") | ||
|
|
||
| secret := &corev1.Secret{} | ||
| secretKey := types.NamespacedName{Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace} |
There was a problem hiding this comment.
Suggest secretNSN or similar, key might be a little misleading here. I'm thinking Data[key] when I see that var.
| err = fakeClient.Get(context.TODO(), secretKey, secret) | ||
| require.NoError(t, err, "unexpected error retriving Secret") | ||
|
|
||
| require.Contains(t, secret.Data, "credentials") |
There was a problem hiding this comment.
Can we be pedantic and also check the old keys are there as expected as well.
| credentialsRequestInfraMismatch = "InfrastructureMismatch" | ||
| ) | ||
|
|
||
| var defaultRequeueTime = time.Hour + time.Minute*10 |
There was a problem hiding this comment.
An hour is conceptually easier to explain to people, is the 10 minute buying us anything? I could see a random jitter like we do in hive but really not a big concern for the scale CCO works at.
There was a problem hiding this comment.
Also please add a comment on why we're doing a requeue instead of just waiting for events.
There was a problem hiding this comment.
Since we explicitly don't re-reconcile more than once every hour, I was just thinking if there is a small difference between when the object is requeued and the lastSync values (where it requeues just before 1 hour so the lastSync is still less than 1 hour ago), then we could effectively slip out to reconciling to every 2 hours. Not a huge deal, but it is double the intended reconcile rate.
|
Good to squash. |
Start storing a usable AWS credentials config file in the 'credentials' field of the Secret. This should allow a consumer of the credentials to just point to the config stored in that field when setting up an AWS client. Also make sure we are re-queueing CredentialsRequest objects every 1hr10min (so that we are at least periodically doing a full reconcile to reestore any lost credentials).
joelddiaz
force-pushed
the
new-credentials-field
branch
from
0885d7d to
452bbc4
Compare
|
/test e2e-aws |
|
seems e2e-aws is failing because CCO clusteroperator is not reporting it's version even though the operator is Progressing=False and Available=True |
|
I think all e2e is broken because of https://bugzilla.redhat.com/show_bug.cgi?id=1891759. @akhil-rane is working on it over in #263, and I think they're actually going to be reverting the OpenShift change entirely, though that will take a little longer to make it to the build clusters. |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, 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 Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/cherrypick release-4.6 |
|
@joelddiaz: new pull request created: #268 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. |
|
/retitle Bug 1896918: add new credentials field for AWS Secrets |
openshift-ci-robot
changed the title
|
@joelddiaz: All pull requests linked via external trackers have merged: Bugzilla bug 1896918 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. |
|
/cherrypick release-4.6 |
|
@joelddiaz: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/cloud-credential-operator\n ! [rejected] cherry-pick-264-to-release-4.6 -> cherry-pick-264-to-release-4.6 (non-fast-forward)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:CENSORED@github.com/openshift-cherrypick-robot/cloud-credential-operator'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n", error: exit status 1 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. |
|
/cherrypick release-4.6 |
|
@sdodson: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/cloud-credential-operator\n ! [rejected] cherry-pick-264-to-release-4.6 -> cherry-pick-264-to-release-4.6 (non-fast-forward)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:CENSORED@github.com/openshift-cherrypick-robot/cloud-credential-operator'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n", error: exit status 1 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. |
Instead of the hard-coded "an hour ago". Catching up with 452bbc4 (add new credentials field for AWS Secrets, 2020-11-03, openshift#264), which created the syncPeriod variable. This pivot avoids the risk of the variable being updated and pushing the logs out of sync with the new duration value.
Instead of the hard-coded "an hour ago". Catching up with 452bbc4 (add new credentials field for AWS Secrets, 2020-11-03, openshift#264), which created the syncPeriod variable. This pivot avoids the risk of the variable being updated and pushing the logs out of sync with the new duration value.
Instead of the hard-coded "an hour ago". Catching up with 452bbc4 (add new credentials field for AWS Secrets, 2020-11-03, openshift#264), which created the syncPeriod variable. This pivot avoids the risk of the variable being updated and pushing the logs out of sync with the new duration value.
Instead of the hard-coded "an hour ago". Catching up with 452bbc4 (add new credentials field for AWS Secrets, 2020-11-03, openshift#264), which created the syncPeriod variable. This pivot avoids the risk of the variable being updated and pushing the logs out of sync with the new duration value.
Instead of the hard-coded "an hour ago". Catching up with 452bbc4 (add new credentials field for AWS Secrets, 2020-11-03, openshift#264), which created the syncPeriod variable. This pivot avoids the risk of the variable being updated and pushing the logs out of sync with the new duration value.
add new credentials field for AWS Secrets
8 participants
Start storing a usable AWS credentials config file in the 'credentials' field of the Secret. This should allow a consumer of the credentials to just point to the config stored in that field when setting up an AWS client.
Also make sure we are re-queuing CredentialsRequest objects every 1hr10min (so that we are at least periodically doing a full reconcile to restore any lost credentials).