Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@gemmahou
gemmahou committed Oct 7, 2024
1 parent 9f74d67 commit 3b84785
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 177 deletions.
272 changes: 100 additions & 172 deletions mockgcp/mockcompute/firewallpoliciesv1.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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],
}
Expand All @@ -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("")
}
}
17 changes: 17 additions & 0 deletions mockgcp/mockcompute/operations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 != nil && *op.OperationType == "createFirewallPolicy" {
op.TargetId = nil
op.TargetLink = nil
}

if op.Progress == nil {
op.Progress = PtrTo(int32(0))
}
Expand Down Expand Up @@ -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 != nil && *op.OperationType == "createFirewallPolicy" {
finished.TargetId = targetId
finished.TargetLink = targetLink
}

if err != nil {
klog.Warningf("TODO: handle LRO error %v", err)
} else {
Expand Down
2 changes: 0 additions & 2 deletions pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computefirewallpolicy/_http.log
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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": "[email protected]"
}

Expand Down
2 changes: 0 additions & 2 deletions pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computefirewallpolicyrule/_http.log
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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": "[email protected]"
}

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/unified_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 3b84785

Please sign in to comment.