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

fix: prevent memoization accessing wrong config-maps #11225

Merged
merged 1 commit into from
Jun 19, 2023
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
3 changes: 2 additions & 1 deletion docs/memoization.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ it stores the outputs of a template into a specified cache with a variable key.
## Cache Method

Currently, caching can only be performed with config-maps.
This allows you to easily manipulate cache entries manually through `kubectl` and the Kubernetes API without having to go through Argo.
This allows you to easily manipulate cache entries manually through `kubectl` and the Kubernetes API without having to go through Argo.
All cache config-maps must have the label `workflows.argoproj.io/configmap-type: Cache` to be used as a cache. This prevents accidental access to other important config-maps in the system

## Using Memoization

Expand Down
26 changes: 26 additions & 0 deletions workflow/controller/cache/configmap_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,22 @@ func (c *configMapCache) logInfo(fields log.Fields, message string) {
log.WithFields(log.Fields{"namespace": c.namespace, "name": c.name}).WithFields(fields).Info(message)
}

func (c *configMapCache) validateConfigmap(cm *apiv1.ConfigMap) error {
label, foundLabel := cm.GetLabels()[common.LabelKeyConfigMapType]
errString := ""
if !foundLabel {
errString = fmt.Sprintf("memoization configmap doesn't have %s label, refusing to use it", common.LabelKeyConfigMapType)
} else if label != common.LabelValueTypeConfigMapCache {
errString = fmt.Sprintf("memoization configmap doesn't have label %s = %s, refusing to use it", common.LabelKeyConfigMapType, common.LabelValueTypeConfigMapCache)
}
if errString != "" {
err := errors.New(errString)
c.logError(err, log.Fields{}, errString)
return err
}
return nil
}

func (c *configMapCache) Load(ctx context.Context, key string) (*Entry, error) {
if !cacheKeyRegex.MatchString(key) {
return nil, fmt.Errorf("invalid cache key: %s", key)
Expand All @@ -58,6 +74,11 @@ func (c *configMapCache) Load(ctx context.Context, key string) (*Entry, error) {
}
c.logError(err, log.Fields{}, "Error loading config map cache")
return nil, fmt.Errorf("could not load config map cache: %w", err)
} else {
err := c.validateConfigmap(cm)
if err != nil {
return nil, err
}
}

c.logInfo(log.Fields{}, "config map cache loaded")
Expand Down Expand Up @@ -115,6 +136,11 @@ func (c *configMapCache) Save(ctx context.Context, key string, nodeId string, va
c.logError(err, log.Fields{"key": key, "nodeId": nodeId}, "Error saving to ConfigMap cache")
return fmt.Errorf("could not save to config map cache: %w", err)
}
} else {
err := c.validateConfigmap(cache)
if err != nil {
return err
}
}

creationTime := time.Now()
Expand Down
10 changes: 8 additions & 2 deletions workflow/controller/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/argoproj/argo-workflows/v3/workflow/common"
"github.com/argoproj/argo-workflows/v3/workflow/controller/cache"
)

Expand All @@ -23,6 +24,9 @@ var sampleConfigMapCacheEntry = apiv1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "whalesay-cache",
ResourceVersion: "1630732",
Labels: map[string]string{
common.LabelKeyConfigMapType: common.LabelValueTypeConfigMapCache,
},
},
}

Expand All @@ -34,6 +38,9 @@ var sampleConfigMapEmptyCacheEntry = apiv1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "whalesay-cache",
ResourceVersion: "1630732",
Labels: map[string]string{
common.LabelKeyConfigMapType: common.LabelValueTypeConfigMapCache,
},
},
}

Expand All @@ -46,9 +53,8 @@ func TestConfigMapCacheLoadHit(t *testing.T) {
assert.NoError(t, err)
c := cache.NewConfigMapCache("default", controller.kubeclientset, "whalesay-cache")

cm, err := controller.kubeclientset.CoreV1().ConfigMaps("default").Get(ctx, sampleConfigMapCacheEntry.Name, metav1.GetOptions{})
_, err = controller.kubeclientset.CoreV1().ConfigMaps("default").Get(ctx, sampleConfigMapCacheEntry.Name, metav1.GetOptions{})
assert.NoError(t, err)
assert.Nil(t, cm.Labels)

entry, err := c.Load(ctx, "hi-there-world")
assert.NoError(t, err)
Expand Down
48 changes: 48 additions & 0 deletions workflow/controller/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5100,6 +5100,9 @@ func TestConfigMapCacheLoadOperate(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "whalesay-cache",
ResourceVersion: "1630732",
Labels: map[string]string{
common.LabelKeyConfigMapType: common.LabelValueTypeConfigMapCache,
},
},
}
wf := wfv1.MustUnmarshalWorkflow(workflowCached)
Expand Down Expand Up @@ -5172,6 +5175,9 @@ func TestConfigMapCacheLoadOperateMaxAge(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "whalesay-cache",
ResourceVersion: "1630732",
Labels: map[string]string{
common.LabelKeyConfigMapType: common.LabelValueTypeConfigMapCache,
},
},
}
}
Expand Down Expand Up @@ -5217,6 +5223,45 @@ func TestConfigMapCacheLoadOperateMaxAge(t *testing.T) {
}
}

func TestConfigMapCacheLoadNoLabels(t *testing.T) {
sampleConfigMapCacheEntry := apiv1.ConfigMap{
Data: map[string]string{
"hi-there-world": `{"ExpiresAt":"2020-06-18T17:11:05Z","NodeID":"memoize-abx4124-123129321123","Outputs":{}}`,
},
TypeMeta: metav1.TypeMeta{
Kind: "ConfigMap",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "whalesay-cache",
ResourceVersion: "1630732",
},
}
wf := wfv1.MustUnmarshalWorkflow(workflowCached)
cancel, controller := newController()
defer cancel()

ctx := context.Background()
_, err := controller.wfclientset.ArgoprojV1alpha1().Workflows(wf.ObjectMeta.Namespace).Create(ctx, wf, metav1.CreateOptions{})
assert.NoError(t, err)
_, err = controller.kubeclientset.CoreV1().ConfigMaps("default").Create(ctx, &sampleConfigMapCacheEntry, metav1.CreateOptions{})
assert.NoError(t, err)

woc := newWorkflowOperationCtx(wf, controller)
fn := func() {
woc.operate(ctx)
}
assert.NotPanics(t, fn)
assert.Equal(t, wfv1.WorkflowError, woc.wf.Status.Phase)

if assert.Len(t, woc.wf.Status.Nodes, 1) {
for _, node := range woc.wf.Status.Nodes {
assert.Nil(t, node.Outputs)
assert.Equal(t, wfv1.NodeError, node.Phase)
}
}
}

func TestConfigMapCacheLoadNilOutputs(t *testing.T) {
sampleConfigMapCacheEntry := apiv1.ConfigMap{
Data: map[string]string{
Expand All @@ -5229,6 +5274,9 @@ func TestConfigMapCacheLoadNilOutputs(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "whalesay-cache",
ResourceVersion: "1630732",
Labels: map[string]string{
common.LabelKeyConfigMapType: common.LabelValueTypeConfigMapCache,
},
},
}
wf := wfv1.MustUnmarshalWorkflow(workflowCached)
Expand Down