Fix CRD mapper blocking all others because caches never sync and revamp backend-mode flag #303
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
[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 |
k8s-ci-robot
added
the
approved
| `mapUsers` and `mapRoles` fields of its configuration. See [Full Configuration | ||
| Format](#full-configuration-format) below for details. | ||
|
|
||
| Using the `--backend-mode` flag, you can configure the server to source |
There was a problem hiding this comment.
Nice Job on Improving the documentation and clarification 👍
There was a problem hiding this comment.
Agreed, this explanation is helpful.
cmd/aws-iam-authenticator/server.go
Outdated
| @@ -97,7 +100,7 @@ func init() { | |||
| viper.BindPFlag("server.address", serverCmd.Flags().Lookup("address")) | |||
|
|
|||
| serverCmd.Flags().StringSlice("backend-mode", | |||
| []string{mapper.ModeCRD, mapper.ModeConfigMap, mapper.ModeFile}, | |||
| []string{mapper.ModeMountedConfigMap}, | |||
There was a problem hiding this comment.
We should mention in the document that if you don't define the MountedConfigMap along with others in the flags it will not be part of chain and whatever user provides will be the only mode available.
Its kind of now the default behavior and will gets overridden.
I was thinking if we would like to have this mode always available in the server and if customer provides other chain that would come first in chain.
There was a problem hiding this comment.
I've added a call-out for this.
Note that when setting a single backend, the server will *only* source from
that one and ignore the others even if they exist. For example, with
`--backend-mode=CRD`, the server will *only* source from `IAMIdentityMappings`
and ignore the mounted file and EKS ConfigMap.
I was thinking if we would like to have this mode always available in the server and if customer provides other chain that would come first in chain.
IMO we should see if these changes are sufficient, and if not we should add this as a final fallback.
|
This fixes my issue of the authenticator failing to initialize as reported in #288. With I then removed Adding Since the CRD mapper is a new feature, should we expect that users will need to set --backend-mode=CRD in order to use it? It'd be great if the order of backend modes would correctly "skip" any modes that arent setup (no file set, no IAMIdentityMappings present, etc) or if all 3 modes could work concurrently with some strategy for dealing with mapping conflicts. That would probably fall outside the scope of this PR though. |
|
Validated that this PR returns to previous behavior. ConfigMap (aka MountedConfigMap) works as-is with no additional changes to a previously working configuration. Documentation updates are much appreciated. Testing steps: And validated it is functioning: Thanks, Matthew! |
Yes, originally there was a feature gate flag, but I felt it was redundant and didn't want users to deal with 2 flags that would do roughly the same thing. i.e.: having CRD in your
Yes, the goal is for all 3 modes to work concurrently and the server to use the first mapping it finds (according to the order of the backends in --backend-mode). So if there's a conflict, the order of the backends resolves it. Any situation that violates this behavior is a bug! |
|
In that case I do seem to be experiencing a bug. Its possible that its related to the CRD and me being on k8s 1.16. With no with What leads me to believe its a k8s 1.16 problem is the CRD's status field: apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
name: iamidentitymappings.iamauthenticator.k8s.aws
spec:
conversion:
strategy: None
group: iamauthenticator.k8s.aws
names:
categories:
- all
kind: IAMIdentityMapping
listKind: IAMIdentityMappingList
plural: iamidentitymappings
singular: iamidentitymapping
preserveUnknownFields: true
scope: Cluster
versions:
- name: v1alpha1
schema:
openAPIV3Schema:
properties:
spec:
properties:
arn:
type: string
groups:
items:
type: string
type: array
username:
type: string
required:
- arn
- username
served: true
storage: true
subresources:
status: {}
status:
acceptedNames:
categories:
- all
kind: IAMIdentityMapping
listKind: IAMIdentityMappingList
plural: iamidentitymappings
singular: iamidentitymapping
conditions:
- lastTransitionTime: "2020-02-14T01:59:29Z"
message: no conflicts found
reason: NoConflicts
status: "True"
type: NamesAccepted
- lastTransitionTime: null
message: the initial names have been accepted
reason: InitialNamesAccepted
status: "True"
type: Established
- lastTransitionTime: "2020-03-05T17:04:08Z"
message: '[spec.validation.openAPIV3Schema.properties[spec].type: Required value:
must not be empty for specified object fields, spec.validation.openAPIV3Schema.type:
Required value: must not be empty at the root]'
reason: Violations
status: "True"
type: NonStructuralSchema
storedVersions:
- v1alpha1So this bug may not be a blocker for this PR. Given that this PR fixed my initial issue of the "MountedFile" not working I think it is good to merge, but we should look at migrating the CRD to apiextensions.k8s.io/v1 in order for the custom resources to work on k8s 1.16+. |
…igMap' & 'EKSConfigMap'
|
@rifelpet thanks for testing, it was a basic coding error (I confused informerFactory.Start with informer.Run ...). I've tested myself that CRD works now. I created a CRD that maps my role to system:masters and added an entry in mapRoles to the file that maps my role to system:asdf. With --backend-mode=CRD,File, logs look like this, I get mapped to system:masters (notice the lines about syncing like With --backend-mode=File,CRD, logs should look like this, I get mapped to system:asdf: Finally, with --backend-mode=File or backend-mode not set at all, there's no mention of CRDs in the log even though I have an IAMIdentityMapping present...this is expected. |
|
I can confirm the behavior with everything you outlined above. I see you updated the default value for --backend-mode to be MountedFile rather than all 3 modes in a list. Is there a reason for that? I think with the current fixes in this PR that it should be safe to preserve the original default of all 3 modes enabled. For Kops' built-in support I'll probably have it default to having all 3 modes enabled. Thanks for your work on this! I'm looking forward to this getting merged and a new release cut. |
Yes, it should be safe…I just wanted to revert to pre-0.5.0 in an (over)abundance of caution. One other thing is that I’ve found the logging from the EKS mode can be annoying when the aws-auth configmap doesn’t exist, so I’d rather wait to turn it on by default. |
| `mapUsers` and `mapRoles` fields of its configuration. See [Full Configuration | ||
| Format](#full-configuration-format) below for details. | ||
|
|
||
| Using the `--backend-mode` flag, you can configure the server to source |
There was a problem hiding this comment.
Agreed, this explanation is helpful.
| ModeConfigMap string = "ConfigMap" | ||
| ModeCRD string = "CRD" | ||
|
|
There was a problem hiding this comment.
Nit: just File might be more techincally correct since it doesn't have to run in a container, but I understand that it probably makes more sense in most cases, so MountedFile might be the better option.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@nckturner any chance we could get a new aws-iam-authenticator release that includes this PR? |
Fixes #297 and #288
Discovered in #297 and #288 that there is a bug where if you place backend-mode CRD before the other backends/mappers...the others never start. And we have CRD as default the first...(which seems in hindsight like quite an oversight since it never left "alpha" :]). So I consider this a critical bugfix. The fix is small and in the first commit.
Now this bug wouldn't exist in the first place if I explicitly maintained behavior from pre-0.5.0 by keeping
backend-modeequal toFileinstead of implicitly trusting that the fallback logic forbackend-modewas sound when it is not. This is the second commit.Moreover, it seems there's consensus that the backend-mode flag is confusing and the README in*sufficient.
FileandConfigMapare super confusing because both are files...and both are configmaps... So to remove ambiguity I've deprecated them and renamed backendConfigMaptoEKSConfigMapandFiletoMountedConfigMap. Open to other suggestions. These are the last 2 commits.I've stopped short of refactoring the code (package
fileand packageconfigmap) to match the new names because I don't want to risk more instability. But that can be done in the future if we get more sophisticated integration/e2e tests which are at the moment hard to write without infrastructure for deploying a kops cluster, etc.