api: Validate provided server group IDs

Don't blindly accept the requested server group ID.

Signed-off-by: Stephen Finucane <[email protected]>

Author: stephenfin

pkg/clients/compute.go

@@ -63,6 +63,7 @@ type ComputeClient interface {
 	DeleteAttachedInterface(serverID, portID string) error
 
 	ListServerGroups() ([]servergroups.ServerGroup, error)
+	GetServerGroup(serverGroupID string) (*servergroups.ServerGroup, error)
 }
 
 type computeClient struct{ client *gophercloud.ServiceClient }
@@ -162,6 +163,16 @@ func (c computeClient) ListServerGroups() ([]servergroups.ServerGroup, error) {
 	return servergroups.ExtractServerGroups(allPages)
 }
 
+func (c computeClient) GetServerGroup(serverGroupID string) (*servergroups.ServerGroup, error) {
+	var serverGroup servergroups.ServerGroup
+	mc := metrics.NewMetricPrometheusContext("server_group", "get")
+	err := servergroups.Get(c.client, serverGroupID).ExtractInto(&serverGroup)
+	if mc.ObserveRequestIgnoreNotFound(err) != nil {
+		return nil, err
+	}
+	return &serverGroup, nil
+}
+
 type computeErrorClient struct{ error }
 
 // NewComputeErrorClient returns a ComputeClient in which every method returns the given error.
@@ -204,3 +215,7 @@ func (e computeErrorClient) DeleteAttachedInterface(_, _ string) error {
 func (e computeErrorClient) ListServerGroups() ([]servergroups.ServerGroup, error) {
 	return nil, e.error
 }
+
+func (e computeErrorClient) GetServerGroup(_ string) (*servergroups.ServerGroup, error) {
+	return nil, e.error
+}

pkg/clients/mock/compute.go

@@ -604,11 +604,27 @@ func (s *Service) getImageID(imageUUID, imageName string) (string, error) {
 
 func (s *Service) getServerGroupID(serverGroupID string, serverGroupFilter *infrav1.ServerGroupFilter) (string, error) {
 	if serverGroupFilter == nil {
-		return serverGroupID, nil
+		if serverGroupID == "" {
+			return "", nil
+		}
+
+		// only fallback to the legacy value if a filter wasn't provided
+		serverGroupFilter = &infrav1.ServerGroupFilter{
+			ID: serverGroupID,
+		}
 	}
 
+	// if we have an ID, use that
 	if serverGroupFilter.ID != "" {
-		return serverGroupFilter.ID, nil
+		serverGroup, err := s.getComputeClient().GetServerGroup(serverGroupFilter.ID)
+		if err != nil {
+			return "", err
+		}
+		if serverGroupFilter.Name != "" && serverGroupFilter.Name != serverGroup.Name {
+			// this is super dumb and no sensible person will do this, but we should do the correct thing
+			return "", fmt.Errorf("found server with ID %s but name %s does not match", serverGroupFilter.ID, serverGroupFilter.Name)
+		}
+		return serverGroup.ID, nil
 	}
 
 	// otherwise fallback to looking up by name, which is slower

pkg/cloud/services/compute/instance.go

@@ -136,13 +136,19 @@ func TestService_getServerGroupID(t *testing.T) {
 			serverGroupID: serverGroupID1,
 			want:          serverGroupID1,
 			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.GetServerGroup(serverGroupID1).Return(
+					&servergroups.ServerGroup{ID: serverGroupID1, Name: "test-server-group"},
+					nil)
 			},
 			wantErr: false,
 		},
 		{
 			testName:          "Return server group ID from filter if only filter (with ID) given",
 			serverGroupFilter: &infrav1.ServerGroupFilter{ID: serverGroupID1},
 			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.GetServerGroup(serverGroupID1).Return(
+					&servergroups.ServerGroup{ID: serverGroupID1, Name: "test-server-group"},
+					nil)
 			},
 			want:    serverGroupID1,
 			wantErr: false,
@@ -382,7 +388,6 @@ func getDefaultInstanceSpec() *InstanceSpec {
 		},
 		ConfigDrive:    *pointer.Bool(true),
 		FailureDomain:  *pointer.String(failureDomain),
-		ServerGroupID:  serverGroupUUID,
 		Tags:           []string{"test-tag"},
 		SecurityGroups: []infrav1.SecurityGroupFilter{{ID: workerSecurityGroupUUID}},
 	}
@@ -421,9 +426,6 @@ func TestService_ReconcileInstance(t *testing.T) {
 					},
 				},
 			},
-			"os:scheduler_hints": map[string]interface{}{
-				"group": serverGroupUUID,
-			},
 		}
 	}
 
@@ -497,6 +499,12 @@ func TestService_ReconcileInstance(t *testing.T) {
 		computeRecorder.GetFlavorFromName(flavorName).Return(&f, nil)
 	}
 
+	// Expected calls when polling for server creation
+	expectGetServerGroup := func(computeRecorder *mock.MockComputeClientMockRecorder, serverGroupUUID string) {
+		serverGroup := servergroups.ServerGroup{ID: serverGroupUUID}
+		computeRecorder.GetServerGroup(serverGroupUUID).Return(&serverGroup, nil)
+	}
+
 	// Expected calls and custom match function for creating a server
 	expectCreateServer := func(computeRecorder *mock.MockComputeClientMockRecorder, expectedCreateOpts map[string]interface{}, wantError bool) {
 		// This nonsense is because ConfigDrive is a bool pointer, so we
@@ -576,6 +584,28 @@ func TestService_ReconcileInstance(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "Boot with server group",
+			getInstanceSpec: func() *InstanceSpec {
+				s := getDefaultInstanceSpec()
+				s.ServerGroupID = serverGroupUUID
+				return s
+			},
+			expect: func(r *recorders) {
+				expectUseExistingDefaultPort(r.network)
+				expectDefaultImageAndFlavor(r.compute, r.image)
+
+				createMap := getDefaultServerMap()
+				createMap["os:scheduler_hints"] = map[string]interface{}{
+					"group": serverGroupUUID,
+				}
+
+				expectGetServerGroup(r.compute, serverGroupUUID)
+				expectCreateServer(r.compute, createMap, false)
+				expectServerPollSuccess(r.compute)
+			},
+			wantErr: false,
+		},
 		{
 			name:            "Delete ports on server create error",
 			getInstanceSpec: getDefaultInstanceSpec,