Skip to content
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

Merged
merged 1 commit into from Nov 24, 2022

Conversation

valerymo
Copy link
Contributor

@valerymo valerymo commented Nov 3, 2022

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

  • ApiManager CR example:
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata: 
  name: apimanager-sample
  namespace: 3scale-test
spec: 
  system: 
    fileStorage: 
      simpleStorageService: 
        configurationSecretRef: 
          name: s3-credentials
        sts:
          enabled: true
          audience: openshift
  wildcardDomain:<wildcardDomain>
  • STS s3 secret example
kind: Secret
apiVersion: v1
metadata:
  name: s3-credentials
  namespace: 3scale
data:
  AWS_ROLE_ARN: arn:aws:iam::<ID>:role/<ROLE>
  AWS_WEB_IDENTITY_TOKEN_FILE: /var/run/secrets/openshift/serviceaccount/token
  AWS_BUCKET: <bucket_name>
  AWS_REGION: <region>
type: Opaque 

Validation

TESTS scenarios

  • Test 1 - CR: NON STS, Secret: Non STS
    expected: 3scale operator installed
    test of image upload (in DevPortal/Content/Images) - Successfull
  • Test 2 - CR: NON STS, Secret: STS
    expected: error - secret field 'AWS_ACCESS_KEY_ID' is required in secret 's3-credentials'
  • Test 3 - CR: STS, Secret: STS , sts.enabled: false, sts.audience missing;
    expected: error - secret field 'AWS_ACCESS_KEY_ID' is required in secret 's3-credentials'
  • Test 4 - CR: STS, Secret: STS , sts.enabled: false, sts.audience: "123"
    expected: error "secret field 'AWS_ACCESS_KEY_ID' is required in secret 's3-credentials'
  • Test 5 - CR: STS, Secret: STS , sts.enabled: true, sts.audience missing
    expected: 3scale operator installed,
    audience (in pod system-app): openshift,
    test of image upload - Successfull
  • Test 6 - CR: STS, Secret: STS , sts.enabled: st.audience "123"
    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

$ cd 3scale-operator
$ make install

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

$ export NAMESPACE=3scale-test
$ oc new-project $NAMESPACE
$ oc project $NAMESPACE
$ make download
$ make run

Secret and CR to be installed on separate terminal window.

Secrets & CRs for test

  • Non STS Secret
oc apply -f - <<EOF
---
kind: Secret
apiVersion: v1
metadata: 
  name: s3-credentials
  namespace: 3scale-test
data: 
  AWS_ACCESS_KEY_ID: XXX=
  AWS_SECRET_ACCESS_KEY: XXX=
  AWS_BUCKET: XXX=
  AWS_REGION: dXMtZWFzdC0y
type: Opaque
EOF
  • STS Secret
oc apply -f - <<EOF
---
kind: Secret
apiVersion: v1
metadata: 
  name: s3-credentials
  namespace: 3scale-test
data: 
  AWS_ROLE_ARN: XXX=
  AWS_WEB_IDENTITY_TOKEN_FILE: XXX=
  AWS_BUCKET: XXX=
  AWS_REGION: dXMtZWFzdC0y
type: Opaque
EOF
  • Non STS CR
oc apply -f - <<EOF
---
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata: 
  name: apimanager-sample
  namespace: 3scale-test
spec: 
  system: 
    image: 'quay.io/3scale/porta:fix-cms-s3-hostname'
    fileStorage: 
      simpleStorageService: 
        configurationSecretRef: 
          name: s3-credentials
  wildcardDomain: apps.vmogilev-rosa.upri.s1.devshift.org
EOF
  • STS CR
oc apply -f - <<EOF
---
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata: 
  name: apimanager-sample
  namespace: 3scale-test
spec: 
  system: 
    image: 'quay.io/3scale/porta:fix-cms-s3-hostname'
    fileStorage: 
      simpleStorageService: 
        configurationSecretRef: 
          name: s3-credentials
        sts:
          enabled: true
          audience: openshift
  wildcardDomain: apps.vmogilev-rosa.upri.s1.devshift.org
EOF

Validation done: All tests passed as expected

@openshift-ci
Copy link

openshift-ci bot commented Nov 3, 2022

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

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)
Copy link
Contributor

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


Copy link
Contributor Author

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

