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

Scorecard2 pod execution #2890

Merged
merged 78 commits into from
Apr 28, 2020
Merged
Show file tree
Hide file tree
Changes from 53 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
32087a9
add build of scorecard-test image, wire it into the scorecard-test im…
Apr 16, 2020
45c3d34
add a sample config file and pod spec, plus remove a log message from…
Apr 16, 2020
857e056
add start of pod definition
Apr 16, 2020
2e6db59
split out kubeclient and testpod
Apr 17, 2020
958e2ae
add random string to test pod names
Apr 17, 2020
6a971a4
add interim pod log capture
Apr 17, 2020
43555bb
add output formatting
Apr 20, 2020
db2d6e5
split out CR helper
Apr 20, 2020
d861b3d
add suggestion for test names if user enters invalid test name in the…
Apr 20, 2020
f009ea8
convert all output to ScorecardOutput
Apr 20, 2020
87ad36d
add initial delete pod defer
Apr 20, 2020
3658568
make the bundle singular, refactor the CRs into a helper
Apr 21, 2020
50981f6
add tar and untar of configmap
Apr 21, 2020
474641b
add cleanup flag
Apr 22, 2020
467c2ae
update kube client logic to look in various places for the config
Apr 22, 2020
7ebf414
add waittime command flag, add pod wait logic
Apr 22, 2020
295bb5d
change waittime to wait-time for the flag name
Apr 22, 2020
0abefbb
refactor some code, create a relation between the test and the test pod
Apr 22, 2020
f7e6e13
fix get bundle to check for valid paths to avoid errors in the api call
Apr 23, 2020
7f0cb6d
refactor for linter and general cleanup
Apr 23, 2020
13d04fe
refactor the TestBundle into just a registry.Bundle
Apr 23, 2020
457046f
remove basic check status test as its not useful
Apr 23, 2020
e3f2b41
remove reference to basic check status test which was removed
Apr 23, 2020
606aa16
fix tests
Apr 23, 2020
34a411b
remove basic status test from config
Apr 23, 2020
3356aa5
updates based on pr review comments
Apr 23, 2020
ec7800e
add build of scorecard-test image, wire it into the scorecard-test im…
Apr 16, 2020
b59aa81
add a sample config file and pod spec, plus remove a log message from…
Apr 16, 2020
99ad3fb
add start of pod definition
Apr 16, 2020
300ee72
split out kubeclient and testpod
Apr 17, 2020
f98e8f4
add random string to test pod names
Apr 17, 2020
cb322e4
add interim pod log capture
Apr 17, 2020
d7c98ab
add output formatting
Apr 20, 2020
0bf89cd
split out CR helper
Apr 20, 2020
9464b84
add suggestion for test names if user enters invalid test name in the…
Apr 20, 2020
38cc7af
convert all output to ScorecardOutput
Apr 20, 2020
fe64d30
add initial delete pod defer
Apr 20, 2020
28224bf
make the bundle singular, refactor the CRs into a helper
Apr 21, 2020
b92ee61
add tar and untar of configmap
Apr 21, 2020
770f91c
add cleanup flag
Apr 22, 2020
0dc37a9
update kube client logic to look in various places for the config
Apr 22, 2020
a4f8884
add waittime command flag, add pod wait logic
Apr 22, 2020
2405569
change waittime to wait-time for the flag name
Apr 22, 2020
a13becc
refactor some code, create a relation between the test and the test pod
Apr 22, 2020
d6e7087
fix get bundle to check for valid paths to avoid errors in the api call
Apr 23, 2020
ba955d3
refactor for linter and general cleanup
Apr 23, 2020
f894e11
refactor the TestBundle into just a registry.Bundle
Apr 23, 2020
4a0baa0
remove basic check status test as its not useful
Apr 23, 2020
9d4cc1d
remove reference to basic check status test which was removed
Apr 23, 2020
ae9d02d
fix tests
Apr 23, 2020
61c1108
remove basic status test from config
Apr 23, 2020
8d21ee3
updates based on pr review comments
Apr 23, 2020
98cfa69
Merge branch 'scorecard2-pod-execution' of github.com:jmccormick2001/…
Apr 23, 2020
67b7518
Update cmd/operator-sdk/alpha/scorecard/cmd.go
Apr 24, 2020
521ae4d
Update cmd/operator-sdk/alpha/scorecard/cmd.go
Apr 24, 2020
c1fdf82
Update internal/scorecard/alpha/tests/test_bundle.go
Apr 24, 2020
61a6344
Update internal/scorecard/alpha/tests/test_bundle.go
Apr 24, 2020
6a41299
make updates based on PR reviews
Apr 24, 2020
a761a26
refactor output formatting into the cmd
Apr 24, 2020
b9cebae
fix linter issue
Apr 24, 2020
2137885
rename tar function name
Apr 24, 2020
1329a4a
Update cmd/operator-sdk/alpha/scorecard/cmd.go
Apr 24, 2020
d457c0e
fix skipcleanup
Apr 24, 2020
b44503a
refactor crhelper to avoid extraneous unmarshalling
Apr 24, 2020
49c3ad7
replace fmt.Print with log.Error in a few cases
Apr 24, 2020
54818cc
updates from PR review
Apr 27, 2020
b67d4ae
Update internal/scorecard/alpha/tests/crhelper.go
Apr 27, 2020
4efa124
Update internal/scorecard/alpha/tests/crhelper.go
Apr 27, 2020
04b2dbc
Update cmd/operator-sdk/alpha/scorecard/cmd.go
Apr 27, 2020
cc62112
Update internal/scorecard/alpha/config.go
Apr 27, 2020
5aa6bdc
rename ScorecardTest to Test and other PR review comments
Apr 27, 2020
7c02717
various updates from PR comments
Apr 27, 2020
660dd7c
cleanup fmt.Error calls
Apr 27, 2020
c06c512
refactor Options struct into Scorecard struct and add struct methods …
Apr 27, 2020
fffa03a
make entrypoints a collection in the config file format
Apr 27, 2020
8dc43b6
fix GetBundle return values
Apr 27, 2020
c3075df
fix unit tests
Apr 28, 2020
397c11f
fix image
Apr 28, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ tags
test/ansible-operator/ansible-operator
test/helm-operator/helm-operator
images/scorecard-proxy/scorecard-proxy
images/scorecard-test/scorecard-test

