move network resources to v1beta1 #182

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

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

Description of your changes

This PR moves all existing network resources to v1beta1

Tested examples in examples/network.

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.

muvaf

I've been able to review only internet gateway. Most of the comments probably apply to other resources, too. So, I'd suggest checking others as well during iteration.

In the future, please consider breaking up PRs by resource so that reviews are easier and we can merge the ones that are approved separately, faster.

pkg/controller/network/internetgateway/controller.go

apis/network/v1beta1/internetgateway_types.go

muvaf · 2020-04-01T23:19:33Z

@sahil-lakhwani On another note, please don't hesitate to make changes to the existing logic or decide to take some small divergences to make things simpler as you see fit.

infinitecompute · 2020-04-09T17:26:49Z

@sahil-lakhwani @muvaf What is the checklist to get this closed out? Can we resolve all conversations that have been addressed please.

apis/network/v1beta1/internetgateway_types.go

apis/network/v1beta1/securitygroup_types.go

apis/network/v1beta1/subnet_types.go

sahil-lakhwani · 2020-04-09T22:40:30Z

@muvaf @hasheddan I have addressed your comments, and taken care of them across resources. I have replied to comments which are not marked Outdated here, they are addressed in the code.

negz · 2020-04-18T01:36:46Z

This PR needs a rebase due to the work we did to migrate all the controllers to support the crossplane.io/external-name annotation. It will likely need further rebasing once #198 lands, so it might be best to rebase on that branch.

negz · 2020-05-12T22:07:45Z

apis/network/v1beta1/routetable_types.go

 	runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1"
 )

 // Route describes a route in a route table.
 type Route struct {
 	// The IPv4 CIDR address block used for the destination match. Routing
 	// decisions are based on the most specific match.
-	DestinationCIDRBlock string `json:"destinationCidrBlock"`
+	DestinationCIDRBlock string `json:"destinationCidrBlock,omitempty"`


I notice you added an omitempty tag here. Is this field optional? If so, should it have the optional comment marker and be a pointer?

Ah - is this to accomodate using Route in both the spec and the status? I suggest we avoid doing this, per my comments below.

apis/network/v1beta1/routetable_types.go

negz · 2020-05-12T22:18:46Z

apis/network/v1beta1/routetable_types.go

+	// The state of the association.
+	State string `json:"state,omitempty"`
+
+	Association Association `json:",inline"`


Similar to my comment about RouteState above, I'd prefer to avoid embedding the Association here and to just include a SubnetID string field in the AssociationState.

apis/network/v1beta1/subnet_types.go

examples/network/securitygroup.yaml

pkg/controller/network/securitygroup/controller.go

negz · 2020-05-13T01:24:31Z

pkg/controller/network/securitygroup/controller.go

+		}
+	}
+
+	if patch.Ingress != nil {


It looks like we create the security group, but never add any ingress or egress rules until update time. Does this mean there will be a brief window in which the security group will appear to be 'ready' but won't actually be allowing the traffic it is configured to allow?

Have added a check for the ingress/egress rules, and then updating the status.

negz · 2020-05-13T01:36:38Z

pkg/controller/network/internetgateway/controller.go

-		return managed.ExternalCreation{}, errors.Wrap(err, errPersistExternalName)
+	meta.SetExternalName(cr, aws.StringValue(ig.InternetGateway.InternetGatewayId))
+
+	return managed.ExternalCreation{}, errors.Wrap(e.kube.Update(ctx, cr), errSpecUpdate)


Here and elsewhere in this PR, it seems like we have a call order of:

Set conditions. (Modifies status)

Set external name. (Modifies metadata)

Call e.kube.Update()

This is all followed by a call to e.kube.Status().Update() made by the managed.Reconciler.

Will calling Update() before we call Status().Update() undo the status conditions that we set? I can never remember whether Update() overwrites status or not.

Will calling Update() before we call Status().Update() undo the status conditions that we set?

Yes. The operation that undoes the rest of the resource is not Update but the call itself actually. What it does when you call either Status().Update() or Update() is that it issues the respective payload (either only status or metadata+spec) and then deep copies the resulting object into what you have sent. So, the local object is the most up-to-date version that exists in the api-server, which means the payload that you didn't send is overwritten by whatever is in api-server.

We have synced with @sahil-lakhwani about this and it seems calling Status().Update() after the SetConditions makes sense because that way if the CreateInternetGatewayRequest call fails, the condition will reflect that the error arose during creation.

negz · 2020-05-13T01:40:54Z

pkg/controller/network/vpc/controller.go

 	if !ok {
 		return managed.ExternalCreation{}, errors.New(errUnexpectedObject)
 	}

-	// if VPCID already exists, skip creating the vpc
-	// this happens when an error has occurred when modifying vpc attributes


Is this no longer a concern?

After the below comment is addressed, I think this won't be needed.

negz · 2020-05-13T01:43:59Z

pkg/controller/network/vpc/controller.go

-		attrReq := e.client.ModifyVpcAttributeRequest(input)
-
-		if _, err := attrReq.Send(ctx); err != nil {
+		if _, err := e.client.ModifyVpcAttributeRequest(input).Send(ctx); err != nil {
 			return managed.ExternalCreation{}, errors.Wrap(err, errModifyVPCAttributes)
 		}
 	}


It seems like this PR mostly has a pattern of creating a resource in Create, then waiting until Update time to set any fields that are modifiable. Should we follow that pattern for VPCs?

muvaf

I have left some comments but they should be easy to fix. Let's wrap up this PR, merge and revisit later for more.

A general comment, +immutable tag is not used in many of the resources. Even though it's not functional yet, it shows up in docs and serves a nice informational purpose. Also it seems the PR needs to be rebased.

apis/network/v1beta1/vpc_types.go

muvaf · 2020-05-20T01:03:28Z

pkg/controller/network/vpc/controller.go

 	}

