Skip to content

Allow to label PD disk with k8s cluster ID #693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Allow to label PD disk with k8s cluster ID #693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cmd/gce-pd-csi-driver/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"k8s.io/klog"

cliflag "k8s.io/component-base/cli/flag"
gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute"
metadataservice "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/metadata"
driver "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-pd-csi-driver"
Expand All @@ -35,6 +36,7 @@ var (
endpoint = flag.String("endpoint", "unix:/tmp/csi.sock", "CSI endpoint")
runControllerService = flag.Bool("run-controller-service", true, "If set to false then the CSI driver does not activate its controller service (default: true)")
runNodeService = flag.Bool("run-node-service", true, "If set to false then the CSI driver does not activate its node service (default: true)")
extraVolumeLabels map[string]string
version string
)

Expand All @@ -53,6 +55,7 @@ func init() {
}

func main() {
flag.Var(cliflag.NewMapStringString(&extraVolumeLabels), "extra-labels", "Extra labels to attach to each PD created. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
flag.Parse()
rand.Seed(time.Now().UnixNano())
handle()
Expand Down Expand Up @@ -104,7 +107,7 @@ func handle() {
nodeServer = driver.NewNodeServer(gceDriver, mounter, deviceUtils, meta, statter)
}

err = gceDriver.SetupGCEDriver(driverName, version, identityServer, controllerServer, nodeServer)
err = gceDriver.SetupGCEDriver(driverName, version, extraVolumeLabels, identityServer, controllerServer, nodeServer)
if err != nil {
klog.Fatalf("Failed to initialize GCE CSI Driver: %v", err)
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ require (
gopkg.in/warnings.v0 v0.1.2 // indirect
k8s.io/apimachinery v0.18.0
k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible
k8s.io/component-base v0.18.0
k8s.io/klog v1.0.0
k8s.io/kubernetes v1.18.0
k8s.io/test-infra v0.0.0-20200115230622-70a5174aa78d
Expand Down
4 changes: 1 addition & 3 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,9 +476,6 @@ github.com/kr/pty v1.1.3/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
github.com/kr/pty v1.1.5/go.mod h1:9r2w37qlBe7rQ6e1fg1S/9xpWHSnaqNdHD3WcMdbPDA=
github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
github.com/kubernetes-csi/csi-proxy v0.2.2 h1:LqablYFEGw7FYBjwoh5TeXFzlcx8C+YQjKfGy6fFWJs=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to remove the csi-proxy? It's used by windows. At any rate we should clean this up in a separate PR.

@jingxu97 We seem to have both 0.2.1 and 0.2.2 of the proxy here, not sure what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At any rate this PR should not be changing go.mod (sorry I wasn't clear on that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above is a result of go mod tidy. I use k8s.io/component-base/cli/flag for the CLI parsing and it was not vendored yet so I actually have to update go.mod for this patch. I think the removal of the (older) csi-proxy v0.2.1 is correct and the v0.2.2 is listed twice in go.sum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fair enough. I'm not going to argue with go mod tidy.

github.com/kubernetes-csi/csi-proxy/client v0.2.1 h1:n21d2U9HvgQ6jfJayafRv8kXXtLvnRNEqoD0mQNucKc=
github.com/kubernetes-csi/csi-proxy/client v0.2.1/go.mod h1:6ptQQmti5QHwBxSsh8Cy00oGdogj0JXewFnu8FFjgOs=
github.com/kubernetes-csi/csi-proxy/client v0.2.2 h1:VpMddHnbYA1oBeU5nrisdyrpOAAT0HqME7fsTi6BG2w=
github.com/kubernetes-csi/csi-proxy/client v0.2.2/go.mod h1:6ptQQmti5QHwBxSsh8Cy00oGdogj0JXewFnu8FFjgOs=
github.com/kubernetes-csi/csi-test/v3 v3.0.0 h1:mVsfA4J67uNm8fdF/Pr84oMqL92qjIhjWbEUH8zv1fU=
Expand Down Expand Up @@ -1149,6 +1146,7 @@ k8s.io/client-go v0.18.0/go.mod h1:uQSYDYs4WhVZ9i6AIoEZuwUggLVEF64HOD37boKAtF8=
k8s.io/cloud-provider v0.18.0/go.mod h1:ZBq1FhoJ+XoQ8JYBYoyx81LS3JV0RAW/UmHf/6w9E6k=
k8s.io/cluster-bootstrap v0.18.0/go.mod h1:xSe+bOZ3asS/ciT91ESQYGhjOql43aBETfvbCzNvad8=
k8s.io/code-generator v0.18.0/go.mod h1:+UHX5rSbxmR8kzS+FAv7um6dtYrZokQvjHpDSYRVkTc=
k8s.io/component-base v0.18.0 h1:I+lP0fNfsEdTDpHaL61bCAqTZLoiWjEEP304Mo5ZQgE=
k8s.io/component-base v0.18.0/go.mod h1:u3BCg0z1uskkzrnAKFzulmYaEpZF7XC9Pf/uFyb1v2c=
k8s.io/cri-api v0.18.0/go.mod h1:OJtpjDvfsKoLGhvcc0qfygved0S0dGX56IJzPbqTG1s=
k8s.io/csi-translation-lib v0.18.0/go.mod h1:iF8TE4ACSaPqN1qxmiIjvcU1A8VgkOrpcFGD7Z0hVu0=
Expand Down
9 changes: 8 additions & 1 deletion pkg/common/parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,20 @@ type DiskParameters struct {
// Values: {map[string]string}
// Default: ""
Tags map[string]string
// Values: {map[string]string}
// Default: ""
Labels map[string]string
}

// ExtractAndDefaultParameters will take the relevant parameters from a map and
// put them into a well defined struct making sure to default unspecified fields
func ExtractAndDefaultParameters(parameters map[string]string, driverName string) (DiskParameters, error) {
func ExtractAndDefaultParameters(parameters map[string]string, driverName string, extraVolumeLabels map[string]string) (DiskParameters, error) {
p := DiskParameters{
DiskType: "pd-standard", // Default
ReplicationType: replicationTypeNone, // Default
DiskEncryptionKMSKey: "", // Default
Tags: make(map[string]string), // Default
Labels: make(map[string]string), // Default
}

for k, v := range parameters {
Expand Down Expand Up @@ -96,5 +100,8 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
if len(p.Tags) > 0 {
p.Tags[tagKeyCreatedBy] = driverName
}
for k, v := range extraVolumeLabels {
p.Labels[k] = v
}
return p, nil
}
28 changes: 27 additions & 1 deletion pkg/common/parameters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,79 +25,105 @@ func TestExtractAndDefaultParameters(t *testing.T) {
tests := []struct {
name string
parameters map[string]string
labels map[string]string
expectParams DiskParameters
expectErr bool
}{
{
name: "defaults",
parameters: map[string]string{},
labels: map[string]string{},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "",
Tags: make(map[string]string),
Labels: make(map[string]string),
},
},
{
name: "specified empties",
parameters: map[string]string{ParameterKeyType: "", ParameterKeyReplicationType: "", ParameterKeyDiskEncryptionKmsKey: ""},
labels: map[string]string{},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "",
Tags: make(map[string]string),
Labels: make(map[string]string),
},
},
{
name: "random keys",
parameters: map[string]string{ParameterKeyType: "", "foo": "", ParameterKeyDiskEncryptionKmsKey: ""},
labels: map[string]string{},
expectErr: true,
},
{
name: "real values",
parameters: map[string]string{ParameterKeyType: "pd-ssd", ParameterKeyReplicationType: "regional-pd", ParameterKeyDiskEncryptionKmsKey: "foo/key"},
labels: map[string]string{},
expectParams: DiskParameters{
DiskType: "pd-ssd",
ReplicationType: "regional-pd",
DiskEncryptionKMSKey: "foo/key",
Tags: make(map[string]string),
Labels: make(map[string]string),
},
},
{
name: "real values, checking balanced pd",
parameters: map[string]string{ParameterKeyType: "pd-balanced", ParameterKeyReplicationType: "regional-pd", ParameterKeyDiskEncryptionKmsKey: "foo/key"},
labels: map[string]string{},
expectParams: DiskParameters{
DiskType: "pd-balanced",
ReplicationType: "regional-pd",
DiskEncryptionKMSKey: "foo/key",
Tags: make(map[string]string),
Labels: make(map[string]string),
},
},
{
name: "partial spec",
parameters: map[string]string{ParameterKeyDiskEncryptionKmsKey: "foo/key"},
labels: map[string]string{},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "foo/key",
Tags: make(map[string]string),
Labels: make(map[string]string),
},
},
{
name: "tags",
parameters: map[string]string{ParameterKeyPVCName: "testPVCName", ParameterKeyPVCNamespace: "testPVCNamespace", ParameterKeyPVName: "testPVName"},
labels: map[string]string{},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "",
Tags: map[string]string{tagKeyCreatedForClaimName: "testPVCName", tagKeyCreatedForClaimNamespace: "testPVCNamespace", tagKeyCreatedForVolumeName: "testPVName", tagKeyCreatedBy: "testDriver"},
Labels: make(map[string]string),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I miss it or shouldn't there be a test case for the new labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code since ExtractAndDefaultParameter now takes more parameters and to have the parameteres structure complete. There's nothing much to test with the labels though -- they're only being copied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they're only we being copied we should test that---these also serve as regression tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the test in the latest commit.

},
},
{
name: "labels",
parameters: map[string]string{},
labels: map[string]string{"label-1": "label-value-1", "label-2": "label-value-2"},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "",
Tags: map[string]string{},
Labels: map[string]string{"label-1": "label-value-1", "label-2": "label-value-2"},
},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
p, err := ExtractAndDefaultParameters(tc.parameters, "testDriver")
p, err := ExtractAndDefaultParameters(tc.parameters, "testDriver", tc.labels)
if gotErr := err != nil; gotErr != tc.expectErr {
t.Fatalf("ExtractAndDefaultParameters(%+v) = %v; expectedErr: %v", tc.parameters, err, tc.expectErr)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/gce-cloud-provider/compute/gce-compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ func (cloud *CloudProvider) insertZonalDisk(
SizeGb: common.BytesToGbRoundUp(capBytes),
Description: description,
Type: cloud.GetDiskTypeURI(volKey, params.DiskType),
Labels: params.Labels,
}

if snapshotID != "" {
Expand Down
4 changes: 2 additions & 2 deletions pkg/gce-pd-csi-driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre

// Apply Parameters (case-insensitive). We leave validation of
// the values to the cloud provider.
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name)
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err)
}
Expand Down Expand Up @@ -475,7 +475,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
}

// Validate the disk parameters match the disk we GET
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name)
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err)
}
Expand Down
8 changes: 5 additions & 3 deletions pkg/gce-pd-csi-driver/gce-pd-driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ import (
)

