Skip to content

Commit

Permalink
OCPBUGS-3779: support only image or version in ClusterVersion when do…
Browse files Browse the repository at this point in the history
…ing ocp precache

refactor code to support cases and add unit tests

add http mock reply when trying to make external calls

remove the extra check

update logic to handle when CV cr is not present

improve check for spec and add additional test make sure ClusterVersion is present

add const to where policies are changed, add another test make sure that empty strings are ok and make polcies minimal

make the upstearm test url more streamlined and obvious that no external calls are being made to test
  • Loading branch information
pixelsoccupied committed Dec 13, 2022
1 parent 1eb8159 commit bdfb0e1
Show file tree
Hide file tree
Showing 2 changed files with 305 additions and 48 deletions.
111 changes: 63 additions & 48 deletions controllers/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,24 @@ import (
"errors"

ranv1alpha1 "github.com/openshift-kni/cluster-group-upgrades-operator/api/v1alpha1"
utils "github.com/openshift-kni/cluster-group-upgrades-operator/controllers/utils"
"github.com/openshift-kni/cluster-group-upgrades-operator/controllers/utils"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

// extractOpenshiftImagePlatformFromPolicies validates that there's ClusterVersion policy, validates the content of ClusterVersion and extracts Image if needed
func (r *ClusterGroupUpgradeReconciler) extractOpenshiftImagePlatformFromPolicies(
policies []*unstructured.Unstructured) (string, error) {

var upstream string
var channel string
var version string
var image string
var (
upstream string
channel string
version string
image string
foundClusterVersionGroupVersion bool
)

// validate ClusterVersionGroupVersionKind and keep track to upstream, channel, version, image
for _, policy := range policies {
objects, err := r.stripPolicy(policy.Object)
if err != nil {
Expand All @@ -26,66 +31,76 @@ func (r *ClusterGroupUpgradeReconciler) extractOpenshiftImagePlatformFromPolicie
kind := object["kind"]
switch kind {
case utils.ClusterVersionGroupVersionKind().Kind:
cvSpec := object["spec"].(map[string]interface{})
desiredUpdate, found := cvSpec["desiredUpdate"]
if !found {
_, foundSpec := object["spec"]
if !foundSpec || object["spec"] == nil {
continue
}
desiredUpdateImage, found := desiredUpdate.(map[string]interface{})["image"]
if found && desiredUpdateImage == "" {
return "", errors.New("platform image defined but value is missing")
}

upgradeDefinedMultipleTimes := false

if object["spec"] != "" {
if object["spec"].(map[string]interface{})["upstream"] != nil {
nextUpstream := object["spec"].(map[string]interface{})["upstream"].(string)
if upstream == "" {
upstream = nextUpstream
} else if upstream != nextUpstream {
return "", errors.New("platform image defined more then once with conflicting upstream values")
}
}

if object["spec"].(map[string]interface{})["upstream"] != nil {
nextUpstream := object["spec"].(map[string]interface{})["upstream"].(string)
if upstream == "" {
upstream = nextUpstream
} else if upstream != nextUpstream {
upgradeDefinedMultipleTimes = true
}
if object["spec"].(map[string]interface{})["channel"] != nil {
nextChannel := object["spec"].(map[string]interface{})["channel"].(string)
if channel == "" {
channel = nextChannel
} else if channel != nextChannel {
return "", errors.New("platform image defined more then once with conflicting channel values")
}
}

if object["spec"].(map[string]interface{})["channel"] != nil {
nextChannel := object["spec"].(map[string]interface{})["channel"].(string)
if channel == "" {
channel = nextChannel
} else if channel != nextChannel {
upgradeDefinedMultipleTimes = true
if object["spec"].(map[string]interface{})["desiredUpdate"] != nil {
if object["spec"].(map[string]interface{})["desiredUpdate"].(map[string]interface{})["version"] != nil {
nextVersion := object["spec"].(map[string]interface{})["desiredUpdate"].(map[string]interface{})["version"].(string)
if version == "" {
version = nextVersion
} else if version != nextVersion {
return "", errors.New("platform image defined more then once with conflicting version values")
}
}

if object["spec"].(map[string]interface{})["desiredUpdate"] != nil {
if object["spec"].(map[string]interface{})["desiredUpdate"].(map[string]interface{})["version"] != nil {
nextVersion := object["spec"].(map[string]interface{})["desiredUpdate"].(map[string]interface{})["version"].(string)
if version == "" {
version = nextVersion
} else if version != nextVersion {
upgradeDefinedMultipleTimes = true
}
if object["spec"].(map[string]interface{})["desiredUpdate"].(map[string]interface{})["image"] != nil {
nextImage := object["spec"].(map[string]interface{})["desiredUpdate"].(map[string]interface{})["image"].(string)
if image == "" {
image = nextImage
} else if image != nextImage {
return "", errors.New("platform image defined more then once with conflicting image values")
}
}
}

if upgradeDefinedMultipleTimes {
return "", errors.New("platform image defined more then once with conflicting values")
}

image, err = r.getImageForVersionFromUpdateGraph(upstream, channel, version)
if err != nil {
return "", err
}
if image == "" {
return "", errors.New("unable to find platform image for specified upstream, channel, and version")
}
foundClusterVersionGroupVersion = true
default:
continue
}
}
}

if !foundClusterVersionGroupVersion {
return "", nil
}

// return early if policy is valid and user provided .Spec.DesiredUpdate.image in ClusterVersion
if image != "" {
return image, nil
}

// check for all the required variables needed to make http call and retrieve image
if upstream == "" || channel == "" || version == "" {
return "", errors.New("policy with ClusterVersion must have upstream, channel, and version when image is not provided")
}
image, err := r.getImageForVersionFromUpdateGraph(upstream, channel, version)
if err != nil {
return "", err
}
if image == "" {
return "", errors.New("unable to find platform image for specified upstream, channel, and version")
}

return image, nil
}

Expand Down
242 changes: 242 additions & 0 deletions controllers/validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,242 @@
package controllers

import (
"fmt"
"github.com/go-logr/logr"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/yaml"
"k8s.io/client-go/tools/record"
"net/http"
"net/http/httptest"
"sigs.k8s.io/controller-runtime/pkg/client"
"testing"
)

func TestClusterGroupUpgradeReconciler_extractOpenshiftImagePlatformFromPolicies(t *testing.T) {

const policy = `---
kind: Policy
spec:
policy-templates:
- objectDefinition:
kind: ConfigurationPolicy
spec:
object-templates:
- objectDefinition:
kind: ClusterVersion
spec:
channel: stable-4.11
desiredUpdate:
image: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6"
version: 4.11.12
upstream: https://my.api.openshift.com/api/upgrades_info/v100/graph
`
const policyWithOnlyImage = `---
kind: Policy
spec:
policy-templates:
- objectDefinition:
kind: ConfigurationPolicy
spec:
object-templates:
- objectDefinition:
kind: ClusterVersion
spec:
desiredUpdate:
image: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6"
upstream: https://my.api.openshift.com/api/upgrades_info/v100/graph
`

const policyWithOnlyImageAndVersionIsEmptyString = `---
kind: Policy
spec:
policy-templates:
- objectDefinition:
kind: ConfigurationPolicy
spec:
object-templates:
- objectDefinition:
kind: ClusterVersion
spec:
channel: stable-4.101
desiredUpdate:
image: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6"
version: ""
upstream: https://my.api.openshift.com/api/upgrades_info/v100/graph
`

policyWithOnlyVersion := `---
kind: Policy
spec:
policy-templates:
- objectDefinition:
kind: ConfigurationPolicy
spec:
object-templates:
- objectDefinition:
kind: ClusterVersion
spec:
channel: stable-4.11
desiredUpdate:
version: 4.11.12
upstream: %s
`

const policyWithTwoClusterVersion = `---
kind: Policy
spec:
policy-templates:
- objectDefinition:
kind: ConfigurationPolicy
spec:
object-templates:
- objectDefinition:
kind: ClusterVersion
spec:
channel: stable-4.11
desiredUpdate:
image: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6"
version: 4.11.12
upstream: https://my.api.openshift.com/api/upgrades_info/v100/graph
- objectDefinition:
kind: ClusterVersion
spec:
channel: stable-4.10 # this will cause the error
desiredUpdate:
image: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6"
version: 4.11.12
upstream: https://my.api.openshift.com/api/upgrades_info/v100/graph
`
const policyWithoutClusterVersion = `---
kind: Policy
spec:
policy-templates:
- objectDefinition:
kind: ConfigurationPolicy
spec:
object-templates:
- objectDefinition:
kind: ARandomType
spec:
channel: stable-4.11
desiredUpdate:
version: 4.11.12
upstream: https://my.api.openshift.com/api/upgrades_info/v100/graph
`
type fields struct {
Client client.Client
Log logr.Logger
Scheme *runtime.Scheme
Recorder record.EventRecorder
}

commonFields := fields{
// currently this object is not used anywhere but keeping in case there's change in the future
Client: nil,
Log: nil,
Scheme: nil,
Recorder: nil,
}

type args struct {
policies []*unstructured.Unstructured
}

tests := []struct {
name string
fields fields
args args
want string
wantErr assert.ErrorAssertionFunc
server *httptest.Server
}{
{
name: "with both Image and Version",
fields: commonFields,
args: args{policies: []*unstructured.Unstructured{mustConvertYamlStrToUnstructured(policy)}},
want: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6",
wantErr: assert.NoError,
},
{
name: "with only Image",
fields: commonFields,
args: args{policies: []*unstructured.Unstructured{mustConvertYamlStrToUnstructured(policyWithOnlyImage)}},
want: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6",
wantErr: assert.NoError,
},
{
name: "with only Image and version is an empty string",
fields: commonFields,
args: args{policies: []*unstructured.Unstructured{mustConvertYamlStrToUnstructured(policyWithOnlyImageAndVersionIsEmptyString)}},
want: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6",
wantErr: assert.NoError,
},
{
name: "with only Version",
fields: commonFields,
want: "quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6",
wantErr: assert.NoError,
server: httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte(`{"version":1,"nodes":[{"version":"4.11.12","payload":"quay.io/openshift-release-dev/ocp-release@sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6","metadata":{"io.openshift.upgrades.graph.previous.remove_regex":"4[.]10[.].*","io.openshift.upgrades.graph.release.channels":"candidate-4.11,fast-4.11,stable-4.11,candidate-4.12","io.openshift.upgrades.graph.release.manifestref":"sha256:0ca14e0f692391970fc23f88188f2a21f35a5ba24fe2f3cb908fd79fa46458e6","url":"https://access.redhat.com/errata/RHSA-2022:7201"}}]}`))
})),
},
{
name: "with both Image and Version and conflicting version with in multiple clusterversiongroup",
fields: commonFields,
args: args{policies: []*unstructured.Unstructured{mustConvertYamlStrToUnstructured(policyWithTwoClusterVersion)}},
wantErr: assert.Error,
},
{
name: "return empty string when ClusterVersion is not found",
fields: commonFields,
args: args{policies: []*unstructured.Unstructured{mustConvertYamlStrToUnstructured(policyWithoutClusterVersion)}},
want: "",
wantErr: assert.NoError,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := &ClusterGroupUpgradeReconciler{
Client: tt.fields.Client,
Log: tt.fields.Log,
Scheme: tt.fields.Scheme,
Recorder: tt.fields.Recorder,
}

// special case when there needs to be a http call
if tt.server != nil {
url := tt.server.URL
policyWithOnlyVersion = fmt.Sprintf(policyWithOnlyVersion, url)
tt.args.policies = []*unstructured.Unstructured{mustConvertYamlStrToUnstructured(policyWithOnlyVersion)}
}

got, err := r.extractOpenshiftImagePlatformFromPolicies(tt.args.policies)
if !tt.wantErr(t, err, fmt.Sprintf("extractOpenshiftImagePlatformFromPolicies(%v)", tt.args.policies)) {
return
}
assert.Equalf(t, tt.want, got, "extractOpenshiftImagePlatformFromPolicies(%v)", tt.args.policies)
})
}
}

// convertYamlStrToUnstructured helper func to convert a CR in Yaml string to Unstructured
func mustConvertYamlStrToUnstructured(cr string) *unstructured.Unstructured {
jCr, err := yaml.ToJSON([]byte(cr))
if err != nil {
panic(err.Error())
}

object, err := runtime.Decode(unstructured.UnstructuredJSONScheme, jCr)
if err != nil {
panic(err.Error())
}

uCr, ok := object.(*unstructured.Unstructured)
if !ok {
panic("unstructured.Unstructured expected")
}
return uCr
}

0 comments on commit bdfb0e1

Please sign in to comment.