move IAMRole and IAMRolePolicyAttachment to v1beta1 #141

sahil-lakhwani · 2020-02-26T17:14:28Z

Description of your changes

Add implementations of IAMRole and IAMRolePolicyAttachment that adhere to the managed resources standards

Tested with the following

IAMRole

apiVersion: identity.aws.crossplane.io/v1beta1
kind: IAMRole
metadata:
  name: sample-cluster-role
  annotations:
   crossplane.io/external-name: managed-resource-role
spec:
  forProvider:
    description: some description
    assumeRolePolicyDocument: |
      {
        "Version": "2012-10-17",
        "Statement": [
          {
            "Effect": "Allow",
            "Principal": {
              "Service": "eks.amazonaws.com"
            },
            "Action": "sts:AssumeRole"
          }
        ]
      }
  reclaimPolicy: Delete
  providerRef:
    name: example

IAMRolePolicyAttament

apiVersion: identity.aws.crossplane.io/v1beta1
kind: IAMRolePolicyAttachment
metadata:
  name: sample-role-servicepolicy
spec:
  forProvider:
    roleNameRef:
      name: sample-cluster-role
    # wellknown policy arn
    policyArn: arn:aws:iam::aws:policy/AmazonEKSServicePolicy
  reclaimPolicy: Delete
  providerRef:
    name: example
~

Checklist

I have:

Run make reviewable to ensure this PR is ready for review.
Ensured this PR contains a neat, self documenting set of commits.
Updated any relevant documentation, examples, or release notes.
Updated the dependencies in app.yaml to include any new role permissions.

sahil-lakhwani · 2020-02-26T17:24:54Z

kubectl get iamrole.identity.aws.crossplane.io/metadata-name

NAME            ROLENAME   DESCRIPTION   READY   SYNCED   AGE
metadata-name              update desc   True    True     12s

For the ROLENAME column, because .spec.rolename has been removed, I tried to pull the value from annotation crossplane.io/external-name in the CRD definition. Didn't work for me.
(Pulling values from other annotations with no special characters does seem to work)

hasheddan

Looking really good so far @sahil-lakhwani! Added a few comments, let me know if you need any additional context :)

hasheddan · 2020-02-26T17:39:05Z

pkg/controller/identity/iamrole/controller_test.go

@@ -57,7 +58,7 @@ func TestMain(m *testing.M) {
 func Test_Connect(t *testing.T) {


Can we refactor these tests to match our table-driven pattern in other v1beta1 resources?

Ref: https://github.com/crossplane/stack-aws/blob/94baed407fc79f3a083c409b42542a00f7f0eb74/pkg/controller/database/rdsinstance_test.go#L106

hasheddan · 2020-02-26T17:39:39Z

pkg/controller/identity/iamrolepolicyattachment/controller_test.go

@@ -57,7 +57,7 @@ func TestMain(m *testing.M) {
 func Test_Connect(t *testing.T) {


Same comment here about table-driven tests.

hasheddan · 2020-02-26T17:44:45Z

pkg/clients/iam/iamrole.go

+	return m
+}
+
+// UpdateRoleExternalStatus updates the external status object, given the observation


Having External in the name here as we are actually updating the name of the kubernetes object rather than the external IAM role on AWS. We typically defer this type of functionality to the controller, so this can likely be removed (see comments in controller).

Instead we would probably want something like a GenerateObservation function, ref: https://github.com/crossplane/stack-aws/blob/94baed407fc79f3a083c409b42542a00f7f0eb74/pkg/clients/rds/rds.go#L220

hasheddan · 2020-02-26T17:45:02Z

pkg/clients/iam/iamrolepolicyattachment.go

+	return m
+}
+
+// UpdateRolePolicyExternalStatus updates the external status object, given the observation


Same comment as in IAM role client.

pkg/controller/identity/iamrole/controller.go

hasheddan · 2020-02-26T17:52:12Z

pkg/controller/identity/iamrole/controller.go


 	result, err := req.Send(ctx)
 	if err != nil {
 		return managed.ExternalCreation{}, errors.Wrap(err, errCreate)
 	}

-	cr.UpdateExternalStatus(*result.Role)
+	iam.UpdateRoleExternalStatus(cr, *result.Role)


We typically would update the status in the subsequent Observe.

Ref: https://github.com/crossplane/stack-aws/blob/94baed407fc79f3a083c409b42542a00f7f0eb74/pkg/controller/database/rdsinstance.go#L140

pkg/controller/identity/iamrolepolicyattachment/controller.go

hasheddan · 2020-02-26T17:56:46Z

pkg/controller/identity/iamrolepolicyattachment/controller.go

@@ -127,33 +127,31 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E

 	cr.SetConditions(runtimev1alpha1.Available())

-	cr.UpdateExternalStatus(*attachedPolicyObject)
+	iam.UpdateRolePolicyExternalStatus(cr, *attachedPolicyObject)


Here we are already updating the status in the Observe method (contrary to the iam role controller), but it would be nice to have a GenerateObservation function that returns the AtProvider observation rather than passing in the object to have its AtProvider status set in the client.

pkg/controller/identity/iamrole/controller_test.go

hasheddan

This is getting very close! Thank you for continuing to push through on this @sahil-lakhwani! Main comments are around adding a few additional fields to the IAMRole spec. Let me know if you have any questions! :)

hasheddan · 2020-03-03T02:12:03Z

apis/identity/v1beta1/iamrole_types.go

@@ -38,13 +34,13 @@ type IAMRoleParameters struct {
 	Description string `json:"description,omitempty"`

 	// RoleName presents the name of the IAM role.
-	RoleName string `json:"roleName"`
+	// RoleName string `json:"roleName"`


You can go ahead and remove this :)

hasheddan · 2020-03-03T02:16:01Z

apis/identity/v1beta1/iamrole_types.go

@@ -38,13 +34,13 @@ type IAMRoleParameters struct {
 	Description string `json:"description,omitempty"`


Suggested change

Description string `json:"description,omitempty"`

Description *string `json:"description,omitempty"`

https://github.com/crossplane/crossplane/blob/master/design/one-pager-managed-resource-api-design.md#pointer-types-and-markers

apis/identity/v1beta1/iamrolepolicyattachment_types.go

apis/identity/v1beta1/iamrole_types.go

pkg/controller/identity/iamrole/controller.go

infinitecompute · 2020-03-06T19:51:18Z

@sahil-lakhwani What is the latest on updating and merging this PR?

sahil-lakhwani · 2020-03-09T10:01:46Z

@sahil-lakhwani What is the latest on updating and merging this PR?

Hi @infinitecompute
There were some confusions, regarding which I had quick discussions with @hasheddan
And then I had to make some more changes to match the name change (provider-aws)
I have updated the PR.

hasheddan · 2020-03-10T20:17:07Z

apis/identity/v1beta1/iamrole_types.go

+
+	// The key name that can be used to look up or retrieve the associated value.
+	// For example, Department or Cost Center are common choices.
+	Key string `json:"key,omitempty"`


This is required per https://docs.aws.amazon.com/IAM/latest/UserGuide/id_tags.html so you can remove omitempty:

"You can create a tag with an empty value such as phoneNumber = . You cannot create an empty tag key."

apis/identity/v1beta1/iamrole_types.go

pkg/clients/iam/iamrole.go

pkg/clients/iam/iamrolepolicyattachment.go

pkg/controller/identity/iamrole/controller.go

hasheddan · 2020-03-10T20:46:43Z

pkg/controller/identity/iamrolepolicyattachment/controller.go

-		return managed.ExternalUpdate{}, errors.Wrapf(err, errDetach, cr.Status.AttachedPolicyARN, cr.Spec.RoleName)
-	}
-
+	// PolivyARN is the only distinguishing field and on update to that, new policy is attached


I think we need to let user know that their spec is out of sync with the external resource. However, returning an error here will cause immediate requeue, which I am not sure we want. @negz @muvaf any thoughts here?

Is PolicyARN actually an immutable field? If so, I think we can skip doing anything in Update.

Letting the user know about the drift is challenging in AWS because update requests are constructed only with update-able parameters, hence API server wouldn't return an error like GCP this field cannot be updated. Ideally, validation should reject editing an immutable field.

@hasheddan @muvaf In the current implementation, if the PolicyARN in spec is changed anyhow, a new policy is being created. And in the Update method, we have:
if cr.Status.AttachedPolicyARN == "" || cr.Spec.PolicyARN == cr.Status.AttachedPolicyARN

I think we can check for the above condition in Observe itself, and decide either to create a new policy or update the existing.

@sahil-lakhwani I am hesitant to delete and recreate an external object as part of the update logic of a reconciler because it feels like it violates that contract of the lifecycle of a custom resource being tied to the lifecycle of an external resource. @muvaf let me know if you think differently here. It sounds like the path forward for the time being is to just always return true for ResourceUpToDate if the IAMRolePolicyAttachment exists at all (per @muvaf comment), and keep the current functionality you have here of doing nothing in the Update method.

@muvaf I wonder if we should expose something in crossplane-runtime that allows consumers to report back that "this resource is not up to date, but we are not trying to fix it". We could do this currently by manually updating status condition here, but I think crossplane-runtime would overwrite it after we returned an empty managed.ExternalUpdate.

Yeah we don't delete anything unless actual deletion request comes in. @sahil-lakhwani this field should be marked as immutable and when we get around implementation of immutability, it will work. But for the time being, that's all we can do.

About the drift in general; there are three kinds of drifts:

In progress: you changed something and the update process is going on but the property is still the same in external resource, like you increased the size of DB from 20GB to 400GB and spec says 400GB immediately while the process is going on. Not much to do here unless provider reports an unavailable state, then we could update the condition. But other than that, status is expected to show that an update is going on if possible.

Immutable field changed by user. Specifically in AWS, Modify requests do not include those fields so we can't have provider return error in those cases. The best we could do is to prevent those fields from being edited.

Update-able field changed but resulted in error. Provider tells you the error and we show that.

@hasheddan For the second case, we could investigate a mechanism where in Observe we compare spec with what exists in the provider and update a condition saying whether there is a drift without going into details like whether this was because an immutable field or an update is due etc. It's a bit challenging to implement this in a good way though (comparison is hard). Maybe use IsUpToDate check to set a condition? But that'll require full scan of fields opposed to selective scanning that current IsUpToDate functions do.

pkg/clients/iam/iamrole.go

pkg/clients/iam/iamrolepolicyattachment.go

pkg/controller/identity/iamrole/controller.go

pkg/controller/identity/iamrolepolicyattachment/controller.go

muvaf · 2020-03-11T11:07:52Z

pkg/controller/identity/iamrolepolicyattachment/controller.go


 	_, err := req.Send(ctx)

-	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.PolicyARN, cr.Spec.RoleName)
+	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.ForProvider.PolicyARN, cr.Spec.ForProvider.RoleName)


