Repository: keikoproj/instance-manager
Pull request #61 (2019-11-02 01:16:04)

Support attach policy which doesn't has default aws policy prefix
Currently, attach managed policy only support policy with default aws prefix `arn:aws:iam::aws:policy/`. This change will support attach managed policy which has prefix `arn:aws:iam::{account_number}:policy/`

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/keikoproj/instance-manager?pullRequest=61) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/keikoproj/instance-manager?pullRequest=61) before we can accept your contribution.<br/><hr/>**senlin** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/keikoproj/instance-manager?pullRequest=61) it.</sub>
codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=h1) Report
> Merging [#61](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=desc) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/5d42dfbf6c943a45be9c5d5f6a6113c7a33d5b29?src=pr&el=desc) will **increase** coverage by `0.13%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/61/graphs/tree.svg?width=650&token=IJbjmSBliL&height=150&src=pr)](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage    72.5%   72.64%   +0.13%     
==========================================
  Files           6        6              
  Lines         793      797       +4     
==========================================
+ Hits          575      579       +4     
  Misses        160      160              
  Partials       58       58
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...rovisioners/ekscloudformation/ekscloudformation.go](https://codecov.io/gh/keikoproj/instance-manager/pull/61/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrc2Nsb3VkZm9ybWF0aW9uL2Vrc2Nsb3VkZm9ybWF0aW9uLmdv) | `81.67% <100%> (+0.23%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=footer). Last update [5d42dfb...36baf07](https://codecov.io/gh/keikoproj/instance-manager/pull/61?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

eytan-avisror: Hi @formuzi 
Did you test this change is working as expected?
Why is Trim not the right solution here?
formuzi: @eytan-avisror the Trim doesn't work, it only trim default aws prefix `arn:aws:iam::aws:policy`

Patch:
diff --git a/controllers/provisioners/ekscloudformation/ekscloudformation.go b/controllers/provisioners/ekscloudformation/ekscloudformation.go
index 1630140..85ca126 100644
--- a/controllers/provisioners/ekscloudformation/ekscloudformation.go
+++ b/controllers/provisioners/ekscloudformation/ekscloudformation.go
@@ -28,6 +28,7 @@ import (
 	"github.com/keikoproj/instance-manager/controllers/common"
 	awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws"
 	"github.com/sirupsen/logrus"
+	"regexp"
 )
 
 var (
@@ -504,8 +505,14 @@ func getManagedPolicyARNs(pNames []string) string {
 	// Add the user supplied policy names if any
 	if len(pNames) != 0 {
 		for _, name := range pNames {
-			// Trim the prefix arn:aws:iam::aws:policy/ if user supplied entire ARN instead of just name
-			managedPolicyARNs = append(managedPolicyARNs, fmt.Sprintf("%s%s", policyPrefix, strings.TrimPrefix(name, policyPrefix)))
+			// Append to list directly if user supplied entire ARN of policy
+			// Notes some customized policies may have prefix like 'arn:aws:iam::{account_number}:policy/' instead of 'arn:aws:iam::aws:policy/'
+			match, _ := regexp.MatchString("^arn:aws:iam::(aws|\\d{12}):policy/", name)
+			if match {
+				managedPolicyARNs = append(managedPolicyARNs, name)
+			} else {
+				managedPolicyARNs = append(managedPolicyARNs, fmt.Sprintf("%s%s", policyPrefix, name))
+			}
 		}
 	}
 


Test patch:
diff --git a/controllers/provisioners/ekscloudformation/ekscloudformation_test.go b/controllers/provisioners/ekscloudformation/ekscloudformation_test.go
index 46416a0..e3a0a3b 100644
--- a/controllers/provisioners/ekscloudformation/ekscloudformation_test.go
+++ b/controllers/provisioners/ekscloudformation/ekscloudformation_test.go
@@ -1231,6 +1231,11 @@ func TestGetManagedPolicyARNs(t *testing.T) {
 			input:    []string{"arn:aws:iam::aws:policy/My-Managed-Policy-1"},
 			expected: "arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy,arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy,arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly,arn:aws:iam::aws:policy/My-Managed-Policy-1",
 		},
+		{
+			testCase: "custom resource creation with managed policy arn with account number",
+			input:    []string{"arn:aws:iam::123456789012:policy/My-Managed-Policy-3"},
+			expected: "arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy,arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy,arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly,arn:aws:iam::123456789012:policy/My-Managed-Policy-3",
+		},
 	}
 
 	for _, tc := range tt {