Skip to content

fix: honour optional on ConfigMap and Secret volumes #590

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

Open
NopeCharon wants to merge 1 commit into
base: main
Choose a base branch
from
+120 −7
Open

fix: honour optional on ConfigMap and Secret volumes #590

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
37 changes: 30 additions & 7 deletions pkg/virtualkubelet/execute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"go.opentelemetry.io/otel/attribute"
trace "go.opentelemetry.io/otel/trace"
v1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
Expand Down Expand Up @@ -803,9 +804,15 @@ func remoteExecutionHandleProjectedSource(
// With InterLink, the Kubernetes internal network is not accessible so this default ca.crt is probably useless.
log.G(ctx).Warning("using default Kubernetes API kube-root-ca.crt (no override found), but the default one might not be compatible with the subject: ", p.config.KubernetesAPIAddr)
}
optional := source.ConfigMap.Optional != nil && *source.ConfigMap.Optional
// Fetch the ConfigMap once, then iterate over items (or all keys if no items are specified).
cfgmap, err := p.clientSet.CoreV1().ConfigMaps(pod.Namespace).Get(ctx, source.ConfigMap.Name, metav1.GetOptions{})
if err != nil {
if optional && apierrors.IsNotFound(err) {
// Like the kubelet, a missing optional ConfigMap projects no keys.
log.G(ctx).Infof("optional ConfigMap %s of pod %s not found, projecting no keys from it", source.ConfigMap.Name, pod.Name)
return nil
}
return fmt.Errorf("error during retrieval of ConfigMap %s error: %w", source.ConfigMap.Name, err)
}
if len(source.ConfigMap.Items) == 0 {
Expand All @@ -817,7 +824,7 @@ func remoteExecutionHandleProjectedSource(
for _, item := range source.ConfigMap.Items {
if value, ok := cfgmap.Data[item.Key]; ok {
projectedVolume.Data[item.Path] = value
} else {
} else if !optional {
return fmt.Errorf("error during retrieval of key %s of (existing) ConfigMap %s", item.Key, source.ConfigMap.Name)
}
}
Expand Down Expand Up @@ -931,13 +938,21 @@ func remoteExecutionHandleVolumes(ctx context.Context, p *Provider, pod *v1.Pod,
switch {
case volume.ConfigMap != nil:
cfgmap, err := p.clientSet.CoreV1().ConfigMaps(pod.Namespace).Get(ctx, volume.ConfigMap.Name, metav1.GetOptions{})
if err != nil {
switch {
case err == nil:
req.ConfigMaps = append(req.ConfigMaps, *cfgmap)
case volume.ConfigMap.Optional != nil && *volume.ConfigMap.Optional && apierrors.IsNotFound(err):
// Like the kubelet, mount an empty volume for a missing optional ConfigMap
// instead of waiting for it. The plugin still gets an object to mount by name.
log.G(ctx).Infof("optional ConfigMap %s of pod %s not found, mounting an empty volume", volume.ConfigMap.Name, pod.Name)
req.ConfigMaps = append(req.ConfigMaps, v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: volume.ConfigMap.Name, Namespace: pod.Namespace},
})
default:
err = failedMount(ctx, &failedAndWait, volume.ConfigMap.Name, pod, p, err)
if err != nil {
return err
}
} else {
req.ConfigMaps = append(req.ConfigMaps, *cfgmap)
}

case volume.Projected != nil:
Expand Down Expand Up @@ -1007,13 +1022,21 @@ func remoteExecutionHandleVolumes(ctx context.Context, p *Provider, pod *v1.Pod,

case volume.Secret != nil:
scrt, err := p.clientSet.CoreV1().Secrets(pod.Namespace).Get(ctx, volume.Secret.SecretName, metav1.GetOptions{})
if err != nil {
switch {
case err == nil:
req.Secrets = append(req.Secrets, *scrt)
case volume.Secret.Optional != nil && *volume.Secret.Optional && apierrors.IsNotFound(err):
// Like the kubelet, mount an empty volume for a missing optional Secret
// instead of waiting for it. The plugin still gets an object to mount by name.
log.G(ctx).Infof("optional Secret %s of pod %s not found, mounting an empty volume", volume.Secret.SecretName, pod.Name)
req.Secrets = append(req.Secrets, v1.Secret{
ObjectMeta: metav1.ObjectMeta{Name: volume.Secret.SecretName, Namespace: pod.Namespace},
})
default:
err = failedMount(ctx, &failedAndWait, volume.Secret.SecretName, pod, p, err)
if err != nil {
return err
}
} else {
req.Secrets = append(req.Secrets, *scrt)
}

case volume.EmptyDir != nil:
Expand Down
90 changes: 90 additions & 0 deletions pkg/virtualkubelet/execute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@
configMapData map[string]string
sourceItems []v1.KeyToPath
overrideCaCrt string
optional bool
expectedData map[string]string
expectErr bool
}{
Expand Down Expand Up @@ -461,6 +462,35 @@
expectedData: nil,
expectErr: true,
},
{
name: "missing key in items of optional configmap is skipped",
configMapName: testConfigMapName,
configMapData: map[string]string{
testConfigKey1: testConfigValue1,
},
sourceItems: []v1.KeyToPath{
{Key: testConfigKey1, Path: testConfigKey1},
{Key: "missing-key", Path: "some-path"},
},
optional: true,
expectedData: map[string]string{
testConfigKey1: testConfigValue1,
},
},
{
name: "missing configmap returns error",
configMapName: testConfigMapName,
configMapData: nil,
expectedData: nil,
expectErr: true,
},
{
name: "missing optional configmap projects no keys",
configMapName: testConfigMapName,
configMapData: nil,
optional: true,
expectedData: map[string]string{},
},
}

for _, tt := range tests {
Expand All @@ -486,10 +516,12 @@
},
}

optional := tt.optional
source := v1.VolumeProjection{
ConfigMap: &v1.ConfigMapProjection{
LocalObjectReference: v1.LocalObjectReference{Name: tt.configMapName},
Items: tt.sourceItems,
Optional: &optional,
},
}

Expand Down Expand Up @@ -657,6 +689,64 @@
assert.Nil(t, pod.Spec.Volumes[0].Projected)
}

func TestRemoteExecutionHandleVolumesOptionalMissing(t *testing.T) {
const (
missingConfigMap = "missing-config"
missingSecret = "missing-secret"
)
ctx := context.Background()
namespace := testNamespace
optional := true
pod := &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: testPodName,
Namespace: namespace,
},
Spec: v1.PodSpec{
Volumes: []v1.Volume{
{
Name: "config",
VolumeSource: v1.VolumeSource{
ConfigMap: &v1.ConfigMapVolumeSource{
LocalObjectReference: v1.LocalObjectReference{Name: missingConfigMap},
Optional: &optional,
},
},
},
{
Name: "secret",
VolumeSource: v1.VolumeSource{
Secret: &v1.SecretVolumeSource{
SecretName: missingSecret,
Optional: &optional,
},
},
},
},
},
}