You don't need to return anything other than the error(err) and the error string(errAttach) as managed reconciler properly fills the error and logs with necessary identifiers.

@muvaf errAttach has format specifiers, and removing the values for those resulted in this for me (kubectl describe)

Warning CannotCreateExternalResource 6m59s managed/iamrolepolicyattachment.identity.aws.crossplane.io failed to attach the policy with arn %!!(MISSING)v(MISSING) to role %!!(MISSING)v(MISSING): NoSuchEntity: Policy arn:aws:iam::aws:policy/AleaForBusinessDeviceSetup does not exist or is not attachable.

The values are not filled and I am not sure how they will. Am I missing something?

I believe @muvaf just means that you can remove the formatted string and return errors.Wrap(err, errAttach) :)

Suggested change

return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.ForProvider.PolicyARN, cr.Spec.ForProvider.RoleName)

return managed.ExternalCreation{}, errors.Wrap(err, errAttach)

I'm suggesting this because this. We don't need to repeat what's on the CR as long as the error string is specific enough.

muvaf · 2020-03-11T11:12:14Z

pkg/controller/identity/iamrolepolicyattachment/controller.go

-		return managed.ExternalUpdate{}, errors.Wrapf(err, errDetach, cr.Status.AttachedPolicyARN, cr.Spec.RoleName)
-	}
-
+	// PolivyARN is the only distinguishing field and on update to that, new policy is attached