type GCEDriver struct {
name string
vendorVersion string
name string
vendorVersion string
extraVolumeLabels map[string]string

ids *GCEIdentityServer
ns *GCENodeServer
Expand All @@ -45,7 +46,7 @@ func GetGCEDriver() *GCEDriver {
return &GCEDriver{}
}

func (gceDriver *GCEDriver) SetupGCEDriver(name, vendorVersion string, identityServer *GCEIdentityServer, controllerServer *GCEControllerServer, nodeServer *GCENodeServer) error {
func (gceDriver *GCEDriver) SetupGCEDriver(name, vendorVersion string, extraVolumeLabels map[string]string, identityServer *GCEIdentityServer, controllerServer *GCEControllerServer, nodeServer *GCENodeServer) error {
if name == "" {
return fmt.Errorf("Driver name missing")
}
Expand Down Expand Up @@ -77,6 +78,7 @@ func (gceDriver *GCEDriver) SetupGCEDriver(name, vendorVersion string, identityS

gceDriver.name = name
gceDriver.vendorVersion = vendorVersion
gceDriver.extraVolumeLabels = extraVolumeLabels
gceDriver.ids = identityServer
gceDriver.cs = controllerServer
gceDriver.ns = nodeServer
Expand Down
2 changes: 1 addition & 1 deletion pkg/gce-pd-csi-driver/gce-pd-driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func initGCEDriverWithCloudProvider(t *testing.T, cloudProvider gce.GCECompute)
vendorVersion := "test-vendor"
gceDriver := GetGCEDriver()
controllerServer := NewControllerServer(gceDriver, cloudProvider)
err := gceDriver.SetupGCEDriver(driver, vendorVersion, nil, controllerServer, nil)
err := gceDriver.SetupGCEDriver(driver, vendorVersion, nil, nil, controllerServer, nil)
if err != nil {
t.Fatalf("Failed to setup GCE Driver: %v", err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/gce-pd-csi-driver/identity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestGetPluginInfo(t *testing.T) {
vendorVersion := "test-vendor"
gceDriver := GetGCEDriver()
identityServer := NewIdentityServer(gceDriver)
err := gceDriver.SetupGCEDriver(driver, vendorVersion, identityServer, nil, nil)
err := gceDriver.SetupGCEDriver(driver, vendorVersion, nil, identityServer, nil, nil)
if err != nil {
t.Fatalf("Failed to setup GCE Driver: %v", err)
}
Expand All @@ -49,7 +49,7 @@ func TestGetPluginInfo(t *testing.T) {
func TestGetPluginCapabilities(t *testing.T) {
gceDriver := GetGCEDriver()
identityServer := NewIdentityServer(gceDriver)
err := gceDriver.SetupGCEDriver(driver, "test-vendor", identityServer, nil, nil)
err := gceDriver.SetupGCEDriver(driver, "test-vendor", nil, identityServer, nil, nil)
if err != nil {
t.Fatalf("Failed to setup GCE Driver: %v", err)
}
Expand Down Expand Up @@ -82,7 +82,7 @@ func TestGetPluginCapabilities(t *testing.T) {
func TestProbe(t *testing.T) {
gceDriver := GetGCEDriver()
identityServer := NewIdentityServer(gceDriver)
err := gceDriver.SetupGCEDriver(driver, "test-vendor", identityServer, nil, nil)
err := gceDriver.SetupGCEDriver(driver, "test-vendor", nil, identityServer, nil, nil)
if err != nil {
t.Fatalf("Failed to setup GCE Driver: %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/gce-pd-csi-driver/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func getTestGCEDriverWithCustomMounter(t *testing.T, mounter *mount.SafeFormatAn
func getCustomTestGCEDriver(t *testing.T, mounter *mount.SafeFormatAndMount, deviceUtils mountmanager.DeviceUtils, metaService metadataservice.MetadataService) *GCEDriver {
gceDriver := GetGCEDriver()
nodeServer := NewNodeServer(gceDriver, mounter, deviceUtils, metaService, mountmanager.NewFakeStatter(mounter))
err := gceDriver.SetupGCEDriver(driver, "test-vendor", nil, nil, nodeServer)
err := gceDriver.SetupGCEDriver(driver, "test-vendor", nil, nil, nil, nodeServer)
if err != nil {
t.Fatalf("Failed to setup GCE Driver: %v", err)
}
Expand All @@ -54,7 +54,7 @@ func getTestBlockingGCEDriver(t *testing.T, readyToExecute chan chan struct{}) *
gceDriver := GetGCEDriver()
mounter := mountmanager.NewFakeSafeBlockingMounter(readyToExecute)
nodeServer := NewNodeServer(gceDriver, mounter, mountmanager.NewFakeDeviceUtils(), metadataservice.NewFakeService(), mountmanager.NewFakeStatter(mounter))
err := gceDriver.SetupGCEDriver(driver, "test-vendor", nil, nil, nodeServer)
err := gceDriver.SetupGCEDriver(driver, "test-vendor", nil, nil, nil, nodeServer)
if err != nil {
t.Fatalf("Failed to setup GCE Driver: %v", err)
}
Expand Down
3 changes: 2 additions & 1 deletion test/sanity/sanity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func TestSanity(t *testing.T) {
zone := "country-region-zone"
vendorVersion := "test-version"
tmpDir := "/tmp/csi"
extraLabels := map[string]string{"test-label": "test-label-value"}
endpoint := fmt.Sprintf("unix:%s/csi.sock", tmpDir)
mountPath := path.Join(tmpDir, "mount")
stagePath := path.Join(tmpDir, "stage")
Expand All @@ -57,7 +58,7 @@ func TestSanity(t *testing.T) {
identityServer := driver.NewIdentityServer(gceDriver)
controllerServer := driver.NewControllerServer(gceDriver, cloudProvider)
nodeServer := driver.NewNodeServer(gceDriver, mounter, deviceUtils, metadataservice.NewFakeService(), mountmanager.NewFakeStatter(mounter))
err = gceDriver.SetupGCEDriver(driverName, vendorVersion, identityServer, controllerServer, nodeServer)
err = gceDriver.SetupGCEDriver(driverName, vendorVersion, extraLabels, identityServer, controllerServer, nodeServer)
if err != nil {
t.Fatalf("Failed to initialize GCE CSI Driver: %v", err)
}
Expand Down
Loading