Add GMSA credential spec passing #1505
Conversation
|
Hi @dcantah. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
|
@mikebrow Thanks, will fix up and add the test :) |
Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
windows-cred-spec
branch
from
7b5841e to
9620b2e
Compare
|
@mikebrow PTAL (assuming the last half of the CI passes 😄) |
| if windowsConfig != nil { | ||
| specOpts = append(specOpts, customopts.WithWindowsResources(windowsConfig.GetResources())) | ||
| securityCtx := windowsConfig.GetSecurityContext() | ||
| if securityCtx != nil { |
There was a problem hiding this comment.
I don't think we need all these nil checks. The Get* functions in protobuf should propagate nils forward, so if you do config.GetWindows().GetSecurityContext().GetRunAsUsername() it should return "" even if GetWindows() returns nil.
There was a problem hiding this comment.
which nil check is not needed they all look valid to me..
There was a problem hiding this comment.
IMO it's not a matter of will it panic.. it won't .. It's a matter of should the code proceed as if the get was successful.
There was a problem hiding this comment.
The caching of securityCtx and if securityCtx != nil could be removed in favor of just checking if windowsConfig.GetSecurityContext.GetRunAsUsername() and windowsConfig.GetSecurityContext.GetCredentialSpec() aren't empty strings. Either way works. I'm going to leave unless you feel strongly about it @kevpar
There was a problem hiding this comment.
I don't feel strongly. It seemed cleaner the other way, but agree it's a bit of an optimization to avoid doing some of the other work if an earlier Get call fails.
|
@mikebrow What's the process of getting cri revendored into containerd? |
summary: we'll update all our dependencies here to cover containerd dependencies considering if we need other dependencies updated as well, then when that tests out ok we'll do the same over on containerd/containerd merging containerd/cri dependency updates into containerd and the merge commit for containerd/cri that we decide to merge from... |
Signed-off-by: Daniel Canter [email protected]