Is PolicyARN actually an immutable field? If so, I think we can skip doing anything in Update.

Letting the user know about the drift is challenging in AWS because update requests are constructed only with update-able parameters, hence API server wouldn't return an error like GCP this field cannot be updated. Ideally, validation should reject editing an immutable field.

hasheddan · 2020-03-12T11:45:00Z

pkg/controller/identity/iamrolepolicyattachment/controller.go

-		return managed.ExternalUpdate{}, errors.Wrapf(err, errDetach, cr.Status.AttachedPolicyARN, cr.Spec.RoleName)
-	}
-
+	// PolivyARN is the only distinguishing field and on update to that, new policy is attached


@sahil-lakhwani I am hesitant to delete and recreate an external object as part of the update logic of a reconciler because it feels like it violates that contract of the lifecycle of a custom resource being tied to the lifecycle of an external resource. @muvaf let me know if you think differently here. It sounds like the path forward for the time being is to just always return true for ResourceUpToDate if the IAMRolePolicyAttachment exists at all (per @muvaf comment), and keep the current functionality you have here of doing nothing in the Update method.

@muvaf I wonder if we should expose something in crossplane-runtime that allows consumers to report back that "this resource is not up to date, but we are not trying to fix it". We could do this currently by manually updating status condition here, but I think crossplane-runtime would overwrite it after we returned an empty managed.ExternalUpdate.

sahil-lakhwani · 2020-03-14T22:55:25Z

@hasheddan @muvaf I have added new commits to address the comments.

hasheddan

I believe there are a few more of @muvaf comments to address, but going to go ahead and give this the LGTM! @sahil-lakhwani would you mind running a few manual e2e tests (like you did earlier) to verify functionality is operating as expected? Thanks so much for continuing to push through this!

pkg/clients/iam/iamrole.go

pkg/controller/identity/iamrole/controller.go

hasheddan · 2020-03-16T11:24:20Z

pkg/controller/identity/iamrole/controller.go

@@ -81,92 +88,97 @@ func (conn *connector) Connect(ctx context.Context, mgd resource.Managed) (manag
 	if err != nil {
 		return nil, errors.Wrap(err, errClient)
 	}
-	return &external{c}, nil
+	return &external{c, conn.client}, nil


No change needed for now, but I generally prefer to include the names of fields in the struct being returned for readability and new contributors :)

hasheddan · 2020-03-16T11:26:08Z

pkg/controller/identity/iamrolepolicyattachment/controller.go


 	_, err := req.Send(ctx)

-	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.PolicyARN, cr.Spec.RoleName)
+	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.ForProvider.PolicyARN, cr.Spec.ForProvider.RoleName)


I believe @muvaf just means that you can remove the formatted string and return errors.Wrap(err, errAttach) :)

sahil-lakhwani · 2020-03-16T18:30:00Z

I believe there are a few more of @muvaf comments to address, but going to go ahead and give this the LGTM! @sahil-lakhwani would you mind running a few manual e2e tests (like you did earlier) to verify functionality is operating as expected? Thanks so much for continuing to push through this!

@hasheddan Couldn't directly reply to your comment #141 (comment) but the thing you mentioned has been taken care of:
https://github.com/sahil-lakhwani/provider-aws/blob/54a5aa32696b9434364e038fd0db05a1b5864fc4/pkg/controller/identity/iamrole/controller.go#L113-L115

