Implement a configurable pattern for the IAM Role Name #73

diranged · 2021-01-19T22:03:12Z

Closes #62

Could you share the solution in high level?

Implement a new parameter (iam.role.pattern) with a default value that matches the current behavior (k8s-<iamrole name>). This new parameter is a template that can be customized by the administrator of the iam-manager controller.

Note: This PR also removes the iam.role.derive.from.namespace property because it becomes unnecessary, the user can simply set iam.role.pattern: k8s-{{ .ObjectMeta.Namespace}} to get the same behavior.

Could you share the test results?

All of the tests pass.

** Note about IAM Role naming restrictions **

The PR does not currently try to protect the end-user from having an IAM role name that doesn't work with IAM (ie, bad characters, too long, etc). In my opinion, it is up to the code that actually creates the IAM role itself to return back a success/failure. If we want to do some kind of validation before trying to make the call, then I think we'd be looking at implementing a validationwebhook that prevents the user from creating the Iamrole object all. That would be great - but
it is pretty far out of the scope of this PR.

CLAassistant · 2021-01-19T22:03:19Z

All committers have signed the CLA.

Signed-off-by: Matt Wise <[email protected]>

codecov · 2021-01-19T22:40:50Z

Codecov Report

Merging #73 (6103fa4) into master (f9c86d2) will increase coverage by 0.20%.
The diff coverage is 72.41%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   66.19%   66.40%   +0.20%     
==========================================
  Files           8        8              
  Lines         991     1003      +12     
==========================================
+ Hits          656      666      +10     
- Misses        304      306       +2     
  Partials       31       31

