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

feat: add deny-label-ns flag which supports wildcard #1051

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 cmd/nfd-master/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,15 @@ func main() {
func initFlags(flagset *flag.FlagSet) *master.Args {
args := &master.Args{
LabelWhiteList: utils.RegexpVal{Regexp: *regexp.MustCompile("")},
DenyLabelNs: map[string]struct{}{"*.kubernetes.io": {}},
}

flagset.StringVar(&args.CaFile, "ca-file", "",
"Root certificate for verifying connections")
flagset.StringVar(&args.CertFile, "cert-file", "",
"Certificate used for authenticating connections")
flagset.Var(&args.DenyLabelNs, "deny-label-ns",
"Comma separated list of denied label namespaces")
flagset.Var(&args.ExtraLabelNs, "extra-label-ns",
Copy link
Contributor

Choose a reason for hiding this comment

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

@marquiz should we have both flags at least for a release, having ExtraLabelNS to print a deprecation warning? I feel that removing the flag all together could break some users deployments if they try a roll-update and the pod get's called with old flags.
WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

@marquiz suggested that we keep the extra-label-ns flag because that would give flexibility to our users.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArangoGutierrez yes, I don't see any reason removing the existing flag. It will also give flexibility in enabling scenarios like "deny all except for example.com/ and acme.io/"

"Comma separated list of allowed extra label namespaces")
flagset.StringVar(&args.Instance, "instance", "",
Expand Down
3 changes: 3 additions & 0 deletions deployment/helm/node-feature-discovery/templates/master.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ spec:
{{- if .Values.master.extraLabelNs | empty | not }}
- "-extra-label-ns={{- join "," .Values.master.extraLabelNs }}"
{{- end }}
{{- if .Values.master.denyLabelNs | empty | not }}
- "-deny-label-ns={{- join "," .Values.master.denyLabelNs }}"
{{- end }}
{{- if .Values.master.resourceLabels | empty | not }}
- "-resource-labels={{- join "," .Values.master.resourceLabels }}"
{{- end }}
Expand Down
1 change: 1 addition & 0 deletions deployment/helm/node-feature-discovery/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ master:
port: 8080
instance:
featureApi:
denyLabelNs: []
extraLabelNs: []
resourceLabels: []
crdController: null
Expand Down
30 changes: 25 additions & 5 deletions docs/reference/master-commandline-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,12 +216,10 @@ nfd-master -label-whitelist='.*cpuid\.'
### -extra-label-ns

The `-extra-label-ns` flag specifies a comma-separated list of allowed feature
label namespaces. By default, nfd-master only allows creating labels in the
default `feature.node.kubernetes.io` and `profile.node.kubernetes.io` label
namespaces and their sub-namespaces (e.g. `vendor.feature.node.kubernetes.io`
and `sub.ns.profile.node.kubernetes.io`). This option can be used to allow
label namespaces. This option can be used to allow
other vendor or application specific namespaces for custom labels from the
local and custom feature sources.
local and custom feature sources, even though these labels were denied using
the `deny-label-ns` flag.

The same namespace control and this flag applies Extended Resources (created
with `-resource-labels`), too.
Expand All @@ -234,6 +232,28 @@ Example:
nfd-master -extra-label-ns=vendor-1.com,vendor-2.io
```

### -deny-label-ns

The `-deny-label-ns` flag specifies a comma-separated list of excluded
label namespaces. By default, nfd-master allows creating labels in all
namespaces, excluding `kubernetes.io` namespace and its sub-namespaces
(i.e. `*.kubernetes.io`). However, you should note that
`kubernetes.io` and its sub-namespaces are always denied.
For example, `nfd-master -deny-label-ns=""` would still disallow
`kubernetes.io` and `*.kubernetes.io`.
This option can be used to exclude some vendors or application specific
namespaces.
Note that the namespaces `feature.node.kubernetes.io` and `profile.node.kubernetes.io`
and their sub-namespaces are always allowed and cannot be denied.

Default: *empty*

Example:

```bash
nfd-master -deny-label-ns=*.vendor.com,vendor-2.io
```

### -resource-labels

The `-resource-labels` flag specifies a comma-separated list of features to be
Expand Down
21 changes: 7 additions & 14 deletions docs/usage/customization-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,6 @@ feature.node.kubernetes.io/my-feature.2: "myvalue"
my.namespace/my-feature.3: "456"
```

Note that in the example above `-extra-label-ns=my.namespace` must be specified
on the nfd-master command line.

### Hooks

**DEPRECATED** The `local` source executes hooks found in
Expand Down Expand Up @@ -303,11 +300,7 @@ key-value pairs, separated by newlines:

The label value defaults to `true`, if not specified.

Label namespace may be specified with `<namespace>/<name>[=<value>]`. The
namespace must be explicitly allowed with the `-extra-label-ns` command line
flag of nfd-master if using something else than
`[<sub-ns>.]feature.node.kubernetes.io` or
`[<sub-ns>.]profile.node.kubernetes.io`.
Label namespace may be specified with `<namespace>/<name>[=<value>]`.

### Mounts

Expand Down Expand Up @@ -414,13 +407,13 @@ The namespace part (i.e. prefix) of the labels is controlled by nfd:

- All built-in labels use `feature.node.kubernetes.io`. This is also
the default for user defined features that don't specify any namespace.
- User-defined labels are allowed to use:
- `feature.node.kubernetes.io` and `profile.node.kubernetes.io` plus their
sub-namespaces (e.g. `vendor.profile.node.kubernetes.io` and
`sub.ns.profile.node.kubernetes.io`) by default
- Additional namespaces may be enabled with the
- Namespaces may be excluded with the
[`-deny-label-ns`](../reference/master-commandline-reference.md#-deny-label-ns)
command line flag of nfd-master
- To allow specific namespaces that were denied, you can use
[`-extra-label-ns`](../reference/master-commandline-reference.md#-extra-label-ns)
command line flag of nfd-master
command line flag of nfd-master.
e.g: `nfd-master -deny-label-ns="*" -extra-label-ns=example.com`

## Label rule format

Expand Down
14 changes: 9 additions & 5 deletions pkg/nfd-master/nfd-master-internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,16 +352,18 @@ func TestSetLabels(t *testing.T) {
})
})

Convey("When -extra-label-ns and -instance are specified", func() {
Convey("When -extra-label-ns, -deny-label-ns and -instance are specified", func() {
// In the gRPC request the label names may omit the default ns
instance := "foo"
vendorFeatureLabel := "vendor." + nfdv1alpha1.FeatureLabelNs + "/feature-4"
vendorProfileLabel := "vendor." + nfdv1alpha1.ProfileLabelNs + "/feature-5"
mockLabels := map[string]string{"feature-1": "val-1",
"valid.ns/feature-2": "val-2",
"invalid.ns/feature-3": "val-3",
vendorFeatureLabel: " val-4",
vendorProfileLabel: " val-5"}
"valid.ns/feature-2": "val-2",
"random.denied.ns/feature-3": "val-3",
"kubernetes.io/feature-4": "val-4",
"sub.ns.kubernetes.io/feature-5": "val-5",
vendorFeatureLabel: " val-6",
vendorProfileLabel: " val-7"}
expectedPatches := []apihelper.JsonPatch{
apihelper.NewJsonPatch("add", "/metadata/annotations", instance+"."+nfdv1alpha1.WorkerVersionAnnotation, workerVer),
apihelper.NewJsonPatch("add", "/metadata/annotations",
Expand All @@ -374,6 +376,8 @@ func TestSetLabels(t *testing.T) {
apihelper.NewJsonPatch("add", "/metadata/labels", vendorProfileLabel, mockLabels[vendorProfileLabel]),
}

mockMaster.deniedNs.normal = map[string]struct{}{"random.denied.ns": {}}
mockMaster.deniedNs.wildcard = map[string]struct{}{"kubernetes.io": {}}
mockMaster.args.ExtraLabelNs = map[string]struct{}{"valid.ns": {}}
mockMaster.args.Instance = instance
mockHelper.On("GetClient").Return(mockClient, nil)
Expand Down
66 changes: 58 additions & 8 deletions pkg/nfd-master/nfd-master.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ type Annotations map[string]string
type Args struct {
CaFile string
CertFile string
DenyLabelNs utils.StringSetVal
ExtraLabelNs utils.StringSetVal
Instance string
KeyFile string
Expand All @@ -78,6 +79,11 @@ type Args struct {
ResourceLabels utils.StringSetVal
}

type deniedNs struct {
normal utils.StringSetVal
wildcard utils.StringSetVal
}

type NfdMaster interface {
Run() error
Stop()
Expand All @@ -95,6 +101,7 @@ type nfdMaster struct {
ready chan bool
apihelper apihelper.APIHelpers
kubeconfig *restclient.Config
deniedNs
}

// NewNfdMaster creates a new NfdMaster server instance.
Expand Down Expand Up @@ -126,6 +133,16 @@ func NewNfdMaster(args *Args) (NfdMaster, error) {
return nfd, fmt.Errorf("-ca-file needs to be specified alongside -cert-file and -key-file")
}
}
if args.DenyLabelNs == nil {
args.DenyLabelNs = make(utils.StringSetVal)
}
// Pre-process DenyLabelNS into 2 lists: one for normal ns, and the other for wildcard ns
normalDeniedNs, wildcardDeniedNs := preProcessDeniedNamespaces(args.DenyLabelNs)
nfd.deniedNs.normal = normalDeniedNs
nfd.deniedNs.wildcard = wildcardDeniedNs
// We forcibly deny kubernetes.io
nfd.deniedNs.normal["kubernetes.io"] = struct{}{}
nfd.deniedNs.wildcard[".kubernetes.io"] = struct{}{}

// Initialize Kubernetes API helpers
if !args.NoPublish {
Expand Down Expand Up @@ -392,7 +409,7 @@ func (m *nfdMaster) updateMasterNode() error {
// into extended resources. This function also handles proper namespacing of
// labels and ERs, i.e. adds the possibly missing default namespace for labels
// arriving through the gRPC API.
func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelWhiteList regexp.Regexp, extendedResourceNames map[string]struct{}) (Labels, ExtendedResources) {
func (m *nfdMaster) filterFeatureLabels(labels Labels) (Labels, ExtendedResources) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, this looks so much cleaner 👍

outLabels := Labels{}

for label, value := range labels {
Expand All @@ -404,23 +421,26 @@ func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelW
// Check label namespace, filter out if ns is not whitelisted
if ns != nfdv1alpha1.FeatureLabelNs && ns != nfdv1alpha1.ProfileLabelNs &&
!strings.HasSuffix(ns, nfdv1alpha1.FeatureLabelSubNsSuffix) && !strings.HasSuffix(ns, nfdv1alpha1.ProfileLabelSubNsSuffix) {
if _, ok := extraLabelNs[ns]; !ok {
klog.Errorf("Namespace %q is not allowed. Ignoring label %q\n", ns, label)
continue
// If the namespace is denied, and not present in the extraLabelNs, label will be ignored
if isNamespaceDenied(ns, m.deniedNs.wildcard, m.deniedNs.normal) {
if _, ok := m.args.ExtraLabelNs[ns]; !ok {
klog.Errorf("Namespace %q is not allowed. Ignoring label %q\n", ns, label)
continue
}
}
}

// Skip if label doesn't match labelWhiteList
if !labelWhiteList.MatchString(name) {
klog.Errorf("%s (%s) does not match the whitelist (%s) and will not be published.", name, label, labelWhiteList.String())
if !m.args.LabelWhiteList.Regexp.MatchString(name) {
klog.Errorf("%s (%s) does not match the whitelist (%s) and will not be published.", name, label, m.args.LabelWhiteList.Regexp.String())
continue
}
outLabels[label] = value
}

// Remove labels which are intended to be extended resources
extendedResources := ExtendedResources{}
for extendedResourceName := range extendedResourceNames {
for extendedResourceName := range m.args.ResourceLabels {
// Add possibly missing default ns
extendedResourceName = addNs(extendedResourceName, nfdv1alpha1.FeatureLabelNs)
if value, ok := outLabels[extendedResourceName]; ok {
Expand Down Expand Up @@ -449,6 +469,20 @@ func verifyNodeName(cert *x509.Certificate, nodeName string) error {
return nil
}

func isNamespaceDenied(labelNs string, wildcardDeniedNs map[string]struct{}, normalDeniedNs map[string]struct{}) bool {
for deniedNs := range normalDeniedNs {
if labelNs == deniedNs {
return true
}
}
for deniedNs := range wildcardDeniedNs {
if strings.HasSuffix(labelNs, deniedNs) {
return true
}
}
return false
}

// SetLabels implements LabelerServer
func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.SetLabelsReply, error) {
err := authorizeClient(c, m.args.VerifyNodeName, r.NodeName)
Expand Down Expand Up @@ -583,7 +617,7 @@ func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName stri
labels[k] = v
}

labels, extendedResources := filterFeatureLabels(labels, m.args.ExtraLabelNs, m.args.LabelWhiteList.Regexp, m.args.ResourceLabels)
labels, extendedResources := m.filterFeatureLabels(labels)

var taints []corev1.Taint
if m.args.EnableTaints {
Expand Down Expand Up @@ -918,6 +952,22 @@ func stringToNsNames(cslist, ns string) []string {
return names
}

// Seperate denied namespaces into two lists:
// one contains wildcard namespaces the other contains normal namespaces
func preProcessDeniedNamespaces(deniedNs map[string]struct{}) (normalDeniedNs map[string]struct{}, wildcardDeniedNs map[string]struct{}) {
normalDeniedNs = map[string]struct{}{}
wildcardDeniedNs = map[string]struct{}{}
for ns := range deniedNs {
if strings.HasPrefix(ns, "*") {
trimedNs := strings.TrimLeft(ns, "*")
wildcardDeniedNs[trimedNs] = struct{}{}
} else {
normalDeniedNs[ns] = struct{}{}
}
}
return
}

func (m *nfdMaster) instanceAnnotation(name string) string {
if m.args.Instance == "" {
return name
Expand Down
3 changes: 1 addition & 2 deletions pkg/utils/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ func (a *StringSetVal) String() string {
if *a == nil {
return ""
}

vals := make([]string, len(*a), 0)
vals := make([]string, 0, len(*a))
marquiz marked this conversation as resolved.
Show resolved Hide resolved
for val := range *a {
vals = append(vals, val)
}
Expand Down