Also, I saw the test coverage results today, will take a look.

infinitecompute · 2020-03-17T18:18:17Z

Is additional code coverage the only blocker to merging?

muvaf · 2020-03-18T20:47:40Z

pkg/controller/identity/iamrolepolicyattachment/controller.go

-		}
-
-		return managed.ExternalObservation{}, errors.Wrapf(err, errGet, cr.Spec.RoleName)
+		return managed.ExternalObservation{}, errors.Wrapf(resource.Ignore(iam.IsErrorNotFound, err), errGet, cr.Spec.ForProvider.RoleName)
 	}

 	var attachedPolicyObject *awsiam.AttachedPolicy
 	for i := 0; i < len(observed.AttachedPolicies); i++ {


Suggested change

for i := 0; i < len(observed.AttachedPolicies); i++ {

for _, policy := range observed.AttachedPolicies {

if cr.Spec.ForProvider.PolicyARN == aws.StringValue(policy.PolicyArn) {

attachedPolicyObject = &policy

break

}

}

Nitpick: using range would probably be easier to read.

muvaf · 2020-03-18T20:51:24Z

pkg/controller/identity/iamrolepolicyattachment/controller.go


 	_, err := req.Send(ctx)

-	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.PolicyARN, cr.Spec.RoleName)
+	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.ForProvider.PolicyARN, cr.Spec.ForProvider.RoleName)


Suggested change

return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.ForProvider.PolicyARN, cr.Spec.ForProvider.RoleName)

return managed.ExternalCreation{}, errors.Wrap(err, errAttach)

I'm suggesting this because this. We don't need to repeat what's on the CR as long as the error string is specific enough.

muvaf

Assuming you tested this manually, it looks good to me.

However, I think we might want to hold off merging this or maybe marking it v1alpha4 until we have a good idea about #157 because the discussion might lead to deletion of IAMRolePolicyAttachment CRD, which is much harder to do if it's v1beta1.

apis/identity/v1beta1/iamrolepolicyattachment_types.go

negz

Thanks for driving this @sahil-lakhwani! It's looking very good. Happy to merge once the remaining comments are addressed.

@infinitecompute Our code coverage check is currently aspirational. Ideally PRs would not lower the overall project coverage, but if the PR author and reviewers agree that there's little value in additional PR coverage we can merge. Based on a quick glance at https://codecov.io/gh/crossplane/provider-aws/pull/141/tree I'm not too concerned about any of this functionality being dramatically undertested.

pkg/controller/identity/iamrolepolicyattachment/controller_test.go

pkg/clients/iam/iamrole.go

pkg/clients/iam/iamrolepolicyattachment_test.go

pkg/controller/identity/iamrole/controller.go

pkg/controller/identity/iamrolepolicyattachment/controller.go

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

muvaf

Based on conclusion in #157 this looks good to me for merge.

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

negz

Thanks @sahil-lakhwani! This looks good to me. I'm going to merge this once we've cut the 0.9 release branch, so this will be included in 0.10.

muvaf · 2020-03-26T14:24:55Z

@negz I was trying to write the YAMLs for the scenario here and realized that we don't support inline IAM policies. Digging more, I found out RolePolicy term is used for inline policy and when we create a IAMRolePolicyAttachment, what happens is that the normal policy gets copied to Role as managed inline policy. The interesting part is that actual inline policies are added in the same way, as seen here and there is no way to create a RolePolicy without attaching it to a role. So, I think some consolidation could make sense there.

I suggest:

Rename IAMRolePolicyAttachment to IAMRolePolicy.
Add a field PolicyDocument to allow specifying the policy inline.
If the PolicyDocument is given, call PutRolePolicy and if PolicyArn is given call AttachRolePolicy.

