feat: Support HelmAddon strategy to deploy NFD

Author: jimmidyson

api/v1alpha1/addon_types.go

@@ -80,12 +80,26 @@ func (CNI) VariableSchema() clusterv1.VariableSchema {
 }
 
 // NFD tells us to enable or disable the node feature discovery addon.
-type NFD struct{}
+type NFD struct {
+	// +optional
+	Strategy AddonStrategy `json:"strategy,omitempty"`
+}
 
 func (NFD) VariableSchema() clusterv1.VariableSchema {
 	return clusterv1.VariableSchema{
 		OpenAPIV3Schema: clusterv1.JSONSchemaProps{
 			Type: "object",
+			Properties: map[string]clusterv1.JSONSchemaProps{
+				"strategy": {
+					Description: "Addon strategy used to deploy the CNI provider to the workload cluster",
+					Type:        "string",
+					Enum: variables.MustMarshalValuesToEnumJSON(
+						AddonStrategyClusterResourceSet,
+						AddonStrategyHelmAddon,
+					),
+				},
+			},
+			Required: []string{"strategy"},
 		},
 	}
 }

api/v1alpha1/constants.go

@@ -4,8 +4,10 @@
 package v1alpha1
 
 const (
-	// CNIVariableName is the external patch variable name.
+	// CNIVariableName is the CNI external patch variable name.
 	CNIVariableName = "cni"
+	// NFDVariableName is the NFD external patch variable name.
+	NFDVariableName = "nfd"
 	// AWSVariableName is the AWS config patch variable name.
 	AWSVariableName = "aws"
 )

charts/capi-runtime-extensions/README.md

@@ -46,6 +46,9 @@ A Helm chart for capi-runtime-extensions
 | hooks.cni.cilium.crsStrategy.defaultCiliumConfigMap.name | string | `"cilium"` |  |
 | hooks.cni.cilium.helmAddonStrategy.defaultValueTemplateConfigMap.create | bool | `true` |  |
 | hooks.cni.cilium.helmAddonStrategy.defaultValueTemplateConfigMap.name | string | `"default-cilium-cni-helm-values-template"` |  |
+| hooks.nfd.crsStrategy.defaultInstallationConfigMap.name | string | `"node-feature-discovery"` |  |
+| hooks.nfd.helmAddonStrategy.defaultValueTemplateConfigMap.create | bool | `true` |  |
+| hooks.nfd.helmAddonStrategy.defaultValueTemplateConfigMap.name | string | `"default-nfd-helm-values-template"` |  |
 | image.pullPolicy | string | `"IfNotPresent"` |  |
 | image.repository | string | `"ghcr.io/d2iq-labs/capi-runtime-extensions"` |  |
 | image.tag | string | `""` |  |

charts/capi-runtime-extensions/templates/deployment.yaml

@@ -34,7 +34,9 @@ spec:
         - --cni.cilium.crs.defaults-namespace={{ .Release.Namespace }}
         - --cni.cilium.helm-addon.defaults-namespace={{ .Release.Namespace }}
         - --cni.cilium.helm-addon.default-values-template-configmap-name={{ .Values.hooks.cni.cilium.helmAddonStrategy.defaultValueTemplateConfigMap.name }}
-        - --nfd.defaults-namespace={{ .Release.Namespace }}
+        - --nfd.crs.defaults-namespace={{ .Release.Namespace }}
+        - --nfd.helm-addon.defaults-namespace={{ .Release.Namespace }}
+        - --nfd.helm-addon.default-values-template-configmap-name={{ .Values.hooks.nfd.helmAddonStrategy.defaultValueTemplateConfigMap.name }}
         {{- range $key, $value := .Values.extraArgs }}
         - --{{ $key }}={{ $value }}
         {{- end }}

charts/capi-runtime-extensions/templates/nfd/manifests/helm-addon-installation.yaml

@@ -0,0 +1,30 @@
+# Copyright 2024 D2iQ, Inc. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+{{- if .Values.hooks.nfd.helmAddonStrategy.defaultValueTemplateConfigMap.create }}
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: '{{ .Values.hooks.nfd.helmAddonStrategy.defaultValueTemplateConfigMap.name }}'
+data:
+  values.yaml: |-
+    master:
+      extraLabelNs:
+        - nvidia.com
+        - beta.amd.com
+        - amd.com
+
+    worker: ### <NFD-WORKER-CONF-START-DO-NOT-REMOVE>
+      config:
+        sources:
+          pci:
+            deviceLabelFields:
+              - "class"
+              - "vendor"
+      tolerations:
+        - effect: NoSchedule
+          key: node-role.kubernetes.io/master
+        - effect: NoSchedule
+          key: node-role.kubernetes.io/control-plane
+    ### <NFD-WORKER-CONF-END-DO-NOT-REMOVE>
+{{- end -}}

charts/capi-runtime-extensions/templates/nfd/manifests/node-feature-discovery-configmap.yaml