fakeClient := fake.NewSimpleClientset(pod.DeepCopy())
p := &Provider{
clientSet: fakeClient,
notifier: func(*v1.Pod) {},
}
req := &types.PodCreateRequests{}

// Without the optional handling this waits five minutes for the objects to appear.

Check notice on line 736 in pkg/virtualkubelet/execute_test.go

View workflow job for this annotation

GitHub Actions / Check duplicated code

Copy/pasted code 

see pkg/virtualkubelet/execute_test.go (616-631)
err := remoteExecutionHandleVolumes(ctx, p, pod, req)
require.NoError(t, err)
assert.Equal(t, v1.PodPending, pod.Status.Phase)
require.Len(t, req.ConfigMaps, 1)
assert.Equal(t, missingConfigMap, req.ConfigMaps[0].Name)
assert.Equal(t, namespace, req.ConfigMaps[0].Namespace)
assert.Empty(t, req.ConfigMaps[0].Data)
require.Len(t, req.Secrets, 1)
assert.Equal(t, missingSecret, req.Secrets[0].Name)
assert.Equal(t, namespace, req.Secrets[0].Namespace)
assert.Empty(t, req.Secrets[0].Data)
}

func TestResolveEnvFromRefs(t *testing.T) {
ctx := context.Background()
namespace := testNamespace
Expand Down
Loading