The idea is that we'd have one managed resource for inline policy operations as opposed to mapping each imperative call to a managed resource. This functionality is additive and not blocking this PR but I wanted to drop it on this PR in case we want to rename the resource, better to do it before bumping to v1beta1.

Also, should we drop IAM prefix from the name of both IAM resources? We already have identity in the group name. Maybe change identity group name to iam to match the API?

negz · 2020-03-27T01:16:32Z

Also, should we drop IAM prefix from the name of both IAM resources? We already have identity in the group name. Maybe change identity group name to iam to match the API?

This sounds reasonable to me. We'd have role.iam.aws.crossplane.io etc, and the Go types would be imported as iamv1beta1.Role. I feel okay merging this PR and making that change in a subsequent one though, to make it easier to review.

negz · 2020-03-27T01:58:17Z

I found out RolePolicy term is used for inline policy

Are you sure this is the case? As far as I can tell there's both PutRolePolicy (which specifies a policy to include inline) and AttachRolePolicy (which references an existing policy ARN) API calls. Both appear to use the term "role policy" for the policy, whether it's inline or distinct.

what happens is that the normal policy gets copied to Role as managed inline policy.

This doesn't match my understanding of the API. There seems to be a ListRolePolicy API call that lists inline policies, and also a ListAttachedRolePolicies that lists attached policies. As far as I can tell when you call GetRole it includes neither attached or inline policies.

we don't support inline IAM policies

@muvaf do we need to support inline IAM policies to support that use case? Under the current model, could we:

Create an IAM role that would be used by the pod that will access the RDS instance
Create an IAM policy that allows access to the RDS instance
Create an IAM role policy attachment to attach the policy from 2 to the role from 1?

I suggest:

Would this mean we could only attach IAM policies to IAM roles that were managed by Crossplane? It seems to me like there are two ways to associate a policy with a role; either include the policy inline, or add it as a distinct API object and create a reference between the two. The latter seems to be the more granular and flexible model.

muvaf · 2020-03-27T11:28:43Z

This sounds reasonable to me. We'd have role.iam.aws.crossplane.io etc, and the Go types would be imported as iamv1beta1.Role. I feel okay merging this PR and making that change in a subsequent one though, to make it easier to review.

Sounds good. The rest of the discussions is not a blocker for this PR. I can prepare a PR to do that.

There is this doc that explains the inline and standalone policies. In all cases, you can use standalone managed policies with any entity (group, role, user etc.). The inline policy has essentially the same semantics except it has a one-to-one relationship with the entity.

Quoting from Inline Policies section of that doc:

Each policy is an inherent part of the user, group, or role. Notice that two roles include the same policy (the DynamoDB-books-app policy), but they are not sharing a single policy; each role has its own copy of the policy.

What I meant by copying is the mechanism that is described in the doc: if we used IAMRolePolicyAttachment that refers to a managed policy, even though that managed policy changes, the policy that is attached to the Role won't change. The moment we attach, a copy of the policy is coupled with the role. IMO, that is essentially the same thing with PutRolePolicy except that you give the policy to attach instead of referring to an existing one. The CLI equivalent of those actions have more description: put-role-policy and attach-role-policy. I think your understanding is closer how it works and attaching is to refer managed policies, not copy it and treat as inline policy.

So, I agree that managed policy attachment and inline policy addition should be different resources.

Create an IAM policy that allows access to the RDS instance

That's what we are not able to do at the moment and I figured if we were able to expand the attachment for inline policy, then it'd have been pretty easy to enable the use-case.

It seems to me like there are two ways to associate a policy with a role; either include the policy inline, or add it as a distinct API object and create a reference between the two. The latter seems to be the more granular and flexible model.

I agree. But both ways have their own use cases. In this instance, even though it's possible to create a standalone policy, doing it inline makes more sense since the policy directly refers to an RDS instance and is the encouraged way. So, at some point we want inline option, too. But I can do with normal managed policy for now.

negz · 2020-03-27T17:49:13Z

Also, should we drop IAM prefix from the name of both IAM resources? We already have identity in the group name. Maybe change identity group name to iam to match the API?

