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.

Sign up for GitHub

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

VAULT-14149 Don't override shareProcessNamespace unless annotation is explicitly set #445

Merged
merged 4 commits into from
Mar 20, 2023
Merged

VAULT-14149 Don't override shareProcessNamespace unless annotation is explicitly set #445

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0573487
Don't override shareProcessNamespace unless annotation is explicitly set
Mar 17, 2023
1dfdd37
Use pointer.Bool
Mar 17, 2023
1107836
go mod tidy
Mar 17, 2023
de96d61
Add test for shareProcessNamespace annotation
Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ Changes:
* golang.org/x/term v0.3.0 => v0.5.0
* golang.org/x/text v0.5.0 => v0.7.0

Bugs:
* Don't override `shareProcessNamespace` if an annotation is not present [GH-445](https://github.com/hashicorp/vault-k8s/pull/445)

## 1.2.0 (February 6, 2023)

Changes:
Expand Down
13 changes: 9 additions & 4 deletions agent-inject/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
jsonpatch "github.com/evanphx/json-patch"
"github.com/hashicorp/vault/sdk/helper/strutil"
corev1 "k8s.io/api/core/v1"
"k8s.io/utils/pointer"
)

const (
Expand All @@ -22,7 +23,6 @@ const (
DefaultAgentRunAsUser = 100
DefaultAgentRunAsGroup = 1000
DefaultAgentRunAsSameUser = false
DefaultAgentShareProcessNamespace = false
DefaultAgentAllowPrivilegeEscalation = false
DefaultAgentDropCapabilities = "ALL"
DefaultAgentSetSecurityContext = true
Expand Down Expand Up @@ -142,7 +142,7 @@ type Agent struct {
RunAsSameID bool

// ShareProcessNamespace sets the shareProcessNamespace value on the pod spec.
ShareProcessNamespace bool
ShareProcessNamespace *bool

// SetSecurityContext controls whether the injected containers have a
// SecurityContext set.
Expand Down Expand Up @@ -445,10 +445,13 @@ func New(pod *corev1.Pod) (*Agent, error) {
return agent, err
}

agent.ShareProcessNamespace, err = agent.setShareProcessNamespace(pod)
setShareProcessNamespace, ok, err := agent.setShareProcessNamespace(pod)
if err != nil {
return agent, err
}
if ok {
agent.ShareProcessNamespace = pointer.Bool(setShareProcessNamespace)
}

agent.SetSecurityContext, err = agent.setSecurityContext()
if err != nil {
Expand Down Expand Up @@ -655,7 +658,9 @@ func (a *Agent) Patch() ([]byte, error) {
}

// Add shareProcessNamespace
patches = append(patches, updateShareProcessNamespace(a.ShareProcessNamespace)...)
if a.ShareProcessNamespace != nil {
patches = append(patches, updateShareProcessNamespace(*a.ShareProcessNamespace)...)
}
}

// Sidecar Container
Expand Down
16 changes: 6 additions & 10 deletions agent-inject/agent/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,6 @@ func Init(pod *corev1.Pod, cfg AgentConfig) error {
var runAsUserIsSet bool
var runAsSameUserIsSet bool
var runAsGroupIsSet bool
var shareProcessNamespaceIsSet bool

if pod == nil {
return errors.New("pod is empty")
Expand Down Expand Up @@ -469,10 +468,6 @@ func Init(pod *corev1.Pod, cfg AgentConfig) error {
pod.ObjectMeta.Annotations[AnnotationAgentRunAsSameUser] = strconv.FormatBool(cfg.SameID)
}

if _, shareProcessNamespaceIsSet = pod.ObjectMeta.Annotations[AnnotationAgentShareProcessNamespace]; !shareProcessNamespaceIsSet {
pod.ObjectMeta.Annotations[AnnotationAgentShareProcessNamespace] = strconv.FormatBool(cfg.ShareProcessNamespace)
}

if _, runAsGroupIsSet = pod.ObjectMeta.Annotations[AnnotationAgentRunAsGroup]; !runAsGroupIsSet {
if cfg.GroupID == "" {
cfg.GroupID = strconv.Itoa(DefaultAgentRunAsGroup)
Expand Down Expand Up @@ -754,26 +749,27 @@ func (a *Agent) runAsSameID(pod *corev1.Pod) (bool, error) {
return runAsSameID, nil
}

func (a *Agent) setShareProcessNamespace(pod *corev1.Pod) (bool, error) {
// returns value, ok, error
func (a *Agent) setShareProcessNamespace(pod *corev1.Pod) (bool, bool, error) {
annotation := AnnotationAgentShareProcessNamespace
raw, ok := a.Annotations[annotation]
if !ok {
return DefaultAgentShareProcessNamespace, nil
return false, false, nil
}
shareProcessNamespace, err := strconv.ParseBool(raw)
if err != nil {
return DefaultAgentShareProcessNamespace, fmt.Errorf(
return false, true, fmt.Errorf(
"invalid value %v for annotation %q, err=%w", raw, annotation, err)
}
if pod.Spec.ShareProcessNamespace != nil {
if !*pod.Spec.ShareProcessNamespace && shareProcessNamespace {
return DefaultAgentShareProcessNamespace,
return false, true,
errors.New("shareProcessNamespace explicitly disabled on the pod, " +
"refusing to enable it")
}
}

return shareProcessNamespace, nil
return shareProcessNamespace, true, nil
}

func (a *Agent) setSecurityContext() (bool, error) {
Expand Down
12 changes: 7 additions & 5 deletions agent-inject/agent/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,17 @@ func TestInitDefaults(t *testing.T) {
{annotationKey: AnnotationAgentImage, annotationValue: DefaultVaultImage},
{annotationKey: AnnotationAgentRunAsUser, annotationValue: strconv.Itoa(DefaultAgentRunAsUser)},
{annotationKey: AnnotationAgentRunAsGroup, annotationValue: strconv.Itoa(DefaultAgentRunAsGroup)},
{annotationKey: AnnotationAgentShareProcessNamespace, annotationValue: strconv.FormatBool(DefaultAgentShareProcessNamespace)},
{annotationKey: AnnotationAgentShareProcessNamespace, annotationValue: ""},
}

for _, tt := range tests {
raw, ok := pod.Annotations[tt.annotationKey]
if !ok {
if tt.annotationValue == "" && !ok {
// okay, we expected it not to be set
continue
} else if tt.annotationValue == "" && ok {
t.Errorf("Default annotation value incorrect, wanted unset, got %s", raw)
} else if !ok {
t.Errorf("Default annotation %s not set, it should be.", tt.annotationKey)
}

Expand Down Expand Up @@ -988,7 +993,6 @@ func TestInjectContainers(t *testing.T) {
internal.AddOp("/spec/containers/1/volumeMounts/-", nil),
internal.AddOp("/spec/containers/2/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(AnnotationAgentStatus), nil),
},
Expand All @@ -1003,7 +1007,6 @@ func TestInjectContainers(t *testing.T) {
internal.AddOp("/spec/volumes", nil),
internal.AddOp("/spec/containers/1/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(AnnotationAgentStatus), nil),
},
Expand All @@ -1019,7 +1022,6 @@ func TestInjectContainers(t *testing.T) {
internal.AddOp("/spec/containers/1/volumeMounts/-", nil),
internal.AddOp("/spec/containers/2/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(AnnotationAgentStatus), nil),
},
Expand Down
7 changes: 0 additions & 7 deletions agent-inject/handler_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to add some tests in here for shareProcessNamespace?

Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
Expand Down Expand Up @@ -192,7 +191,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/initContainers", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/1/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
Expand Down Expand Up @@ -222,7 +220,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
Expand Down Expand Up @@ -253,7 +250,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
Expand Down Expand Up @@ -284,7 +280,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
Expand Down Expand Up @@ -343,7 +338,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
},
Expand Down Expand Up @@ -372,7 +366,6 @@ func TestHandlerHandle(t *testing.T) {
internal.AddOp("/spec/containers/0/volumeMounts/-", nil),
internal.AddOp("/spec/initContainers/-", nil),
internal.AddOp("/spec/initContainers/0/volumeMounts/-", nil),
internal.AddOp("/spec/shareProcessNamespace", nil),
internal.AddOp("/spec/containers/-", nil),
internal.AddOp("/metadata/annotations/"+internal.EscapeJSONPointer(agent.AnnotationAgentStatus), nil),
},
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ require (
k8s.io/api v0.25.4
k8s.io/apimachinery v0.25.4
k8s.io/client-go v0.25.4
k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed
)

require (
Expand Down Expand Up @@ -86,7 +87,6 @@ require (
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/klog/v2 v2.70.1 // indirect
k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect
k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed // indirect
sigs.k8s.io/controller-runtime v0.12.1 // indirect
sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
Expand Down