From 0ab3debaba2afcffcf16827798c876c7f35a9477 Mon Sep 17 00:00:00 2001 From: Jeremy Poulin Date: Mon, 10 Apr 2023 18:14:24 -0400 Subject: [PATCH] Update multi-arch validator to check node affinity for multi-platform environments (#276) * Updated .gitignore to ignore vscode files. * Added validation for node affinity boundaries in the CSV. --- .gitignore | 2 + pkg/validation/internal/multiarch.go | 409 +++++++++++++----- pkg/validation/internal/multiarch_test.go | 210 +++++++-- ...operator.v0.9.4.clusterserviceversion.yaml | 20 +- 4 files changed, 485 insertions(+), 156 deletions(-) diff --git a/.gitignore b/.gitignore index d6395083f..48684a7b8 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,5 @@ .idea/* vendor/ bin/ + +.vscode diff --git a/pkg/validation/internal/multiarch.go b/pkg/validation/internal/multiarch.go index 4321a73bd..a533ffe98 100644 --- a/pkg/validation/internal/multiarch.go +++ b/pkg/validation/internal/multiarch.go @@ -10,6 +10,9 @@ import ( "github.com/operator-framework/api/pkg/manifests" "github.com/operator-framework/api/pkg/validation/errors" interfaces "github.com/operator-framework/api/pkg/validation/interfaces" + corev1 "k8s.io/api/core/v1" + + log "github.com/sirupsen/logrus" ) // MultipleArchitecturesValidator validates the bundle against criteria to support Multiple Architectures. For further @@ -26,11 +29,13 @@ import ( // // ### What is checked? // -// On this check, we aggregate the archetype(s) and OS(s) provided via the labels and those which are found by checking the images so that, we can check: +// On this check, we aggregate the platform architecture(s) and OS(s) provided via the labels and those which are found by checking the images so that, we can check: // // - If your CSV is missing labels // -// - If your Operator bundle specifies images which do not support all archetypes found for your Operator image(s) (probably supported by your project) +// - If your Operator bundle specifies images which do not support all architectures found for your Operator image(s) (probably supported by your project) +// +// - If your deployment spec follows the best practice of setting nodeAffinity to ensure image(s) are only scheduled on compatible platform nodes. // // Note: To better guess the case scenarios where authors might have missed the labels, the following check will verify all architectures supported by the Operator image(s). However, by looking at the CSV we are not able to ensure what is the Operator image because this info is not provided. Therefore, we know by SDK the Operator image container will be called manager. // @@ -62,8 +67,10 @@ type multiArchValidator struct { // InfraCVSOSLabels store the OS labels from // operatorframework.io/os.: supported infraCSVOSLabels []string - // allOtherImages stores the allOtherImages defined in the bundle with the platform.arch supported - allOtherImages map[string][]platform + // otherCSVDeploymentImages stores the non-manager images in the CSV deployment + otherCSVDeploymentImages map[string][]platform + // related stores the images listed in the related images section of the CSV + relatedImages map[string][]platform // managerImages stores the images that we could consider as from the manager managerImages map[string][]platform // managerImagesString stores the images only @@ -72,9 +79,11 @@ type multiArchValidator struct { managerArchs map[string]string // managerOs contains a map of the OSes found managerOs map[string]string + // imageNodeAffinity maps the image to its nodeAffinity boundaries + imageNodeAffinity map[string][]platform // Store the bundle load bundle *manifests.Bundle - // containerTool defines the container tool which will be used to inspect the allOtherImages + // containerTool defines the container tool which will be used to inspect the images containerTool string // warns stores the errors faced by the validator to return the warnings warns []error @@ -94,8 +103,13 @@ type manifestData struct { // platform store the Architecture and OS supported by the image type platform struct { - Architecture string `json:"architecture"` OS string `json:"os"` + Architecture string `json:"architecture"` +} + +// formatting for logs +func (p platform) String() string { + return fmt.Sprintf("%s/%s", p.OS, p.Architecture) } func multipleArchitecturesValidate(objs ...interface{}) (results []errors.ManifestResult) { @@ -108,6 +122,7 @@ func multipleArchitecturesValidate(objs ...interface{}) (results []errors.Manife containerTool = obj.(map[string]string)[ContainerToolsKey] if len(containerTool) > 0 { // Make lower for we compare and use it + log.Infof("Container tool set to %q", containerTool) containerTool = strings.ToLower(containerTool) break } @@ -120,6 +135,10 @@ func multipleArchitecturesValidate(objs ...interface{}) (results []errors.Manife results = append(results, validateMultiArchWith(v, containerTool)) } } + + if len(results) == 0 { + log.Error("No bundles found.") + } return results } @@ -146,7 +165,6 @@ func validateMultiArchWith(bundle *manifests.Bundle, containerTool string) error } // Performs the checks - multiArchValidator := multiArchValidator{bundle: bundle, containerTool: containerTool} multiArchValidator.validate() @@ -176,15 +194,16 @@ func validateContainerTool(containerTool string) (string, error) { // validate performs all required checks to validate the bundle against the Multiple Architecture // configuration to guess the missing labels and/or highlight what are the missing Architectures -// for the allOtherImages (for what is configured to be supported AND for what we guess that is supported +// for the images (for what is configured to be supported AND for what we guess that is supported // and just is missing a label). func (data *multiArchValidator) validate() { data.loadInfraLabelsFromCSV() data.loadImagesFromCSV() data.managerImages = data.inspectImages(data.managerImages) - data.allOtherImages = data.inspectImages(data.allOtherImages) + data.otherCSVDeploymentImages = data.inspectImages(data.otherCSVDeploymentImages) + data.relatedImages = data.inspectImages(data.relatedImages) data.loadAllPossibleArchSupported() - data.loadAllPossibleSoSupported() + data.loadAllPossibleOsSupported() data.doChecks() } @@ -205,58 +224,123 @@ func (data *multiArchValidator) loadInfraLabelsFromCSV() { } } -// loadImagesFromCSV will add all allOtherImages found in the CSV -// it will be looking for all containers allOtherImages and what is defined -// via the spec.relatedImages (required for disconnect support) +// loadImagesFromCSV will add all images found in the CSV to one of three lists +// managerImages will search for a manager container, or the default deployment images +// otherCSVDeploymentImages is for the other images in the deployment that aren't the manager +// relatedImages collects the images referenced by spec.relatedImages (required for disconnect support) func (data *multiArchValidator) loadImagesFromCSV() { // We need to try looking for the manager image so that we can // be more assertive in the guess to warning the Operator // authors that when forgotten to use add the labels // because we found images that provides more support data.managerImages = make(map[string][]platform) + + // We will store the nodeAffinity information in the CSV as we encounter it + data.imageNodeAffinity = make(map[string][]platform) + for _, v := range data.bundle.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { foundManager := false // For the default scaffold we have a container called manager for _, c := range v.Spec.Template.Spec.Containers { - if c.Name == "manager" && len(data.managerImages[c.Image]) == 0 { - data.managerImages[c.Image] = append(data.managerImages[c.Image], platform{}) + // Skip duplicate images + _, exists := data.managerImages[c.Image] + if exists { + continue + } + + // Store the manager container information for later validation + if c.Name == "manager" { + data.managerImages[c.Image] = make([]platform, 0) data.managerImagesString = append(data.managerImagesString, c.Image) foundManager = true - break } + + // Collect nodeAffinity boundaries for all images + data.imageNodeAffinity[c.Image] = append(extractNodeAffinityPlatforms(v.Spec.Template.Spec)) } + // If we do not find a container called manager then we // will add all from the Deployment Specs which is not the // kube-rbac-proxy image scaffold by default if !foundManager { for _, c := range v.Spec.Template.Spec.Containers { - if c.Name != default_container_scaffold_by_sdk && len(data.managerImages[c.Image]) == 0 { - data.managerImages[c.Image] = append(data.managerImages[c.Image], platform{}) - data.managerImagesString = append(data.managerImagesString, c.Image) + // Skip kube-rbac-proxy or already added images + _, exists := data.managerImages[c.Image] + if c.Name == default_container_scaffold_by_sdk || exists { + continue } + + data.managerImages[c.Image] = make([]platform, 0) + data.managerImagesString = append(data.managerImagesString, c.Image) } } } - data.allOtherImages = make(map[string][]platform) + data.otherCSVDeploymentImages = make(map[string][]platform) if data.bundle.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs != nil { for _, v := range data.bundle.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { for _, c := range v.Spec.Template.Spec.Containers { - // If not be from manager the add - if len(data.managerImages[c.Image]) == 0 { - data.allOtherImages[c.Image] = append(data.allOtherImages[c.Image], platform{}) + // Skip images in the manager image list + _, exists := data.managerImages[c.Image] + if exists { + continue } + + data.otherCSVDeploymentImages[c.Image] = make([]platform, 0) } } } + data.relatedImages = make(map[string][]platform) for _, v := range data.bundle.CSV.Spec.RelatedImages { - data.allOtherImages[v.Image] = append(data.allOtherImages[v.Image], platform{}) + data.relatedImages[v.Image] = make([]platform, 0) + } +} + +// extractNodeAffinityPlatforms scans the deployment spec for +// affinity.nodeAffinity.requiredDuringSchedulingIngoredDringExecution.nodeSelectorTerms +// that set platform requirements for kubernetes.io/arch and kubernetes.io/os labels +func extractNodeAffinityPlatforms(spec corev1.PodSpec) []platform { + var platforms = make([]platform, 0) + if spec.Affinity == nil || + spec.Affinity.NodeAffinity == nil || + spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil || + spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms == nil { + // No platforms set + return platforms + } + + var terms = spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms + for _, t := range terms { + var arches = make([]string, 0) + var os = make([]string, 0) + + for _, e := range t.MatchExpressions { + if e.Operator != "In" { + continue + } + + if e.Key == "kubernetes.io/arch" { + arches = e.Values + continue + } else if e.Key == "kubernetes.io/os" { + os = e.Values + continue + } + } + + for _, o := range os { + for _, a := range arches { + platforms = append(platforms, platform{Architecture: a, OS: o}) + } + } } + + return platforms } // runManifestInspect executes the command for we are able to check what -// are the Architecture(s) and SO(s) supported per each image found +// are the Architecture(s) and OS(s) supported per each image found func runManifestInspect(image, tool string) (manifestInspect, error) { cmd := exec.Command(tool, "pull", image) _, err := runCommand(cmd) @@ -287,7 +371,7 @@ func runCommand(cmd *exec.Cmd) ([]byte, error) { return output, nil } -// inspectAllOtherImages will perform the required steps to inspect all allOtherImages found +// inspectImages will lookup a list of images via a container tool to get a list of supported platforms func (data *multiArchValidator) inspectImages(images map[string][]platform) map[string][]platform { for k := range images { manifest, err := runManifestInspect(k, data.containerTool) @@ -297,12 +381,12 @@ func (data *multiArchValidator) inspectImages(images map[string][]platform) map[ if err != nil { data.warns = append(data.warns, fmt.Errorf("unable to inspect the image (%s) : %s", k, err)) - // We set the Arch and SO as error for we are able to deal witth these cases further - // Se that make no sense we raise a warning to notify the user that the image - // does not provide some kind of support only because we were unable to inspect it. + // We set the Arch and OS as error so we can identify that the container inspection failed later + // We raise a warning to notify the user that the image does not provide some kind of support + // only because we were unable to inspect it. // Be aware that the validator raise warnings for all cases scenarios to let // the author knows that those were not checked at all and why. - images[k][0] = platform{"error", "error"} + images[k] = []platform{platform{"error", "error"}} continue } } @@ -325,21 +409,28 @@ func (data *multiArchValidator) doChecks() { // on the check above. The following check raise a warning when it is possible to check that the Operator // manager image(s) supports architecture(s) not defined via labels. Therefore, it shows like the labels are missing data.checkMissingLabelsForArchs() - data.checkMissingLabelsForSO() - // the following check will raise warnings when is possible to verify that the images defined in the CSV + data.checkMissingLabelsForOS() + // the following checks will raise warnings when is possible to verify that the images defined in the CSV // does not provide the same architecture(s) supported by the Operator manager or defined via the labels - data.checkMissingSupportForOtherImages() + data.checkMissingSupportForOtherImages(data.otherCSVDeploymentImages) + data.checkMissingSupportForOtherImages(data.relatedImages) + + // the following check will raise warnings when nodeAffinity isn't set to ensure that the pod spec will only + // target nodes matching the platforms (os/arch) specified in the manifest. + data.checkNodeAffinity(data.managerImages) + data.checkNodeAffinity(data.otherCSVDeploymentImages) } -// checkMissingSupportForOtherImages checks if any image is missing some arch or so found -// (probably the image should support the arch or SO ) -func (data *multiArchValidator) checkMissingSupportForOtherImages() { - for image, plaformFromImage := range data.allOtherImages { +// checkMissingSupportForOtherImages checks if any image is missing some arch or os found +// among the manager platforms. Ideally, all images should support the same platforms. +// This is called for both the non-manager CSV images and the related images +func (data *multiArchValidator) checkMissingSupportForOtherImages(images map[string][]platform) { + for image, platformFromImage := range images { listArchNotFound := []string{} for archFromList := range data.managerArchs { found := false - for _, imageData := range plaformFromImage { - // Ignore the case when the Plataform.Architecture == "error" since that means + for _, imageData := range platformFromImage { + // Ignore the case when the Platform.Architecture == "error" since that means // that was not possible to inspect the image if imageData.Architecture == "error" { found = true @@ -367,11 +458,11 @@ func (data *multiArchValidator) checkMissingSupportForOtherImages() { data.managerImagesString)) } - listAllSoNotFound := []string{} + listAllOsNotFound := []string{} for archOSList := range data.managerOs { found := false - for _, imageData := range plaformFromImage { - // Ignore the case when the Plataform.Architecture == "error" since that means + for _, imageData := range platformFromImage { + // Ignore the case when the Platform.Architecture == "error" since that means // that was not possible to inspect the image if imageData.OS == "error" { found = true @@ -384,27 +475,128 @@ func (data *multiArchValidator) checkMissingSupportForOtherImages() { } } if !found && archOSList != "error" { - listAllSoNotFound = append(listAllSoNotFound, archOSList) + listAllOsNotFound = append(listAllOsNotFound, archOSList) } } - if len(listAllSoNotFound) > 0 { - sort.Strings(listAllSoNotFound) + if len(listAllOsNotFound) > 0 { + sort.Strings(listAllOsNotFound) data.warns = append(data.warns, fmt.Errorf("check if the image %s should not support %q. "+ - "Note that this CSV has labels for this SO(s) "+ + "Note that this CSV has labels for this OS(s) "+ "Your manager image %q are providing this support OR the CSV is configured via labels "+ "to support it. Then, please verify if this image should not support it", image, - listAllSoNotFound, + listAllOsNotFound, data.managerImagesString)) } } } -// verify if 1 or more allOtherImages has support for a SO not defined via the labels -// (probably the label for this SO is missing ) -func (data *multiArchValidator) checkMissingLabelsForSO() { - notFoundSoLabel := []string{} +// checkNodeAffinity checks if any image is missing nodeAffinity configuration corresponding to +// the supports os/arch platforms in the manifest. +func (data *multiArchValidator) checkNodeAffinity(images map[string][]platform) { + for image, platformFromImage := range images { + + // Verify we were able to gather valid platform data for the image + imagePlatformDataValid := true + for _, imageData := range platformFromImage { + // Ignore the case when the Platform.Architecture == "error" since that means + // that was not possible to inspect the image + if imageData.Architecture == "error" { + imagePlatformDataValid = false + break + } + } + + // Ensure we have a node affinity configuration for the image + if len(data.imageNodeAffinity[image]) == 0 { + if !imagePlatformDataValid { + // Node affinity info is missing from CSV (or invalid) + data.warns = append(data.warns, + fmt.Errorf("check if the CSV is missing a node affinity configuration for the image: %q. "+ + image, + )) + } + + // We have valid platform data for the image but a missing or invalid affinity configuration + data.warns = append(data.warns, fmt.Errorf("check if the CSV has a missing or invalid node affinity configuration for the image: %q. "+ + "The image data suggests the following platforms are supported: %q", + image, + platformFromImage)) + + continue + } + + // We have a valid node affinity config + // Scan for extra and missing platforms + extra, missing := compareAffinityToPlatforms(data.imageNodeAffinity[image], platformFromImage) + if len(extra) == 0 && len(missing) == 0 { + // Node affinity matches exactly + continue + } + + // Warn author about extra affinities + if len(extra) != 0 { + data.warns = append(data.warns, fmt.Errorf("the CSV includes %q in the node affinity configuration for the image: %q, but "+ + "the image data suggests the following platforms are supported: %q", + extra, + image, + platformFromImage)) + } + + // Warn author about missing affinities + if len(missing) != 0 { + data.warns = append(data.warns, fmt.Errorf("the image data indicates %q is supported for the image: %q, but "+ + "the node affinity configuration for the image only specifies %q", + missing, + image, + data.imageNodeAffinity[image])) + } + } +} + +func compareAffinityToPlatforms(affinities []platform, platforms []platform) ([]platform, []platform) { + var extra = []platform{} + var missing = []platform{} + + // Find extras + for _, affinity := range affinities { + found := false + for _, platform := range platforms { + if affinity.Architecture == platform.Architecture && affinity.OS == platform.OS { + found = true + break + } + } + + if !found { + extra = append(extra, affinity) + } + } + + // Find missing + for _, platform := range platforms { + found := false + for _, affinity := range affinities { + if platform.Architecture == affinity.Architecture && platform.OS == affinity.OS { + found = true + break + } + } + + if !found { + missing = append(missing, platform) + } + } + + return extra, missing + +} + +// verify if 1 or more images have support for an OS not defined via the labels +// (probably the label for this OS is missing ) +func (data *multiArchValidator) checkMissingLabelsForOS() { + notFoundOsLabel := []string{} for supported := range data.managerOs { found := false for _, infra := range data.infraCSVOSLabels { @@ -417,29 +609,29 @@ func (data *multiArchValidator) checkMissingLabelsForSO() { if !found && supported != "error" { // if the only arch supported is linux then, we should not ask for the label if !(supported == "linux" && len(data.managerOs) == 1 && len(data.managerOs["linux"]) > 0) { - notFoundSoLabel = append(notFoundSoLabel, supported) + notFoundOsLabel = append(notFoundOsLabel, supported) } } } - if len(notFoundSoLabel) > 0 { + if len(notFoundOsLabel) > 0 { // We need to sort, otherwise it is possible verify in the tests that we have // this message as result - sort.Strings(notFoundSoLabel) + sort.Strings(notFoundOsLabel) data.warns = append(data.warns, - fmt.Errorf("check if the CSV is missing the label (%s) for the SO(s): %q. "+ + fmt.Errorf("check if the CSV is missing the label (%s) for the OS(s): %q. "+ "Be aware that your Operator manager image %q provides this support. "+ - "Thus, it is very likely that you want to provide it and if you support more than linux SO you MUST,"+ + "Thus, it is very likely that you want to provide it and if you support more than linux OS you MUST,"+ "use the required labels for all which are supported."+ "Otherwise, your solution cannot be listed on the cluster for these architectures", operatorFrameworkOSLabel, - notFoundSoLabel, + notFoundOsLabel, data.managerImagesString)) } } -// checkMissingLabelsForArchs verify if 1 or ore allOtherImages has support for a Arch not defined via the labels +// checkMissingLabelsForArchs verify if 1 or more images have support for a Arch not defined via the labels // (probably the label for this Arch is missing ) func (data *multiArchValidator) checkMissingLabelsForArchs() { notFoundArchLabel := []string{} @@ -489,26 +681,26 @@ func (data *multiArchValidator) loadAllPossibleArchSupported() { data.managerArchs["amd64"] = "amd64" } - // Get all ARCH from the provided allOtherImages + // Get all ARCH from the provided manager image(s) for _, imageData := range data.managerImages { - for _, plataform := range imageData { - if len(plataform.Architecture) > 0 { - data.managerArchs[plataform.Architecture] = plataform.Architecture + for _, platform := range imageData { + if len(platform.Architecture) > 0 { + data.managerArchs[platform.Architecture] = platform.Architecture } } } } -// loadAllPossibleSoSupported will verify all SO that this bundle can support -// for then, we aare able to check if it is missing labels. +// loadAllPossibleOsSupported will verify all OS that this bundle can support +// for then, we are able to check if it is missing labels. // Note: -// - we check what are the SO of all allOtherImages informed -// - we ensure that the linux SO will be added when none so labels were informed -// - we check all labels to know what are the SO(s) to obtain the list of them which the bundle is defining -func (data *multiArchValidator) loadAllPossibleSoSupported() { +// - we check which OS where found for manager images +// - we ensure that the linux OS will be added when none were found +// - we check all labels to know which OS(s) to obtain the bundle could define +func (data *multiArchValidator) loadAllPossibleOsSupported() { // Add the values provided via label for _, v := range data.infraCSVOSLabels { - label := extractValueFromSoLabel(v) + label := extractValueFromOsLabel(v) data.managerOs[label] = label } @@ -517,25 +709,25 @@ func (data *multiArchValidator) loadAllPossibleSoSupported() { data.managerOs["linux"] = "linux" } - // Get all SO from the provided allOtherImages + // Get all OS from the provided managerImages for _, imageData := range data.managerImages { - for _, plataform := range imageData { - if len(plataform.OS) > 0 { - data.managerOs[plataform.OS] = plataform.OS + for _, platform := range imageData { + if len(platform.OS) > 0 { + data.managerOs[platform.OS] = platform.OS } } } } -// checkSupportDefined checks if all allOtherImages supports the ARCHs and SOs defined +// checkSupportDefined checks if all images supports the ARCHs and OSs defined func (data *multiArchValidator) checkSupportDefined() { - configuredS0 := []string{} + configuredOS := []string{} if len(data.infraCSVOSLabels) == 0 { - configuredS0 = []string{"linux"} + configuredOS = []string{"linux"} } for _, label := range data.infraCSVOSLabels { - configuredS0 = append(configuredS0, extractValueFromSoLabel(label)) + configuredOS = append(configuredOS, extractValueFromOsLabel(label)) } configuredArch := []string{} @@ -548,37 +740,35 @@ func (data *multiArchValidator) checkSupportDefined() { } allSupportedConfiguration := []string{} - for _, so := range configuredS0 { + for _, os := range configuredOS { for _, arch := range configuredArch { - allSupportedConfiguration = append(allSupportedConfiguration, fmt.Sprintf("%s.%s", so, arch)) + allSupportedConfiguration = append(allSupportedConfiguration, fmt.Sprintf("%s.%s", os, arch)) } } - notFoundImgPlat := map[string][]string{} - for _, config := range allSupportedConfiguration { - for image, allPlataformFromImage := range data.managerImages { - found := false - for _, imgPlat := range allPlataformFromImage { - // Ignore the errors since they mean that was not possible to inspect - // the image - if imgPlat.OS == "error" { - found = true - break - } + var unsupported = make(map[string][]string) + appendUnsupportedConfigurations(unsupported, allSupportedConfiguration, data.managerImages) + appendUnsupportedConfigurations(unsupported, allSupportedConfiguration, data.otherCSVDeploymentImages) + appendUnsupportedConfigurations(unsupported, allSupportedConfiguration, data.relatedImages) - if config == fmt.Sprintf("%s.%s", imgPlat.OS, imgPlat.Architecture) { - found = true - break - } - } - - if !found { - notFoundImgPlat[config] = append(notFoundImgPlat[config], image) - } + if len(unsupported) > 0 { + for platform, images := range unsupported { + // Sort the images so we can check results in the tests + sort.Strings(images) + data.errors = append(data.errors, + fmt.Errorf("not all images specified are providing the support described via the CSV labels. "+ + "Note that (OS.architecture): (%s) was not found for the image(s) %s", + platform, images)) } - for image, allPlataformFromImage := range data.allOtherImages { + } +} + +// appendUnsupportedConfigurations takes a map by reference and appends any supportedConfiguration mismatches for each image provided in the images map +func appendUnsupportedConfigurations(unsupported map[string][]string, supportedConfigurations []string, images map[string][]platform) { + for _, config := range supportedConfigurations { + for image, allPlatformFromImage := range images { found := false - for _, imgPlat := range allPlataformFromImage { + for _, imgPlat := range allPlatformFromImage { // Ignore the errors since they mean that was not possible to inspect // the image if imgPlat.OS == "error" { @@ -593,25 +783,14 @@ func (data *multiArchValidator) checkSupportDefined() { } if !found { - notFoundImgPlat[config] = append(notFoundImgPlat[config], image) + unsupported[config] = append(unsupported[config], image) } } } - - if len(notFoundImgPlat) > 0 { - for platform, images := range notFoundImgPlat { - // If we not sort the allOtherImages we cannot check its result in the tests - sort.Strings(images) - data.errors = append(data.errors, - fmt.Errorf("not all images specified are providing the support described via the CSV labels. "+ - "Note that (SO.architecture): (%s) was not found for the image(s) %s", - platform, images)) - } - } } -// extractValueFromSoLabel returns only the value of the SO label (i.e. linux) -func extractValueFromSoLabel(v string) string { +// extractValueFromOsLabel returns only the value of the OS label (i.e. linux) +func extractValueFromOsLabel(v string) string { label := strings.ReplaceAll(v, operatorFrameworkOSLabel, "") return label } diff --git a/pkg/validation/internal/multiarch_test.go b/pkg/validation/internal/multiarch_test.go index f400e158e..115fa840c 100644 --- a/pkg/validation/internal/multiarch_test.go +++ b/pkg/validation/internal/multiarch_test.go @@ -1,11 +1,12 @@ package internal import ( + "reflect" + "testing" + "github.com/operator-framework/api/pkg/manifests" "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/stretchr/testify/require" - "reflect" - "testing" ) const opm_test_image = "quay.io/operator-framework/opm:latest" @@ -82,6 +83,9 @@ func Test_ValidateMultiArchFrom(t *testing.T) { } results := validateMultiArchWith(tt.args.bundle, "") + t.Log(results.Warnings) + t.Log(results.Errors) + require.Equal(t, tt.wantWarning, len(results.Warnings) > 0) if tt.wantWarning { require.Equal(t, len(tt.warnStrings), len(results.Warnings)) @@ -127,8 +131,10 @@ func Test_LoadImagesFromCSV(t *testing.T) { } mb.loadImagesFromCSV() require.Equal(t, len(mb.managerImages), 1) - require.Greater(t, len(mb.managerImages["quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b"]), 0) - require.Equal(t, 0, len(mb.allOtherImages)) + require.Equal(t, len(mb.managerImages["quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b"]), 0) + require.Equal(t, 1, len(mb.otherCSVDeploymentImages)) + require.Equal(t, len(mb.otherCSVDeploymentImages["quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b"]), 0) + require.Equal(t, 0, len(mb.relatedImages)) }) } } @@ -160,10 +166,12 @@ func Test_LoadImagesFromCSVWithRelatedImage(t *testing.T) { bundle: tt.fields.Bundle, } mb.loadImagesFromCSV() - require.Equal(t, 1, len(mb.managerImages)) - require.Greater(t, len(mb.managerImages["quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b"]), 0) - require.Equal(t, 1, len(mb.allOtherImages)) - require.Greater(t, len(mb.allOtherImages["related-image-test"]), 0) + require.Equal(t, len(mb.managerImages), 1) + require.Equal(t, len(mb.managerImages["quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b"]), 0) + require.Equal(t, 1, len(mb.otherCSVDeploymentImages)) + require.Equal(t, len(mb.otherCSVDeploymentImages["quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b"]), 0) + require.Equal(t, 1, len(mb.relatedImages)) + require.Equal(t, len(mb.relatedImages["related-image-test"]), 0) }) } } @@ -317,20 +325,20 @@ func Test_LoadAllPossibleArchSupported(t *testing.T) { } } -func Test_LoadAllPossibleSoSupported(t *testing.T) { +func Test_LoadAllPossibleOsSupported(t *testing.T) { validBundle, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") - managerSoLinux := map[string]string{} - managerSoLinux["linux"] = "linux" + managerOsLinux := map[string]string{} + managerOsLinux["linux"] = "linux" validBundleWithInfraLabels, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") - mockInfraLabelsSo := map[string]string{} - mockInfraLabelsSo["operatorframework.io/os.linux"] = "supported" - mockInfraLabelsSo["operatorframework.io/os.other"] = "supported" - validBundleWithInfraLabels.CSV.Labels = mockInfraLabelsSo + mockInfraLabelsOs := map[string]string{} + mockInfraLabelsOs["operatorframework.io/os.linux"] = "supported" + mockInfraLabelsOs["operatorframework.io/os.other"] = "supported" + validBundleWithInfraLabels.CSV.Labels = mockInfraLabelsOs - managerMultiSo := map[string]string{} - managerMultiSo["linux"] = "linux" - managerMultiSo["other"] = "other" + managerMultiOs := map[string]string{} + managerMultiOs["linux"] = "linux" + managerMultiOs["other"] = "other" type fields struct { bundle *manifests.Bundle @@ -345,14 +353,14 @@ func Test_LoadAllPossibleSoSupported(t *testing.T) { fields: fields{ bundle: validBundle, }, - want: managerSoLinux, + want: managerOsLinux, }, { name: "should return the infra labels when informed", fields: fields{ bundle: validBundleWithInfraLabels, }, - want: managerMultiSo, + want: managerMultiOs, }, } for _, tt := range tests { @@ -363,10 +371,10 @@ func Test_LoadAllPossibleSoSupported(t *testing.T) { data.loadImagesFromCSV() data.loadInfraLabelsFromCSV() - data.loadAllPossibleSoSupported() + data.loadAllPossibleOsSupported() if !reflect.DeepEqual(data.managerOs, tt.want) { - t.Errorf("loadAllPossibleSoSupported() got = %v, want %v", data.managerOs, tt.want) + t.Errorf("loadAllPossibleOsSupported() got = %v, want %v", data.managerOs, tt.want) } }) } @@ -405,8 +413,8 @@ func Test_multiArchValidator_checkSupportDefined(t *testing.T) { bundle: validBundleWithLabels, }, wantError: true, - errStrings: []string{"not all images specified are providing the support described via the CSV labels. Note that (SO.architecture): (linux.arm64) was not found for the image(s) [quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b]", - "not all images specified are providing the support described via the CSV labels. Note that (SO.architecture): (other.arm64) was not found for the image(s) [quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b]"}, + errStrings: []string{"not all images specified are providing the support described via the CSV labels. Note that (OS.architecture): (linux.arm64) was not found for the image(s) [quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b]", + "not all images specified are providing the support described via the CSV labels. Note that (OS.architecture): (other.arm64) was not found for the image(s) [quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b]"}, }, } for _, tt := range tests { @@ -420,14 +428,20 @@ func Test_multiArchValidator_checkSupportDefined(t *testing.T) { // Mock inspected platform for key, _ := range data.managerImages { - data.managerImages[key] = []platform{{"amd64", "linux"}} + data.managerImages[key] = []platform{{"linux", "amd64"}} + } + for key, _ := range data.otherCSVDeploymentImages { + data.otherCSVDeploymentImages[key] = []platform{{"linux", "amd64"}} } data.loadAllPossibleArchSupported() - data.loadAllPossibleSoSupported() + data.loadAllPossibleOsSupported() data.checkSupportDefined() + t.Log(data.warns) + t.Log(data.errors) + require.Equal(t, tt.wantWarning, len(data.warns) > 0) if tt.wantWarning { require.Equal(t, len(tt.warnStrings), len(data.warns)) @@ -475,7 +489,7 @@ func Test_multiArchValidator_checkMissingLabels(t *testing.T) { name: "should raise no error or warning when only supports linux.amd64 (no labels are required)", fields: fields{ bundle: validBundle, - supportedPlatforms: []platform{{"amd64", "linux"}}, + supportedPlatforms: []platform{{"linux", "amd64"}}, }, }, { @@ -483,10 +497,10 @@ func Test_multiArchValidator_checkMissingLabels(t *testing.T) { fields: fields{ bundle: validBundleWithLabels, supportedPlatforms: []platform{ - {"amd64", "other"}, - {"arm64", "other"}, - {"amd64", "linux"}, - {"arm64", "linux"}, + {"other", "amd64"}, + {"other", "arm64"}, + {"linux", "amd64"}, + {"linux", "arm64"}, }, }, }, @@ -495,16 +509,16 @@ func Test_multiArchValidator_checkMissingLabels(t *testing.T) { fields: fields{ bundle: validBundleWithLabels, supportedPlatforms: []platform{ - {"amd64", "other"}, - {"arm64", "other"}, - {"missing", "other"}, - {"amd64", "linux"}, - {"arm64", "linux"}, - {"missing", "linux"}, + {"other", "amd64"}, + {"other", "arm64"}, + {"other", "missing"}, + {"linux", "amd64"}, + {"linux", "arm64"}, + {"linux", "missing"}, }, }, wantWarning: true, - warnStrings: []string{"check if the CSV is missing the label (operatorframework.io/arch.) for the Arch(s): [\"missing\"]. Be aware that your Operator manager image [\"quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b\"] provides this support. Thus, it is very likely that you want to provide it and if you support more than amd64 architectures, you MUST,use the required labels for all which are supported.Otherwise, your solution cannot be listed on the cluster for these architectures"}, + warnStrings: []string{"check if the CSV is missing the label (operatorframework.io/arch.) for the Arch(s): [\"missing\"]. Be aware that your Operator manager image [\"quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b\"] provides this support. Thus, it is very likely that you want to provide it and if you support more than amd64 architectures, you MUST,use the required labels for all which are supported.Otherwise, your solution cannot be listed on the cluster for these architectures"}, }, } for _, tt := range tests { @@ -522,9 +536,126 @@ func Test_multiArchValidator_checkMissingLabels(t *testing.T) { } data.loadAllPossibleArchSupported() - data.loadAllPossibleSoSupported() + data.loadAllPossibleOsSupported() data.checkMissingLabelsForArchs() + t.Log(data.warns) + t.Log(data.errors) + + require.Equal(t, tt.wantWarning, len(data.warns) > 0) + if tt.wantWarning { + require.Equal(t, len(tt.warnStrings), len(data.warns)) + for _, w := range data.warns { + wString := w.Error() + require.Contains(t, tt.warnStrings, wString) + } + } + + require.Equal(t, tt.wantError, len(data.errors) > 0) + if tt.wantError { + require.Equal(t, len(tt.errStrings), len(data.errors)) + for _, w := range data.errors { + wString := w.Error() + require.Contains(t, tt.errStrings, wString) + } + } + }) + } +} + +func Test_multiArchValidator_checkNodeAffinity(t *testing.T) { + validBundle, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + + validBundleMissingNodeAffinity, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + for i := range validBundleMissingNodeAffinity.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { + validBundleMissingNodeAffinity.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[i].Spec.Template.Spec.Affinity = nil + } + + type fields struct { + bundle *manifests.Bundle + supportedPlatforms []platform + } + tests := []struct { + name string + fields fields + wantWarning bool + wantError bool + warnStrings []string + errStrings []string + }{ + { + name: "should raise no error or warning when image data matches node affinity", + fields: fields{ + bundle: validBundle, + supportedPlatforms: []platform{ + {"linux", "amd64"}, + {"linux", "arm64"}, + {"linux", "ppc64le"}, + {"linux", "s390x"}, + }, + }, + }, + { + name: "should raise warning when the node affinity information is missing or invalid", + fields: fields{ + bundle: validBundleMissingNodeAffinity, + supportedPlatforms: []platform{ + {"linux", "amd64"}, + {"linux", "arm64"}, + {"linux", "ppc64le"}, + {"linux", "s390x"}, + }, + }, + wantWarning: true, + warnStrings: []string{"check if the CSV has a missing or invalid node affinity configuration for the image: \"quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b\". The image data suggests the following platforms are supported: [\"linux/amd64\" \"linux/arm64\" \"linux/ppc64le\" \"linux/s390x\"]"}, + }, + { + name: "should raise a warning if node affinity supports more arches than the image data", + fields: fields{ + bundle: validBundle, + supportedPlatforms: []platform{ + {"linux", "amd64"}, + {"linux", "arm64"}, + }, + }, + wantWarning: true, + warnStrings: []string{"the CSV includes [\"linux/ppc64le\" \"linux/s390x\"] in the node affinity configuration for the image: \"quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b\", but the image data suggests the following platforms are supported: [\"linux/amd64\" \"linux/arm64\"]"}, + }, + { + name: "should raise a warning if the image data supports more arches than the node affinity", + fields: fields{ + bundle: validBundle, + supportedPlatforms: []platform{ + {"other", "amd64"}, + {"other", "arm64"}, + {"linux", "amd64"}, + {"linux", "arm64"}, + {"linux", "ppc64le"}, + {"linux", "s390x"}, + }, + }, + wantWarning: true, + warnStrings: []string{"the image data indicates [\"other/amd64\" \"other/arm64\"] is supported for the image: \"quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b\", but the node affinity configuration for the image only specifies [\"linux/amd64\" \"linux/arm64\" \"linux/ppc64le\" \"linux/s390x\"]"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := &multiArchValidator{ + bundle: tt.fields.bundle, + } + + data.loadImagesFromCSV() + + // Mock inspected platform + for key, _ := range data.managerImages { + data.managerImages[key] = tt.fields.supportedPlatforms + } + + data.checkNodeAffinity(data.managerImages) + + t.Log(data.warns) + t.Log(data.errors) + require.Equal(t, tt.wantWarning, len(data.warns) > 0) if tt.wantWarning { require.Equal(t, len(tt.warnStrings), len(data.warns)) @@ -543,5 +674,6 @@ func Test_multiArchValidator_checkMissingLabels(t *testing.T) { } } }) + } } diff --git a/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml b/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml index f5ff405eb..29d1f682a 100644 --- a/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml +++ b/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml @@ -192,6 +192,22 @@ spec: name: etcd-operator-alm-owned name: etcd-operator-alm-owned spec: + affinity: + nodeAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + nodeSelectorTerms: + - matchExpressions: + - key: kubernetes.io/arch + operator: In + values: + - amd64 + - arm64 + - ppc64le + - s390x + - key: kubernetes.io/os + operator: In + values: + - linux containers: - command: - etcd-operator @@ -205,8 +221,8 @@ spec: valueFrom: fieldRef: fieldPath: metadata.name - image: quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b - name: etcd-operator + image: quay.io/coreos/etcd-operator2@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b + name: manager resources: limits: cpu: 500m