I mentioned this seemed like a good idea earlier, but now I'm thinking we can probably leave it as is. It seems like https://aws.amazon.com/iam/ is one service of https://aws.amazon.com/identity/, which also include SSO, Cognito, etc. I also suspect including IAM in the CR names will help folks, because you don't usually include the group in kubectl commands, and kubectl get iamrole, kubectl get iamrolepolicyattachment probably makes more sense to AWS users than kubectl get role, etc.

muvaf · 2020-03-28T10:32:01Z

I mentioned this seemed like a good idea earlier, but now I'm thinking we can probably leave it as is. It seems like https://aws.amazon.com/iam/ is one service of https://aws.amazon.com/identity/, which also include SSO, Cognito, etc.

I have a feeling that identity is merely a marketing term. The grouping in the SDK seems like a better candidate to use in our groupings. You can see cognito and iam on the same level of services there without identity encapsulating them. Note that adopting SDK grouping has some nice implications as well such as reuse of some structs in our structs. For example ec2.Tag is used by all managed resources in ec2 group or rds.Tag is used by all RDS-related stuff.

I also suspect including IAM in the CR names will help folks, because you don't usually include the group in kubectl commands, and kubectl get iamrole, kubectl get iamrolepolicyattachment probably makes more sense to AWS users than kubectl get role, etc.

Yeah, kubectl get role does have an overlap. I'm not sure whether it'd be weird to make group iam and still call the role iamrole.

FWIW, you don't have to include the full group when there is collision. kubectl get provider.gcp and kubectl get provider.aws both return the right results. Maybe we could get away with using kubectl get role.iam to not have collision with kubectl get role (kubernetes Role resource).

…ib#141) Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

…ib#141)

…-timeout-annotation Fix Uptest timeout annotation

sahil-lakhwani force-pushed the v1b1iam branch from c19b3a4 to e7cfb89 Compare February 26, 2020 17:17

hasheddan self-requested a review February 26, 2020 17:26

hasheddan requested changes Feb 26, 2020

View reviewed changes

golangcibot reviewed Feb 27, 2020

View reviewed changes

pkg/controller/identity/iamrole/controller_test.go Outdated Show resolved Hide resolved

sahil-lakhwani force-pushed the v1b1iam branch 2 times, most recently from fe284fb to f50bd6c Compare March 2, 2020 11:18

hasheddan mentioned this pull request Mar 2, 2020

AWS IAM resources to v1beta1 #100

Closed

hasheddan self-assigned this Mar 2, 2020

hasheddan requested changes Mar 3, 2020

View reviewed changes

infinitecompute mentioned this pull request Mar 4, 2020

Additional AWS managed resources #149

Closed

26 tasks

hasheddan requested changes Mar 4, 2020

View reviewed changes

pkg/controller/identity/iamrole/controller.go Show resolved Hide resolved

sahil-lakhwani force-pushed the v1b1iam branch from 88c0aa7 to 17f7f7f Compare March 9, 2020 09:38

hasheddan requested changes Mar 10, 2020

View reviewed changes

muvaf reviewed Mar 11, 2020

View reviewed changes

hasheddan requested changes Mar 12, 2020

View reviewed changes

sahil-lakhwani force-pushed the v1b1iam branch from acb8865 to 54a5aa3 Compare March 14, 2020 22:44

hasheddan approved these changes Mar 16, 2020

View reviewed changes

muvaf reviewed Mar 18, 2020

View reviewed changes

apis/identity/v1beta1/iamrolepolicyattachment_types.go Outdated Show resolved Hide resolved

infinitecompute mentioned this pull request Mar 18, 2020

AWS SDK resources are not object-based as we assume #157

Closed

negz reviewed Mar 20, 2020

View reviewed changes

sahil-lakhwani added 3 commits March 20, 2020 11:43

move IAMRole and IAMRolePolicyAttachment to v1beta1

