Skip to content
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.

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

Should not pass in mount option of awscredsuri #755

Merged
merged 1 commit into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion examples/kubernetes/access_points/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,6 @@ as this could subject you to
if you configured your access point with `/ap1`, the above would mount to
`/ap1/my/subpath`.
- As with normal volume path, the `[Subpath]` must already exist prior to consuming
the volume from a pod.
the volume from a pod.

- `awscredsuri` mount option is not supported through efs-csi-driver as it's designed and used by ECS tasks.
11 changes: 6 additions & 5 deletions pkg/driver/config_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ import (
// (i.e. when the user does not need to durably store configs and thus does not mount host directories), an empty
// directory will be created at etcAmazonEfs.
//
// - legacyDir is the path to a config directory where previous versions of this driver may have written config
// files. In previous versions of this driver, a host path that was not writeable on Bottlerocket hosts
// was being used, so we introduce preferredDir.
// - legacyDir is the path to a config directory where previous versions of this driver may have written config
// files. In previous versions of this driver, a host path that was not writeable on Bottlerocket hosts
// was being used, so we introduce preferredDir.
//
// - preferredDir is the path to config directory that we will use so long as we do not find files in legacyDir.
//
// - etcAmazonEfs is the path where the symlink will be written. In practice, this will always be /etc/amazon/efs, but
// we take it as an input so the function can be tested.
// - etcAmazonEfs is the path where the symlink will be written. In practice, this will always be /etc/amazon/efs, but
// we take it as an input so the function can be tested.
//
// Examples:
// On a host that has EFS mounts created by an earlier version of this driver, InitConfigDir will detect a conf file in
// legacyDir and write a symlink at etcAmazonEfs pointing to legacyDir.
Expand Down
3 changes: 2 additions & 1 deletion pkg/driver/efs_watch_dog.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ func (w *execWatchdog) setup(efsClientSource string) error {
return nil
}

/**
/*
*
At image build time, static files installed by efs-utils in the config directory, i.e. CAs file, need
to be saved in another place so that the other stateful files created at runtime, i.e. private key for
client certificate, in the same config directory can be persisted to host with a host path volume.
Expand Down
6 changes: 3 additions & 3 deletions pkg/driver/gid_allocator.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func NewGidAllocator() GidAllocator {
}
}

//Retrieves the next available GID
// Retrieves the next available GID
func (g *GidAllocator) getNextGid(fsId string, gidMin, gidMax int) (int, error) {
g.mu.Lock()
defer g.mu.Unlock()
Expand Down Expand Up @@ -74,7 +74,7 @@ func (g *GidAllocator) releaseGid(fsId string, gid int) {
gidHeap.Push(gid)
}

//Creates an entry fsIdGidMap if fsId does not exist.
// Creates an entry fsIdGidMap if fsId does not exist.
func (g *GidAllocator) initFsId(fsId string, gidMin, gidMax int) {
h := initHeap(gidMin, gidMax)
heap.Init(h)
Expand All @@ -87,7 +87,7 @@ func (g *GidAllocator) removeFsId(fsId string) {
delete(g.fsIdGidMap, fsId)
}

//Initializes a heap inclusive of min & max
// Initializes a heap inclusive of min & max
func initHeap(min, max int) *IntHeap {
h := make(IntHeap, max-min+1)
val := min
Expand Down
18 changes: 12 additions & 6 deletions pkg/driver/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
switch strings.ToLower(k) {
//Deprecated
case "path":
klog.Warning("Use of path under volumeAttributes is depracated. This field will be removed in future release")
klog.Warning("Use of path under volumeAttributes is deprecated. This field will be removed in future release")
if !filepath.IsAbs(v) {
return nil, status.Errorf(codes.InvalidArgument, "Volume context property %q must be an absolute path", k)
}
Expand Down Expand Up @@ -165,6 +165,11 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
}
}

if f == "awscredsuri" {
Copy link

@Cappuccinuo Cappuccinuo Aug 12, 2022

Choose a reason for hiding this comment

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

  1. I would suggest to have a non supported options list, and check whether that option is in that list
  2. If that option is not supported, I think we should fail gracefully by just ignore that option + warn log, instead of returning an error. If we do want an exception here, I would recommend we update document somewhere and tell customer the option is not supported by csi-driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, logged this behavior, return nil but do not fail

Updated this behavior in ReadMe

klog.Warning("awscredsuri mount option is not supported by efs-csi-driver.")
return nil, nil
}

if !hasOption(mountOptions, f) {
mountOptions = append(mountOptions, f)
}
Expand Down Expand Up @@ -375,11 +380,12 @@ func (d *Driver) validateFStype(volCaps []*csi.VolumeCapability) error {

// parseVolumeId accepts a NodePublishVolumeRequest.VolumeId as a colon-delimited string of the
// form `{fileSystemID}:{mountPath}:{accessPointID}`.
// - The `{fileSystemID}` is required, and expected to be of the form `fs-...`.
// - The other two fields are optional -- they may be empty or omitted entirely. For example,
// `fs-abcd1234::`, `fs-abcd1234:`, and `fs-abcd1234` are equivalent.
// - The `{mountPath}`, if specified, is not required to be absolute.
// - The `{accessPointID}` is expected to be of the form `fsap-...`.
// - The `{fileSystemID}` is required, and expected to be of the form `fs-...`.
// - The other two fields are optional -- they may be empty or omitted entirely. For example,
// `fs-abcd1234::`, `fs-abcd1234:`, and `fs-abcd1234` are equivalent.
// - The `{mountPath}`, if specified, is not required to be absolute.
// - The `{accessPointID}` is expected to be of the form `fsap-...`.
//
// parseVolumeId returns the parsed values, of which `subpath` and `apid` may be empty; and an
// error, which will be a `status.Error` with `codes.InvalidArgument`, or `nil` if the `volumeId`
// was parsed successfully.
Expand Down
4 changes: 3 additions & 1 deletion pkg/driver/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ Copyright 2019 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

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.
Expand Down
4 changes: 3 additions & 1 deletion pkg/driver/version_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ Copyright 2019 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

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.
Expand Down
4 changes: 3 additions & 1 deletion pkg/driver/vol_statter.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ Copyright 2019 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

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.
Expand Down