-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
THREESCALE-8772 - Adding support for STS authentication for S3 for system components #792
THREESCALE-8772 - Adding support for STS authentication for S3 for system components #792
Conversation
Hi @valerymo. Thanks for your PR. I'm waiting for a 3scale member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
b7f82a2
to
0e385a1
Compare
return fmt.Errorf("Neither '%s' nor '%s' Secret field found in secret '%s'", component.AwsAccessKeyID, component.AwsRoleArn, awsCredentialsSecretName) | ||
} | ||
sts = true //if we are here - it's sts mode | ||
result = helper.GetSecretDataValue(secretData, component.AwsSecretAccessKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does look right think we should be checking the AWS_WEB_IDENTITY_TOKEN_FILE here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ... sorry .. didn't complete. Fixed. Thank you very much
0e385a1
to
4a02376
Compare
@@ -22,6 +22,8 @@ type SystemReconciler struct { | |||
*BaseAPIManagerLogicReconciler | |||
} | |||
|
|||
var sts = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could move this inside validateS3StorageProvidedConfiguration() function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sure, fixed. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @austincunningham , I attached file with details of testing done (sts and non sts, valid and not valid secrets)
4a02376
to
25d5585
Compare
/ok-to-test |
@valerymo have you checked that the system-sidekiq pods environment variables have been applied when you changed to the STS formatted secret ? |
The tests are checking the existence or omission of the fields. I think that one e2e manual test would be necessary for this particular case. |
Hi @austincunningham , I'm working to add volumes now (not pushed yet), and I see env vars in system-sidekiq , I added it. Will submit EOD oc describe pod system-sidekiq-1-wclkf |grep aws-token
. . . |
b80fae6
to
c87086c
Compare
c87086c
to
f1a1676
Compare
8a27caf
to
e467851
Compare
e467851
to
4eb892a
Compare
I suggest you push commits with small contributions. And only when it is ready to merge, it is your call if you want to squash them in a single commit or instead keep all the commits. But for reviewing, it makes it easier to have small commits |
80e7b84
to
05c9aad
Compare
ok, I will not use commit --amend |
2cb691c
to
44467bd
Compare
Docs updated. STS section was moved from openapi-user-guide.md (I wrote there by mistake before) to operator-user-guide.md. |
1cefca9
to
35b5578
Compare
/test test-e2e |
53f4ede
to
d072570
Compare
Code Climate has analyzed commit 7f7abc1 and detected 15 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. If tests pass, ready to be merged
Awesome work @valerymo 🎖️
/test test-e2e |
…stem components Co-authored-by: valerymo <vmogilev@redhat.com> Co-authored-by: eguzki <eastizle@redhat.com>
7f7abc1
to
3dc1ca0
Compare
Confirming that diff before I pushed the sqaush produce 0 differences. @valerymo can you also confirm from your local copy pre squash. |
what
Jira: https://issues.redhat.com/browse/THREESCALE-8772
AWS secret format with STS
STS object will be added to ApiManager CR to recognize if it's STS cluster or IAM
Validation
TESTS scenarios
expected: 3scale operator installed
test of image upload (in DevPortal/Content/Images) - Successfull
expected: error - secret field 'AWS_ACCESS_KEY_ID' is required in secret 's3-credentials'
expected: error - secret field 'AWS_ACCESS_KEY_ID' is required in secret 's3-credentials'
expected: error "secret field 'AWS_ACCESS_KEY_ID' is required in secret 's3-credentials'
expected: 3scale operator installed,
audience (in pod system-app): openshift,
test of image upload - Successfull
expected: 3scale operator installed,
audience(in pod system-app): "123",
test upload of image - Failed, Internal error notification appears in 3scale-operator.
- Developer Portal/Content - Sections templates are missing (https://3scale-admin.xxxxxx.devshift.org/p/admin/cms/templates)
- Images section and other missing.
If create new section and load file - Internal error.
Install 3scale operator
Notes: make install required to create/recreate ApiManager CRD. The following commands to be done before each test. It's better to delete 3scale-test before each test, but in some cases, it's enough to delete the secret and CR, and stop running operator
Secret and CR to be installed on separate terminal window.
Secrets & CRs for test
Validation done: All tests passed as expected