-
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
Feature azurefirewall #201
base: master
Are you sure you want to change the base?
Feature azurefirewall #201
Conversation
Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com> Signed-off-by: shreya <shreynamdeo@gmail.com>
Signed-off-by: shreya <shreynamdeo@gmail.com>
Signed-off-by: shreya <shreynamdeo@gmail.com>
Signed-off-by: shreya <shreynamdeo@gmail.com>
89e5d9b
to
d77f8e2
Compare
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.
Thanks for working on this @escortnotice! I have added some review comments below. It looks like the resource schema needs to be cleaned up some and tests need to be added. Also, I think you may have accidentally included an already existing commit in your history. Let me know if you have any questions :)
apis/network/v1alpha3/types.go
Outdated
|
||
Spec AzureFirewallSpec `json:"spec"` | ||
Status AzureFirewallStatus `json:"status,omitempty"` | ||
///Properties SecurityGroupPropertiesFormat `json:"properties,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.
Looks like this needs to be removed
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.
Delete the commented code at line 246
apis/network/v1alpha3/types.go
Outdated
// +kubebuilder:printcolumn:name="SYNCED",type="string",JSONPath=".status.conditions[?(@.type=='Synced')].status" | ||
// +kubebuilder:printcolumn:name="STATE",type="string",JSONPath=".status.state" | ||
// +kubebuilder:printcolumn:name="LOCATION",type="string",JSONPath=".spec.location" | ||
// +kubebuilder:printcolumn:name="RECLAIM-POLICY",type="string",JSONPath=".spec.reclaimPolicy" |
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.
nitpick: reclaimPolicy
is deprecated so we probably shouldn't show it here
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.
Removed reclaim policy declaration code.
apis/network/v1alpha3/types.go
Outdated
// Location - Resource location. | ||
Location string `json:"location"` | ||
|
||
//AzureFirewallPropertiesFormat - Properties of AzureFirewall |
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.
//AzureFirewallPropertiesFormat - Properties of AzureFirewall | |
// AzureFirewallPropertiesFormat - Properties of AzureFirewall |
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.
Added space in comment line 275
Location string `json:"location"` | ||
|
||
//AzureFirewallPropertiesFormat - Properties of AzureFirewall | ||
AzureFirewallPropertiesFormat `json:"properties,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 it should have // +optional
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.
'AzureFirewallPropertiesFormat' is not optional.
Etag string `json:"etag,omitempty"` | ||
|
||
// ID - Resource ID. | ||
ID string `json:"id,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.
// +optional
plus *string
. Can you actually set the ID at creation time?
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.
Could you please give us a reference code or explain the changes needed as we are not aware.
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.
The Crossplane managed resource API conventions doc is a good resource that discusses these topics. The convention is to have pointer types for optional fields, and to use the // +optional
marker comment for them.
apis/network/v1alpha3/types.go
Outdated
|
||
// Type - READ-ONLY; Resource type. | ||
Type string `json:"type,omitempty"` | ||
// Location - Resource location. |
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.
// Location - Resource location. |
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.
Removed extra comment from the code.
apis/network/v1alpha3/types.go
Outdated
// Name - READ-ONLY; Resource name. | ||
Name string `json:"name,omitempty"` | ||
|
||
// Type - READ-ONLY; Resource type. | ||
Type string `json:"type,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.
These should be in status if read only
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 need to specify these properties while creating firewall , thus we kept them in Spec and we have removed the READ-ONLY signature from comment.
pkg/clients/network/azurefirewall.go
Outdated
Location: azure.ToStringPtr(v.Spec.Location), | ||
Tags: azure.ToStringPtrMap(v.Spec.Tags), | ||
AzureFirewallPropertiesFormat: &networkmgmt.AzureFirewallPropertiesFormat{ | ||
//ApplicationRuleCollections: nil, |
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.
Should this be removed?
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 have removed this commented code.
if az.Name != nil { | ||
azurefirewall.UpdateAzureFirewallStatusFromAzure(v, 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.
Do any spec fields need to be late initialized also?
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.
Can you please explain the statement we did not understand the context.
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.
Late initialization is the process of setting spec
fields that are not provided by the user when they create the resource, but are set to default values by Azure. Here is an example of doing this: https://github.com/crossplane/provider-azure/blob/45f75a573ad4cc1727cfcee86d616b4892a11f74/pkg/clients/database/mysql.go#L306
// +kubebuilder:printcolumn:name="AGE",type="date",JSONPath=".metadata.creationTimestamp" | ||
// +kubebuilder:subresource:status | ||
// +kubebuilder:resource:scope=Cluster,categories={crossplane,managed,azure} | ||
type AzureFirewall 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.
Since this is a new resource it should be introduced at v1alpha1
version
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.
In the existing codebase we can see v1beta1 or v1alpha3 outside the network folder and v1alpha3 inside network folder , so do you suggest us to create v1alpha1 module also , if so we will need some direction or help from you as we are not aware of the process for doing that.
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.
@ShreyNamdeo yes creating a new package would be appropriate here 👍
Description of your changes
Create a Azure Firewall through crossplane and add NAT and Network Rules to restrict traffic.
Issue: #198
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
This code has been tested manually, using the configuration files in the "example" folder which can referred for the same.