-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds Support for Azure Express Route Circuit. #259
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Praveen Ghuge <praveen.ghuge@outlook.com>
Signed-off-by: Praveen Ghuge <praveen.ghuge@outlook.com>
Signed-off-by: Praveen Ghuge <praveen.ghuge@outlook.com>
Signed-off-by: Praveen Ghuge <praveen.ghuge@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @praveenghuge for working on this. We need to add tests for the client package and the controller package before merging this. Please also take a look at the Crossplane managed resource API conventions doc for how we use the '// +optionalmarker comment and the pointer types for optional fields. Also, could you please rebase this PR onto the current HEAD of the
masterbranch, and make sure all your commits are signed following the guide [here](https://docs.github.com/en/github/authenticating-to-github/managing-commit-signature-verification/signing-commits). We also require them to be signed-off using something similar to
git commit -s.
git log --show-signature` should display commit signatures.
Thank you very much!
@@ -226,3 +226,104 @@ type SubnetList struct { | |||
metav1.ListMeta `json:"metadata,omitempty"` | |||
Items []Subnet `json:"items"` | |||
} | |||
|
|||
// ExpressRouteCircuitsPropertiesFormat defines properties of a ExpressRouteCircuits. | |||
type ExpressRouteCircuitsPropertiesFormat struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type ExpressRouteCircuitsPropertiesFormat struct { | |
type ExpressRouteCircuitPropertiesFormat struct { |
The corresponding model type name in azure-sdk-for-go
is singular: network.ExpressRouteCircuitPropertiesFormat
.
ServiceProviderName *string `json:"serviceProviderName,omitempty"` | ||
|
||
// BandwidthInMbps - Flag denotes bandwithwidth in Mbps. | ||
BandwidthInMbps *int32 `json:"bandwidthInMbps,omitempty"` | ||
|
||
// PeeringLocation - Flag denotes peering location. | ||
PeeringLocation *string `json:"peeringLocation,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per Crossplane MR API conventions document, we had better reflect the azure-sdk-for-go
model of this resource for our high fidelity goal. I think we had better move these fields to their own struct, ExpressRouteCircuitServiceProviderProperties
, reflecting its SDK model. This will also pay off when we add, for instance, the ExpressRouteCircuitPropertiesFormat.BandwidthInGbps
in addition to the proposed ExpressRouteCircuitServiceProviderProperties.BandwidthInMbps
.
@@ -226,3 +226,104 @@ type SubnetList struct { | |||
metav1.ListMeta `json:"metadata,omitempty"` | |||
Items []Subnet `json:"items"` | |||
} | |||
|
|||
// ExpressRouteCircuitsPropertiesFormat defines properties of a ExpressRouteCircuits. | |||
type ExpressRouteCircuitsPropertiesFormat struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We may consider adding the currently missing fields like ExpressRouteCircuitPropertiesFormat.BandwidthInGbps
, ExpressRouteCircuitPropertiesFormat.Peerings
, etc.
// SKU contains SKU in an ExpressRouteCircuit. | ||
type SKU struct { | ||
// Tier - The tier of the SKU. Possible values include: 'ExpressRouteCircuitSkuTierStandard', 'ExpressRouteCircuitSkuTierPremium', 'ExpressRouteCircuitSkuTierBasic', 'ExpressRouteCircuitSkuTierLocal' | ||
Tier string `json:"tier,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is optional:
Tier string `json:"tier,omitempty"` | |
// +optional | |
Tier *string `json:"tier,omitempty"` |
// Tier - The tier of the SKU. Possible values include: 'ExpressRouteCircuitSkuTierStandard', 'ExpressRouteCircuitSkuTierPremium', 'ExpressRouteCircuitSkuTierBasic', 'ExpressRouteCircuitSkuTierLocal' | ||
Tier string `json:"tier,omitempty"` | ||
// Family - The family of the SKU. Possible values include: 'UnlimitedData', 'MeteredData' | ||
Family string `json:"family,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is optional:
Family string `json:"family,omitempty"` | |
// +optional | |
Family *string `json:"family,omitempty"` |
if azureclients.IsNotFound(err) { | ||
return managed.ExternalObservation{ResourceExists: false}, nil | ||
} | ||
if err != nil { | ||
return managed.ExternalObservation{}, errors.Wrap(err, errGetExpressRouteCircuits) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if azureclients.IsNotFound(err) { | |
return managed.ExternalObservation{ResourceExists: false}, nil | |
} | |
if err != nil { | |
return managed.ExternalObservation{}, errors.Wrap(err, errGetExpressRouteCircuits) | |
} | |
if err != nil { | |
return managed.ExternalObservation{}, errors.Wrap(resource.Ignore(azure.IsNotFound, err), errGetExpressRouteCircuits) | |
} |
network.UpdateExpressRouteCircuitStatusFromAzure(exp, az) | ||
exp.SetConditions(xpv1.Available()) | ||
|
||
o := managed.ExternalObservation{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to check if the MR is up-to-date with the external resource, and report back (to the managed reconciler) this information via managed.ExternalObservation.ResourceUpToDate
. The current implementation is always telling the managed reconciler that the managed resource is not up-to-date with the external resource, scheduling probably unnecessary reconciliations for updates.
return managed.ExternalCreation{}, errors.New(errNotExpressRouteCircuits) | ||
} | ||
|
||
exp.Status.SetConditions(xpv1.Creating()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exp.Status.SetConditions(xpv1.Creating()) |
crossplane-runtime can now handle setting "creating" and "deleting" conditions.
return managed.ExternalUpdate{}, errors.Wrap(err, errGetExpressRouteCircuits) | ||
} | ||
|
||
if network.ExpressRouteCircuitNeedsUpdate(exp, az) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be reporting drifts via the return value managed.ExternalObservation.ResourceUpToDate
of external.Observe
. Please see the above comments.
return errors.New(errNotExpressRouteCircuits) | ||
} | ||
|
||
mg.SetConditions(xpv1.Deleting()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mg.SetConditions(xpv1.Deleting()) |
crossplane-runtime can now handle setting "creating" and "deleting" conditions.
Hi @praveenghuge, |
Description of your changes
Adds Support for Express Route Circuit.
Fixes #258
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Manually tested.