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

WIP: ✨ Support AWS multitenancy #1919

Closed

Conversation

andrewmyhre
Copy link
Contributor

@andrewmyhre andrewmyhre commented Sep 3, 2020

(Rebased for v0.6.0 / main)

What this PR does / why we need it: Allows the use of multiple credentials from as single deployment of CAPA. Static credentials and IAM roles are both support. These can be provided as custom resources along with a CAPx spec.

Fixes #1552

TODO:

  • Wire up namespace selector (@randomvariable )
  • Tests for provider hashing, maybe.
  • Add a way to test via e2e, even if we can't do it directly in Prow?

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 3, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @andrewmyhre. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 3, 2020
@randomvariable
Copy link
Member

Not sure the rebase has gone correctly? Seems to include the multi-tenancy stuff as well.

@andrewmyhre andrewmyhre force-pushed the 1713-multitenancy-v0.5.5 branch from 32f0d3a to 43a55b3 Compare September 3, 2020 14:13
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2020
@andrewmyhre andrewmyhre changed the title ✨ Unmanaged VPCs don't require public subnets ✨ Support AWS multitenancy Sep 3, 2020
@andrewmyhre
Copy link
Contributor Author

Not sure the rebase has gone correctly? Seems to include the multi-tenancy stuff as well.

I copied the title and description from the wrong PR :)
This is only the multi-tenancy work.

@randomvariable
Copy link
Member

Cool, thanks. I'm going to be out until Tuesday, but will start reviewing then.

@randomvariable
Copy link
Member

/milestone v0.6.1

@k8s-ci-robot k8s-ci-robot added this to the v0.6.1 milestone Sep 9, 2020
@randomvariable
Copy link
Member

/status in-review

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2020
@randomvariable
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 10, 2020
@randomvariable
Copy link
Member

don't worry about rebasing just yet.

Copy link
Member

@randomvariable randomvariable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @andrewmyhre . It's a great start.

There's obviously some bits we need to finish to complete the implementation, and would be willing to help in the effort.

Should we work on a common branch, either a feature branch in this repo or your repo, work on the bits together and then PR it as one?

pkg/cloud/scope/session.go Outdated Show resolved Hide resolved
test/e2e/e2e_suite_test.go Outdated Show resolved Hide resolved
pkg/cloud/services/elb/loadbalancer.go Show resolved Hide resolved
pkg/cloud/scope/principal.go Outdated Show resolved Hide resolved
api/v1alpha3/awsprincipal_types.go Outdated Show resolved Hide resolved
api/v1alpha3/awsprincipal_types.go Outdated Show resolved Hide resolved
api/v1alpha3/awsprincipal_types.go Outdated Show resolved Hide resolved
controllers/awscluster_controller.go Outdated Show resolved Hide resolved
pkg/cloud/scope/principal.go Show resolved Hide resolved
@randomvariable randomvariable changed the title ✨ Support AWS multitenancy WIP: ✨ Support AWS multitenancy Sep 11, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2020
@andrewmyhre andrewmyhre force-pushed the 1713-multitenancy-v0.5.5 branch from 194c2e1 to 0db7f6b Compare September 11, 2020 14:26
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2020
@andrewmyhre andrewmyhre force-pushed the 1713-multitenancy-v0.5.5 branch 2 times, most recently from 2ae4430 to 77a95bd Compare September 11, 2020 17:56
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020
@randomvariable
Copy link
Member

I'll do the rebase and add the namespace selector tomorrow.

@@ -8,5 +8,5 @@ spec:
spec:
containers:
# Change the value of image field below to your controller image URL
- image: gcr.io/k8s-staging-cluster-api-aws/cluster-api-aws-controller:latest
- image: artifactory.cloud.capitalone.com/onekube/cluster-api-aws-controller-amd64:dev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- image: artifactory.cloud.capitalone.com/onekube/cluster-api-aws-controller-amd64:dev
- image: gcr.io/k8s-staging-cluster-api-aws/cluster-api-aws-controller:latest

awsprinciple types are now being registered, initial session test giving good fail

1713 tests for stub providers/principals implemented

1713 wire up session.go to use providers if specified

update rbac permissions to allow managing *principal resources

avoid nil reference error when creating a session. log creating a session

creating a session logs more verbosely

debugging role assumption issues

pass a session with a region when requesting an stscreds.Credentials

update test for new getProviderForCluster signature

look for principals in specific namespaces

add conversion for PrincipalRef. Add stubs for checking principal allow namespaces

update conversion.go

build providers into a chain

load sourcePrincipalRef into provider chain

update multitenancy tests

change Principal types to Namespaced scope

when looking up a principal default to looking in the same namespace as the cluster

AWSServiceAccountPrincipals can be chained

fmt/lint updates

dial down logging

remove unused imports from e2e test suite

ignore AWSServiceAccountPrincipals for now

revert change to handling an error

make generate

go fmt

remove service account principal constructor

Signed-off-by: rbtr <[email protected]>
@rbtr rbtr force-pushed the 1713-multitenancy-v0.5.5 branch from 77a95bd to b26838c Compare January 12, 2021 22:41
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign timothysc after the PR has been reviewed.
You can assign the PR to them by writing /assign @timothysc in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@rbtr rbtr force-pushed the 1713-multitenancy-v0.5.5 branch from b26838c to 7fd7d67 Compare January 12, 2021 22:47
@rbtr rbtr force-pushed the 1713-multitenancy-v0.5.5 branch from 7fd7d67 to e42b5fc Compare January 12, 2021 23:19
@sedefsavas sedefsavas mentioned this pull request Feb 16, 2021
5 tasks
@randomvariable
Copy link
Member

Completed in #2253

Credit should be copied over to the other commits.

/close

@k8s-ci-robot
Copy link
Contributor

@randomvariable: Closed this PR.

In response to this:

Completed in #2253

Credit should be copied over to the other commits.

/close

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.

@k8s-ci-robot
Copy link
Contributor

@andrewmyhre: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS account multitenancy using a single instance of the controller
5 participants