+	meta.SetExternalName(cr, aws.StringValue(result.Vpc.VpcId))


Right after setting the external name, we should call e.kube.Update() so that the annotation is not lost if the next steps fail for some reason.

pkg/controller/network/vpc/controller.go

muvaf · 2020-05-20T01:07:34Z

pkg/controller/network/vpc/controller.go

+	current := cr.Spec.ForProvider.DeepCopy()
+	ec2.LateInitializeVPC(&cr.Spec.ForProvider, &observed)
+	if !cmp.Equal(current, &cr.Spec.ForProvider) {
+		if err := e.kube.Update(ctx, cr); err != nil {


Once you call e.kube.Update(), the cr object will be populated with whatever the resulting object is in the api-server and that doesn't include status, i.e. all changes you make on status before this line gets lost. It's probably called only once or twice as late-init frequency is low but would be better if you just moved the SetConditions calls below e.kube.Update.

pkg/clients/ec2/vpc.go

muvaf · 2020-05-20T01:35:40Z

pkg/controller/network/routetable/controller.go

@@ -142,99 +158,134 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 		cr.SetConditions(runtimev1alpha1.Available())
 	}



Why don't we need a late-init for this resource?

pkg/clients/ec2/internetgateway.go

muvaf · 2020-05-20T01:49:23Z

pkg/clients/ec2/routetable.go

+	if len(in.Tags) == 0 && len(rt.Tags) != 0 {
+		in.Tags = v1beta1.BuildFromEC2Tags(rt.Tags)
+	}
+}


Ideally, there should be a field of the struct and we should late-init only the elements that we don't have locally. But that doesn't handle the fact that user might have wanted to delete that entry. Lately, I've been thinking that maybe we'd require user to prefix - in the key field value to let us know that this entry should be removed, similar to kubectl unlabel notation.

The late-init only in case the array is empty is the safe solution but there are cases where there is a constant default entry even if you didn't create it, so, we'd be missing that. But I think skipping if the spec array is not empty should be fine for this PR

muvaf · 2020-05-20T01:50:27Z

pkg/clients/ec2/routetable.go

+	if len(in.Tags) == 0 && len(rt.Tags) != 0 {
+		in.Tags = v1beta1.BuildFromEC2Tags(rt.Tags)
+	}
+}


FWIW, this function is not used in Observe. Only in UpToDate check as far as I can see.

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

sahil-lakhwani · 2020-06-10T07:54:34Z

@muvaf @negz I have addressed the comments. I have resolved some trivial conversations.
Also, have made sure that the CR Status is updated before it is lost.

muvaf

Assuming all is manually tested, LGTM! I have one small comment but this should be ready to merge. @negz

apis/network/v1beta1/vpc_types.go

muvaf · 2020-06-10T09:01:23Z

@hasheddan Is the current group OK for these resources or should it be ec2.aws.crossplane.io ?

muvaf · 2020-06-10T10:37:18Z

apis/network/v1beta1/vpc_types.go

+// +kubebuilder:object:root=true
+
+// A VPC is a managed resource that represents an AWS Virtual Private Cloud.
+// +kubebuilder:printcolumn:name="VPCID",type="string",JSONPath=".status.atProvider.vpcId"


Suggested change

// +kubebuilder:printcolumn:name="VPCID",type="string",JSONPath=".status.atProvider.vpcId"

// +kubebuilder:printcolumn:name="VPCID",type="string",JSONPath=".metadata.annotations['crossplane.io/external-name']"

I believe this should work but didn't test manually.

I checked this, it doesn't work and I have removed that printcolumn annotation for now.

Here is the working form:

// +kubebuilder:printcolumn:name="VPCID",type="string",JSONPath=".metadata.annotations.crossplane\\.io/external-name"

Thanks @muvaf. Added this in a new commit.

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

hasheddan

@muvaf I feel like they probably should be in ec2 group, but at this point I think we should either stick with network, or merge this then circle back around to update group before next release.

muvaf · 2020-06-13T10:14:56Z

@hasheddan we need your approval because of your request for changes.

hasheddan

Approving to remove my requested changes. I haven’t gotten a chance to do a final review but will defer to @muvaf here

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

muvaf · 2020-06-15T14:54:55Z

Tested with Wordpress and it's working as expected.

Merging this and I will open a follow-up PR to move these network CRDs to ec2 group per @hasheddan 's comment and then update AWS Sample Stack to use the new specs.

hasheddan · 2020-06-15T14:55:49Z

@muvaf nice, thanks!

…work move network resources to v1beta1

sahil-lakhwani marked this pull request as ready for review March 31, 2020 04:34

muvaf mentioned this pull request Mar 31, 2020

Support backup and restore of all AWS managed resources #181

Closed

7 tasks

infinitecompute added the new-resource label Mar 31, 2020

muvaf self-requested a review April 1, 2020 20:51

muvaf reviewed Apr 1, 2020

View reviewed changes

infinitecompute added the ic label Apr 9, 2020

hasheddan requested changes Apr 9, 2020

View reviewed changes

apis/network/v1beta1/internetgateway_types.go Outdated Show resolved Hide resolved

apis/network/v1beta1/securitygroup_types.go Show resolved Hide resolved

apis/network/v1beta1/subnet_types.go Outdated Show resolved Hide resolved

sahil-lakhwani force-pushed the v1b1network branch from ca42cf2 to 55c9952 Compare April 9, 2020 22:30

sahil-lakhwani force-pushed the v1b1network branch from 55c9952 to 54a826f Compare May 12, 2020 07:49

negz reviewed May 13, 2020

View reviewed changes

muvaf reviewed May 20, 2020

View reviewed changes

muvaf self-assigned this Jun 2, 2020

sahil-lakhwani added 9 commits June 10, 2020 00:50

move network resources to v1beta1

5af91ce

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

network: user external name as identifier

7da571f

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

network: add tags to resources

d1584ed

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

network/internetgateway: remove nested if

38a9036

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

network/internetgateway: check for AlreadyAttached

b5e535d

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

network: make vpcId optional

a6b5026

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

Apply new references paattern

4f3c2e4

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

network: ignore reference and selector types while comparing

8ac360d

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

network: update CR status before loosing it

18b01c9

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

sahil-lakhwani force-pushed the v1b1network branch from 54a826f to 18b01c9 Compare June 9, 2020 19:21

network: uppercase struct and field names

fa44381

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

muvaf approved these changes Jun 10, 2020

View reviewed changes

apis/network/v1beta1/vpc_types.go Outdated Show resolved Hide resolved

muvaf reviewed Jun 10, 2020

View reviewed changes

network/vpc: remove observation field

7ec3228

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

hasheddan self-requested a review June 10, 2020 14:04

hasheddan reviewed Jun 10, 2020

View reviewed changes

muvaf requested a review from hasheddan June 13, 2020 10:14

hasheddan approved these changes Jun 13, 2020

View reviewed changes

network/vpc: added vpcid printer column

6019ce7

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

muvaf merged commit 534d6d9 into crossplane-contrib:master Jun 15, 2020

prasek mentioned this pull request Aug 6, 2020

AWS networking and VPC resources to v1beta1 #145

Closed

infinitecompute mentioned this pull request Aug 6, 2020

Additional AWS managed resources #149

Closed

26 tasks

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

Merge pull request crossplane-contrib#182 from sahil-lakhwani/v1b1net…

e1993e4

…work move network resources to v1beta1

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

Merge pull request crossplane-contrib#182 from sahil-lakhwani/v1b1net…

5b8b5c2

…work move network resources to v1beta1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move network resources to v1beta1 #182

move network resources to v1beta1 #182

sahil-lakhwani commented Mar 30, 2020

muvaf left a comment

muvaf commented Apr 1, 2020 •

edited

Loading

infinitecompute commented Apr 9, 2020

sahil-lakhwani commented Apr 9, 2020

negz commented Apr 18, 2020

negz May 12, 2020

negz May 12, 2020

negz May 12, 2020

negz May 13, 2020

sahil-lakhwani Jun 9, 2020

negz May 13, 2020

muvaf Jun 8, 2020

negz May 13, 2020

sahil-lakhwani May 17, 2020

negz May 13, 2020

muvaf left a comment

muvaf May 20, 2020

muvaf May 20, 2020

muvaf May 20, 2020

muvaf May 20, 2020

muvaf May 20, 2020

sahil-lakhwani commented Jun 10, 2020

muvaf left a comment

muvaf commented Jun 10, 2020

muvaf Jun 10, 2020

sahil-lakhwani Jun 10, 2020

muvaf Jun 13, 2020

sahil-lakhwani Jun 15, 2020

hasheddan left a comment

muvaf commented Jun 13, 2020

hasheddan left a comment

muvaf commented Jun 15, 2020

hasheddan commented Jun 15, 2020

		}

		meta.SetExternalName(cr, aws.StringValue(result.Vpc.VpcId))

		@@ -142,99 +158,134 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
		cr.SetConditions(runtimev1alpha1.Available())
		}

	// +kubebuilder:printcolumn:name="VPCID",type="string",JSONPath=".status.atProvider.vpcId"
	// +kubebuilder:printcolumn:name="VPCID",type="string",JSONPath=".metadata.annotations['crossplane.io/external-name']"

move network resources to v1beta1 #182

move network resources to v1beta1 #182

Conversation

sahil-lakhwani commented Mar 30, 2020

Description of your changes

Checklist

muvaf left a comment

Choose a reason for hiding this comment

muvaf commented Apr 1, 2020 • edited Loading

infinitecompute commented Apr 9, 2020

sahil-lakhwani commented Apr 9, 2020

negz commented Apr 18, 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

muvaf 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

sahil-lakhwani commented Jun 10, 2020

muvaf left a comment

Choose a reason for hiding this comment

muvaf commented Jun 10, 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

hasheddan left a comment

Choose a reason for hiding this comment

muvaf commented Jun 13, 2020

hasheddan left a comment

Choose a reason for hiding this comment

muvaf commented Jun 15, 2020

hasheddan commented Jun 15, 2020

muvaf commented Apr 1, 2020 •

edited

Loading