Fix an issue where clusteractuator.Delete(...) would never succeed if the firewall rules it is attempting to delete do not exist. Move GCEClientComputeServieMock to an appropriate shared location. #378
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| Copyright 2018 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package google_test | ||
|
|
||
| import compute "google.golang.org/api/compute/v1" | ||
|
|
||
| type GCEClientComputeServiceMock struct { | ||
| mockImagesGet func(project string, image string) (*compute.Image, error) | ||
| mockImagesGetFromFamily func(project string, family string) (*compute.Image, error) | ||
| mockInstancesDelete func(project string, zone string, targetInstance string) (*compute.Operation, error) | ||
| mockInstancesGet func(project string, zone string, instance string) (*compute.Instance, error) | ||
| mockInstancesInsert func(project string, zone string, instance *compute.Instance) (*compute.Operation, error) | ||
| mockZoneOperationsGet func(project string, zone string, operation string) (*compute.Operation, error) | ||
| mockGlobalOperationsGet func(project string, operation string) (*compute.Operation, error) | ||
| mockFirewallsGet func(project string) (*compute.FirewallList, error) | ||
| mockFirewallsInsert func(project string, firewallRule *compute.Firewall) (*compute.Operation, error) | ||
| mockFirewallsDelete func(project string, name string) (*compute.Operation, error) | ||
| mockWaitForOperation func(project string, op *compute.Operation) error | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) ImagesGet(project string, image string) (*compute.Image, error) { | ||
| if c.mockImagesGet == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockImagesGet(project, image) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) ImagesGetFromFamily(project string, family string) (*compute.Image, error) { | ||
| if c.mockImagesGetFromFamily == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockImagesGetFromFamily(project, family) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) InstancesDelete(project string, zone string, targetInstance string) (*compute.Operation, error) { | ||
| if c.mockInstancesDelete == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockInstancesDelete(project, zone, targetInstance) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) InstancesGet(project string, zone string, instance string) (*compute.Instance, error) { | ||
| if c.mockInstancesGet == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockInstancesGet(project, zone, instance) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) InstancesInsert(project string, zone string, instance *compute.Instance) (*compute.Operation, error) { | ||
| if c.mockInstancesInsert == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockInstancesInsert(project, zone, instance) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) ZoneOperationsGet(project string, zone string, operation string) (*compute.Operation, error) { | ||
| if c.mockZoneOperationsGet == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockZoneOperationsGet(project, zone, operation) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) GlobalOperationsGet(project string, operation string) (*compute.Operation, error) { | ||
| if c.mockGlobalOperationsGet == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockGlobalOperationsGet(project, operation) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) FirewallsGet(project string) (*compute.FirewallList, error) { | ||
| if c.mockFirewallsGet == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockFirewallsGet(project) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) FirewallsInsert(project string, firewallRule *compute.Firewall) (*compute.Operation, error) { | ||
| if c.mockFirewallsInsert == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockFirewallsInsert(project, firewallRule) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) FirewallsDelete(project string, name string) (*compute.Operation, error) { | ||
| if c.mockFirewallsDelete == nil { | ||
| return nil, nil | ||
| } | ||
| return c.mockFirewallsDelete(project, name) | ||
| } | ||
|
|
||
| func (c *GCEClientComputeServiceMock) WaitForOperation(project string, op *compute.Operation) error { | ||
| if c.mockWaitForOperation == nil { | ||
| return nil | ||
| } | ||
| return c.mockWaitForOperation(project, op) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| /* | ||
| Copyright 2018 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package google_test | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| "sigs.k8s.io/cluster-api/cloud/google" | ||
| "sigs.k8s.io/cluster-api/pkg/controller/cluster" | ||
| "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1" | ||
|
|
||
| compute "google.golang.org/api/compute/v1" | ||
| ) | ||
|
|
||
| func TestDelete(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| firewallsDeleteOpResult *compute.Operation | ||
| firewallsDeleteErr error | ||
| expectedErrorMessage string | ||
| }{ | ||
| {"successs", &compute.Operation{}, nil, ""}, | ||
| {"error", nil, fmt.Errorf("random error"), "error deleting firewall rule for internal cluster traffic: error deleting firewall rule: random error"}, | ||
| {"404/NotFound error should succeed", nil, errors.NewNotFound(v1alpha1.Resource("cluster"), "404 not found"), ""}, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| computeServiceMock := GCEClientComputeServiceMock{ | ||
| mockFirewallsDelete: func(project string, name string) (*compute.Operation, error) { | ||
| return tc.firewallsDeleteOpResult, tc.firewallsDeleteErr | ||
| }, | ||
| } | ||
| params := google.ClusterActuatorParams{ComputeService: &computeServiceMock} | ||
| actuator := newClusterActuator(t, params) | ||
| cluster := newDefaultClusterFixture(t) | ||
| err := actuator.Delete(cluster) | ||
| if err != nil || tc.expectedErrorMessage != "" { | ||
| if err == nil { | ||
| t.Errorf("unexpected error message") | ||
| } else if err.Error() != tc.expectedErrorMessage { | ||
| t.Errorf("error mismatch: got '%v', want '%v'", err, tc.expectedErrorMessage) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func newClusterActuator(t *testing.T, params google.ClusterActuatorParams) cluster.Actuator { | ||
| t.Helper() | ||
| actuator, err := google.NewClusterActuator(params) | ||
|
There was a problem hiding this comment. Since this is just making one fn call and checking for an error is there a reason you put it into a separate fn instead of putting it inline? There was a problem hiding this comment. Just did this out of force of habit since it is useful once there is more than one test (they can all make a one line function call instead of 3 lines of function call & error handling). |
||
| if err != nil { | ||
| t.Fatalf("error creating cluster actuator: %v", err) | ||
| } | ||
| return actuator | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
You need to fix this copyright now that we have the verify boilerplate script (it needs some blank lines).
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.
Fixed and added the copyright to the clientcomputeservice_test.go as well where it was missing.