Skip to content

Commit

Permalink
Address the comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@zylxjtu
zylxjtu committed Oct 14, 2024
1 parent d4e74b5 commit 5d4918d
Show file tree
Hide file tree
Showing 4 changed files with 142 additions and 18 deletions.
2 changes: 1 addition & 1 deletion admission-webhook/Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ SHELL := /bin/bash
WEBHOOK_ROOT := $(CURDIR)

REGISTRY ?= sigwindowstools
STAGING_REGISTRY ?= gcr.io/k8s-staging-gmsa-webhook
STAGING_REGISTRY ?= containerrollingregistry.azurecr.io/k8s-staging-gmsa-webhook
IMAGE_NAME ?= k8s-gmsa-webhook
TAG ?= $(shell git describe --tags --always `git rev-parse HEAD`)
WEBHOOK_IMG ?= $(REGISTRY)/$(IMAGE_NAME)
Expand Down
26 changes: 22 additions & 4 deletions admission-webhook/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ type dummyKubeClient struct {
retrieveCredSpecContentsFunc func(ctx context.Context, credSpecName string) (contents string, httpCode int, err error)
}


func (dkc *dummyKubeClient) isAuthorizedToUseCredSpec(ctx context.Context, serviceAccountName, namespace, credSpecName string) (authorized bool, reason string) {
if dkc.isAuthorizedToUseCredSpecFunc != nil {
return dkc.isAuthorizedToUseCredSpecFunc(ctx, serviceAccountName, namespace, credSpecName)
Expand Down Expand Up @@ -59,6 +58,14 @@ func setWindowsOptions(winOptions *corev1.WindowsSecurityContextOptions, credSpe
// case a `*corev1.WindowsSecurityContextOptions` is built using that string as the name of the cred spec to use.
// Same goes for the values of `containerNamesAndWindowsOptions`.
func buildPod(serviceAccountName string, podWindowsOptions *corev1.WindowsSecurityContextOptions, containerNamesAndWindowsOptions map[string]*corev1.WindowsSecurityContextOptions) *corev1.Pod {
return buildPodWithHostName(serviceAccountName, nil, podWindowsOptions, containerNamesAndWindowsOptions)
}

// buildPod builds a pod for unit tests.
// `podWindowsOptions` should be either a full `*corev1.WindowsSecurityContextOptions` or a string, in which
// case a `*corev1.WindowsSecurityContextOptions` is built using that string as the name of the cred spec to use.
// Same goes for the values of `containerNamesAndWindowsOptions`.
func buildPodWithHostName(serviceAccountName string, hostname *string, podWindowsOptions *corev1.WindowsSecurityContextOptions, containerNamesAndWindowsOptions map[string]*corev1.WindowsSecurityContextOptions) *corev1.Pod {
containers := make([]corev1.Container, len(containerNamesAndWindowsOptions))
i := 0
for name, winOptions := range containerNamesAndWindowsOptions {
Expand All @@ -70,10 +77,21 @@ func buildPod(serviceAccountName string, podWindowsOptions *corev1.WindowsSecuri
}

shuffleContainers(containers)
podSpec := corev1.PodSpec{
ServiceAccountName: serviceAccountName,
Containers: containers,

var podSpec corev1.PodSpec
if hostname != nil {
podSpec = corev1.PodSpec{
ServiceAccountName: serviceAccountName,
Containers: containers,
Hostname: *hostname,
}
} else {
podSpec = corev1.PodSpec{
ServiceAccountName: serviceAccountName,
Containers: containers,
}
}

if podWindowsOptions != nil {
podSpec.SecurityContext = &corev1.PodSecurityContext{WindowsOptions: podWindowsOptions}
}
Expand Down
56 changes: 44 additions & 12 deletions admission-webhook/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,9 +367,11 @@ func compareCredSpecContents(fromResource, fromCRD string) (bool, error) {
// mutateCreateRequest inlines the requested GMSA's into the pod's and containers' `WindowsSecurityOptions` structs.
func (webhook *webhook) mutateCreateRequest(ctx context.Context, pod *corev1.Pod) (*admissionV1.AdmissionResponse, *podAdmissionError) {
var patches []map[string]string
hasGMSA := false

if err := iterateOverWindowsSecurityOptions(pod, func(windowsOptions *corev1.WindowsSecurityContextOptions, resourceKind gmsaResourceKind, resourceName string, containerIndex int) *podAdmissionError {
if credSpecName := windowsOptions.GMSACredentialSpecName; credSpecName != nil {
hasGMSA = true
// if the user has pre-set the GMSA's contents, we won't override it - it'll be down
// to the validation endpoint to make sure the contents actually are what they should
if credSpecContents := windowsOptions.GMSACredentialSpec; credSpecContents == nil {
Expand Down Expand Up @@ -401,10 +403,10 @@ func (webhook *webhook) mutateCreateRequest(ctx context.Context, pod *corev1.Pod

admissionResponse := &admissionV1.AdmissionResponse{Allowed: true}

if len(patches) != 0 {
if hasGMSA {
// pods are GMSA related, we will need further check if we need to randomize the hostname
hostName := pod.Spec.Hostname
// Patch the hostname only if it is not empty
// Patch the hostname only if it is empty
if webhook.config.EnableRandomHostName {
if hostName == "" {
hostName = generateUUID()
Expand All @@ -414,22 +416,52 @@ func (webhook *webhook) mutateCreateRequest(ctx context.Context, pod *corev1.Pod
"value": hostName,
})
} else {
// This will not happan in GMSA scenario, but added for completeness, just in case
return nil, &podAdmissionError{error: fmt.Errorf("can not set hostname in spec and also inidcating random hostname in ENV")}
// Will honor the hostname set in the spec, print out a message
logrus.Infof("hostname is set in spec and will be hornored instead of being randomized")
}
}

patchesBytes, err := json.Marshal(patches)
if err != nil {
return nil, &podAdmissionError{error: fmt.Errorf("unable to marshall patch JSON %v: %v", patches, err), pod: pod, code: http.StatusInternalServerError}
}
if len(patches) != 0 {
patchesBytes, err := json.Marshal(patches)
if err != nil {
return nil, &podAdmissionError{error: fmt.Errorf("unable to marshall patch JSON %v: %v", patches, err), pod: pod, code: http.StatusInternalServerError}
}

admissionResponse.Patch = patchesBytes
patchType := admissionV1.PatchTypeJSONPatch
admissionResponse.PatchType = &patchType
admissionResponse.Patch = patchesBytes
patchType := admissionV1.PatchTypeJSONPatch
admissionResponse.PatchType = &patchType
}
}

return admissionResponse, nil

// if len(patches) != 0 {
// // pods are GMSA related, we will need further check if we need to randomize the hostname
// hostName := pod.Spec.Hostname
// // Patch the hostname only if it is empty
// if webhook.config.EnableRandomHostName {
// if hostName == "" {
// hostName = generateUUID()
// patches = append(patches, map[string]string{
// "op": "add",
// "path": "/spec/hostname",
// "value": hostName,
// })
// } else {
// // This will not happan in GMSA scenario, but added for completeness, just in case
// return nil, &podAdmissionError{error: fmt.Errorf("can not set hostname in spec and also inidcating random hostname in ENV")}
// }
// }

// patchesBytes, err := json.Marshal(patches)
// if err != nil {
// return nil, &podAdmissionError{error: fmt.Errorf("unable to marshall patch JSON %v: %v", patches, err), pod: pod, code: http.StatusInternalServerError}
// }

// admissionResponse.Patch = patchesBytes
// patchType := admissionV1.PatchTypeJSONPatch
// admissionResponse.PatchType = &patchType
// }
}

// validateUpdateRequest ensures that there are no updates to any of the GMSA names or contents.
Expand Down Expand Up @@ -567,7 +599,7 @@ func (ln tcpKeepAliveListener) Accept() (net.Conn, error) {
func generateUUID() string {
// Generate a new UUID
id := uuid.New()
// Convert to string and get the first 8 characters in lower case
// Convert to string and get the first 15 characters in lower case
shortUUID := strings.ToLower(id.String()[:15])
return shortUUID
}
76 changes: 75 additions & 1 deletion admission-webhook/webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,29 @@ func TestMutateCreateRequest(t *testing.T) {
"with no GMSA settings, it passes and does nothing": func() *corev1.WindowsSecurityContextOptions {
return nil
},
} {
t.Run(testCaseName, func(t *testing.T) {
webhook := newWebhookWithOptions(nil, WithRandomHostname(false))
pod := buildPod(dummyServiceAccoutName, winOptionsFactory(), map[string]*corev1.WindowsSecurityContextOptions{dummyContainerName: winOptionsFactory()})

response, err := webhook.mutateCreateRequest(context.Background(), pod)
assert.Nil(t, err)

require.NotNil(t, response)
assert.True(t, response.Allowed)

assert.Nil(t, response.Patch)

})
}

for testCaseName, winOptionsFactory := range map[string]func() *corev1.WindowsSecurityContextOptions{
"with randome hostname env set and empty GMSA settings, it passes and does nothing": func() *corev1.WindowsSecurityContextOptions {
return &corev1.WindowsSecurityContextOptions{}
},
"with randome hostname env set and no GMSA settings, it passes and does nothing": func() *corev1.WindowsSecurityContextOptions {
return nil
},
} {
t.Run(testCaseName, func(t *testing.T) {
webhook := newWebhookWithOptions(nil, WithRandomHostname(true))
Expand All @@ -200,6 +223,57 @@ func TestMutateCreateRequest(t *testing.T) {
})
}

testCaseName, winOptionsFactory1 := "with randome hostname env set and dummy GMSA settings, it passes and set random hostname", func() *corev1.WindowsSecurityContextOptions {
dummyCredSpecNameVar := dummyCredSpecName
dummyCredSpecContentsVar := dummyCredSpecContents
return &corev1.WindowsSecurityContextOptions{GMSACredentialSpecName: &dummyCredSpecNameVar, GMSACredentialSpec: &dummyCredSpecContentsVar}
}
t.Run(testCaseName, func(t *testing.T) {
webhook := newWebhookWithOptions(nil, WithRandomHostname(true))
pod := buildPod(dummyServiceAccoutName, winOptionsFactory1(), map[string]*corev1.WindowsSecurityContextOptions{dummyContainerName: winOptionsFactory1()})

response, err := webhook.mutateCreateRequest(context.Background(), pod)
assert.Nil(t, err)

require.NotNil(t, response)
assert.True(t, response.Allowed)

var patches []map[string]string
// one more because we're adding the hostname
if err := json.Unmarshal(response.Patch, &patches); assert.Nil(t, err) && assert.Equal(t, 1, len(patches)) {
foundHostname := false
for _, patch := range patches {
if value, hasValue := patch["value"]; assert.True(t, hasValue) {
if patch["path"] == "/spec/hostname" {
foundHostname = true
assert.Equal(t, "add", patch["op"])
assert.Equal(t, 15, len(value))
}
}
}
assert.True(t, foundHostname)
}
})

testCaseName, winOptionsFactory1 = "with randome hostname env set and dummy GMSA settings and hostname set in spec, it passes and do nothing", func() *corev1.WindowsSecurityContextOptions {
dummyCredSpecNameVar := dummyCredSpecName
dummyCredSpecContentsVar := dummyCredSpecContents
return &corev1.WindowsSecurityContextOptions{GMSACredentialSpecName: &dummyCredSpecNameVar, GMSACredentialSpec: &dummyCredSpecContentsVar}
}
t.Run(testCaseName, func(t *testing.T) {
webhook := newWebhookWithOptions(nil, WithRandomHostname(true))
dummyPodNameVar := dummyPodName
pod := buildPodWithHostName(dummyServiceAccoutName, &dummyPodNameVar, winOptionsFactory1(), map[string]*corev1.WindowsSecurityContextOptions{dummyContainerName: winOptionsFactory1()})

response, err := webhook.mutateCreateRequest(context.Background(), pod)
assert.Nil(t, err)

require.NotNil(t, response)
assert.True(t, response.Allowed)

assert.Nil(t, response.Patch)
})

kubeClientFactory := func() *dummyKubeClient {
return &dummyKubeClient{
retrieveCredSpecContentsFunc: func(ctx context.Context, credSpecName string) (contents string, httpCode int, err error) {
Expand All @@ -218,7 +292,7 @@ func TestMutateCreateRequest(t *testing.T) {
}

runWebhookValidateOrMutateTests(t, winOptionsFactory, map[string]webhookValidateOrMutateTest{
"with a GMSA cred spec name, it passes and inlines the cred-spec's contents": func(t *testing.T, pod *corev1.Pod, optionsSelector winOptionsSelector, resourceKind gmsaResourceKind, resourceName string) {
"with random hostname env and a GMSA cred spec name, it passes and inlines the cred-spec's contents and generate random hostname": func(t *testing.T, pod *corev1.Pod, optionsSelector winOptionsSelector, resourceKind gmsaResourceKind, resourceName string) {
webhook := newWebhookWithOptions(kubeClientFactory(), WithRandomHostname(true))

setWindowsOptions(optionsSelector(pod), dummyCredSpecName, "")
Expand Down

0 comments on commit 5d4918d

Please sign in to comment.