Merge pull request #121 from chenchun/api

Fix list ip api regression

CHANGELOG.md

@@ -8,6 +8,9 @@
 ## Bug fix
 
 - Fix veth device name conflict when multi network enabled [#115](https://github.com/tkestack/galaxy/pull/115)
+- Disable rp_filter in container [#119](https://github.com/tkestack/galaxy/pull/119)
+- Fix release IP API race condition [#120](https://github.com/tkestack/galaxy/pull/120)
+- Fix list ip API lists reserved IPs [#121] https://github.com/tkestack/galaxy/pull/121
 
 # v1.0.7
 

doc/galaxy-ipam-config.md

@@ -130,6 +130,7 @@ to stop reserving. But please don't delete any floatingip that is not created by
 # creating a floatingip crd object to reserve IP
 # please replace name with the IP you want to reserve.
 # don't delete ipType/reserved label or it won't work
+# start from v1.0.8, ipType: internalIP label is no longer needed
 
 apiVersion: galaxy.k8s.io/v1alpha1
 kind: FloatingIP

doc/portmapping.md

@@ -59,7 +59,7 @@ spec:
 ```
 
 Galaxy will map a random host port to each container port via iptables and write back the random port back as the value 
-of `tkestack.io/portmapping` annotation. The value is a json encoding of `[]Port`. `Port` is the following struct.
+of each pod's `tkestack.io/portmapping` annotation. The value is a json encoding of `[]Port`. `Port` is the following struct.
 
 ```
 type Port struct {

pkg/api/galaxy/constant/constant.go

@@ -77,7 +77,6 @@ const (
 	ResourceKind = "FloatingIP"
 	ApiVersion   = "galaxy.k8s.io/v1alpha1"
 	NameSpace    = "floating-ip"
-	IpType       = "ipType"
 )
 
 // CniArgs is the cni args in pod annotation

pkg/ipam/api/api.go

@@ -137,7 +137,12 @@ func (c *Controller) checkReleasableAndStatus(fip *FloatingIP) (releasable bool,
 			return
 		}
 	}
+	if fip.PodName == "" && fip.AppName == "" && fip.PoolName == "" {
+		return
+	}
 	if fip.PodName == "" {
+		releasable = true
+		status = "Deleted"
 		return
 	}
 	pod, err := c.podLister.Pods(fip.Namespace).Get(fip.PodName)

pkg/ipam/api/api_test.go

@@ -0,0 +1,97 @@
+/*
+ * Tencent is pleased to support the open source community by making TKEStack available.
+ *
+ * Copyright (C) 2012-2019 Tencent. All Rights Reserved.
+ *
+ * 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
+ *
+ * https://opensource.org/licenses/Apache-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 OF ANY KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package api
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes/fake"
+	"tkestack.io/galaxy/pkg/api/galaxy/constant"
+	"tkestack.io/galaxy/pkg/ipam/apis/galaxy/v1alpha1"
+	"tkestack.io/galaxy/pkg/ipam/floatingip"
+)
+
+func TestReserveFIP(t *testing.T) {
+	fip := &v1alpha1.FloatingIP{
+		TypeMeta:   metav1.TypeMeta{Kind: constant.ResourceKind, APIVersion: constant.ApiVersion},
+		ObjectMeta: metav1.ObjectMeta{Name: "10.49.27.216", Labels: map[string]string{constant.ReserveFIPLabel: ""}},
+		Spec:       v1alpha1.FloatingIPSpec{Key: "hello-xx"},
+	}
+	ipam, _ := floatingip.CreateTestIPAM(t, fip)
+	fips, err := listIPs("xx", ipam, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(fips) != 1 {
+		t.Fatal(fips)
+	}
+
+	if fips[0].labels == nil {
+		t.Fatal()
+	}
+	if _, ok := fips[0].labels[constant.ReserveFIPLabel]; !ok {
+		t.Fatal()
+	}
+	c := NewController(ipam, nil, nil)
+	releasable, _ := c.checkReleasableAndStatus(&fips[0])
+	if releasable {
+		t.Fatal()
+	}
+}
+
+func TestCheckReleasableAndStatus(t *testing.T) {
+	client := fake.NewSimpleClientset(&v1.Pod{
+		TypeMeta:   metav1.TypeMeta{Kind: "Pod", APIVersion: "v1"},
+		ObjectMeta: metav1.ObjectMeta{Name: "xx-1", Namespace: "demo"},
+		Spec:       v1.PodSpec{},
+		Status:     v1.PodStatus{Phase: v1.PodRunning},
+	})
+	stop := make(chan struct{})
+	factory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute)
+	lister := factory.Core().V1().Pods().Lister()
+	factory.Start(stop)
+	factory.WaitForCacheSync(stop)
+	c := NewController(nil, lister, nil)
+	for i, testCase := range []struct {
+		fip              *FloatingIP
+		expectReleasable bool
+		expectStatus     string
+	}{
+		{fip: &FloatingIP{PoolName: "pool1"}, expectReleasable: true, expectStatus: "Deleted"},
+		{fip: &FloatingIP{AppName: "dep1", AppType: "deployment"}, expectReleasable: true, expectStatus: "Deleted"},
+		{fip: &FloatingIP{}, expectReleasable: false},
+		{fip: &FloatingIP{AppName: "dep1", AppType: "statefulset", PodName: "xx-1", Namespace: "demo"},
+			expectReleasable: false, expectStatus: string(v1.PodRunning)},
+		{fip: &FloatingIP{AppName: "dep1", AppType: "statefulset", PodName: "xx-2", Namespace: "demo"},
+			expectReleasable: true, expectStatus: "Deleted"},
+	} {
+		t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
+			releasable, status := c.checkReleasableAndStatus(testCase.fip)
+			if releasable != testCase.expectReleasable {
+				t.Fatalf("expect %v, got %v", testCase.expectReleasable, releasable)
+			}
+			if status != testCase.expectStatus {
+				t.Fatalf("expect %v, got %v", testCase.expectStatus, status)
+			}
+		})
+	}
+}

pkg/ipam/floatingip/floatingip_test.go

@@ -22,6 +22,7 @@ import (
 	"net"
 	"testing"
 
+	"tkestack.io/galaxy/pkg/ipam/utils"
 	"tkestack.io/galaxy/pkg/utils/nets"
 )
 
@@ -155,3 +156,12 @@ func TestInsertRemoveIP(t *testing.T) {
 		t.Fatal(fip.IPRanges)
 	}
 }
+
+func TestTestConfig(t *testing.T) {
+	var conf struct {
+		Floatingips []*FloatingIPPool `json:"floatingips"`
+	}
+	if err := json.Unmarshal([]byte(utils.TestConfig), &conf); err != nil {
+		t.Fatal(err)
+	}
+}

pkg/ipam/floatingip/ipam_crd.go

@@ -34,26 +34,9 @@ import (
 	"tkestack.io/galaxy/pkg/utils/nets"
 )
 
-// Type is struct of IP type.
-type Type uint16
-
-const (
-	// InternalIp is enum of pod's internal IP.
-	InternalIp Type = iota
-)
-
-// String used to transform IP Type to string.
-func (t *Type) String() (string, error) {
-	if *t == InternalIp {
-		return "internalIP", nil
-	}
-	return "", fmt.Errorf("unknown ip type %v", *t)
-}
-
 type crdIpam struct {
 	FloatingIPs []*FloatingIPPool `json:"floatingips,omitempty"`
 	client      crd_clientset.Interface
-	ipType      Type
 	//caches for FloatingIP crd, both stores allocated FloatingIPs and unallocated FloatingIPs
 	cacheLock *sync.RWMutex
 	// key is ip string
@@ -64,10 +47,9 @@ type crdIpam struct {
 }
 
 // NewCrdIPAM init IPAM struct.
-func NewCrdIPAM(fipClient crd_clientset.Interface, ipType Type, informer crdInformer.FloatingIPInformer) IPAM {
+func NewCrdIPAM(fipClient crd_clientset.Interface, informer crdInformer.FloatingIPInformer) IPAM {
 	ipam := &crdIpam{
 		client:          fipClient,
-		ipType:          ipType,
 		cacheLock:       new(sync.RWMutex),
 		allocatedFIPs:   make(map[string]*FloatingIP),
 		unallocatedFIPs: make(map[string]*FloatingIP),
@@ -386,6 +368,7 @@ func (ci *crdIpam) ConfigurePool(floatIPs []*FloatingIPPool) error {
 				if err := tmpFip.unmarshalAttr(ip.Spec.Attribute); err != nil {
 					glog.Error(err)
 				}
+				tmpFip.Labels = ip.Labels
 				tmpCacheAllocated[ip.Name] = tmpFip
 				break
 			}

pkg/ipam/floatingip/ipam_crd_test.go

@@ -28,15 +28,12 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"tkestack.io/galaxy/pkg/api/galaxy/constant"
-	fakeGalaxyCli "tkestack.io/galaxy/pkg/ipam/client/clientset/versioned/fake"
-	crdInformer "tkestack.io/galaxy/pkg/ipam/client/informers/externalversions"
-	"tkestack.io/galaxy/pkg/ipam/utils"
 	"tkestack.io/galaxy/pkg/utils/nets"
 )
 
 const (
-	pod1CRD = `{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.205","creationTimestamp":null,"labels":{"ipType":"internalIP"}},"spec":{"key":"pod1","attribute":"{\"NodeName\":\"212\",\"Uid\":\"xx1\"}","policy":2,"updateTime":null}}`
-	pod2CRD = `{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.216","creationTimestamp":null,"labels":{"ipType":"internalIP"}},"spec":{"key":"pod2","attribute":"{\"NodeName\":\"333\",\"Uid\":\"xx2\"}","policy":1,"updateTime":null}}`
+	pod1CRD = `{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.205","creationTimestamp":null},"spec":{"key":"pod1","attribute":"{\"NodeName\":\"212\",\"Uid\":\"xx1\"}","policy":2,"updateTime":null}}`
+	pod2CRD = `{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.216","creationTimestamp":null},"spec":{"key":"pod2","attribute":"{\"NodeName\":\"333\",\"Uid\":\"xx2\"}","policy":1,"updateTime":null}}`
 
 	policy = constant.ReleasePolicyPodDelete
 )
@@ -64,25 +61,8 @@ var (
 	allNodeSubnet  = []*net.IPNet{node1IPNet, node2IPNet, node3IPNet, node4IPNet, node5IPNet1, node5IPNet2, node6IPNet1, node6IPNet2, node7IPNet}
 )
 
-func createIPAM(t *testing.T, objs ...runtime.Object) (*crdIpam, crdInformer.SharedInformerFactory) {
-	galaxyCli := fakeGalaxyCli.NewSimpleClientset(objs...)
-	crdInformerFactory := crdInformer.NewSharedInformerFactory(galaxyCli, 0)
-	fipInformer := crdInformerFactory.Galaxy().V1alpha1().FloatingIPs()
-	crdIPAM := NewCrdIPAM(galaxyCli, InternalIp, fipInformer).(*crdIpam)
-	var conf struct {
-		Floatingips []*FloatingIPPool `json:"floatingips"`
-	}
-	if err := json.Unmarshal([]byte(utils.TestConfig), &conf); err != nil {
-		t.Fatal(err)
-	}
-	if err := crdIPAM.ConfigurePool(conf.Floatingips); err != nil {
-		t.Fatal(err)
-	}
-	return crdIPAM, crdInformerFactory
-}
-
 func createTestCrdIPAM(t *testing.T, objs ...runtime.Object) *crdIpam {
-	crdIPAM, _ := createIPAM(t, objs...)
+	crdIPAM, _ := CreateTestIPAM(t, objs...)
 	return crdIPAM
 }
 
@@ -116,9 +96,6 @@ func TestConfigurePoolWithAllocatedIP(t *testing.T) {
 	}
 	fipCrd := newFIPCrd(expectFip.IP.String())
 	fipCrd.Labels[constant.ReserveFIPLabel] = ""
-	internalIP := InternalIp
-	ipType, _ := internalIP.String()
-	fipCrd.Labels[constant.IpType] = ipType
 	if err := assign(fipCrd, expectFip); err != nil {
 		t.Fatal(err)
 	}
@@ -133,6 +110,9 @@ func TestConfigurePoolWithAllocatedIP(t *testing.T) {
 	if fip.Key != expectFip.Key {
 		t.Fatal()
 	}
+	if _, ok := fip.Labels[constant.ReserveFIPLabel]; !ok {
+		t.Fatal("labels missing")
+	}
 }
 
 func TestAllocateSpecificIP(t *testing.T) {
@@ -202,8 +182,8 @@ func TestReserveIP(t *testing.T) {
 		}
 	}
 	if err := checkFIP(ipam,
-		`{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.205","creationTimestamp":null,"labels":{"ipType":"internalIP"}},"spec":{"key":"p1","attribute":"{\"NodeName\":\"node2\",\"Uid\":\"xx2\"}","policy":2,"updateTime":null}}`,
-		`{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.216","creationTimestamp":null,"labels":{"ipType":"internalIP"}},"spec":{"key":"p1","attribute":"{\"NodeName\":\"node2\",\"Uid\":\"xx2\"}","policy":2,"updateTime":null}}`); err != nil {
+		`{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.205","creationTimestamp":null},"spec":{"key":"p1","attribute":"{\"NodeName\":\"node2\",\"Uid\":\"xx2\"}","policy":2,"updateTime":null}}`,
+		`{"kind":"FloatingIP","apiVersion":"galaxy.k8s.io/v1alpha1","metadata":{"name":"10.49.27.216","creationTimestamp":null},"spec":{"key":"p1","attribute":"{\"NodeName\":\"node2\",\"Uid\":\"xx2\"}","policy":2,"updateTime":null}}`); err != nil {
 		t.Fatal(err)
 	}
 	// reserve again, should not succeed
@@ -686,3 +666,40 @@ func TestNodeSubnetsByKeyAndIPRanges(t *testing.T) {
 		}
 	}
 }
+
+func TestLabels(t *testing.T) {
+	fip := newFIPCrd("10.49.27.216")
+	fip.Labels[constant.ReserveFIPLabel] = ""
+	fip.Spec.Key = "hello-xx"
+	ipam, informerFactory := CreateTestIPAM(t, fip)
+	stop := make(chan struct{})
+	informerFactory.Start(stop)
+	informerFactory.WaitForCacheSync(stop)
+	defer func() { close(stop) }()
+	fips, err := ipam.ByKeyword("xx")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(fips) != 1 {
+		t.Fatal(fips)
+	}
+	if fips[0].Labels == nil {
+		t.Fatal()
+	}
+	if _, ok := fips[0].Labels[constant.ReserveFIPLabel]; !ok {
+		t.Fatal()
+	}
+	fipInfos, err := ipam.ByPrefix("hello")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(fipInfos) != 1 {
+		t.Fatal(fipInfos)
+	}
+	if fipInfos[0].Labels == nil {
+		t.Fatal()
+	}
+	if _, ok := fipInfos[0].Labels[constant.ReserveFIPLabel]; !ok {
+		t.Fatal()
+	}
+}

pkg/ipam/floatingip/store_crd.go

@@ -28,14 +28,7 @@ import (
 )
 
 func (ci *crdIpam) listFloatingIPs() (*v1alpha1.FloatingIPList, error) {
-	val, err := ci.ipType.String()
-	if err != nil {
-		return nil, err
-	}
-	listOpt := metav1.ListOptions{
-		LabelSelector: fmt.Sprintf("%s=%s", constant.IpType, val),
-	}
-	fips, err := ci.client.GalaxyV1alpha1().FloatingIPs().List(listOpt)
+	fips, err := ci.client.GalaxyV1alpha1().FloatingIPs().List(metav1.ListOptions{})
 	if err != nil {
 		return nil, err
 	}
@@ -146,10 +139,7 @@ func checkForReserved(obj interface{}) (*v1alpha1.FloatingIP, error) {
 }
 
 func (ci *crdIpam) newFIPCrd(name string) *v1alpha1.FloatingIP {
-	ipType, _ := ci.ipType.String()
-	crd := newFIPCrd(name)
-	crd.Labels[constant.IpType] = ipType
-	return crd
+	return newFIPCrd(name)
 }
 
 func newFIPCrd(name string) *v1alpha1.FloatingIP {

pkg/ipam/floatingip/store_crd_test.go

@@ -29,7 +29,7 @@ import (
 )
 
 func TestAddFloatingIPEventByUser(t *testing.T) {
-	ipam, informerFactory := createIPAM(t)
+	ipam, informerFactory := CreateTestIPAM(t)
 	stop := make(chan struct{})
 	go informerFactory.Start(stop)
 	defer func() { close(stop) }()
@@ -98,7 +98,7 @@ func TestAddFloatingIPEventByIPAM(t *testing.T) {
 }
 
 func TestDeleteFloatingIPEvent(t *testing.T) {
-	ipam, informerFactory := createIPAM(t)
+	ipam, informerFactory := CreateTestIPAM(t)
 	stop := make(chan struct{})
 	go informerFactory.Start(stop)
 	defer func() { close(stop) }()