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

Consider removing UID mutation from request.context #59775

Closed
enj opened this Issue Feb 12, 2018 · 11 comments

Comments

Projects
None yet
8 participants
@enj
Member

enj commented Feb 12, 2018

Injecting a UID into the context of a request via WithUID will cause it to override the UID of an object (nothing does this currently):

// allows admission controllers to assign a UID earlier in the request processing
// to support tracking resources pending creation.
uid, found := genericapirequest.UIDFrom(ctx)

It is unclear when this would be useful or safe. There is also concern in regards to MutatingWebhooks that set a UID on the object (FillObjectMetaSystemFields prevents this from being an issue):

https://groups.google.com/d/msg/kubernetes-sig-architecture/mVGobfD4TpY/vIJXeQTMAQAJ

Perhaps we should remove WithUID?

@kubernetes/sig-api-machinery-misc

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Feb 13, 2018

I remember see it somewhere this is used for test.

@enj

This comment has been minimized.

Member

enj commented Feb 13, 2018

I remember see it somewhere this is used for test.

All the tests would be deleted with it since it is not actually used in any real code.

@mbohlool

This comment has been minimized.

Member

mbohlool commented Feb 15, 2018

/cc @cheftako

@fejta-bot

This comment has been minimized.

fejta-bot commented May 16, 2018

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

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented May 17, 2018

/assign

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented May 17, 2018

I can help remove WithUID

@lavalamp

This comment has been minimized.

Member

lavalamp commented May 17, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented May 17, 2018

Added in #25307 with reference to quota. Not used anywhere I know of.

@liggitt

This comment has been minimized.

Member

liggitt commented May 17, 2018

I think the split of the validation phase of admission to run after uid has been allocated obviates the need for this, and we can remove it

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented May 17, 2018

As create handler admission code shows, it does not modify ctx.

So below comment is not accurate.

// allows admission controllers to assign a UID earlier in the request processing
// to support tracking resources pending creation.
uid, found := genericapirequest.UIDFrom(ctx)

@deads2k

This comment has been minimized.

Contributor

deads2k commented May 17, 2018

Is this the thing we added to help auth code know the UID before the object
has been created? If so I think it's still needed.

It was for quota. To my knowledge we never used it.

k8s-merge-robot added a commit that referenced this issue May 17, 2018

Merge pull request #63957 from hzxuzhonghu/rm-UID-ctx
Automatic merge from submit-queue (batch tested with PRs 63871, 63927, 63966, 63957, 63844). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

remove UID mutation from request.context

**What this PR does / why we need it**:

remove UID mutation from request.context, which is no use currently.

Fixes #59775

**Special notes for your reviewer**:

**Release note**:

```release-note
Remove UID mutation from request.context.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment