Skip to content
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

tests: Adds Windows RunAsUserName tests #79539

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@bclau
Copy link
Contributor

commented Jun 28, 2019

What type of PR is this?

/kind feature

/sig testing
/sig windows

What this PR does / why we need it:

Currently, Kubernetes supports running as different user (RunAsUser), but it only supports UIDs, which does not work on Windows.

Which is why the field SecurityContext.WindowsOptions.RunAsUserName was introduced, to allow us to run the container entrypoints with a different user than its default one.

This PR adds E2E tests which will validate this behaviour. The tests are Windows only, and they will be skipped if --node-os-distro is not windows.

Which issue(s) this PR fixes:

Fixes: #69905
Depends On: #79489

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Added E2E tests validating WindowsOptions.RunAsUserName.
@fejta-bot

This comment has been minimized.

Copy link

commented Jul 4, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

/test pull-kubernetes-e2e-gce

@bclau bclau force-pushed the bclau:tests/run-as-username branch from b5b53a5 to f204546 Jul 16, 2019
@bclau bclau force-pushed the bclau:tests/run-as-username branch from f204546 to e5e9a3f Jul 18, 2019
@k8s-ci-robot k8s-ci-robot removed the size/XXL label Jul 18, 2019
@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

/retest

@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

FYI, in order for those tests to properly pass on Windows, the feature-gate WindowsRunAsUserName is needed to be set in kube-apiserver service: --enable-admission-plugins=WindowsRunAsUserName (on master, 1.16)

/assign @yujuhong @benmoss

test/e2e/common/configmap_volume.go Outdated Show resolved Hide resolved
test/e2e/common/configmap_volume.go Outdated Show resolved Hide resolved
test/e2e/common/configmap_volume.go Outdated Show resolved Hide resolved
*/
framework.ConformanceIt("should be consumable from pods in volume with mappings as non-root [LinuxOnly] [NodeConformance]", func() {
doConfigMapE2EWithMappings(f, 1000, 0, nil)
framework.ConformanceIt("should be consumable from pods in volume with mappings as non-root [NodeConformance]", func() {

This comment has been minimized.

Copy link
@yujuhong

yujuhong Aug 20, 2019

Member

Isn't "RunAsUserName" still an alpha feature and is gated? With the LinuxOnly tag removed, we'd be running these tests against windows nodes and they'd fail if the feature is gated (i.e., the default).

This comment has been minimized.

Copy link
@bclau

bclau Aug 21, 2019

Author Contributor

True, which is why I was saying that we should add --feature-gates=WindowsRunAsUserName=true on kube-apiserver controllers in our runs.

Note that this change will only affect the master branch (and probably the release-1.16 branch too).

This comment has been minimized.

Copy link
@yujuhong

yujuhong Aug 21, 2019

Member

I don't think it's a good idea to force all the test jobs to run with an alpha feature enabled for a conformance test. I'd prefer keeping the [LinuxOnly] tag until the feature has been promoted to beta.

In the meantime, we can copy the test(s) and add [Feature:Windows] if we want the coverage for the alpha feature.

This comment has been minimized.

Copy link
@bclau

bclau Aug 27, 2019

Author Contributor

Ok, I'll leave only the basic RunAsUserName tests for now, and I'll resubmit this part when the features goes to Beta.

test/e2e/windows/security_context.go Outdated Show resolved Hide resolved
test/e2e/windows/security_context.go Outdated Show resolved Hide resolved
@benmoss

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

I'm getting one failure on my cluster:

• Failure [16.846 seconds]
[sig-storage] ConfigMap
/home/ben/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/common/configmap_volume.go:34
  should be consumable from pods in volume as non-root with defaultMode and fsGroup set [NodeFeature:FSGroup] [It]
  /home/ben/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/common/configmap_volume.go:57

  Unexpected error:
      <*errors.errorString | 0xc0020a5640>: {
          s: "expected \"(mode of file \\\"/etc/configmap-volume/data-1\\\": -r--r-----|mode of Windows file \\\"/etc/configmap-volume/data-1\\\": -r--r-----)\" in container output: Expected\n    <string>: mode of Windows file \"/etc/configmap-volume/data-1\": -rwxrwxr-x\n    content of file \"/etc/configmap-volume/data-1\": value-1\n    \nto match regular expression\n    <string>: (mode of file \"/etc/configmap-volume/data-1\": -r--r-----|mode of Windows file \"/etc/configmap-volume/data-1\": -r--r-----)",
      }
      expected "(mode of file \"/etc/configmap-volume/data-1\": -r--r-----|mode of Windows file \"/etc/configmap-volume/data-1\": -r--r-----)" in container output: Expected
          <string>: mode of Windows file "/etc/configmap-volume/data-1": -rwxrwxr-x
          content of file "/etc/configmap-volume/data-1": value-1

      to match regular expression
          <string>: (mode of file "/etc/configmap-volume/data-1": -r--r-----|mode of Windows file "/etc/configmap-volume/data-1": -r--r-----)
  occurred

  /home/ben/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/util.go:1672

I wonder if this is another test that inadvertently depends on host filesystem permissions instead of container permissions.

@bclau bclau force-pushed the bclau:tests/run-as-username branch from e5e9a3f to 0ace7b3 Aug 21, 2019
@k8s-ci-robot k8s-ci-robot added needs-rebase and removed lgtm labels Aug 21, 2019
@bclau bclau force-pushed the bclau:tests/run-as-username branch from 0ace7b3 to a1ba036 Aug 21, 2019
@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I'm getting one failure on my cluster:

• Failure [16.846 seconds]
[sig-storage] ConfigMap
/home/ben/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/common/configmap_volume.go:34
  should be consumable from pods in volume as non-root with defaultMode and fsGroup set [NodeFeature:FSGroup] [It]
  /home/ben/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/common/configmap_volume.go:57

  Unexpected error:
      <*errors.errorString | 0xc0020a5640>: {
          s: "expected \"(mode of file \\\"/etc/configmap-volume/data-1\\\": -r--r-----|mode of Windows file \\\"/etc/configmap-volume/data-1\\\": -r--r-----)\" in container output: Expected\n    <string>: mode of Windows file \"/etc/configmap-volume/data-1\": -rwxrwxr-x\n    content of file \"/etc/configmap-volume/data-1\": value-1\n    \nto match regular expression\n    <string>: (mode of file \"/etc/configmap-volume/data-1\": -r--r-----|mode of Windows file \"/etc/configmap-volume/data-1\": -r--r-----)",
      }
      expected "(mode of file \"/etc/configmap-volume/data-1\": -r--r-----|mode of Windows file \"/etc/configmap-volume/data-1\": -r--r-----)" in container output: Expected
          <string>: mode of Windows file "/etc/configmap-volume/data-1": -rwxrwxr-x
          content of file "/etc/configmap-volume/data-1": value-1

      to match regular expression
          <string>: (mode of file "/etc/configmap-volume/data-1": -r--r-----|mode of Windows file "/etc/configmap-volume/data-1": -r--r-----)
  occurred

  /home/ben/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/util.go:1672

I wonder if this is another test that inadvertently depends on host filesystem permissions instead of container permissions.

That test shouldn't run on Windows anyways. Keywords to look out for: fsGroup (implies setting GID, which is not supported on Windows), defaultMode, Item mode (implies setting file permissions is expected, which is not supported on Windows).

I have a PR which adds the [LinuxOnly] tag to such tests: #80213

@bclau bclau force-pushed the bclau:tests/run-as-username branch from a1ba036 to e91537f Aug 21, 2019
@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

/test pull-kubernetes-bazel-build

Currently, Kubernetes supports running as different user (RunAsUser),
but it only supports UIDs, which does not work on Windows.

Which is why the field SecurityContext.WindowsOptions.RunAsUserName
was introduced, to allow us to run the container entrypoints with
a different user than its default one.

This commit adds E2E tests which will validate this behaviour. The
tests are Windows only, and they will be skipped if --node-os-distro
is not "windows".
@bclau bclau force-pushed the bclau:tests/run-as-username branch from e91537f to f63a2da Aug 26, 2019
@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

/retest

@bclau bclau force-pushed the bclau:tests/run-as-username branch 2 times, most recently from eb7b847 to e461bdb Aug 26, 2019
@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

pull-kubernetes-conformance-kind-ipv6 failure is due to
#81846 , not related

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 28, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, PatrickLang

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 28, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 6c45b3c into kubernetes:master Aug 28, 2019
24 checks passed
24 checks passed
cla/linuxfoundation bclau authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
SIG-Windows automation moved this from In Progress (v1.16) to Done (v1.16) Aug 28, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.