From dc8f894e987d7b30bad9020f25332e3ff3e377a2 Mon Sep 17 00:00:00 2001 From: Gemma Hou Date: Fri, 4 Oct 2024 00:11:12 +0000 Subject: [PATCH] address comments --- mockgcp/mockcompute/firewallpoliciesv1.go | 272 +++++++----------- mockgcp/mockcompute/operations.go | 17 ++ .../v1beta1/computefirewallpolicy/_http.log | 2 - tests/e2e/unified_test.go | 1 - 4 files changed, 117 insertions(+), 175 deletions(-) diff --git a/mockgcp/mockcompute/firewallpoliciesv1.go b/mockgcp/mockcompute/firewallpoliciesv1.go index 55a50dc993..4c9ec1a0c9 100644 --- a/mockgcp/mockcompute/firewallpoliciesv1.go +++ b/mockgcp/mockcompute/firewallpoliciesv1.go @@ -82,76 +82,7 @@ func (s *FirewallPoliciesV1) Insert(ctx context.Context, req *pb.InsertFirewallP // Use default rules if obj.Rules == nil { - obj.Rules = []*pb.FirewallPolicyRule{ - { - Action: PtrTo("goto_next"), - Description: PtrTo("default egress rule ipv6"), - Direction: PtrTo("EGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - DestIpRanges: []string{"::/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483644)), - RuleTupleCount: PtrTo(int32(2)), - }, - { - Action: PtrTo("goto_next"), - Description: PtrTo("default ingress rule ipv6"), - Direction: PtrTo("INGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - SrcIpRanges: []string{"::/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483645)), - RuleTupleCount: PtrTo(int32(2)), - }, - { - Action: PtrTo("goto_next"), - Description: PtrTo("default egress rule"), - Direction: PtrTo("EGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - DestIpRanges: []string{"0.0.0.0/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483646)), - RuleTupleCount: PtrTo(int32(2)), - }, - { - Action: PtrTo("goto_next"), - Description: PtrTo("default ingress rule"), - Direction: PtrTo("INGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - SrcIpRanges: []string{"0.0.0.0/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483647)), - RuleTupleCount: PtrTo(int32(2)), - }, - } + populateDefaultRules(obj) } if err := s.storage.Create(ctx, fqn, obj); err != nil { @@ -197,7 +128,7 @@ func (s *FirewallPoliciesV1) Patch(ctx context.Context, req *pb.PatchFirewallPol // patch operation finished super fast Progress: PtrTo(int32(100)), Status: PtrTo(pb.Operation_DONE), - EndTime: PtrTo("2024-04-01T12:34:56.123456Z"), + EndTime: PtrTo(s.nowString()), } return s.startGlobalOrganizationLRO(ctx, op, func() (proto.Message, error) { return obj, nil @@ -242,83 +173,6 @@ func (s *FirewallPoliciesV1) GetRule(ctx context.Context, req *pb.GetRuleFirewal return nil, err } - if obj.Rules == nil { - // add default rule - obj.Rules = []*pb.FirewallPolicyRule{ - { - Action: PtrTo("goto_next"), - Description: PtrTo("default egress rule ipv6"), - Direction: PtrTo("EGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - DestIpRanges: []string{"::/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483644)), - RuleTupleCount: PtrTo(int32(2)), - }, - { - Action: PtrTo("goto_next"), - Description: PtrTo("default ingress rule ipv6"), - Direction: PtrTo("INGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - SrcIpRanges: []string{"::/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483645)), - RuleTupleCount: PtrTo(int32(2)), - }, - { - Action: PtrTo("goto_next"), - Description: PtrTo("default egress rule"), - Direction: PtrTo("EGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - DestIpRanges: []string{"0.0.0.0/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483646)), - RuleTupleCount: PtrTo(int32(2)), - }, - { - Action: PtrTo("goto_next"), - Description: PtrTo("default ingress rule"), - Direction: PtrTo("INGRESS"), - EnableLogging: PtrTo(false), - Kind: PtrTo("compute#firewallPolicyRule"), - Match: &pb.FirewallPolicyRuleMatcher{ - SrcIpRanges: []string{"0.0.0.0/0"}, - Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ - { - IpProtocol: PtrTo("all"), - }, - }, - }, - Priority: PtrTo(int32(2147483647)), - RuleTupleCount: PtrTo(int32(2)), - }, - } - if err := s.storage.Update(ctx, fqn, obj); err != nil { - return nil, err - } - } - var rule *pb.FirewallPolicyRule rules := obj.GetRules() @@ -349,17 +203,7 @@ func (s *FirewallPoliciesV1) AddRule(ctx context.Context, req *pb.AddRuleFirewal } r := req.GetFirewallPolicyRuleResource() - // RuleTupleCount is output only, calculation of the complexity of a single firewall policy rule. - // Manually set different ruleTupleCount to match the realGCP log - if r.TargetResources != nil { - r.RuleTupleCount = PtrTo(int32(4)) - } else { - r.RuleTupleCount = PtrTo(int32(2)) - } - r.Kind = PtrTo("compute#firewallPolicyRule") - if r.Description == nil { - r.Description = PtrTo("") - } + mockFieldValuesForRule(r) obj.Rules = []*pb.FirewallPolicyRule{r} if err := s.storage.Update(ctx, fqn, obj); err != nil { @@ -397,17 +241,7 @@ func (s *FirewallPoliciesV1) PatchRule(ctx context.Context, req *pb.PatchRuleFir // update the rule r := req.GetFirewallPolicyRuleResource() r.Priority = PtrTo(*rule.Priority) - // RuleTupleCount is output only, calculation of the complexity of a single firewall policy rule. - // Manually set different ruleTupleCount to match the realGCP log - if r.TargetResources != nil { - r.RuleTupleCount = PtrTo(int32(4)) - } else { - r.RuleTupleCount = PtrTo(int32(2)) - } - r.Kind = PtrTo("compute#firewallPolicyRule") - if r.Description == nil { - r.Description = PtrTo("") - } + mockFieldValuesForRule(r) rules = append(rules, r) } else { rules = append(rules, rule) @@ -453,7 +287,14 @@ func (s *FirewallPoliciesV1) RemoveRule(ctx context.Context, req *pb.RemoveRuleF } } - obj.Rules = rules + if len(rules) == 0 { + // When the target policy has no rules, i.e. all the custom rules are deleted, + // we update the policy to add default rules to it. + populateDefaultRules(obj) + } else { + obj.Rules = rules + } + if err := s.storage.Update(ctx, fqn, obj); err != nil { return nil, err } @@ -482,7 +323,7 @@ func (n *firewallPolicyName) String() string { func (s *MockService) parseFirewallPolicyName(name string) (*firewallPolicyName, error) { tokens := strings.Split(name, "/") - if len(tokens) == 4 && tokens[2] == "firewallPolicies" { + if len(tokens) == 4 && tokens[0] == "locations" && tokens[1] == "global" && tokens[2] == "firewallPolicies" { name := &firewallPolicyName{ Name: tokens[3], } @@ -491,3 +332,90 @@ func (s *MockService) parseFirewallPolicyName(name string) (*firewallPolicyName, return nil, status.Errorf(codes.InvalidArgument, "name %q is not valid", name) } } + +func populateDefaultRules(obj *pb.FirewallPolicy) { + obj.Rules = []*pb.FirewallPolicyRule{ + { + Action: PtrTo("goto_next"), + Description: PtrTo("default egress rule ipv6"), + Direction: PtrTo("EGRESS"), + EnableLogging: PtrTo(false), + Kind: PtrTo("compute#firewallPolicyRule"), + Match: &pb.FirewallPolicyRuleMatcher{ + DestIpRanges: []string{"::/0"}, + Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ + { + IpProtocol: PtrTo("all"), + }, + }, + }, + Priority: PtrTo(int32(2147483644)), + RuleTupleCount: PtrTo(int32(2)), + }, + { + Action: PtrTo("goto_next"), + Description: PtrTo("default ingress rule ipv6"), + Direction: PtrTo("INGRESS"), + EnableLogging: PtrTo(false), + Kind: PtrTo("compute#firewallPolicyRule"), + Match: &pb.FirewallPolicyRuleMatcher{ + SrcIpRanges: []string{"::/0"}, + Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ + { + IpProtocol: PtrTo("all"), + }, + }, + }, + Priority: PtrTo(int32(2147483645)), + RuleTupleCount: PtrTo(int32(2)), + }, + { + Action: PtrTo("goto_next"), + Description: PtrTo("default egress rule"), + Direction: PtrTo("EGRESS"), + EnableLogging: PtrTo(false), + Kind: PtrTo("compute#firewallPolicyRule"), + Match: &pb.FirewallPolicyRuleMatcher{ + DestIpRanges: []string{"0.0.0.0/0"}, + Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ + { + IpProtocol: PtrTo("all"), + }, + }, + }, + Priority: PtrTo(int32(2147483646)), + RuleTupleCount: PtrTo(int32(2)), + }, + { + Action: PtrTo("goto_next"), + Description: PtrTo("default ingress rule"), + Direction: PtrTo("INGRESS"), + EnableLogging: PtrTo(false), + Kind: PtrTo("compute#firewallPolicyRule"), + Match: &pb.FirewallPolicyRuleMatcher{ + SrcIpRanges: []string{"0.0.0.0/0"}, + Layer4Configs: []*pb.FirewallPolicyRuleMatcherLayer4Config{ + { + IpProtocol: PtrTo("all"), + }, + }, + }, + Priority: PtrTo(int32(2147483647)), + RuleTupleCount: PtrTo(int32(2)), + }, + } +} + +func mockFieldValuesForRule(r *pb.FirewallPolicyRule) { + // RuleTupleCount is output only, calculation of the complexity of a single firewall policy rule. + // Manually set different ruleTupleCount to match the realGCP log + if r.TargetResources != nil { + r.RuleTupleCount = PtrTo(int32(4)) + } else { + r.RuleTupleCount = PtrTo(int32(2)) + } + r.Kind = PtrTo("compute#firewallPolicyRule") + if r.Description == nil { + r.Description = PtrTo("") + } +} diff --git a/mockgcp/mockcompute/operations.go b/mockgcp/mockcompute/operations.go index 292a0809bb..f6d15d776c 100644 --- a/mockgcp/mockcompute/operations.go +++ b/mockgcp/mockcompute/operations.go @@ -95,6 +95,16 @@ func (s *computeOperations) startLRO0(ctx context.Context, op *pb.Operation, fqn op.InsertTime = PtrTo(formatTime(now)) op.Id = PtrTo(uint64(nanos)) + // Specific to ComputeFirewallPolicy + // Remove targetId and targetLink when status is RUNNING to match realGCP operation + // ref: https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/2800/commits/32fdacd53d59c36626fce16f2b0125a8a455f3d6#r1783642429 + targetId := op.TargetId + targetLink := op.TargetLink + if *op.OperationType == "createFirewallPolicy" { + op.TargetId = nil + op.TargetLink = nil + } + if op.Progress == nil { op.Progress = PtrTo(int32(0)) } @@ -123,6 +133,13 @@ func (s *computeOperations) startLRO0(ctx context.Context, op *pb.Operation, fqn finished.Status = PtrTo(pb.Operation_DONE) finished.EndTime = PtrTo(formatTime(time.Now())) + // Specific to ComputeFirewallPolicy + // Add targetId and targetLink back when status is DONE to match realGCP operation + if *op.OperationType == "createFirewallPolicy" { + finished.TargetId = targetId + finished.TargetLink = targetLink + } + if err != nil { klog.Warningf("TODO: handle LRO error %v", err) } else { diff --git a/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computefirewallpolicy/_http.log b/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computefirewallpolicy/_http.log index 90bdc8063f..e2a8c5212a 100644 --- a/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computefirewallpolicy/_http.log +++ b/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computefirewallpolicy/_http.log @@ -29,8 +29,6 @@ X-Xss-Protection: 0 "selfLink": "https://www.googleapis.com/compute/v1/locations/global/operations/${operationID}", "startTime": "2024-04-01T12:34:56.123456Z", "status": "RUNNING", - "targetId": "${firewallPolicyId}", - "targetLink": "https://www.googleapis.com/compute/v1/locations/global/firewallPolicies/${firewallPolicyId}", "user": "user@example.com" } diff --git a/tests/e2e/unified_test.go b/tests/e2e/unified_test.go index ac4c82e697..fa90c6aba6 100644 --- a/tests/e2e/unified_test.go +++ b/tests/e2e/unified_test.go @@ -530,7 +530,6 @@ func runScenario(ctx context.Context, t *testing.T, testPause bool, fixture reso // Matches the mock ip address of Compute forwarding rule addReplacement("IPAddress", "8.8.8.8") addReplacement("pscConnectionId", "111111111111") - addReplacement("pscConnectionId", "111111111111") // Extract resource targetID numbers from compute operations for _, event := range events {