admission: unify plugin constructors

[sttts]

It's common in Go to return the actual object in constructors, not one interface
it implements. This allows us to implement multiple interfaces, but only have
one constructor. As having private types in constructors, we export all plugin structs, of course with private fields.

Note: super interfaces do not work if there are overlapping methods.

[liggitt]

is it common to return concrete unexported types? I guess I get the multiple interface thing, but it seems weird

[sttts]

is it common to return concrete unexported types? I guess I get the multiple interface thing, but it seems weird

It does not make the type exported and you cannot observe the actual type programmatically without reflection. Hence, there is no harm. To the contrary, I have never seen multiple constructors for concrete types so fulfill multiple interfaces in Golang. It's used everywhere for non-private types. Making a type private seems to be orthogonal to that pattern.

[sttts]

@liggitt golint also thinks it's weird, for Godoc reasons golang/lint#210. The alternative in the multi-interface case is to define a interface for each admission plugin that is public and subsumes all implemented interfaces. Certainly doable (remember we might get MutationInterface and ValidationInterface soon) and semi-ugly. Wdyt?

[sttts]

Our 3 options: https://gist.github.com/sttts/fa5d9d63dc1077c90005aa3fe2fc052d

[liggitt]

the superinterface is the most appealing to me. in most places, we're already doing essentially this with a series of private package type assertions

[sttts]

A local super interface doesn't work unfortunately:

type A interface { Foo() }
type B interface { Foo() }

type C interface {
  A
  B
}

This fails because Foo() is defined twice in C. So we are left with a public struct and private fields.

[sttts]

After super interfaces are broken in Golang, I don't care whether we

• use unexported types in constructors (plus a golint exception)
• or make the plugin structs public.

Which way do we choose? @deads2k @liggitt

[deads2k]

Which way do we choose? @deads2k @liggitt

I don't care either way. Public structs just make some golint-ing sugar over the private structs. Whatever you've got working works for me.

[k8s-ci-robot]

@sttts: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

[sttts]

Ok, made the struct public now, with unexported fields (with the exception of the embedded Handler. But that is another issue we had before this PR, so no regression).

[liggitt]

/lgtm

[deads2k]

/approve no-issue

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, sttts

Associated issue requirement bypassed by: deads2k

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

Needs approval from an approver in each of these OWNERS Files:

• plugin/pkg/admission/OWNERS [deads2k]
• staging/src/k8s.io/apiserver/OWNERS [deads2k,liggitt,sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 54761, 54748, 53991, 54485, 46951). If you want to cherry-pick this change to another branch, please follow the instructions here.