@@ -924,4 +924,4 @@ data:
 kind: ConfigMap
 metadata:
   creationTimestamp: null
-  name: node-feature-discovery
+  name: '{{ .Values.hooks.nfd.crsStrategy.defaultInstallationConfigMap.name }}'

charts/capi-runtime-extensions/values.yaml

@@ -35,6 +35,14 @@ hooks:
         defaultValueTemplateConfigMap:
           create: true
           name: default-cilium-cni-helm-values-template
+  nfd:
+    crsStrategy:
+      defaultInstallationConfigMap:
+        name: node-feature-discovery
+    helmAddonStrategy:
+      defaultValueTemplateConfigMap:
+        create: true
+        name: default-nfd-helm-values-template
 
 deployDefaultClusterClasses: true
 

docs/content/addons/nfd.md

@@ -25,5 +25,6 @@ spec:
       - name: clusterConfig
         value:
           addons:
-            nfd: {}
+            nfd:
+              strategy: HelmAddon
 ```

hack/addons/update-node-feature-discovery-manifests.sh

@@ -24,7 +24,7 @@ envsubst -no-unset <"${KUSTOMIZE_BASE_DIR}/kustomization.yaml.tmpl" >"${ASSETS_D
 cp "${KUSTOMIZE_BASE_DIR}"/*.yaml "${ASSETS_DIR}"
 kustomize build --enable-helm "${ASSETS_DIR}" >"${ASSETS_DIR}/${FILE_NAME}"
 
-kubectl create configmap node-feature-discovery --dry-run=client --output yaml \
+kubectl create configmap "{{ .Values.hooks.nfd.crsStrategy.defaultInstallationConfigMap.name }}" --dry-run=client --output yaml \
   --from-file "${ASSETS_DIR}/${FILE_NAME}" \
   >"${ASSETS_DIR}/node-feature-discovery-configmap.yaml"
 

make/addons.mk

@@ -3,7 +3,7 @@
 
 export CALICO_VERSION := $(shell goprintconst -file pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go -name defaultCalicoHelmChartVersion)
 export CILIUM_VERSION := $(shell goprintconst -file pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go -name defaultCiliumHelmChartVersion)
-export NODE_FEATURE_DISCOVERY_VERSION := 0.14.1
+export NODE_FEATURE_DISCOVERY_VERSION := $(shell goprintconst -file pkg/handlers/generic/lifecycle/nfd/strategy_helmaddon.go -name defaultHelmChartVersion)
 export AWS_CSI_SNAPSHOT_CONTROLLER_VERSION := v6.3.0
 export AWS_EBS_CSI_VERSION := v1.25.0
 # a map of AWS CPI versions

pkg/handlers/generic/lifecycle/handlers.go

@@ -22,7 +22,7 @@ import (
 type Handlers struct {
 	calicoCNIConfig *calico.CNIConfig
 	ciliumCNIConfig *cilium.CNIConfig
-	nfdConfig       *nfd.NFDConfig
+	nfdConfig       *nfd.Config
 	ebsConfig       *awsebs.AWSEBSConfig
 	awsCPIConfig    *awscpi.AWSCPIConfig
 }
@@ -31,7 +31,7 @@ func New() *Handlers {
 	return &Handlers{
 		calicoCNIConfig: &calico.CNIConfig{},
 		ciliumCNIConfig: &cilium.CNIConfig{},
-		nfdConfig:       &nfd.NFDConfig{},
+		nfdConfig:       &nfd.Config{},
 		ebsConfig:       &awsebs.AWSEBSConfig{},
 		awsCPIConfig:    &awscpi.AWSCPIConfig{},
 	}

pkg/handlers/generic/lifecycle/nfd/handler.go

@@ -7,62 +7,55 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
+	commonhandlers "github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
+	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/lifecycle"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/variables"
-	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/k8s/client"
 	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers/generic/clusterconfig"
-	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers/generic/lifecycle/utils"
 )
 
-type NFDConfig struct {
-	defaultsNamespace   string
-	defaultNFDConfigMap string
+type addonStrategy interface {
+	apply(context.Context, *runtimehooksv1.AfterControlPlaneInitializedRequest, logr.Logger) error
+}
+
+type Config struct {
+	crsConfig       crsConfig
+	helmAddonConfig helmAddonConfig
+}
+
+func (c *Config) AddFlags(prefix string, flags *pflag.FlagSet) {
+	c.crsConfig.AddFlags(prefix+".crs", flags)
+	c.helmAddonConfig.AddFlags(prefix+".helm-addon", flags)
 }
 
 type DefaultNFD struct {
 	client ctrlclient.Client
-	config *NFDConfig
+	config *Config
 
 	variableName string   // points to the global config variable
 	variablePath []string // path of this variable on the global config variable
 }
 
-func (n *NFDConfig) AddFlags(prefix string, flags *pflag.FlagSet) {
-	flags.StringVar(
-		&n.defaultsNamespace,
-		prefix+".defaults-namespace",
-		corev1.NamespaceDefault,
-		"namespace location of ConfigMap used to deploy Node Feature Discovery (NFD).",
-	)
-	flags.StringVar(
-		&n.defaultNFDConfigMap,
-		prefix+".default-nfd-configmap-name",
-		"node-feature-discovery",
-		"name of the ConfigMap used to deploy Node Feature Discovery (NFD)",
-	)
-}
-
-const (
-	variableName = "nfd"
+var (
+	_ commonhandlers.Named                   = &DefaultNFD{}
+	_ lifecycle.AfterControlPlaneInitialized = &DefaultNFD{}
 )
 
 func New(
 	c ctrlclient.Client,
-	cfg *NFDConfig,
+	cfg *Config,
 ) *DefaultNFD {
 	return &DefaultNFD{
 		client:       c,
 		config:       cfg,
 		variableName: clusterconfig.MetaVariableName,
-		variablePath: []string{"addons", variableName},
+		variablePath: []string{"addons", v1alpha1.NFDVariableName},
 	}
 }
 
@@ -81,77 +74,51 @@ func (n *DefaultNFD) AfterControlPlaneInitialized(
 		"cluster",
 		clusterKey,
 	)
+
 	varMap := variables.ClusterVariablesToVariablesMap(req.Cluster.Spec.Topology.Variables)
 
-	_, found, err := variables.Get[v1alpha1.NFD](varMap, n.variableName, n.variablePath...)
+	cniVar, found, err := variables.Get[v1alpha1.NFD](varMap, n.variableName, n.variablePath...)
 	if err != nil {
+		log.Error(
+			err,
+			"failed to read NFD variable from cluster definition",
+		)
 		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
-		log.Error(err, "failed to get NFD variable")
+		resp.SetMessage(
+			fmt.Sprintf("failed to read NFD variable from cluster definition: %v",
+				err,
+			),
+		)
 		return
 	}
-	// If the variable isn't there or disabled we can ignore it.
 	if !found {
-		log.V(4).Info(
-			"Skipping NFD handler. Not specified in cluster config.",
-		)
+		log.Info("Skipping NFD handler, cluster does not specify request NFDaddon deployment")
 		return
 	}
 
-	cm, err := n.ensureNFDConfigMapForCluster(ctx, &req.Cluster)
-	if err != nil {
+	var strategy addonStrategy
+	switch cniVar.Strategy {
+	case v1alpha1.AddonStrategyClusterResourceSet:
+		strategy = crsStrategy{
+			config: n.config.crsConfig,
+			client: n.client,
+		}
+	case v1alpha1.AddonStrategyHelmAddon:
+		strategy = helmAddonStrategy{
+			config: n.config.helmAddonConfig,
+			client: n.client,
+		}
+	default:
 		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
-		log.Error(err, "failed to apply NFD ConfigMap for cluster")
+		resp.SetMessage(fmt.Sprintf("unknown NFD addon deployment strategy %q", cniVar.Strategy))
 		return
 	}
-	err = utils.EnsureCRSForClusterFromConfigMaps(
-		ctx,
-		cm.Name+"-"+req.Cluster.Name,
-		n.client,
-		&req.Cluster,
-		cm,
-	)
-	if err != nil {
+
+	if err := strategy.apply(ctx, req, log); err != nil {
 		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
-		log.Error(err, "failed to apply NFD ClusterResourceSet for cluster")
+		resp.SetMessage(err.Error())
 		return
 	}
 
 	resp.SetStatus(runtimehooksv1.ResponseStatusSuccess)
 }
-
-// ensureNFDConfigMapForCluster is a private function that creates a configMap for the cluster.
-func (n *DefaultNFD) ensureNFDConfigMapForCluster(
-	ctx context.Context,
-	cluster *capiv1.Cluster,
-) (*corev1.ConfigMap, error) {
-	key := ctrlclient.ObjectKey{
-		Namespace: n.config.defaultsNamespace,
-		Name:      n.config.defaultNFDConfigMap,
-	}
-	cm := &corev1.ConfigMap{}
-	err := n.client.Get(ctx, key, cm)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to fetch the configmap specified by %v: %w",
-			n.config,
-			err,
-		)
-	}
-	// Base configmap is there now we create one in the cluster namespace if needed.
-	cmForCluster := &corev1.ConfigMap{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: corev1.SchemeGroupVersion.String(),
-			Kind:       "ConfigMap",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: cluster.Namespace,
-			Name:      n.config.defaultNFDConfigMap,
-		},
-		Data: cm.Data,
-	}
-	err = client.ServerSideApply(ctx, n.client, cmForCluster)
-	if err != nil {
-		return nil, fmt.Errorf("failed to apply NFD ConfigMap for cluster: %w", err)
-	}
-	return cmForCluster, nil
-}