Update status of GMSA KEP to Implementable

[ddebroy]

Thanks so much for the detailed review and feedback on the GMSA KEP so far.

Updating status of GMSA KEP to implementable based on having addressed all outstanding feedback so far from latest review in #694. Approvers: @PatrickLang , @yujuhong and @liggitt PTAL so that we can get started on the Alpha implementation of GMSA.

cc @JeremyWx @wk8

[ddebroy]

/assign @liggitt @yujuhong

[michmike]

/hold

[michmike]

/lgtm
/approve
please remove the hold when @liggitt approves

[liggitt]

Before moving to implementable, I had these questions:

1. Is the initial out-of-tree implementation expected to be supported in parallel with an eventual in-tree implementation, or would it be mutually exclusive, or would the annotation-based implementation be dropped? (we've had experience trying to migrate/support annotations and promoted fields, and it didn't work well). The KEP describes "promoting" the annotations to fields but doesn't describe what the implications of that are for existing annotation values and users.
2. Should the annotations use k8s.io prefixes, sig-specific k8s.io prefixes, or something else?
3. What is the API group for the custom resource (I'd suggest something specific to sig-windows)
4. Is the credential spec resource structured or does it inline the credspec as a string blob? The examples in the KEP indicate it is structured, but the validation suggested in Incorporate feedback from reviews of GMSA KEP #694 (comment) made it sound like a single blob. Do we anticipate the eventual API field being a blob or structured?
5. With respect to proposed addition to the pod API, I think it's too early to declare what windows fields will look like in the pod spec. Can we make that section more clearly indicate that it is speculative and subject to change?

[michmike]

Before moving to implementable, I had these questions:

1. Is the initial out-of-tree implementation expected to be supported in parallel with an eventual in-tree implementation, or would it be mutually exclusive, or would the annotation-based implementation be dropped? (we've had experience trying to migrate/support annotations and promoted fields, and it didn't work well). The KEP describes "promoting" the annotations to fields but doesn't describe what the implications of that are for existing annotation values and users.
2. Should the annotations use k8s.io prefixes, sig-specific k8s.io prefixes, or something else?
3. What is the API group for the custom resource (I'd suggest something specific to sig-windows)
4. Is the credential spec resource structured or does it inline the credspec as a string blob? The examples in the KEP indicate it is structured, but the validation suggested in #694 (comment) made it sound like a single blob. Do we anticipate the eventual API field being a blob or structured?
5. With respect to proposed addition to the pod API, I think it's too early to declare what windows fields will look like in the pod spec. Can we make that section more clearly indicate that it is speculative and subject to change?

Deep will likely reply to all of these as we already addressed some of them like 2 and 4 out of band (he already updated the comment for 4).
I wanted to mention that the point you made on item 1 is very important. it will likely have upgrade considerations as we go from beta to stable.

[ddebroy]

@liggitt @michmike please see #722 which covers all the above points. Let's continue any discussion/comments around the above points in PR or other outstanding items in #722

[ddebroy]

Rebased PR to resolve conflicts due to changes in the KEP from other branches.

[michmike]

/lgtm
you can remove the hold once @liggitt approves/lgtm

[liggitt]

• the proposed admission/authz checks lgtm from a sig-auth perspective
• the symbolic name annotation -> admission expansion to full spec lgtm from an API perspective

open questions:

• for sig-windows:
  • is this feature necessary for windows GA?
• for sig-node:
  • is the proposal to add (alpha, feature-gated) code in kubelet/dockershim to drive CRI behavior from the pod annotation considered acceptable?
  • is a goal to reconcile the capabilities of the pod API, the CRI API, and the OCI spec? if so, has there been discussion about what that would look like (and if what this proposes looks like it cloud align with that)?

[michmike]

• the proposed admission/authz checks lgtm from a sig-auth perspective
• the symbolic name annotation -> admission expansion to full spec lgtm from an API perspective

open questions:

• for sig-windows:

  • is this feature necessary for windows GA?

• for sig-node:

  • is the proposal to add (alpha, feature-gated) code in kubelet/dockershim to drive CRI behavior from the pod annotation considered acceptable?
  • is a goal to reconcile the capabilities of the pod API, the CRI API, and the OCI spec? if so, has there been discussion about what that would look like (and if what this proposes looks like it cloud align with that)?

hi @liggitt , i answered the first portion of your sig-windows question to Brian the other day. here's what i had mentioned This feature is not necessary for Windows GA, but this is a very important feature for the viability of Windows workloads on Kubernetes. As such we want to be making forward progress and deliver this as alpha as soon as possible. without this feature and since windows workloads can't be domain joined, it is hard to implement enterprise apps on Kubernetes. Such enterprise apps usually need access to external resources like file shares and databases. gMSA is the recommended way to provide a container with an identity in the Active Directory network

[ddebroy]

Regarding the question for sig-node:

is the proposal to add (alpha, feature-gated) code in kubelet/dockershim to drive CRI behavior from the pod annotation considered acceptable?

I found the following comment that I think is relevant in the context of annotations being used in the alpha phase for this feature at https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/cri/runtime/v1alpha2/api.proto#L352 and our plan to eventually convert it into a string field later.

    // Annotations can also be useful for runtime authors to experiment with
    // new features that are opaque to the Kubernetes APIs (both user-facing
    // and the CRI). Whenever possible, however, runtime authors SHOULD
    // consider proposing new typed fields for any new features instead.

[liggitt]

// Annotations can also be useful for runtime authors to experiment with
// new features that are opaque to the Kubernetes APIs (both user-facing
// and the CRI). Whenever possible, however, runtime authors SHOULD
// consider proposing new typed fields for any new features instead.

agreed, and if the changes were limited to the runtime impl, I'd have no concerns with this experimentation. since the changes touch kubelet/dockershim, I wanted sig-node to ack the plan.

[ddebroy]

I brought up the questions for SIG Node at the SIG Node meeting today [Jan 29th]:

is the proposal to add (alpha, feature-gated) code in kubelet/dockershim to drive CRI behavior from the pod annotation considered acceptable?

SIG Node clarified that as long as the code is feature gated, there is no major concern. @PatrickLang also pointed out that in dockershim, this is already done today for Hyper-V isolation at: applyExperimentalCreateConfig => ShouldIsolatedByHyperV in dockershim.

is a goal to reconcile the capabilities of the pod API, the CRI API, and the OCI spec? if so, has there been discussion about what that would look like (and if what this proposes looks like it cloud align with that)?

SIG Node mentioned that there is no effort or initiative underway to look at this reconciliation right now. The main challenge is that on end, pod API is scoped at the pod level while on the other end, OCI is scoped at the container level. Therefore some amount of code is necessary to transform relevant portions of a pod spec to an OCI spec for each container in the pod.

Based on the above discussion in SIG node in the Jan 29th meeting, I think at this point, the above two questions for SIG Node are resolved. @liggitt let me know if we missed something.

[yujuhong]

SIG Node mentioned that there is no effort or initiative underway to look at this reconciliation right now. The main challenge is that on end, pod API is scoped at the pod level while on the other end, OCI is scoped at the container level. Therefore some amount of code is necessary to transform relevant portions of a pod spec to an OCI spec for each container in the pod.

To add to that, there are also many different implementations that translates CRI -> OCI to achieve different runtimes. To switch to using OCI, kubelet will have to absorb all the code, defeating the extensibility brought by the interface.

There's definitely some middle ground, and plenty more improvements can be made on the APIs. No one is actively looking at this area in sig node AFAIK though.

[yujuhong]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, michmike, yujuhong

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:

• keps/sig-windows/OWNERS [michmike]

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

[michmike]

/hold cancel
we are good to go. great work everyone (approvers, reviewers, and Deep/Jeremy that created this)