62e9abd

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

crossplaneio to crossplane in URLs in identity crds file

08cee04

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

Fix TestUpdate for IAMRole and IAMRolePolicyAttachment

c639094

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

sahil-lakhwani added 9 commits March 20, 2020 11:43

Add GenerateRoleObservation method for identity resources

fb38ebd

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

Table-driven test cases for identity resources

ea3487e

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

IAMRolePolicyAttachment controller: refactor error check

8ada788

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

Add fields to IAMRole

de06f8e

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

IAM Role: check need for update

e9740a6

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

Add dependency for mitchellh/copystructure

7140af4

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

test cases for IAMRole and IAMRolePolicyAttachment

112eaf8

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

identity controllers: refactor to chain methods

a23963a

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

identity: inline generate inputs

d17c6c9

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

muvaf approved these changes Mar 20, 2020

View reviewed changes

escape AssumeRolePolicyDocument string, remove dead code

6a45e2f

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

sahil-lakhwani force-pushed the v1b1iam branch from 54a5aa3 to 6a45e2f Compare March 21, 2020 15:06

negz mentioned this pull request Mar 25, 2020

Expanding IAM support #151

Closed

negz approved these changes Mar 25, 2020

View reviewed changes

muvaf mentioned this pull request Mar 27, 2020

Support inline IAM policies #177

Closed

negz merged commit 90e7044 into crossplane-contrib:master Mar 27, 2020

sahil-lakhwani mentioned this pull request Apr 14, 2020

IAMUser #194

Merged

4 tasks

wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021

Move IAMRole and IAMRolePolicyAttachment to v1beta1 (crossplane-contr…

3f74755

…ib#141) Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021

Move IAMRole and IAMRolePolicyAttachment to v1beta1 (crossplane-contr…

9ba2def

…ib#141)

tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 2024

Merge pull request crossplane-contrib#141 from ezgidemirel/fix-uptest…

fc5c34b

…-timeout-annotation Fix Uptest timeout annotation

		@@ -57,7 +58,7 @@ func TestMain(m *testing.M) {
		func Test_Connect(t *testing.T) {

		@@ -57,7 +57,7 @@ func TestMain(m *testing.M) {
		func Test_Connect(t *testing.T) {

		@@ -38,13 +34,13 @@ type IAMRoleParameters struct {
		Description string `json:"description,omitempty"`

	Description string `json:"description,omitempty"`
	Description *string `json:"description,omitempty"`

	return managed.ExternalCreation{}, errors.Wrapf(err, errAttach, cr.Spec.ForProvider.PolicyARN, cr.Spec.ForProvider.RoleName)
	return managed.ExternalCreation{}, errors.Wrap(err, errAttach)

-	for i := 0; i < len(observed.AttachedPolicies); i++ {
+	for _, policy := range observed.AttachedPolicies {
+		if cr.Spec.ForProvider.PolicyARN == aws.StringValue(policy.PolicyArn) {
+			attachedPolicyObject = &policy
+			break
+		}
+	}

move IAMRole and IAMRolePolicyAttachment to v1beta1 #141

move IAMRole and IAMRolePolicyAttachment to v1beta1 #141

Conversation

sahil-lakhwani commented Feb 26, 2020

Description of your changes

Checklist

sahil-lakhwani commented Feb 26, 2020

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinitecompute commented Mar 6, 2020

sahil-lakhwani commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil-lakhwani commented Mar 14, 2020

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil-lakhwani commented Mar 16, 2020

infinitecompute commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

negz left a comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

negz left a comment

Choose a reason for hiding this comment

muvaf commented Mar 26, 2020 • edited Loading

negz commented Mar 27, 2020 • edited Loading

negz commented Mar 27, 2020

muvaf commented Mar 27, 2020

negz commented Mar 27, 2020

muvaf commented Mar 28, 2020

muvaf commented Mar 26, 2020 •

edited

Loading

negz commented Mar 27, 2020 •

edited

Loading