diff --git a/cns/restserver/const.go b/cns/restserver/const.go index e9540eaf0..193ec6fb8 100644 --- a/cns/restserver/const.go +++ b/cns/restserver/const.go @@ -27,9 +27,10 @@ const ( NetworkJoinFailed = 24 NetworkContainerPublishFailed = 25 NetworkContainerUnpublishFailed = 26 - PrimaryCANotSpecified = 27 + InvalidPrimaryIPConfig = 27 PrimaryCANotSame = 28 InconsistentIPConfigState = 29 + InvalidSecndaryIPConfig = 30 UnexpectedError = 99 ) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 1c0723f11..d86ba8e18 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -35,9 +35,20 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C } // Validate PrimaryCA must never be empty - if req.PrimaryInterfaceIdentifier == "" { - logger.Errorf("[Azure CNS] Error. PrimaryCA is empty, NCId %s", req.NetworkContainerid) - return PrimaryCANotSpecified + err := validateIPConfig(req.IPConfiguration.IPSubnet) + if err != nil { + logger.Errorf("[Azure CNS] Error. PrimaryCA is invalid, NC Req: %v", req) + return InvalidPrimaryIPConfig + } + + // Validate SecondaryIPConfig + for ipId, ipconfig := range req.SecondaryIPConfigs { + // Validate Ipconfig + err := validateIPConfig(ipconfig.IPSubnet) + if err != nil { + logger.Errorf("[Azure CNS] Error. SecondaryIpConfig, Id:%s is invalid, NC Req: %v", ipId, req) + return InvalidSecndaryIPConfig + } } // Validate if state exists already diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go new file mode 100644 index 000000000..0a7a673d6 --- /dev/null +++ b/cns/restserver/internalapi_test.go @@ -0,0 +1,87 @@ +// Copyright 2020 Microsoft. All rights reserved. +// MIT License + +package restserver + +import ( +) + +// func TestIPAMExpectFailWhenAddingBadIPConfig(t *testing.T) { +// svc := getTestService() + +// var err error + +// // set state as already allocated +// state1, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Available, testPod1Info) + +// ipconfigs := map[string]IpConfigurationStatus{ +// state1.ID: state1, +// } + +// err = UpdatePodIpConfigState(svc, ipconfigs) +// if err != nil { +// t.Fatalf("Expected to not fail when good ipconfig is added") +// } + +// // create bad ipconfig +// state2, _ := NewPodStateWithOrchestratorContext("", 24, "", testNCID, cns.Available, testPod1Info) + +// ipconfigs2 := map[string]IpConfigurationStatus{ +// state2.ID: state2, +// } + +// // add a bad ipconfig +// err = UpdatePodIpConfigState(svc, ipconfigs2) +// if err == nil { +// t.Fatalf("Expected add to fail when bad ipconfig is added.") +// } + +// // ensure state remains untouched +// if len(svc.PodIPConfigState) != 1 { +// t.Fatalf("Expected bad ipconfig to not be added added.") +// } +// } + +// func TestIPAMExpectStateToNotChangeWhenChangingAllocatedToAvailable(t *testing.T) { +// svc := getTestService() +// // add two ipconfigs, one as available, the other as allocated +// state1, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Available, testPod1Info) +// state2, _ := NewPodStateWithOrchestratorContext(testIP2, 24, testPod2GUID, testNCID, cns.Allocated, testPod2Info) + +// ipconfigs := map[string]IpConfigurationStatus{ +// state1.ID: state1, +// state2.ID: state2, +// } + +// err := UpdatePodIpConfigState(svc, ipconfigs) +// if err != nil { +// t.Fatalf("Expected to not fail adding IP's to state: %+v", err) +// } + +// // create state2 again, but as available +// state2Available, _ := NewPodStateWithOrchestratorContext(testIP2, 24, testPod2GUID, testNCID, cns.Available, testPod2Info) + +// // add an available and allocated ipconfig +// ipconfigsTest := map[string]IpConfigurationStatus{ +// state1.ID: state1, +// state2.ID: state2Available, +// } + +// // expect to fail overwriting an allocated state with available +// err = UpdatePodIpConfigState(svc, ipconfigsTest) +// if err == nil { +// t.Fatalf("Expected to fail when overwriting an allocated state as available: %+v", err) +// } + +// // get allocated ipconfigs, should only be one from the inital call, and not 2 from the failed call +// availableIPconfigs := svc.GetAvailableIPConfigs() +// if len(availableIPconfigs) != 1 { +// t.Fatalf("More than expected available IP configs in state") +// } + +// // get allocated ipconfigs, should only be one from the inital call, and not 0 from the failed call +// allocatedIPconfigs := svc.GetAllocatedIPConfigs() +// if len(allocatedIPconfigs) != 1 { +// t.Fatalf("More than expected allocated IP configs in state") +// } +// } \ No newline at end of file diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 021cf0b15..da2979438 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -5,6 +5,7 @@ package restserver import ( "encoding/json" + "fmt" "reflect" "testing" @@ -77,6 +78,25 @@ func NewPodStateWithOrchestratorContext(ipaddress string, prefixLength uint8, id }, err } +// Test function to populate the IPConfigState +func UpdatePodIpConfigState(svc *HTTPRestService, ipconfigs map[string]IpConfigurationStatus) error { + // add ipconfigs to state + for ipId, ipconfig := range ipconfigs { + + svc.PodIPConfigState[ipId] = ipconfig + if ipconfig.State == cns.Allocated { + var podInfo cns.KubernetesPodInfo + + if err := json.Unmarshal(ipconfig.OrchestratorContext, &podInfo); err != nil { + return fmt.Errorf("Failed to add IPConfig to state: %+v with error: %v", ipconfig, err) + } + + svc.PodIPIDByOrchestratorContext[podInfo.GetOrchestratorContextKey()] = ipId + } + } + return nil +} + // Want first IP func TestIPAMGetAvailableIPConfig(t *testing.T) { svc := getTestService() @@ -85,7 +105,7 @@ func TestIPAMGetAvailableIPConfig(t *testing.T) { ipconfigs := map[string]IpConfigurationStatus{ testState.ID: testState, } - svc.addIPConfigsToState(ipconfigs) + UpdatePodIpConfigState(svc, ipconfigs) req := cns.GetIPConfigRequest{} b, _ := json.Marshal(testPod1Info) @@ -117,7 +137,7 @@ func TestIPAMGetNextAvailableIPConfig(t *testing.T) { state1.ID: state1, state2.ID: state2, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -143,10 +163,10 @@ func TestIPAMGetAlreadyAllocatedIPConfigForSamePod(t *testing.T) { // Add Allocated Pod IP to state testState, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Allocated, testPod1Info) - ipconfigs := map[string]secondaryIpConfigState{ + ipconfigs := map[string]IpConfigurationStatus{ testState.ID: testState, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -176,7 +196,7 @@ func TestIPAMAttemptToRequestIPNotFoundInPool(t *testing.T) { testState.ID: testState, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -184,7 +204,10 @@ func TestIPAMAttemptToRequestIPNotFoundInPool(t *testing.T) { req := cns.GetIPConfigRequest{} b, _ := json.Marshal(testPod2Info) req.OrchestratorContext = b - req.DesiredIPConfig = newSecondaryIPConfig(testIP2, 24) + req.DesiredIPConfig = cns.IPSubnet{ + IPAddress: testIP2, + PrefixLength: 24, + } _, err = requestIPConfigHelper(svc, req) if err == nil { @@ -201,7 +224,7 @@ func TestIPAMGetDesiredIPConfigWithSpecfiedIP(t *testing.T) { testState.ID: testState, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -209,7 +232,10 @@ func TestIPAMGetDesiredIPConfigWithSpecfiedIP(t *testing.T) { req := cns.GetIPConfigRequest{} b, _ := json.Marshal(testPod1Info) req.OrchestratorContext = b - req.DesiredIPConfig = newSecondaryIPConfig(testIP1, 24) + req.DesiredIPConfig = cns.IPSubnet{ + IPAddress: testIP1, + PrefixLength: 24, + } actualstate, err := requestIPConfigHelper(svc, req) if err != nil { @@ -232,7 +258,7 @@ func TestIPAMFailToGetDesiredIPConfigWithAlreadyAllocatedSpecfiedIP(t *testing.T ipconfigs := map[string]IpConfigurationStatus{ testState.ID: testState, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -241,7 +267,10 @@ func TestIPAMFailToGetDesiredIPConfigWithAlreadyAllocatedSpecfiedIP(t *testing.T req := cns.GetIPConfigRequest{} b, _ := json.Marshal(testPod2Info) req.OrchestratorContext = b - req.DesiredIPConfig = newSecondaryIPConfig(testIP1, 24) + req.DesiredIPConfig = cns.IPSubnet{ + IPAddress: testIP1, + PrefixLength: 24, + } _, err = requestIPConfigHelper(svc, req) if err == nil { @@ -260,7 +289,7 @@ func TestIPAMFailToGetIPWhenAllIPsAreAllocated(t *testing.T) { state1.ID: state1, state2.ID: state2, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -289,12 +318,15 @@ func TestIPAMRequestThenReleaseThenRequestAgain(t *testing.T) { state1.ID: state1, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } - desiredIPConfig := newSecondaryIPConfig(testIP1, 24) + desiredIPConfig := cns.IPSubnet{ + IPAddress: testIP1, + PrefixLength: 24, + } // Use TestPodInfo2 to request TestIP1, which has already been allocated req := cns.GetIPConfigRequest{} @@ -326,7 +358,7 @@ func TestIPAMRequestThenReleaseThenRequestAgain(t *testing.T) { desiredState, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Allocated, testPod1Info) // want first available Pod IP State - desiredState.IPConfig = desiredIPConfig + desiredState.IPConfig.IPSubnet = desiredIPConfig desiredState.OrchestratorContext = b if reflect.DeepEqual(desiredState, actualstate) != true { @@ -334,97 +366,6 @@ func TestIPAMRequestThenReleaseThenRequestAgain(t *testing.T) { } } -func TestIPAMExpectFailWhenAddingBadIPConfig(t *testing.T) { - svc := getTestService() - - var err error - - // set state as already allocated - state1, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Available, testPod1Info) - - ipconfigs := map[string]IpConfigurationStatus{ - state1.ID: state1, - } - - err = svc.addIPConfigsToState(ipconfigs) - if err != nil { - t.Fatalf("Expected to not fail when good ipconfig is added") - } - - // create bad ipconfig - state2, _ := NewPodStateWithOrchestratorContext("", 24, "", testNCID, cns.Available, testPod1Info) - - ipconfigs2 := map[string]IpConfigurationStatus{ - state2.ID: state2, - } - - // add a bad ipconfig - err = svc.addIPConfigsToState(ipconfigs2) - if err == nil { - t.Fatalf("Expected add to fail when bad ipconfig is added.") - } - - // ensure state remains untouched - if len(svc.PodIPConfigState) != 1 { - t.Fatalf("Expected bad ipconfig to not be added added.") - } -} - -func TestIPAMStateCleanUpWhenAddingGoodIPConfigWithBadOrchestratorContext(t *testing.T) { - svc := getTestService() - - var err error - - // add available state - state1, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Available, testPod1Info) - - ipconfigs := map[string]IpConfigurationStatus{ - state1.ID: state1, - } - - err = svc.addIPConfigsToState(ipconfigs) - if err != nil { - t.Fatalf("Expected to not fail when good ipconfig is added") - } - - // create a good ipconfig - state2, _ := NewPodStateWithOrchestratorContext(testIP2, 24, testPod2GUID, testNCID, cns.Allocated, testPod1Info) - - // make it bad with a bad orchestratorcontext and add to good ipconfig - b, err := json.Marshal("badstring") - state2.OrchestratorContext = b - - ipconfigs2 := map[string]IpConfigurationStatus{ - state2.ID: state2, - } - - err = svc.addIPConfigsToState(ipconfigs2) - if err == nil { - t.Fatalf("Expected add to fail when bad ipconfig is added.") - } - - // ensure state remains untouched - if len(svc.PodIPConfigState) != 1 { - t.Fatalf("Expected bad ipconfig to not be added added.") - } - - // ensure we can still get the available ipconfig - req := cns.GetIPConfigRequest{} - b, _ = json.Marshal(testPod1Info) - req.OrchestratorContext = b - actualstate, err := requestIPConfigHelper(svc, req) - if err != nil { - t.Fatalf("Expected IP retrieval to be nil: %v", err) - } - - desiredState, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Allocated, testPod1Info) - desiredState.OrchestratorContext = b - - if reflect.DeepEqual(desiredState, actualstate) != true { - t.Fatalf("Desired state not matching actual state, expected: %+v, actual: %+v", desiredState, actualstate) - } -} - func TestIPAMReleaseIPIdempotency(t *testing.T) { svc := getTestService() // set state as already allocated @@ -433,7 +374,7 @@ func TestIPAMReleaseIPIdempotency(t *testing.T) { state1.ID: state1, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } @@ -459,57 +400,13 @@ func TestIPAMAllocateIPIdempotency(t *testing.T) { state1.ID: state1, } - err := svc.addIPConfigsToState(ipconfigs) + err := UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } - err = svc.addIPConfigsToState(ipconfigs) + err = UpdatePodIpConfigState(svc, ipconfigs) if err != nil { t.Fatalf("Expected to not fail adding IP's to state: %+v", err) } } - -func TestIPAMExpectStateToNotChangeWhenChangingAllocatedToAvailable(t *testing.T) { - svc := getTestService() - // add two ipconfigs, one as available, the other as allocated - state1, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Available, testPod1Info) - state2, _ := NewPodStateWithOrchestratorContext(testIP2, 24, testPod2GUID, testNCID, cns.Allocated, testPod2Info) - - ipconfigs := map[string]IpConfigurationStatus{ - state1.ID: state1, - state2.ID: state2, - } - - err := svc.addIPConfigsToState(ipconfigs) - if err != nil { - t.Fatalf("Expected to not fail adding IP's to state: %+v", err) - } - - // create state2 again, but as available - state2Available, _ := NewPodStateWithOrchestratorContext(testIP2, 24, testPod2GUID, testNCID, cns.Available, testPod2Info) - - // add an available and allocated ipconfig - ipconfigsTest := map[string]IpConfigurationStatus{ - state1.ID: state1, - state2.ID: state2Available, - } - - // expect to fail overwriting an allocated state with available - err = svc.addIPConfigsToState(ipconfigsTest) - if err == nil { - t.Fatalf("Expected to fail when overwriting an allocated state as available: %+v", err) - } - - // get allocated ipconfigs, should only be one from the inital call, and not 2 from the failed call - availableIPconfigs := svc.GetAvailableIPConfigs() - if len(availableIPconfigs) != 1 { - t.Fatalf("More than expected available IP configs in state") - } - - // get allocated ipconfigs, should only be one from the inital call, and not 0 from the failed call - allocatedIPconfigs := svc.GetAllocatedIPConfigs() - if len(allocatedIPconfigs) != 1 { - t.Fatalf("More than expected allocated IP configs in state") - } -} diff --git a/cns/restserver/util.go b/cns/restserver/util.go index bbdb5cac8..74b31d000 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -232,13 +232,6 @@ func (service *HTTPRestService) updateIpConfigsStateUntransacted(req cns.CreateN func (service *HTTPRestService) addIPConfigStateUntransacted(ncId string, ipconfigs map[string]cns.SecondaryIPConfig) { // add ipconfigs to state for ipId, ipconfig := range ipconfigs { - err := validateIPConfig(ipId, ipconfig) - if err != nil { - // todo panic crash, we have already updated some state until this point, - // this validation is required before we process the request - logger.Errorf("PanicCrash: SecondaryIPConfig is not valid") - } - // if this IPConfig already exists in the map, then ignore as this is an idempotent state if _, exists := service.PodIPConfigState[ipId]; exists { continue @@ -260,15 +253,12 @@ func (service *HTTPRestService) addIPConfigStateUntransacted(ncId string, ipconf } // Todo: call this when request is received -func validateIPConfig(ipId string, ipconfig cns.SecondaryIPConfig) error { - if ipId == "" { - return fmt.Errorf("Failed to add IPConfig to state: empty IP ID") +func validateIPConfig(ipSubnet cns.IPSubnet) error { + if ipSubnet.IPAddress == "" { + return fmt.Errorf("Failed to add IPConfig to state: %+v, empty IPSubnet.IPAddress", ipSubnet) } - if ipconfig.IPSubnet.IPAddress == "" { - return fmt.Errorf("Failed to add IPConfig to state: %+v, empty IPSubnet.IPAddress", ipconfig) - } - if ipconfig.IPSubnet.PrefixLength == 0 { - return fmt.Errorf("Failed to add IPConfig to state: %+v, empty IPSubnet.PrefixLength", ipconfig) + if ipSubnet.PrefixLength == 0 { + return fmt.Errorf("Failed to add IPConfig to state: %+v, empty IPSubnet.PrefixLength", ipSubnet) } return nil }