Expand coverage/support for atomics

[jpbetz]

We have a bunch of cases involving the composition of atomics that we don't yet test.

We need to define and test the behavior of all of them. Creating a grid out all possible combinations might be a good first step.

Here are a few we are aware of:

map typeref is tagged with +mapType=atomic (not supported or tested)

type Container struct {
  Field AtomicMapTyperef
}

// +mapType=atomic
type AtomicMapTyperef map[string]string

reference to map typeref is tagged with +mapType=atomic (not tested, seems to work)

type Container struct {
  //+mapType=atomic
  Field AtomicMapTyperef
}

type AtomicMapTyperef map[string]string

set of atomic structs (not tested, not sure if it works)

type Container struct {
  //+listType=set
  SetField []AtomicStruct
}

//+mapType=atomic
type AtomicStruct struct {
  StringField string
}

map value is atomic struct (not tested, not sure if it works)

type Container struct {
  //+listType=map
  //+listMapKey=name
  ListField []ListItem
}

//+mapType=atomic
type ListItem struct {
  Name string
  AtomicStructField AtomicStruct
}

//+mapType=atomic
type AtomicStruct struct {
  StringField string
}

map key is atomic struct (not tested, not sure if it works)

type Container struct {
  //+listType=map
  //+listMapKey=atomicStructField
  ListField []ListItem
}

//+mapType=atomic
type ListItem struct {
  atomicStructField AtomicStruct
}

//+mapType=atomic
type AtomicStruct struct {
  StringField string
}

[jpbetz]

/cc @apelisse

[jpbetz]

xref kubernetes-sigs/structured-merge-diff#163

[apelisse]

I think you've captured the interesting cases, thanks

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[apelisse]

I said in the other issue that this is fixed, but I can't find what fixed it?

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[Jefftree]

/remove-lifecycle rotten

[Jefftree]

I think this issue can be split into two parts:

1. Aliases: The first two examples deal with using aliases for a type. This is not currently supported since we don't resolve aliases when generating the OpenAPI spec, but I think it should be a nice addition. The changes required to support this should be solely on the kube-openapi side to propagate certain keys up from their aliases. I did something similar with the propagation for defaulting, so implementing this shouldn't be too challenging.

map typeref is tagged with +mapType=atomic (not supported or tested)

type Container struct {
  Field AtomicMapTyperef
}

// +mapType=atomic
type AtomicMapTyperef map[string]string

reference to map typeref is tagged with +mapType=atomic (not tested, seems to work)

type Container struct {
  //+mapType=atomic
  Field AtomicMapTyperef
}

type AtomicMapTyperef map[string]string

The use case for both of these looks valid, but I think we should disallow both of them to prevent potential misconfiguration. A map is by default "granular" according to the documentation, and I think we should enforce that the atomic annotation is kept throughout all aliases in order for us to treat the type as properly configured. For example, the second code block above is technically also equivalent to

type Container struct {
  //+mapType=atomic
  Field AtomicMapTyperef
}

//+mapType=granular
type AtomicMapTyperef map[string]string

which should obviously throw an error.

2. Compositions of atomics in collections (lists, maps, etc): This seems to be a purely SMD feature (See https://github.com/kubernetes-sigs/structured-merge-diff/blob/17912732856ce1817c889fd8a8815083f6e0f826/typed/helpers.go#L210-L218).

set of atomic structs (not tested, not sure if it works)

type Container struct {
  //+listType=set
  SetField []AtomicStruct
}

//+mapType=atomic
type AtomicStruct struct {
  StringField string
}

This is the same scenario as described in kubernetes-sigs/structured-merge-diff#163 I believe. Sets containing atomic maps and atomic lists should be accepted, but at the moment it looks like we don't support it. I'm looking into what we'd need to change to support this.

[apelisse]

https://v1-18.docs.kubernetes.io/docs/reference/using-api/api-concepts/#merge-strategy

[Jefftree]

Discussions from yesterday:

1. The atomic property should be attached to a field and not a type. Aliases are fine, but only the atomic property in the field will be read. Adding an atomic property to an alias type should have no effect. I'll try to see if the documentation can be updated to make this more clear. The below code is the proper way to use atomic with aliases and already works with our code, so no additional code changes should be necessary.

type Container struct {
  //+mapType=atomic
  Field AtomicMapTyperef
}

type AtomicMapTyperef map[string]string

2. Per https://v1-18.docs.kubernetes.io/docs/reference/using-api/api-concepts/#merge-strategy

"atomic and set apply to lists with scalar elements only"

Based on the documentation we do not support sets with nonscalar elements. For now, there's not much of a use case to expand this support.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.