@@ -22,6 +22,8 @@ type SystemReconciler struct {
*BaseAPIManagerLogicReconciler
}

var sts = false
Copy link
Contributor

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

Copy link
Contributor Author

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!

Copy link
Contributor Author

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)

@austincunningham
Copy link
Contributor

/ok-to-test

@valerymo valerymo changed the title [WIP] THREESCALE-8772 - AWS secret format with STS THREESCALE-8772 - AWS secret format with STS Nov 6, 2022
@austincunningham
Copy link
Contributor

@valerymo have you checked that the system-sidekiq pods environment variables have been applied when you changed to the STS formatted secret ?

@eguzki
Copy link
Member

eguzki commented Nov 7, 2022

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.
It would be nice to test that 3scale works with AWS secret with STS format. It should be simple (maybe not easy). The operator needs to be run on a STS supported openshift instance and then upload one asset for the developer portal (image or whatever). Check that the asset is there in the developer portal.

@valerymo
Copy link
Contributor Author

valerymo commented Nov 9, 2022

@valerymo have you checked that the system-sidekiq pods environment variables have been applied when you changed to the STS formatted secret ?

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
...

  AWS_ROLE_ARN:                                  <set to the key 'AWS_ROLE_ARN' in secret 'aws-token'>                      Optional: false
  AWS_WEB_IDENTITY_TOKEN_FILE:                   <set to the key 'AWS_WEB_IDENTITY_TOKEN_FILE' in secret 'aws-token'>       Optional: false

. . .
/var/run/secrets/eks.amazonaws.com/serviceaccount/ from aws-token (ro)

@valerymo valerymo force-pushed the valery_THREESCALE-8772 branch 2 times, most recently from b80fae6 to c87086c Compare November 10, 2022 08:14
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/s3.go Outdated Show resolved Hide resolved
pkg/3scale/amp/operator/system_options_provider.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/s3_options.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system_options.go Outdated Show resolved Hide resolved
pkg/3scale/amp/operator/system_options_provider.go Outdated Show resolved Hide resolved
pkg/3scale/amp/operator/system_reconciler.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
@valerymo valerymo force-pushed the valery_THREESCALE-8772 branch 7 times, most recently from 8a27caf to e467851 Compare November 14, 2022 06:10
@eguzki
Copy link
Member

eguzki commented Nov 17, 2022

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

@valerymo
Copy link
Contributor Author

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

ok, I will not use commit --amend

@valerymo valerymo force-pushed the valery_THREESCALE-8772 branch 4 times, most recently from 2cb691c to 44467bd Compare November 20, 2022 09:46
@valerymo
Copy link
Contributor Author

Docs updated. STS section was moved from openapi-user-guide.md (I wrote there by mistake before) to operator-user-guide.md.

doc/apimanager-reference.md Show resolved Hide resolved
doc/apimanager-reference.md Show resolved Hide resolved
doc/operator-user-guide.md Outdated Show resolved Hide resolved
doc/operator-user-guide.md Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
doc/operator-user-guide.md Outdated Show resolved Hide resolved
doc/operator-user-guide.md Outdated Show resolved Hide resolved
pkg/3scale/amp/operator/system_reconciler.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
pkg/3scale/amp/component/system.go Outdated Show resolved Hide resolved
@valerymo
Copy link
Contributor Author

/test test-e2e

@valerymo valerymo requested a review from a team as a code owner November 22, 2022 16:26
@codeclimate
Copy link

codeclimate bot commented Nov 23, 2022

Code Climate has analyzed commit 7f7abc1 and detected 15 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 7
Duplication 2
Style 6

View more on Code Climate.

Copy link
Member

@eguzki eguzki left a 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 🎖️

@eguzki
Copy link
Member

eguzki commented Nov 23, 2022

/test test-e2e

…stem components

Co-authored-by: valerymo <vmogilev@redhat.com>
Co-authored-by: eguzki <eastizle@redhat.com>
@laurafitzgerald
Copy link

Confirming that diff before I pushed the sqaush produce 0 differences. @valerymo can you also confirm from your local copy pre squash.

@valerymo valerymo changed the title THREESCALE-8772 - AWS secret format with STS THREESCALE-8772 - Adding support for STS authentication for S3 for system components Nov 24, 2022
@austincunningham austincunningham merged commit 7d48466 into 3scale:master Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants