TransitGatewayRouteObservation is missing DestinationCidrBlock

[haarchri]

Describe the bug if we run code-generator in crossplane to create ec2 TransitGatewayRoute it will generate a struct for TransitGatewayRouteObservation

// TransitGatewayRouteObservation defines the observed state of TransitGatewayRoute
type TransitGatewayRouteObservation struct {
    // The ID of the prefix list used for destination matches.
    PrefixListID *string `json:"prefixListID,omitempty"`
    // The state of the route.
    State *string `json:"state,omitempty"`
    // The attachments.
    TransitGatewayAttachments []*TransitGatewayRouteAttachment `json:"transitGatewayAttachments,omitempty"`
    // The route type.
    Type *string `json:"type_,omitempty"`
}

but it is missing:

// The CIDR block used for destination matches.
    DestinationCidrBlock *string `locationName:"destinationCidrBlock" type:"string"`

without DestinationCidrBlock it is very hard to implement updates - because we didn't know the last state of the DestinationCidrBlock - so we thought that the DestinationCidrBlock is missing and we will create a new Route entry without to remove the old one ...

https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#TransitGatewayRoute

Steps to reproduce

Expected outcome DestinationCidrBlock is part of TransitGatewayRouteObservation struct or can we inject via CodeGenerator DestinationCidrBlock to TransitGatewayRouteObservation struct ?

Environment

• Kubernetes version
• Using EKS (yes/no), if so version?
• AWS service targeted (S3, RDS, etc.)

[a-hilaly]

@haarchri Did you try to generate the CRDs using the latest aws-sdk-go version?

[haarchri]

no we only tried latest code-generator master and here we have 1.37.10 if i am correct https://github.com/aws-controllers-k8s/code-generator/blob/main/go.mod#L7

[a-hilaly]

@haarchri I tried to generate TransitGatewayRoute and here is the Spec field that was generated:

// TransitGatewayRouteSpec defines the desired state of TransitGatewayRoute.
//
// Describes a route for a transit gateway route table.
type TransitGatewayRouteSpec struct {
     // Indicates whether to drop traffic that matches this route.
     Blackhole *bool `json:"blackhole,omitempty"` 
     // The CIDR range used for destination matches. Routing decisions are based
         // on the most specific match.
         // +kubebuilder:validation:Required
     DestinationCIDRBlock *string `json:"destinationCIDRBlock"`
     // Checks whether you have the required permissions for the action, without
         // actually making the request, and provides an error response. If you have
         // the required permissions, the error response is DryRunOperation. Otherwise,
         // it is UnauthorizedOperation.
     DryRun *bool `json:"dryRun,omitempty"` 
     // The ID of the attachment.
     TransitGatewayAttachmentID *string `json:"transitGatewayAttachmentID,omitempty"` 
     // The ID of the transit gateway route table.
     // +kubebuilder:validation:Required
     TransitGatewayRouteTableID *string `json:"transitGatewayRouteTableID"`
}

Using latest code-gen commit and https://github.com/aws-controllers-k8s/ec2-controller/blob/main/go.mod#L7

[a-hilaly]

Here the Status field generated (equivalent of Observation struct):

// TransitGatewayRouteStatus defines the observed state of TransitGatewayRoute
type TransitGatewayRouteStatus struct {
        // ... //
    // The ID of the prefix list used for destination matches.
    // +kubebuilder:validation:Optional
    PrefixListID *string `json:"prefixListID,omitempty"`
    // The state of the route.
    // +kubebuilder:validation:Optional
    State *string `json:"state,omitempty"`
    // The attachments.
    // +kubebuilder:validation:Optional
    TransitGatewayAttachments []*TransitGatewayRouteAttachment `json:"transitGatewayAttachments,omitempty"`
    // The route type.
    // +kubebuilder:validation:Optional
    Type *string `json:"type_,omitempty"`
}

As you can see the DestinationCidrBlock field exists in the Spec struct but not in the status one.

[haarchri]

thanks for sharing - is this SDK specific that the DestinationCidrBlock is missing in TransitGatewayRouteStatus ? because this is the important field if we publish routes for status 👯

[haarchri]

at the end the API for TransitGatewayRoute is special if we want to Describe the TransitGatewayRoute - we need to use SearchTransitGatewayRoutes and here we get back for Routes Routes []*TransitGatewayRoute and then we have DestinationCidrBlock but our Struct for Status/Observation is without - but we need DestinationCidrBlock in status otherwise we don't know if a user changed the DestinationCidrBlock or not - because this is our Identifier :/

[a-hilaly]

The way ACK code-generator determines whether a field should be part of status or spec is the following:

• If a field is part of the Create shape (structs) it's a Spec. e.g CreateTransitGatewayRouteRequest
• If a field is part of the Output but not of the Create shapes it's a status

More details:

• Determining Spec fields
• Determining status fields

[a-hilaly]

@haarchri Do you have a DestinationCidrBlock in the Spec struct? I believe you still need it in the Spec and not the observation because that field is part of the CreateTransitGatewayRoute shape.

[haarchri]

Yes we have but - what If someone changed it we lose the information and lost the old entry because the DestinationBlock is not part of Status after Creation

[a-hilaly]

That field is required and should not be deleted by the user who is managing it. Same thing goes with dynamodb table names, s3 bucket names etc... ACK controllers use a kubebuilder option (// +kubebuilder:validation:Required) to mark fields as required and instruct the kubeap to validate resources before applying the updates

Are you requesting to replicate DestinationCIDRBlock in both Status and Spec?

[haarchri]

Are you requesting to replicate DestinationCIDRBlock in both Status and Spec? - this would be the perfect case

[vijtrip2]

Are you requesting to replicate DestinationCIDRBlock in both Status and Spec? - this would be the perfect case

Just a note, This is not the pattern we have followed any where in ACK controller CRD generation. To handle custom logic, ACK supports code hooks and for storing additional metadata and previous states, ACK team has preferred using metadata.Annotations

what If someone changed it we lose the information and lost the old entry because the DestinationBlock is not part of Status after Creation

Currently, if a user updates the identifier, the ACK controller will search for a resource with the identifier mentioned in desired state, receive NotFoundException, and create the new desired resource.

The above will work with the caveat that old resource is never deleted. This problem exists for all ACK controllers which have identifier field in spec(Ex: 'Name' in s3 controller's Bucket resource) and I believe ACK team will need to discuss this separately and come up with a standard solution. I will create a Github issue for this.

Interested to hear your thought on above, @haarchri

[jaypipes]

Are you requesting to replicate DestinationCIDRBlock in both Status and Spec? - this would be the perfect case

This is actually not what we recommend doing (and AFAIK, isn't the way that Crossplane's modeling works either [0]). The guidance is to not duplicate the same field in Spec and Status. Instead, if the field is settable (either in Create or in Update calls), it should go in Spec. If not, it should go in Status.

[0] refer to @muvaf and my discussion on https://groups.google.com/g/kubernetes-sig-architecture/c/d96tt2Mw69s

Also see #814