Impacted Files	Coverage Δ
controllers/iamrole_controller.go	`8.94% <0.00%> (-0.10%)`	⬇️
internal/config/properties.go	`64.90% <100.00%> (-0.69%)`	⬇️
internal/utils/utils.go	`83.33% <100.00%> (+3.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9c86d2...6103fa4. Read the comment docs.

mnkg561 · 2021-01-22T14:22:23Z

Thanks @diranged. I will check this out today and also meanwhile let me think about this point

** Note about IAM Role naming restrictions **

The PR does not currently try to protect the end-user from having an IAM role name that doesn't work with IAM (ie, bad characters, too long, etc). In my opinion, it is up to the code that actually creates the IAM role itself to return back a success/failure. If we want to do some kind of validation before trying to make the call, then I think we'd be looking at implementing a validationwebhook that prevents the user from creating the Iamrole object all. That would be great - but
it is pretty far out of the scope of this PR.

and see if we are leaving any holes which will create an issue. This was not an issue in present setup because we do know that namespace name follows DNS naming conventions and can not have more than 63 characters which is what IAM Role restrictions as well. Allowing the pattern completely open creates the possibility of having new special characters which might not be allowed in IAM role name.

My only worry is, if for some reason administrator running into this issue AFTER few valid iam roles created then, changing the pattern will create all new sets of iam roles and that need changes in the application deployment specs etc if they are referring that iam role anywhere.

mnkg561 · 2021-01-22T16:21:34Z

controllers/iamrole_controller.go

-
-	if config.Props.DeriveNameFromNamespace() && config.Props.MaxRolesAllowed() == 1 {
-		roleName = fmt.Sprintf("k8s-%s", iamRole.ObjectMeta.Namespace)
+	// TODO: Remove the need to do this here.


Good point and yes I included ARN in the status of iam role object itself in the last release so we should be able to skip this for delete in future

mnkg561 · 2021-01-25T18:46:35Z

Change looks overall good!! If you tested it out in your cluster, can you post some screenshots with results? May be with different type of patterns and one with "invalid" pattern too to see how the error is surfaced all the way to IamRole spec/status?

diranged · 2021-01-28T14:11:15Z

Sorry - yes, this has been tested and is working today. Here's a failure example:

$ k describe cm -n iam-manager-system iam-manager-iamroles-v1alpha1-configmap 
Name:         iam-manager-iamroles-v1alpha1-configmap
Namespace:    iam-manager-system
Labels:       app=iam-manager
Annotations:  <none>

Data
====
aws.accountId:
----
xxx
iam.policy.resource.blacklist:
----
nil
iam.role.pattern:
----
k8s-badpattern-{{ .Object.Mistake }}  <<<< bad pattern
iam.role.prefix:
----
aws.region:
----
us-west-2
iam.irsa.enabled:
----
true
iam.managed.permission.boundary.policy:
----
EKS-xxx-permissions-boundary
iam.managed.policies:
----
EKS-xxx-default-policy
k8s.cluster.name:
----
dev1-test
webhook.enabled:
----
true
Events:  <none>

Name:         test-role
Namespace:    observability
Labels:       <none>
Annotations:  <none>
API Version:  iammanager.keikoproj.io/v1alpha1
Kind:         Iamrole
Metadata:
  Creation Timestamp:  2021-01-28T14:09:10Z
  Generation:          1
  Managed Fields:
    API Version:  iammanager.keikoproj.io/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:PolicyDocument:
          .:
          f:Statement:
    Manager:         kubectl-client-side-apply
    Operation:       Update
    Time:            2021-01-28T14:09:10Z
  Resource Version:  47387530
  Self Link:         /apis/iammanager.keikoproj.io/v1alpha1/namespaces/observability/iamroles/test-role
  UID:               6f54a3f7-7d97-4bdc-8ab2-cffeacc07e5b
Spec:
  Policy Document:
    Statement:
      Action:
        s3:PutObject*
      Effect:  Allow
      Resource:
        *
    Version:  2012-10-17
Events:
  Type     Reason  Age   From         Message
  ----     ------  ----  ----         -------
  Warning  Error   8s    iam-manager  Unable to construct iam role name to error template: rolename:1:25: executing "rolename" at <.Object.Mistake>: can't evaluate field Object in type v1alpha1.Iamrole

diranged · 2021-01-28T14:35:35Z

Here's it working:

$ k describe iamrole test-role
Name:         test-role
Namespace:    observability
Labels:       <none>
Annotations:  <none>
API Version:  iammanager.keikoproj.io/v1alpha1
Kind:         Iamrole
Metadata:
  Creation Timestamp:  2021-01-28T14:34:44Z
  Finalizers:
    iamrole.finalizers.iammanager.keikoproj.io
  Generation:  1
  Managed Fields:
    API Version:  iammanager.keikoproj.io/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:PolicyDocument:
          .:
          f:Statement:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2021-01-28T14:34:44Z
    API Version:  iammanager.keikoproj.io/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"iamrole.finalizers.iammanager.keikoproj.io":
      f:status:
        .:
        f:lastUpdatedTimestamp:
        f:retryCount:
        f:roleARN:
        f:roleID:
        f:roleName:
        f:state:
    Manager:         manager
    Operation:       Update
    Time:            2021-01-28T14:34:45Z
  Resource Version:  47401129
  Self Link:         /apis/iammanager.keikoproj.io/v1alpha1/namespaces/observability/iamroles/test-role
  UID:               1fc40f45-8845-4a28-b534-e94d454c2c9e
Spec:
  Policy Document:
    Statement:
      Action:
        s3:PutObject*
      Effect:  Allow
      Resource:
        *
    Version:  2012-10-17
Status:
  Last Updated Timestamp:  2021-01-28T14:34:45Z
  Retry Count:             0
  Role ARN:                arn:aws:iam::xxx:role/k8s-xxx-observability-test-role
  Role ID:                 AROAxxx
  Role Name:               k8s-xxx-observability-test-role
  State:                   Ready
Events:
  Type    Reason  Age   From         Message
  ----    ------  ----  ----         -------
  Normal  Ready   2s    iam-manager  Successfully created/updated iam role

mnkg561

LGTM

diranged · 2021-01-29T01:50:48Z

Do I need to refactor for the conflicts - or will you take care of that?

mnkg561 · 2021-01-29T04:38:36Z

Do I need to refactor for the conflicts - or will you take care of that?

Can you handle the conflicts please?

Signed-off-by: Matt Wise <[email protected]>

diranged · 2021-02-01T14:05:54Z

@mnkg561 Sorry for the delay - here ya go!

Implement a configurable pattern for the IAM Role Name

f23a216

Signed-off-by: Matt Wise <[email protected]>

diranged force-pushed the prefix_tweak branch from d6d7217 to f23a216 Compare January 19, 2021 22:03

mnkg561 reviewed Jan 25, 2021

View reviewed changes

diranged mentioned this pull request Jan 28, 2021

Bug: IAM Trust Policy is not optional in the CM? #74

Open

mnkg561 approved these changes Jan 28, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into prefix_tweak

6103fa4

Signed-off-by: Matt Wise <[email protected]>

diranged force-pushed the prefix_tweak branch from 1f92f60 to 6103fa4 Compare February 1, 2021 14:05

mnkg561 merged commit 420573a into keikoproj:master Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a configurable pattern for the IAM Role Name #73

Implement a configurable pattern for the IAM Role Name #73

diranged commented Jan 19, 2021

CLAassistant commented Jan 19, 2021 •

edited

Loading

codecov bot commented Jan 19, 2021 •

edited

Loading

mnkg561 commented Jan 22, 2021

mnkg561 Jan 22, 2021

mnkg561 commented Jan 25, 2021

diranged commented Jan 28, 2021

diranged commented Jan 28, 2021

mnkg561 left a comment

diranged commented Jan 29, 2021

mnkg561 commented Jan 29, 2021

diranged commented Feb 1, 2021

Implement a configurable pattern for the IAM Role Name #73

Implement a configurable pattern for the IAM Role Name #73

Conversation

diranged commented Jan 19, 2021

CLAassistant commented Jan 19, 2021 • edited Loading

codecov bot commented Jan 19, 2021 • edited Loading

Codecov Report

mnkg561 commented Jan 22, 2021

mnkg561 Jan 22, 2021

Choose a reason for hiding this comment

mnkg561 commented Jan 25, 2021

diranged commented Jan 28, 2021

diranged commented Jan 28, 2021

mnkg561 left a comment

Choose a reason for hiding this comment

diranged commented Jan 29, 2021

mnkg561 commented Jan 29, 2021

diranged commented Feb 1, 2021

CLAassistant commented Jan 19, 2021 •

edited

Loading

codecov bot commented Jan 19, 2021 •

edited

Loading