@timstclair yes, this is what I was thinking. In my downstream implementation I did a copy of the annotations in the provider so it could be overwritten wholesale just like what's done for the security context. Do you think there is value in that rather than a merge? Perhaps if we ever have a use case where an annotation should be removed? I can't think of an example.

@pmorie and @liggitt thoughts on ^

Yeah, that's the more general API -- I'll go with that.

@pweil- friendly ping :)

@pweil- friendly ping :)

sorry, on PTO 😄 We should probably perform a copy in the provider and return the copy. That way usage of the provider outside of admission still maintains the assumption that it will not modify the original values like we do with the SC.

Admission still needs to hang on to the original so it can apply the generated values (so that subsequent validations or container generations will be working on the latest and greatest), and rollback if necessary so I think that part is fine.

Other than that this LGTM. WDYT? Nothing is using the provider outside of admission right now, just seems more reusable if we ever want to use the provider outside of admission.

on PTO 😄

Oh, np! I'll find someone else to review.

We should probably perform a copy in the provider and return the copy

My thought was that we should only perform the copy if we're actually modifying the annotations (for efficiency). I didn't do the copy here since this PR is just providing the API, and not actually touching the annotations.

@erictune - Would you mind reviewing this since @pweil- is OOO?

I'm ok with adding the copy when something actually starts changing
annotations. LGTM then.

On Thursday, August 11, 2016, Tim St. Clair notifications@github.com
wrote:

on PTO 😄

Oh, np! I'll find someone else to review.

We should probably perform a copy in the provider and return the copy

My thought was that we should only perform the copy if we're actually
modifying the annotations (for efficiency). I didn't do the copy here since
this PR is just providing the API, and not actually touching the
annotations.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30257 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AE_PU81-fV79-naeWXdUuz9de25RDeUXks5qe5zqgaJpZM4JfpYq
.

Spreading authz enforcement across two objects and returning a sometimes-copy seems dangerous to me

@liggitt I hear you - I don't like returning multiple non-error values, but I don't see a great alternative. I suppose we could add a new method to the Provider interface for getting the Annotations, but it's not clear to me that that is any less error prone. Would you be more comfortable with always returning a copy, or do you have a better suggestion?

This is currently blocking #30183, which needs to be merged by Friday for v1.4 feature-complete. Are there any outstanding concerns on this PR or can we move forward with it as-is?

I think if this is updated to always return a copy of the annotations then @liggitt 's comment is satisfied? I also think the multiple return values are the cleanest solution to having fields start off as annotations.

Updated to always return a copy, squashed & rebased. PTAL.

this LGTM

One of the verifications failed. Please fix it.

The pr lgtm. Feel free to apply the label once all checks passed.

Done.

Sorry, pushed to the wrong branch. Now done.

GCE e2e build/test passed for commit c99d7fd.

• Test Results
• Build Log
• Test Artifacts
• Internal Jenkins Results

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

GCE e2e build/test passed for commit c99d7fd.

• Test Results
• Build Log
• Test Artifacts
• Internal Jenkins Results

Automatic merge from submit-queue