Disable host process containers when disable unsafe operations is enabled #2164
Conversation
pkg/annotations/annotations.go
Outdated
| @@ -322,5 +322,6 @@ var AnnotationExpansions = map[string][]string{ | |||
| WCOWDisableGMSA, | |||
| DisableWritableFileShares, | |||
| VSMBNoDirectMap, | |||
| HostProcessContainer, | |||
There was a problem hiding this comment.
shouldn't this be set to false, when disable unsafe annotations is true?
There was a problem hiding this comment.
oh yeah, i missed that
either an annotation for DisableHostProcessContainer needs to be created, or we need an AnnotationDenyList thingy
There was a problem hiding this comment.
@helsaawy I added a DisableHostProcessContainer annotation and updated the PR
katiewasnothere
force-pushed
the
kabaldau/host_process_unsafe_op
branch
from
4767980
to
3fdc8da
Compare
|
@helsaawy and @anmaxvl I reworked this PR to instead change how we handle annotation expanding. Instead of the annotations sharing the same value as the parent annotation, I added the ability to set a target value for the child annotations. Do y'all have any thoughts on this approach? Note: optionally in the future we could even expand this to use parent annotations that are not true or false. For example, I could envision an annotation like "memorybacking" and when that annotation's value is "physical" then it could expand with child annotations allowovercommit to false and vpmemdevicecount to 0, like we have for the annotation "fullyphysicallybacked" |
There was a problem hiding this comment.
i think this implementation breaks the logic where we assume that parent annotation's must match the value of their expansions, and don't try to parse values as booleans.
i would rather we either:
- double down on the bool approach, and have
AnnotationExpansionsbemap[string]map[string]bool; or - add an
DisableHostProcessContainerthat is the antithesis of theHostProcessContainerannotation, with a separate check after annotation expansion that only one of the two can be defined
pkg/annotations/annotations.go
Outdated
| var AnnotationExpansions = map[string][]string{ | ||
| // other annotations and their values. It is an error for the expansions to already | ||
| // exist and have a value that differs from the value set here. | ||
| var AnnotationExpansions = map[string]map[string]string{ |
There was a problem hiding this comment.
since we are already changing the type, which is breaking, can we make it:
| var AnnotationExpansions = map[string]map[string]string{ | |
| func AnnotationExpansions() map[string]map[string]string { |
that way, no-one can (inadvertently) modify the map
There was a problem hiding this comment.
Could we just make it a const instead?
annotation to DisableUnsafeOperations Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
katiewasnothere
force-pushed
the
kabaldau/host_process_unsafe_op
branch
from
3fdc8da
to
f5f103d
Compare
| @@ -55,6 +56,19 @@ func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) { | |||
| } | |||
| } | |||
|
|
|||
| // validate host process containers annotations are not conflicting | |||
| disableHPC := ParseAnnotationsBool(ctx, s.Annotations, annotations.DisableHostProcessContainer, false) | |||
There was a problem hiding this comment.
This is probably the only annotation that needs to be handled like this for now. In the future we may want to consider defining a "conflict annotations map" or similar, e.g.
conflictingAnnotations := map[string]string{
annotations.DisableHostProcessContainer: annotations.HostProcessContainer,
}
for k, v := range conflictingAnnotations {
v1 := ParseAnnotationsBool(ctx, s.Annotations, k, false)
v2 := ParseAnnotationsBool(ctx, s.Annotations, v, false)
if v1 && v2 {
// handle conflict
}
}
Maybe it's worth just doing it now too, rather than special casing it to HPC? thoughts?
Host process containers are not safe for multi-tenant scenarios since they run directly on the host and use the host's network. Don't allow host process containers to be used when
DisableUnsafeOperationsis set.