diff --git a/pkg/k8s/deployer.go b/pkg/k8s/deployer.go index 1341515ac8..ab1f55f8ae 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -69,6 +69,24 @@ func WithDeployerDecorator(decorator deployer.DeployDecorator) DeployerOpt { } } +// References holds the names of cluster resources referenced by a function. +// It is populated during deployment manifest generation and consumed by +// CheckResourcesArePresent to validate that every referenced resource exists. +type References struct { + Secrets sets.Set[string] + ConfigMaps sets.Set[string] + PVCs sets.Set[string] +} + +// NewReferences returns a References with all sets initialized and ready for use. +func NewReferences() *References { + return &References{ + Secrets: sets.New[string](), + ConfigMaps: sets.New[string](), + PVCs: sets.New[string](), + } +} + func onClusterFix(f fn.Function) fn.Function { // This only exists because of a bootstrapping problem with On-Cluster // builds: It appears that, when sending a function to be built on-cluster @@ -147,16 +165,14 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu var status fn.Status if err == nil { // Update the existing function - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVCs := sets.New[string]() + tracker := NewReferences() - deployment, err := d.generateDeployment(f, namespace, daprInstalled, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + deployment, err := d.generateDeployment(f, namespace, daprInstalled, tracker) if err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to generate deployment resources: %w", err) } - if err = CheckResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret); err != nil { + if err = CheckResourcesArePresent(ctx, namespace, *tracker, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret); err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to validate referenced resources: %w", err) } @@ -196,16 +212,14 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to check for existing deployment: %w", err) } - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVCs := sets.New[string]() + tracker := NewReferences() - deployment, err := d.generateDeployment(f, namespace, daprInstalled, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + deployment, err := d.generateDeployment(f, namespace, daprInstalled, tracker) if err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to generate deployment resources: %w", err) } - if err = CheckResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret); err != nil { + if err = CheckResourcesArePresent(ctx, namespace, *tracker, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret); err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to validate referenced resources: %w", err) } @@ -379,7 +393,7 @@ func deleteStaleTriggers(ctx context.Context, eventingClient clienteventingv1.Kn return nil } -func (d *Deployer) generateDeployment(f fn.Function, namespace string, daprInstalled bool, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (*appsv1.Deployment, error) { +func (d *Deployer) generateDeployment(f fn.Function, namespace string, daprInstalled bool, tracker *References) (*appsv1.Deployment, error) { labels, err := deployer.GenerateCommonLabels(f, d.decorator) if err != nil { return nil, err @@ -391,12 +405,12 @@ func (d *Deployer) generateDeployment(f fn.Function, namespace string, daprInsta podAnnotations := make(map[string]string) maps.Copy(podAnnotations, annotations) - envVars, envFrom, err := ProcessEnvs(f.Run.Envs, referencedSecrets, referencedConfigMaps) + envVars, envFrom, err := tracker.ProcessEnvs(f.Run.Envs) if err != nil { return nil, fmt.Errorf("failed to process environment variables: %w", err) } - volumes, volumeMounts, err := ProcessVolumes(f.Run.Volumes, referencedSecrets, referencedConfigMaps, referencedPVCs) + volumes, volumeMounts, err := tracker.ProcessVolumes(f.Run.Volumes) if err != nil { return nil, fmt.Errorf("failed to process volumes: %w", err) } @@ -488,11 +502,13 @@ func (d *Deployer) generateService(f fn.Function, namespace string, daprInstalle return service, nil } -// CheckResourcesArePresent returns error if Secrets or ConfigMaps -// referenced in input sets are not deployed on the cluster in the specified namespace -func CheckResourcesArePresent(ctx context.Context, namespace string, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string], referencedServiceAccount, imagePullSecret string) error { +// CheckResourcesArePresent returns an error if any of the cluster resources +// referenced by refs — Secrets, ConfigMaps, or PersistentVolumeClaims — are +// absent from the given namespace, or if the optional ServiceAccount or +// imagePullSecret do not exist there. +func CheckResourcesArePresent(ctx context.Context, namespace string, refs References, referencedServiceAccount, imagePullSecret string) error { errMsg := "" - for s := range *referencedSecrets { + for s := range refs.Secrets { _, err := GetSecret(ctx, s, namespace) if err != nil { if errors.IsForbidden(err) { @@ -503,14 +519,14 @@ func CheckResourcesArePresent(ctx context.Context, namespace string, referencedS } } - for cm := range *referencedConfigMaps { + for cm := range refs.ConfigMaps { _, err := GetConfigMap(ctx, cm, namespace) if err != nil { errMsg += fmt.Sprintf(" referenced ConfigMap \"%s\" is not present in namespace \"%s\"\n", cm, namespace) } } - for pvc := range *referencedPVCs { + for pvc := range refs.PVCs { _, err := GetPersistentVolumeClaim(ctx, pvc, namespace) if err != nil { errMsg += fmt.Sprintf(" referenced PersistentVolumeClaim \"%s\" is not present in namespace \"%s\"\n", pvc, namespace) @@ -609,8 +625,7 @@ func SetSecurityContext(container *corev1.Container) { // - name: EXAMPLE4 // value: {{ configMap:configMapName:key }} # ENV from a key in ConfigMap // - value: {{ configMap:configMapName }} # all key-pair values from ConfigMap are set as ENV -func ProcessEnvs(envs []fn.Env, referencedSecrets, referencedConfigMaps *sets.Set[string]) ([]corev1.EnvVar, []corev1.EnvFromSource, error) { - +func (t *References) ProcessEnvs(envs []fn.Env) ([]corev1.EnvVar, []corev1.EnvFromSource, error) { envs = withOpenAddress(envs) // prepends ADDRESS=0.0.0.0 if not extant envVars := []corev1.EnvVar{{Name: "BUILT", Value: time.Now().Format("20060102T150405")}} @@ -620,7 +635,7 @@ func ProcessEnvs(envs []fn.Env, referencedSecrets, referencedConfigMaps *sets.Se if env.Name == nil && env.Value != nil { // all key-pair values from secret/configMap are set as ENV, eg. {{ secret:secretName }} or {{ configMap:configMapName }} if strings.HasPrefix(*env.Value, "{{") { - envFromSource, err := createEnvFromSource(*env.Value, referencedSecrets, referencedConfigMaps) + envFromSource, err := t.createEnvFromSource(*env.Value) if err != nil { return nil, nil, err } @@ -632,7 +647,7 @@ func ProcessEnvs(envs []fn.Env, referencedSecrets, referencedConfigMaps *sets.Se slices := strings.Split(strings.Trim(*env.Value, "{} "), ":") if len(slices) == 3 { // ENV from a key in secret/configMap, eg. FOO={{ secret:secretName:key }} FOO={{ configMap:configMapName.key }} - valueFrom, err := createEnvVarSource(slices, referencedSecrets, referencedConfigMaps) + valueFrom, err := t.createEnvVarSource(slices) envVars = append(envVars, corev1.EnvVar{Name: *env.Name, ValueFrom: valueFrom}) if err != nil { return nil, nil, err @@ -698,7 +713,7 @@ func withOpenAddress(ee []fn.Env) []fn.Env { return ee } -func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps *sets.Set[string]) (*corev1.EnvFromSource, error) { +func (t *References) createEnvFromSource(value string) (*corev1.EnvFromSource, error) { slices := strings.Split(strings.Trim(value, "{} "), ":") if len(slices) != 2 { return nil, fmt.Errorf("env requires a value in form \"resourceType:name\" where \"resourceType\" can be one of \"configMap\" or \"secret\"; got %q", slices) @@ -719,8 +734,8 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * Name: sourceName, }} - if !referencedConfigMaps.Has(sourceName) { - referencedConfigMaps.Insert(sourceName) + if !t.ConfigMaps.Has(sourceName) { + t.ConfigMaps.Insert(sourceName) } case "secret": sourceType = "Secret" @@ -728,8 +743,8 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, }} - if !referencedSecrets.Has(sourceName) { - referencedSecrets.Insert(sourceName) + if !t.Secrets.Has(sourceName) { + t.Secrets.Insert(sourceName) } default: return nil, fmt.Errorf("unsupported env source type %q; supported source types are \"configMap\" or \"secret\"", slices[0]) @@ -742,7 +757,7 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * return &envVarSource, nil } -func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps *sets.Set[string]) (*corev1.EnvVarSource, error) { +func (t *References) createEnvVarSource(slices []string) (*corev1.EnvVarSource, error) { if len(slices) != 3 { return nil, fmt.Errorf("env requires a value in form \"resourceType:name:key\" where \"resourceType\" can be one of \"configMap\" or \"secret\"; got %q", slices) } @@ -764,8 +779,8 @@ func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps }, Key: sourceKey} - if !referencedConfigMaps.Has(sourceName) { - referencedConfigMaps.Insert(sourceName) + if !t.ConfigMaps.Has(sourceName) { + t.ConfigMaps.Insert(sourceName) } case "secret": sourceType = "Secret" @@ -775,8 +790,8 @@ func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps }, Key: sourceKey} - if !referencedSecrets.Has(sourceName) { - referencedSecrets.Insert(sourceName) + if !t.Secrets.Has(sourceName) { + t.Secrets.Insert(sourceName) } default: return nil, fmt.Errorf("unsupported env source type %q; supported source types are \"configMap\" or \"secret\"", slices[0]) @@ -826,7 +841,7 @@ func processLocalEnvValue(val string) (string, error) { // path: /etc/secret-volume // - emptyDir: {} # mount EmptyDir as Volume // path: /etc/configMap-volume -func ProcessVolumes(volumes []fn.Volume, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) ([]corev1.Volume, []corev1.VolumeMount, error) { +func (t *References) ProcessVolumes(volumes []fn.Volume) ([]corev1.Volume, []corev1.VolumeMount, error) { createdVolumes := sets.NewString() usedPaths := sets.NewString() @@ -851,8 +866,8 @@ func ProcessVolumes(volumes []fn.Volume, referencedSecrets, referencedConfigMaps }) createdVolumes.Insert(volumeName) - if !referencedSecrets.Has(*vol.Secret) { - referencedSecrets.Insert(*vol.Secret) + if !t.Secrets.Has(*vol.Secret) { + t.Secrets.Insert(*vol.Secret) } } } else if vol.ConfigMap != nil { @@ -871,8 +886,8 @@ func ProcessVolumes(volumes []fn.Volume, referencedSecrets, referencedConfigMaps }) createdVolumes.Insert(volumeName) - if !referencedConfigMaps.Has(*vol.ConfigMap) { - referencedConfigMaps.Insert(*vol.ConfigMap) + if !t.ConfigMaps.Has(*vol.ConfigMap) { + t.ConfigMaps.Insert(*vol.ConfigMap) } } } else if vol.PersistentVolumeClaim != nil { @@ -890,8 +905,8 @@ func ProcessVolumes(volumes []fn.Volume, referencedSecrets, referencedConfigMaps }) createdVolumes.Insert(volumeName) - if !referencedPVCs.Has(*vol.PersistentVolumeClaim.ClaimName) { - referencedPVCs.Insert(*vol.PersistentVolumeClaim.ClaimName) + if !t.PVCs.Has(*vol.PersistentVolumeClaim.ClaimName) { + t.PVCs.Insert(*vol.PersistentVolumeClaim.ClaimName) } } } else if vol.EmptyDir != nil { diff --git a/pkg/k8s/deployer_test.go b/pkg/k8s/deployer_test.go index ec48829493..2b130a4842 100644 --- a/pkg/k8s/deployer_test.go +++ b/pkg/k8s/deployer_test.go @@ -5,7 +5,6 @@ import ( "testing" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" fn "knative.dev/func/pkg/functions" ) @@ -121,8 +120,8 @@ func Test_generateDeployment_ImagePullSecret(t *testing.T) { ImagePullSecret: "my-registry-secret", }, } - rs, rcm, rpvc := sets.New[string](), sets.New[string](), sets.New[string]() - deployment, err := d.generateDeployment(f, "default", false, &rs, &rcm, &rpvc) + tracker := NewReferences() + deployment, err := d.generateDeployment(f, "default", false, tracker) if err != nil { t.Fatal(err) } @@ -139,8 +138,8 @@ func Test_generateDeployment_ImagePullSecret(t *testing.T) { Image: "registry.example.com/test:latest", }, } - rs, rcm, rpvc := sets.New[string](), sets.New[string](), sets.New[string]() - deployment, err := d.generateDeployment(f, "default", false, &rs, &rcm, &rpvc) + tracker := NewReferences() + deployment, err := d.generateDeployment(f, "default", false, tracker) if err != nil { t.Fatal(err) } diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 1d1f8fd3bb..447431e172 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -23,7 +23,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" v1 "k8s.io/client-go/kubernetes/typed/core/v1" clienteventingv1 "knative.dev/client/pkg/eventing/v1" "knative.dev/client/pkg/flags" @@ -207,19 +206,17 @@ consider using the --image-pull-secret flag, or setting up pull secrets manually } if errors.IsNotFound(err) { - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVCs := sets.New[string]() + tracker := k8s.NewReferences() - service, err := generateNewService(f, d.decorator, daprInstalled, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + service, err := generateNewService(f, d.decorator, daprInstalled, tracker) if err != nil { err = fmt.Errorf("knative deployer failed to generate the Knative Service: %v", err) return fn.DeploymentResult{}, err } - err = k8s.CheckResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret) + err = k8s.CheckResourcesArePresent(ctx, namespace, *tracker, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret) if err != nil { - err = fmt.Errorf("knative deployer failed to generate the Knative Service: %v", err) + err = fmt.Errorf("knative deployer referenced resource validation failed: %v", err) return fn.DeploymentResult{}, err } @@ -305,23 +302,21 @@ consider using the --image-pull-secret flag, or setting up pull secrets manually } } else { // Update the existing Service - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVCs := sets.New[string]() + tracker := k8s.NewReferences() - newEnv, newEnvFrom, err := k8s.ProcessEnvs(f.Run.Envs, &referencedSecrets, &referencedConfigMaps) + newEnv, newEnvFrom, err := tracker.ProcessEnvs(f.Run.Envs) if err != nil { return fn.DeploymentResult{}, err } - newVolumes, newVolumeMounts, err := k8s.ProcessVolumes(f.Run.Volumes, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + newVolumes, newVolumeMounts, err := tracker.ProcessVolumes(f.Run.Volumes) if err != nil { return fn.DeploymentResult{}, err } - err = k8s.CheckResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret) + err = k8s.CheckResourcesArePresent(ctx, namespace, *tracker, f.Deploy.ServiceAccountName, f.Deploy.ImagePullSecret) if err != nil { - err = fmt.Errorf("knative deployer failed to update the Knative Service: %v", err) + err = fmt.Errorf("knative deployer referenced resource validation failed: %v", err) return fn.DeploymentResult{}, err } @@ -413,7 +408,7 @@ func createTriggers(ctx context.Context, f fn.Function, client clientservingv1.K return nil } -func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprInstalled bool, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (*servingv1.Service, error) { +func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprInstalled bool, tracker *k8s.References) (*servingv1.Service, error) { container := corev1.Container{ Image: f.Deploy.Image, } @@ -421,14 +416,14 @@ func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprI k8s.SetSecurityContext(&container) k8s.SetHealthEndpoints(f, &container) - newEnv, newEnvFrom, err := k8s.ProcessEnvs(f.Run.Envs, referencedSecrets, referencedConfigMaps) + newEnv, newEnvFrom, err := tracker.ProcessEnvs(f.Run.Envs) if err != nil { return nil, err } container.Env = newEnv container.EnvFrom = newEnvFrom - newVolumes, newVolumeMounts, err := k8s.ProcessVolumes(f.Run.Volumes, referencedSecrets, referencedConfigMaps, referencedPVCs) + newVolumes, newVolumeMounts, err := tracker.ProcessVolumes(f.Run.Volumes) if err != nil { return nil, err } diff --git a/pkg/knative/deployer_test.go b/pkg/knative/deployer_test.go index f8989fe4d7..1897441d03 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -24,10 +24,10 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" fn "knative.dev/func/pkg/functions" + k8s "knative.dev/func/pkg/k8s" "knative.dev/pkg/ptr" servingv1 "knative.dev/serving/pkg/apis/serving/v1" ) @@ -417,10 +417,8 @@ func TestUpdateService_EnvsPropagated(t *testing.T) { // TestGenerateNewService_ResourceSetsPopulated is a regression test for the // create (first-deploy) path. It proves that generateNewService populates the -// caller-supplied referencedSecrets and referencedConfigMaps sets so that the -// subsequent CheckResourcesArePresent call in Deploy() actually validates them. -// Before the fix, generateNewService allocated its own internal sets and the -// caller's sets remained empty, causing validation to be silently skipped. +// tracker's References so that the subsequent CheckResourcesArePresent call in +// Deploy() actually validates them. func TestGenerateNewService_ResourceSetsPopulated(t *testing.T) { secretName := "my-secret" configMapName := "my-configmap" @@ -434,20 +432,18 @@ func TestGenerateNewService_ResourceSetsPopulated(t *testing.T) { f.Run.Envs.Add("FROM_SECRET", "{{ secret:"+secretName+":key }}") f.Run.Envs.Add("FROM_CM", "{{ configMap:"+configMapName+":key }}") - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVCs := sets.New[string]() + tracker := k8s.NewReferences() - _, err := generateNewService(f, nil, false, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + _, err := generateNewService(f, nil, false, tracker) if err != nil { t.Fatalf("generateNewService returned unexpected error: %v", err) } - if !referencedSecrets.Has(secretName) { - t.Errorf("expected referencedSecrets to contain %q after generateNewService, got: %v", secretName, referencedSecrets) + if !tracker.Secrets.Has(secretName) { + t.Errorf("expected tracker.Secrets to contain %q after generateNewService, got: %v", secretName, tracker.Secrets) } - if !referencedConfigMaps.Has(configMapName) { - t.Errorf("expected referencedConfigMaps to contain %q after generateNewService, got: %v", configMapName, referencedConfigMaps) + if !tracker.ConfigMaps.Has(configMapName) { + t.Errorf("expected tracker.ConfigMaps to contain %q after generateNewService, got: %v", configMapName, tracker.ConfigMaps) } }