🐛 Allow nested maps in Go types

[Porges]

These map to nested maps in CRDs. Also improve an error message.

I have marked this as a bug, as the omission of support for these nested maps seems like an oversight, since there is already support for and tests for nested maps in the deep-copy code (in legacy_deepcopy_cases.go).

Note that in addition a type like map[string]Foo where Foo has a field map[string]string is already permitted, so this only allows an “unwrapped” version of the above. (An example of this is also added into the test as part of this PR.)

This will help address some cases in #287.

[Porges]

I noticed that previously an error was being generated, but it does not get surfaced anywhere and controller-gen does not fail. Is there something else that needs fixing here?

[munnerz]

On the other hand maybe this doesn't work with Kubernetes itself? We shall soon find out!

Using maps in Kubernetes API resources at all is not recommended (as per the sig-architecture api-conventions doc):
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps

Not too sure if it's something the project still wants to accept anyway - but it'd be good for us to encourage following the api conventions doc through the feature-set exposed

[Porges]

On the other hand maybe this doesn't work with Kubernetes itself? We shall soon find out!

Using maps in Kubernetes API resources at all is not recommended (as per the sig-architecture api-conventions doc):
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps

Not too sure if it's something the project still wants to accept anyway - but it'd be good for us to encourage following the api conventions doc through the feature-set exposed

In our case we are accepting specs for external resources which require maps and nested maps in their configuration. Without support in controller-gen and friends we have to accept arbitrary JSON in the spec (and maybe write a custom acceptor/validator). Perhaps controller-gen could produce a warning along the lines of "maps are not recommended for use in Kubernetes APIs", or require a flag to enable support? There is precedence for similar things where float values are “not recommended” but are permitted with a flag to enable them.

[vijtrip2]

This will also unblock ACK (AWS Controllers for Kubernetes) project. ACK uses controller-gen to create Kubernetes custom resources for AWS resources. There are AWS resources which contain nested map and fails with controller-gen.

---

I also second @Porges 's thought here.
Perhaps controller-gen could produce a warning along the lines of "maps are not recommended for use in Kubernetes APIs", or require a flag to enable support? There is precedence for similar things where float values are “not recommended” but are permitted with a flag to enable them

[vijtrip2]

LGTM!

[vijtrip2]

/assign @DirectXMan12

[munnerz]

Sorry for the delay on this - for context, I do not have approval rights here :)

In our case we are accepting specs for external resources which require maps and nested maps in their configuration.

Embedding external types (i.e. ones that were not originally designed to be used as part of a Kubernetes API) is also something that has been typically best avoided. I would always strongly recommend either building your own abstraction over these types (and writing functions that can be used to convert from your API type to the external project's types), or otherwise using something like apiextensions.JSON as you noted.

When you begin to generate e.g. openapi schemas, conversion functions, and even DeepCopy functions, you can quickly run into all sorts of issues if you have embedded an external type. I'm conscious that by supporting it, we'd encourage this kind of thing in other projects/APIs.

I can appreciate that there are some projects wanting to use maps, but I think as a project we should help users to do the right thing. Interested to hear viewpoints from others (cc @DirectXMan12 @pwittrock)

[Porges]

@munnerz The kinds of things we are embedding are configurations, so it's nice for the end user if the configuration format is the same inside Kubernetes or outside it.

---

With regards to API complexity: I'd note that you can already write map[string]Foo where Foo contains a map[string]… field, so allowing map[string]map[string]… is essentially the same, but dropping one layer of indirection. This to me seems the strongest argument for permitting it, since something “almost isomorphic” is already permitted.

I’ve added an example of this to the test file.

[DirectXMan12]

With regards to API complexity: I'd note that you can already write map[string]Foo

This is not supposed to be possible, and I'd almost consider it a bug that we allow it. At the very least, it's a non-reccomended type -- maps in Kubernetes APIs are generally restricted to scalar values, and the occasional slice in very specific circumstances. Otherwise, Kubernetes-style associated lists are the reccomended pattern.

[DirectXMan12]

/hold

[k8s-triage-robot]

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

[Porges]

/remove-lifecycle stale

[smarterclayton]

To comment directly here, kube discourages but does not disallow certain constructs in core apis. I would not consider any construct described here as dangerous, nor would I say our api conventions are a counter to the stated goal for CRDs to allow the kube server to accommodate new concepts to extend Kube. As an API reviewer I might guide a core API author away from map of map, but that should not constrain how users can express valid CRD APIs in wide use.

Map of map should absolutely be allowed under a CRD, and in general any valid JSON structure should be allowed inside of the body of a CRD (any limitations are bugs).

[alvaroaleman]

/test all

[alvaroaleman]

/label tide/merge-method-squash
/hold cancel
/assign @vincepri

[alvaroaleman]

/lgtm

[Porges]

@smarterclayton Map of map should absolutely be allowed under a CRD, and in general any valid JSON structure should be allowed inside of the body of a CRD (any limitations are bugs).

In that case the array-value case should be loosened as well (currently only map[…][]string is permitted). But I won’t do it in this PR, for fear of postponing it 😉

[vijtrip2]

/assign @droot

[alvaroaleman]

@Porges can you resolve the merge conflicts, please?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, Porges, vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Porges]

@alvaroaleman can you resolve the merge conflicts, please?

voilà!

[alvaroaleman]

/test all
/lgtm