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
Fixing a small bug with GMSA support and adding an e2e test on it. #74708
Conversation
Hi @wk8. Thanks for your PR. I'm waiting for a kubernetes 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. |
@yujuhong @dchen1107 @michmike @PatrickLang : guidance appreciated as to how to make sure that e2e test can run successfully as part of builds (see the |
A previous PR (kubernetes#73726) added GMSA support to the dockershim. Unfortunately, there was a bug in there: the registry keys used to pass the cred specs down to Docker were being cleaned up too early, right after the containers' creation - before Docker would ever try to read them, when trying to actually start the container. This patch fixes this, and also adds an e2e test on this. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
4f7b386
to
6002a6c
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wk8 If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good. However I think it will be better at this point to split the PRs into at least different commits or maybe independent PRs for:
- The GMSA dockershim bugfix
- The e2e test for GMSA plus other Windows test refactoring
- Enhancements in hack/make-rules/test-e2e-node.sh
echo "Running on CGE, not asking for sudo credentials" | ||
elif sudo --non-interactive "$(which /bin/bash)" -c true 2> /dev/null; then | ||
# if we can run bash without a password, it's a pretty safe bet that either | ||
# we can run any commant without a password, or that sudo credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: commant => command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to #74733
@@ -438,3 +447,20 @@ func (ds *dockerService) UpdateContainerResources(_ context.Context, r *runtimea | |||
} | |||
return &runtimeapi.UpdateContainerResourcesResponse{}, nil | |||
} | |||
|
|||
func (ds *dockerService) performPlatformSpecificContainerCleanupForContainer(containerID string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about calling this performPlatformSpecificCleanupForContainer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
for _, err := range ds.performPlatformSpecificContainerCleanup(cleanupInfo) { | ||
klog.Warningf("error when cleaning up after container %q's: %v", containerNameOrID, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 's
can be removed in the message after container %q's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Typo.
f := framework.NewDefaultFramework("density-test-windows") | ||
|
||
BeforeEach(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block seems relevant for a Windows test. Should this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realized this has been shifted to the top-level down below in test/e2e/windows/framework.go .. makes sense.
What type of PR is this?
/kind bug
What this PR does / why we need it:
A previous PR (#73726)
added GMSA support to the dockershim. Unfortunately, there was a
bug in there: the registry keys used to pass the cred specs down
to Docker were being cleaned up too early, right after the containers'
creation - before Docker would ever try to read them, when trying to
actually start the container.
This patch fixes this, and also adds an e2e test on this.
Which issue(s) this PR fixes:
@yujuhong requested an e2e test on this at #73726 (review).
Special notes for your reviewer:
The e2e test has been verified to pass on a cluster with a Windows node with the
WindowsGMSA=true
feature gate enabled; however, I don't believe that's the casewhen running a "regular" build. Would it be possible to:
set the config it needs?
Does this PR introduce a user-facing change?: