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.

Sign up for GitHub

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

Jump to bottom

OTA-559: pkg/manifest: Add 'overrides' handling #1502

Merged
merged 1 commit into from
Apr 26, 2023
Merged

OTA-559: pkg/manifest: Add 'overrides' handling #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 36 additions & 5 deletions pkg/manifest/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,12 @@ func (m *Manifest) UnmarshalJSON(in []byte) error {
if !ok {
return fmt.Errorf("expected manifest to decode into *unstructured.Unstructured, got %T", ud)
}
m.GVK = ud.GroupVersionKind()
m.Obj = ud
return m.populateFromObj()
}

func (m *Manifest) populateFromObj() error {
m.GVK = m.Obj.GroupVersionKind()
m.id = resourceId{
Group: m.GVK.Group,
Kind: m.GVK.Kind,
Expand Down Expand Up @@ -170,8 +174,8 @@ func checkFeatureSets(requiredFeatureSet string, annotations map[string]string)
// processing by cluster version operator. Pointer arguments can be set nil to avoid excluding based on that
// filter. For example, setting profile non-nil and capabilities nil will return an error if the manifest's
// profile does not match, but will never return an error about capability issues.
func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus) error {
return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, false)
func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride) error {
return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, false)
}

// IncludeAllowUnknownCapabilities returns an error if the manifest fails an inclusion filter and should be excluded from
Expand All @@ -181,7 +185,7 @@ func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string
// to capabilities filtering. When set to true a manifest will not be excluded simply because it contains an unknown
// capability. This is necessary to allow updates to an OCP version containing newly defined capabilities.
func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, requiredFeatureSet *string, profile *string,
capabilities *configv1.ClusterVersionCapabilitiesStatus, allowUnknownCapabilities bool) error {
capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, allowUnknownCapabilities bool) error {

annotations := m.Obj.GetAnnotations()
if annotations == nil {
Expand Down Expand Up @@ -213,7 +217,34 @@ func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, re

// If there is no capabilities defined in a release then we do not need to check presence of capabilities in the manifest
if capabilities != nil {
return checkResourceEnablement(annotations, capabilities, allowUnknownCapabilities)
err := checkResourceEnablement(annotations, capabilities, allowUnknownCapabilities)
if err != nil {
return err
}
}

if override := m.getOverrideForManifest(overrides); override != nil && override.Unmanaged {
return fmt.Errorf("overridden")
Copy link
Member

Choose a reason for hiding this comment

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

nit: we could surface what is being overriden?

Copy link
Member Author

Choose a reason for hiding this comment

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

4.13.0-ec.4 CI:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/periodic-ci-openshift-release-master-nightly-4.13-e2e-aws-sdn-serial/1631695148700143616/artifacts/e2e-aws-sdn-serial/gather-extra/artifacts/pods/openshift-cluster-version_cluster-version-operator-6f5d958c6b-k8t5l_cluster-version-operator.log | grep excluding | head -n2
I0303 16:57:25.646863       1 payload.go:210] excluding 0000_10_config-operator_01_infrastructure-TechPreviewNoUpgrade.crd.yaml group=apiextensions.k8s.io kind=CustomResourceDefinition namespace= name=infrastructures.config.openshift.io: "Default" is required, and release.openshift.io/feature-set=TechPreviewNoUpgrade
I0303 16:57:25.676524       1 payload.go:210] excluding 0000_30_cluster-api_00_credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-cluster-api-aws: "Default" is required, and release.openshift.io/feature-set=TechPreviewNoUpgrade

And that excluding ... context gets wrapped around in:

cluster-verison-operator $ git --no-pager grep 'excluding ' pkg
pkg/payload/payload.go:                                 klog.V(2).Infof("excluding %s group=%s kind=%s namespace=%s name=%s: %v\n", manifest.OriginalFilename, manifest.GVK.Group, manifest.GVK.Kind, manifest.Obj.GetNamespace(), manifest.Obj.GetName(), err)

That could probably be cleaned up by leaning on (*Manifest).String(), but it's there. Do we have similar wrapping around each call site?

cluster-version-operator $ git --no-pager grep -A1 '[.]Include('
pkg/cvo/sync_worker.go:                 if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides); err != nil {
pkg/cvo/sync_worker.go-                         klog.V(2).Infof("Skipping precreation of %s: %s", task, err)
--
pkg/cvo/sync_worker.go:                 if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides); err != nil {
pkg/cvo/sync_worker.go-                         klog.V(2).Infof("Skipping %s: %s", task, err)
--
pkg/payload/payload.go:                         if err := manifest.Include(&excludeIdentifier, &requiredFeatureSet, &profile, onlyKnownCaps, nil); err != nil {
pkg/payload/payload.go-                                 klog.V(2).Infof("excluding %s group=%s kind=%s namespace=%s name=%s: %v\n", manifest.OriginalFilename, manifest.GVK.Group, manifest.GVK.Kind, manifest.Obj.GetNamespace(), manifest.Obj.GetName(), err)

So Include is covered. What about IncludeAllowUnknownCapabilities?

$ git --no-pager grep -A4 '[.]IncludeAllowUnknownCapabilities(' | grep -v vendor/
pkg/payload/payload.go:         updateManErr := updateManifest.IncludeAllowUnknownCapabilities(nil, nil, nil, &clusterCaps, nil, true)
pkg/payload/payload.go-
pkg/payload/payload.go-         // update manifest is enabled, no need to check
pkg/payload/payload.go-         if updateManErr == nil {
pkg/payload/payload.go-                 continue
--
pkg/payload/payload.go:                 if err := currentManifest.IncludeAllowUnknownCapabilities(nil, nil, nil, &clusterCaps, nil, true); err != nil {
pkg/payload/payload.go-                         continue
pkg/payload/payload.go-                 }
pkg/payload/payload.go-                 caps := capability.GetImplicitlyEnabledCapabilities(currentManifest.GetManifestCapabilities(),
pkg/payload/payload.go-                         updateManifest.GetManifestCapabilities(), capabilities)
--
$ git --no-pager grep '^func .*\|[.]IncludeAllowUnknownCapabilities(' pkg/payload/payload.go | grep -B1 IncludeAllowUnknownCapabilities
pkg/payload/payload.go:func GetImplicitlyEnabledCapabilities(updatePayloadManifests []manifest.Manifest, currentPayloadManifests []manifest.Manifest,
pkg/payload/payload.go:         updateManErr := updateManifest.IncludeAllowUnknownCapabilities(nil, nil, nil, &clusterCaps, nil, true)
pkg/payload/payload.go:                 if err := currentManifest.IncludeAllowUnknownCapabilities(nil, nil, nil, &clusterCaps, nil, true); err != nil {

So unless we wanted context logging in GetImplicitlyEnabledCapabilities, I think we're good. And if we do want that context logging in GetImplicitlyEnabledCapabilities, we can handle that on the CVO side without adding "here's the manifest I am" context on this side.

/hold cancel

}

return nil
}

// getOverrideForManifest returns the override when override exists and nil otherwise.
func (m *Manifest) getOverrideForManifest(overrides []configv1.ComponentOverride) *configv1.ComponentOverride {
for _, override := range overrides {
namespace := override.Namespace
if m.id.Namespace == "" {
namespace = "" // cluster-scoped objects don't have namespace.
}
if m.id.equal(resourceId{
Group: override.Group,
Kind: override.Kind,
Name: override.Name,
Namespace: namespace,
}) {
return &override
}
}
return nil
}
Expand Down
56 changes: 52 additions & 4 deletions pkg/manifest/manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,7 @@ func Test_include(t *testing.T) {
profile *string
annotations map[string]interface{}
caps *configv1.ClusterVersionCapabilitiesStatus
overrides []configv1.ComponentOverride

expected error
}{
Expand Down Expand Up @@ -765,6 +766,44 @@ func Test_include(t *testing.T) {
EnabledCapabilities: []configv1.ClusterVersionCapability{"cap1", "cap2", "cap3"},
},
},
{
name: "unrelated override",
annotations: map[string]interface{}{},
overrides: []configv1.ComponentOverride{
{
Kind: "Pod",
Name: "my-pod",
Namespace: "my-namespace",
Unmanaged: true,
},
},
},
{
name: "override, but managed",
annotations: map[string]interface{}{},
overrides: []configv1.ComponentOverride{
{
Group: "apps",
Kind: "Deployment",
Name: "my-deployment",
Namespace: "my-namespace",
},
},
},
{
name: "unmanaged override",
annotations: map[string]interface{}{},
overrides: []configv1.ComponentOverride{
{
Group: "apps",
Kind: "Deployment",
Name: "my-deployment",
Namespace: "my-namespace",
Unmanaged: true,
},
},
expected: fmt.Errorf("overridden"),
},
{
name: "all nil",
profile: nil,
Expand All @@ -774,19 +813,27 @@ func Test_include(t *testing.T) {
}

for _, tt := range tests {
metadata := map[string]interface{}{}
metadata := map[string]interface{}{
"name": "my-deployment",
"namespace": "my-namespace",
}
t.Run(tt.name, func(t *testing.T) {
if tt.annotations != nil {
metadata["annotations"] = tt.annotations
}
m := Manifest{
Obj: &unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": metadata,
"apiVersion": "apps/v1",
"kind": "Deployment",
"metadata": metadata,
},
},
}
err := m.Include(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps)
err := m.populateFromObj()
assert.Equal(t, nil, err)

err = m.Include(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides)
assert.Equal(t, tt.expected, err)
})
}
Expand All @@ -801,6 +848,7 @@ func TestIncludeAllowUnknownCapabilities(t *testing.T) {
profile *string
annotations map[string]interface{}
caps *configv1.ClusterVersionCapabilitiesStatus
overrides []configv1.ComponentOverride
allowUnknown bool

expected error
Expand Down Expand Up @@ -844,7 +892,7 @@ func TestIncludeAllowUnknownCapabilities(t *testing.T) {
},
},
}
err := m.IncludeAllowUnknownCapabilities(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.allowUnknown)
err := m.IncludeAllowUnknownCapabilities(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides, tt.allowUnknown)
assert.Equal(t, tt.expected, err)
})
}
Expand Down