# Test artifacts
pkg/ansible/runner/testdata/valid.yaml
Expand Down
53 changes: 40 additions & 13 deletions cmd/operator-sdk/alpha/scorecard/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package scorecard

import (
"fmt"
"log"

scorecard "github.com/operator-framework/operator-sdk/internal/scorecard/alpha"
"github.com/spf13/cobra"
Expand All @@ -25,10 +24,16 @@ import (

func NewCmd() *cobra.Command {
var (
config string
//bundle string // TODO - to be implemented
selector string
//list bool // TODO - to be implemented
config string
output string
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
bundle string
selector string
kubeconfig string
namespace string
serviceAccount string
list bool
cleanup bool
jmccormick2001 marked this conversation as resolved.
Show resolved Hide resolved
waitTime int
jmccormick2001 marked this conversation as resolved.
Show resolved Hide resolved
)
scorecardCmd := &cobra.Command{
Use: "scorecard",
Expand All @@ -38,31 +43,53 @@ func NewCmd() *cobra.Command {
RunE: func(cmd *cobra.Command, args []string) error {

var err error
o := scorecard.Options{}
o := scorecard.Options{
ServiceAccount: serviceAccount,
Namespace: namespace,
List: list,
joelanford marked this conversation as resolved.
Show resolved Hide resolved
BundlePath: bundle,
OutputFormat: output,
joelanford marked this conversation as resolved.
Show resolved Hide resolved
Cleanup: cleanup,
WaitTime: waitTime,
}
o.Client, err = scorecard.GetKubeClient(kubeconfig)
Copy link
Member

Choose a reason for hiding this comment

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

Does client-go or controller-runtime have a function we can use to get the client? It doesn't seem like something that should be in the scorecard package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I found that the controller-runtime has a GetConfig(), I've updated the code to use that since it was used in the 'operator-sdk new' command. I found that to support a --kubeconfig cobra flag with this library that I had to set the KUBECONFIG env var if --kubeconfig is specified. I found an example of that in cmd/operator-sdk/run/local.go.

if err != nil {
return fmt.Errorf("could not get Kube connection %s", err.Error())
jmccormick2001 marked this conversation as resolved.
Show resolved Hide resolved
}
o.Config, err = scorecard.LoadConfig(config)
joelanford marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("could not find config file %s", err.Error())
}

if bundle == "" {
return fmt.Errorf("bundle flag required")
}
Copy link
Member

Choose a reason for hiding this comment

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

Flag validation should happen before attempting to read files, open clients, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the bundle validation up.


o.Selector, err = labels.Parse(selector)
if err != nil {
return fmt.Errorf("could not parse selector %s", err.Error())
}

// TODO - process list and output formatting here?

if err := scorecard.RunTests(o); err != nil {
log.Fatal(err)
if list {
return scorecard.ListTests(o)
}
return nil

return scorecard.RunTests(o)
},
}

scorecardCmd.Flags().StringVarP(&config, "config", "c", "",
"path to a new to be defined DSL yaml formatted file that configures what tests get executed")
// scorecardCmd.Flags().StringVar(&bundle, "bundle", "", "path to the operator bundle contents on disk")
scorecardCmd.Flags().StringVar(&kubeconfig, "kubeconfig", "", "kubeconfig path")

scorecardCmd.Flags().StringVar(&bundle, "bundle", "", "path to the operator bundle contents on disk")
scorecardCmd.Flags().StringVarP(&selector, "selector", "l", "", "label selector to determine which tests are run")
// scorecardCmd.Flags().BoolVarP(&list, "list", "L", false, "option to enable listing which tests are run")
scorecardCmd.Flags().StringVarP(&namespace, "namespace", "n", "default", "namespace to run the test images in")
scorecardCmd.Flags().StringVarP(&output, "output", "o", "text", "Output format for results. Valid values: text, json")
scorecardCmd.Flags().StringVarP(&serviceAccount, "service-account", "s", "default", "service account to use for tests")
scorecardCmd.Flags().BoolVarP(&list, "list", "L", false, "option to enable listing which tests are run")
scorecardCmd.Flags().BoolVarP(&cleanup, "cleanup", "x", true, "option to disable resource cleanup after tests are run")
scorecardCmd.Flags().IntVarP(&waitTime, "wait-time", "w", 30, "time in seconds to wait for tests to complete")

return scorecardCmd
}
16 changes: 8 additions & 8 deletions hack/image/build-scorecard-test-image.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ set -eux

source hack/lib/image_lib.sh

# TODO build test image
#WD="$(dirname "$(pwd)")"
#GOOS=linux CGO_ENABLED=0 \
# go build \
# -gcflags "all=-trimpath=${WD}" \
# -asmflags "all=-trimpath=${WD}" \
# -o images/scorecard-test/scorecard-test \
# images/scorecard-test/cmd/test/main.go
# build scorecard test image
WD="$(dirname "$(pwd)")"
GOOS=linux CGO_ENABLED=0 \
go build \
-gcflags "all=-trimpath=${WD}" \
-asmflags "all=-trimpath=${WD}" \
-o images/scorecard-test/scorecard-test \
images/scorecard-test/cmd/test/main.go

# Build base image
pushd images/scorecard-test
Expand Down
5 changes: 2 additions & 3 deletions images/scorecard-test/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ ENV TEST=/usr/local/bin/scorecard-test \
USER_UID=1001 \
USER_NAME=test

# TODO install test binary
# COPY scorecard-test ${TEST}
# install test binary
COPY scorecard-test ${TEST}

COPY bin /usr/local/bin
RUN /usr/local/bin/user_setup


ENTRYPOINT ["/usr/local/bin/entrypoint"]

USER ${USER_UID}
43 changes: 36 additions & 7 deletions images/scorecard-test/cmd/test/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@ package main
import (
"encoding/json"
"fmt"
"io/ioutil"
"log"
"os"

"github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha2"
scapiv1alpha2 "github.com/operator-framework/operator-sdk/pkg/apis/scorecard/v1alpha2"

"github.com/operator-framework/operator-sdk/internal/scorecard/alpha"
"github.com/operator-framework/operator-sdk/internal/scorecard/alpha/tests"
)

Expand All @@ -35,7 +38,8 @@ import (
// test image.

const (
bundlePath = "/scorecard"
// bundleTar is the tar file containing the bundle contents
bundleTar = "/scorecard/bundle.tar"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I'm assuming you resorted to using a tar file because it isn't possible to create a single configmap from a directory recursively. However this approach is a little problematic because it would require every custom image implementation to untar the bundle before using it, which isn't the best.

We need to think about this a little bit. Other options include:

  • Keep the tar file (maybe also use gzip to keep the config map as small as possible since there's a 1MB limit). Every pod has the configmap volume AND an emptydir volume. The pod has an initContainer that does the untar-ing into the emptyDir so that the actual test image sees just the extracted directory structure. If UBI has tar built in, we wouldn't need to maintain a tar implementation.
  • Have separate config maps for each directory, one for manifests, one for metadata, one for scorecard. This won't really work well for kuttl tests though since each of those tests is a separate directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I could see using an initcontainer for this purpose. also, gzipping this file would be good to do too.

)

func main() {
Expand All @@ -44,7 +48,19 @@ func main() {
log.Fatal("test name argument is required")
}

cfg, err := tests.GetBundle(bundlePath)
// Create tmp directory for the untar'd bundle
tmpDir, err := ioutil.TempDir("/tmp", "scorecard-bundle")
if err != nil {
log.Fatal(err)
}
defer os.Remove(tmpDir)

err = alpha.Untartar(bundleTar, tmpDir)
if err != nil {
log.Fatalf("error untarring bundle %s", err.Error())
}

cfg, err := tests.GetBundle(tmpDir)
if err != nil {
log.Fatal(err.Error())
}
Expand All @@ -62,14 +78,10 @@ func main() {
result = tests.SpecDescriptorsTest(cfg)
case tests.OLMStatusDescriptorsTest:
result = tests.StatusDescriptorsTest(cfg)
case tests.BasicCheckStatusTest:
result = tests.CheckStatusTest(cfg)
case tests.BasicCheckSpecTest:
result = tests.CheckSpecTest(cfg)
default:
log.Fatal("invalid test name argument passed")
// TODO print out full list of test names to give a hint
// to the end user on what the valid tests are
result = printValidTests()
}

prettyJSON, err := json.MarshalIndent(result, "", " ")
Expand All @@ -79,3 +91,20 @@ func main() {
fmt.Printf("%s\n", string(prettyJSON))

}

// printValidTests will print out full list of test names to give a hint to the end user on what the valid tests are
func printValidTests() (result v1alpha2.ScorecardTestResult) {
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
result.State = scapiv1alpha2.FailState
result.Errors = make([]string, 0)
result.Suggestions = make([]string, 0)

str := fmt.Sprintf("Valid tests for this image include: %s, %s, %s, %s, %s, %s",
tests.OLMBundleValidationTest,
tests.OLMCRDsHaveValidationTest,
tests.OLMCRDsHaveResourcesTest,
tests.OLMSpecDescriptorsTest,
tests.OLMStatusDescriptorsTest,
tests.BasicCheckSpecTest)
result.Suggestions = append(result.Suggestions, str)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this seems more like an error than a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, change made to use Errors instead of Suggestions.

return result
}
49 changes: 49 additions & 0 deletions internal/scorecard/alpha/bundle.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2020 The Operator-SDK Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package alpha

import (
"fmt"
"io/ioutil"
"os"
)

// getBundleData tars up the contents of a bundle from a path, and returns that tar file in []byte
func getBundleData(bundlePath string) (bundleData []byte, err error) {

// make sure the bundle exists on disk
_, err = os.Stat(bundlePath)
if os.IsNotExist(err) {
return bundleData, fmt.Errorf("bundle path is not valid %s", err.Error())
}

tempTarFileName := fmt.Sprintf("%s%ctempBundle-%s.tar", os.TempDir(), os.PathSeparator, randomString())

paths := []string{bundlePath}
err = Tartar(tempTarFileName, paths)
if err != nil {
return bundleData, fmt.Errorf("error creating tar of bundle %s", err.Error())
}

defer os.Remove(tempTarFileName)

var buf []byte
buf, err = ioutil.ReadFile(tempTarFileName)
if err != nil {
return bundleData, fmt.Errorf("error reading tar of bundle %s", err.Error())
}

return buf, err
}
27 changes: 27 additions & 0 deletions internal/scorecard/alpha/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,43 @@

package alpha

import (
"io/ioutil"

"gopkg.in/yaml.v2"
jmccormick2001 marked this conversation as resolved.
Show resolved Hide resolved
v1 "k8s.io/api/core/v1"
)

type ScorecardTest struct {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: rename this to Test since this will be in the scorecard package and it would stutter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed ScorecardTest to Test.

Name string `yaml:"name"` // The container test name
Image string `yaml:"image"` // The container image name
Entrypoint string `yaml:"entrypoint,omitempty"` // An optional entrypoint passed to the test image
Labels map[string]string `yaml:"labels"` // User defined labels used to filter tests
Description string `yaml:"description"` // User readable test description
TestPod *v1.Pod `yaml:"-"` // Pod that ran the test
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
}

// Config represents the set of test configurations which scorecard
// would run based on user input
type Config struct {
Tests []ScorecardTest `yaml:"tests"`
}

// LoadConfig will find and return the scorecard config, the config file
// can be passed in via command line flag or from a bundle location or
// bundle image
func LoadConfig(configFilePath string) (Config, error) {
c := Config{}

// TODO handle getting config from bundle (ondisk or image)
yamlFile, err := ioutil.ReadFile(configFilePath)
if err != nil {
return c, err
}

if err := yaml.Unmarshal(yamlFile, &c); err != nil {
return c, err
}

return c, nil
}
11 changes: 6 additions & 5 deletions internal/scorecard/alpha/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ func TestInvalidConfigPath(t *testing.T) {
for _, c := range cases {
t.Run(c.configPathValue, func(t *testing.T) {
_, err := LoadConfig(c.configPathValue)
if err != nil && c.wantError {
t.Logf("Wanted error and got error : %v", err)
return
} else if err != nil && !c.wantError {
t.Errorf("Wanted result but got error: %v", err)
if err == nil && c.wantError {
t.Fatalf("Wanted error but got no error")
} else if err != nil {
if !c.wantError {
t.Fatalf("Wanted result but got error: %v", err)
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about the default one:

   if (err != nil) != c.wantError {
	t.Errorf("TestInvalidConfigPath error = %v, wantErr %v", err, tt.wantErr)
	return
   }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, not sure, for some reason that syntax I find hard to read. I'll think about it however.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok if you would prefer this way 👍 all fine .. just to clarifies .. it is that I found a little weird since usually, we see something as :

func TestAbs(t *testing.T) {
    got := Abs(-1)
    if got != 1 {
        t.Errorf("Abs(-1) = %d; want 1", got)
    }
}

}

Expand Down