Add Firewall resource #333

psinghal20 · 2021-06-08T21:56:53Z

Description of your changes

Fixes a part of #329

Some of the things I have assumed while defining types and controller that can be incorrect and would require a look:

The fields marked as OUTPUT_ONLY are to be treated as observations.
The fields ForceSendFields, NullFields and ServerResponse in google API types can be ignored for our use case.
All the comments in Type fields are directly used from google API and were a little verbose.

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

This code has primarily been tested locally using the example file attached with this change. I have also added unit tests similar to what we have for other compute resources.

hasheddan

This is looking really great @psinghal20! A few comments below and it would be great to also add some unit tests for the firewall client :)

hasheddan · 2021-06-09T00:51:07Z

pkg/controller/gcp.go

 		container.SetupCluster,
+		container.SetupGKECluster,


I believe this may have been inadvertently added during rebase.

Oh, I think I messed up while rebasing the master to fix the conflicts, I will fix this up, and I think ideally it should just be container.SetupCluster. My bad, I will fix this up.

hasheddan · 2021-06-09T00:52:32Z

apis/compute/v1beta1/firewall_types.go

+type FirewallLogConfig struct {
+	// Enable: This field denotes whether to enable logging for a particular
+	// firewall rule.
+	Enable bool `json:"enable,omitempty"`


If this is optional it should be *bool and marked with + optional. However, sometimes when there is a single field in an embedded struct I will opt for making it required since the embedded struct really shouldn't be provided if the only field it supports is not set.

Right, I agree with that. I have made FirewallLogConfig an optional object but made Enable as required and removed omitempty for it.

apis/compute/v1beta1/firewall_types.go

psinghal20 · 2021-06-09T14:18:05Z

Hi @hasheddan, I have resolved all the above comments and added some small unit tests for firewall client code which helped uncover an issue in the generateFirewall method. 😬

hasheddan

@psinghal20 was revisiting this PR and I noticed this resource is being introduced at v1beta1, but we typically introduced new resources at v1alpha1, would you mind updating?

turkenh

@psinghal20, first of all, sorry for the long review process but I hope there will be some improvement in the near future.
Your PR looking very good and thanks a lot for your contribution 💪

While testing on my side, I observed some missing pieces which I added as comments.

@psinghal20 was revisiting this PR and I noticed this resource is being introduced at v1beta1, but we typically introduced new resources at v1alpha1, would you mind updating?

I also noticed you already moved to v1alpha1 but didn't register to schema which causes controller to fail at startup. Basically, we just need the following patch:

diff --git a/apis/gcp.go b/apis/gcp.go
index 14c59b3..269efbe 100644
--- a/apis/gcp.go
+++ b/apis/gcp.go
@@ -21,6 +21,7 @@ import (
        "k8s.io/apimachinery/pkg/runtime"

        cachev1beta1 "github.com/crossplane/provider-gcp/apis/cache/v1beta1"
+       computev1alpha1 "github.com/crossplane/provider-gcp/apis/compute/v1alpha1"
        computev1beta1 "github.com/crossplane/provider-gcp/apis/compute/v1beta1"
        containerv1beta1 "github.com/crossplane/provider-gcp/apis/container/v1beta1"
        containerv1beta2 "github.com/crossplane/provider-gcp/apis/container/v1beta2"
@@ -41,6 +42,7 @@ func init() {
                gcpv1alpha3.SchemeBuilder.AddToScheme,
                gcpv1beta1.SchemeBuilder.AddToScheme,
                cachev1beta1.SchemeBuilder.AddToScheme,
+               computev1alpha1.SchemeBuilder.AddToScheme,
                computev1beta1.SchemeBuilder.AddToScheme,
                containerv1beta2.SchemeBuilder.AddToScheme,
                containerv1beta1.SchemeBuilder.AddToScheme,

pkg/controller/compute/firewall.go

pkg/clients/firewall/firewall.go

psinghal20 · 2021-08-30T20:22:10Z

Hi @turkenh, I have resolved the above-mentioned comments. Please take a look, thanks!

turkenh · 2021-08-31T19:16:16Z

@psinghal20 thanks, looking good but there seem to be some linter errors.
Could you check them?

fahedouch · 2021-09-01T08:37:53Z

Hi teams,

thank you for your work @psinghal20 . @hasheddan @turkenh we really need this feature, any idea in which release it will land?

This commit adds the types and generated files for the Firewall managed resource. Signed-off-by: Pratyush Singhal <[email protected]>

managed resource. Signed-off-by: Pratyush Singhal <[email protected]>

…resource Signed-off-by: Pratyush Singhal <[email protected]>

Signed-off-by: Pratyush Singhal <[email protected]>

…nd denied rules Signed-off-by: Pratyush Singhal <[email protected]>

Signed-off-by: Pratyush Singhal <[email protected]>

refactor for firewall resource Signed-off-by: Pratyush Singhal <[email protected]>

Signed-off-by: Pratyush Singhal <[email protected]>

psinghal20 · 2021-09-01T20:13:59Z

Hi @turkenh, I have updated the PR, the change required a rebase with the master. I have updated some of the tests accordingly, and removed the test case checking the Kubernetes resource update which we removed here. Let me know if anything else is required, thanks! 😄

turkenh

Great work @psinghal20, thanks!

psinghal20 force-pushed the firewall-resource branch 2 times, most recently from e10a836 to c6ce334 Compare June 8, 2021 22:03

psinghal20 changed the title ~~Firewall resource~~ Add Firewall resource Jun 8, 2021

hasheddan reviewed Jun 9, 2021

View reviewed changes

hasheddan reviewed Jul 1, 2021

View reviewed changes

psinghal20 force-pushed the firewall-resource branch from d86e460 to 0cb4503 Compare July 1, 2021 20:07

psinghal20 requested a review from hasheddan July 5, 2021 16:58

turkenh reviewed Aug 19, 2021

View reviewed changes

pkg/controller/compute/firewall.go Outdated Show resolved Hide resolved

pkg/controller/compute/firewall.go Outdated Show resolved Hide resolved

pkg/clients/firewall/firewall.go Outdated Show resolved Hide resolved

psinghal20 added 17 commits September 2, 2021 01:21

feat(compute/firewall): add firewall managed resource type

4e5c8b2

This commit adds the types and generated files for the Firewall managed resource. Signed-off-by: Pratyush Singhal <[email protected]>

feat(controller/firewall): add controller and client for firewall

9b8913a

managed resource. Signed-off-by: Pratyush Singhal <[email protected]>

feat(firewall): add referencer resolver support for firewall managed …

7b812ba

…resource Signed-off-by: Pratyush Singhal <[email protected]>

fix(controller/firewall): fix isUpToDate helper for firewall controller

60d2e36

Signed-off-by: Pratyush Singhal <[email protected]>

chore(firewall/tests): add unit tests for firewall controller

5451b69

Signed-off-by: Pratyush Singhal <[email protected]>

chore(firewall): remove unneccessary consts and fix comments

ef45385

Signed-off-by: Pratyush Singhal <[email protected]>

chore(example/firewall): add example for firewall managed resource

f563d12

Signed-off-by: Pratyush Singhal <[email protected]>

chore(firewall): update crd description for firewall

9359f48

Signed-off-by: Pratyush Singhal <[email protected]>

fix(firewall): fix firewall type markings and fix earlier rebase issues

6a2b8d6

Signed-off-by: Pratyush Singhal <[email protected]>

chore(firewall): run make reviewable to generate updated crd

592d539

Signed-off-by: Pratyush Singhal <[email protected]>

fix(firewall): fix generateFirewall func to properly update allowed a…

6c659ae

…nd denied rules Signed-off-by: Pratyush Singhal <[email protected]>

test(firewall): add unit tests for firewall client

cff845d

Signed-off-by: Pratyush Singhal <[email protected]>

chore(firewall/types): update doc comment for firewall observation

98e42bf

Signed-off-by: Pratyush Singhal <[email protected]>

fix(firewall/client): fix lateIntialize for allowed and denied rules

ddd2c6a

Signed-off-by: Pratyush Singhal <[email protected]>

refactor: move firewall resource to v1alpha1 version

25a6fa3

Signed-off-by: Pratyush Singhal <[email protected]>

refactor: register computev1alphav1 schema, fix isUpToDate and minor

b393ec4

refactor for firewall resource Signed-off-by: Pratyush Singhal <[email protected]>

refactor: fix lint errors and update unit tests

83637f3

Signed-off-by: Pratyush Singhal <[email protected]>

psinghal20 force-pushed the firewall-resource branch from ea7f030 to 83637f3 Compare September 1, 2021 20:11

turkenh approved these changes Sep 2, 2021

View reviewed changes

turkenh merged commit 95d1725 into crossplane-contrib:master Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Firewall resource #333

Add Firewall resource #333

psinghal20 commented Jun 8, 2021 •

edited

Loading

hasheddan left a comment

hasheddan Jun 9, 2021

psinghal20 Jun 9, 2021

hasheddan Jun 9, 2021

psinghal20 Jun 9, 2021

psinghal20 commented Jun 9, 2021

hasheddan left a comment

turkenh left a comment

psinghal20 commented Aug 30, 2021

turkenh commented Aug 31, 2021

fahedouch commented Sep 1, 2021

psinghal20 commented Sep 1, 2021

turkenh left a comment

Add Firewall resource #333

Add Firewall resource #333

Conversation

psinghal20 commented Jun 8, 2021 • edited Loading

Description of your changes

How has this code been tested

hasheddan left a comment

Choose a reason for hiding this comment

hasheddan Jun 9, 2021

Choose a reason for hiding this comment

psinghal20 Jun 9, 2021

Choose a reason for hiding this comment

hasheddan Jun 9, 2021

Choose a reason for hiding this comment

psinghal20 Jun 9, 2021

Choose a reason for hiding this comment

psinghal20 commented Jun 9, 2021

hasheddan left a comment

Choose a reason for hiding this comment

turkenh left a comment

Choose a reason for hiding this comment

psinghal20 commented Aug 30, 2021

turkenh commented Aug 31, 2021

fahedouch commented Sep 1, 2021

psinghal20 commented Sep 1, 2021

turkenh left a comment

Choose a reason for hiding this comment

psinghal20 commented Jun 8, 2021 •

edited

Loading