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

Refactor to allow configurable backends (configmap, eks configmap, crd) #269

Merged
merged 12 commits into from
Nov 25, 2019

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Sep 12, 2019

This enables the authenticator server to support multiple backends (sources of IAM->user+group mappings) at the same time with a well-defined order of precedence according to the backend-mode argument.

Fixes #247
also fixes #261

TODO: more tests -- will do in a follow-up
TODO: update README -- done
TODO: fix server_test.go -- done
TODO: sts regional endpoint -- needs some discussion, will do in a follow-up

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wongma7

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2019
@nckturner nckturner self-assigned this Sep 12, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Sep 13, 2019

Will put last 2 commits in a separate PR.

Some concerns about whether the user has sts regional endpoints activated or not.

@nckturner
Copy link
Contributor

@wongma7 awesome, yeah I think the regional endpoint is a separate concern.

@wongma7 wongma7 force-pushed the mega-refactor branch 3 times, most recently from b43b647 to d9ecbee Compare September 24, 2019 23:09
Copy link
Member

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

There are a bunch of TODOs, do we want to address those in this PR or at a later time?

Thanks for doing this!

cmd/aws-iam-authenticator/server.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
@wongma7
Copy link
Contributor Author

wongma7 commented Sep 26, 2019

I think tests can be a follow-up. But I need to update the documentation (and fix the travis build). Will ping when that is ready.

@wongma7 wongma7 force-pushed the mega-refactor branch 2 times, most recently from 40c9f5c to da63a56 Compare October 3, 2019 22:11
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nckturner nckturner left a comment

Choose a reason for hiding this comment

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

Looks like we are close 😄

Do we have a test with multiple backends?

README.md Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/mapper.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Nov 8, 2019

RE: tests there are a couple tests that inadvertently use multiple mappers but no explicit tests yet. https://github.com/kubernetes-sigs/aws-iam-authenticator/pull/269/files#diff-8151a8a5ca3b510f06b426f0cfb48708R646 I would like to refactor server_test.go in a followup as it will be quite verbose to have a configmap+file+crd version of every testcase. Then add some explicit tests.

@nckturner
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2019
@k8s-ci-robot k8s-ci-robot merged commit e02e263 into kubernetes-sigs:master Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

userarn vs userARN Add flag for configurable backends
6 participants