Skip to content

Bug 2081895: lib/resourcebuilder/apiext: Restore check for Established=True CRDs #771

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

Merged
openshift-merge-robot merged 3 commits into from
May 10, 2022
Merged

Bug 2081895: lib/resourcebuilder/apiext: Restore check for Established=True CRDs #771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50af6f3
lib/resourcedelete: Drop unused DeleteAPIServicev1
wking May 9, 2022
0833bba
lib/resourcebuilder: Use actual resource in check*Health calls
wking May 4, 2022
6e12b9f
lib/resourcebuilder/apiext: Restore check for Established=True CRDs
wking May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions hack/generate-lib-resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks
imports = {}
for import_name in [
'github.com/openshift/cluster-version-operator/lib/resourceapply',
'github.com/openshift/cluster-version-operator/lib/resourcedelete',
'github.com/openshift/cluster-version-operator/lib/resourceread',
'github.com/openshift/library-go/pkg/manifest',
'k8s.io/client-go/rest',
Expand Down Expand Up @@ -176,6 +177,8 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks
'',
'func (b *builder) Do(ctx context.Context) error {',
'\tobj := resourceread.ReadOrDie(b.raw)',
'\tupdatingMode := b.mode == UpdatingMode',
'\treconcilingMode := b.mode == ReconcilingMode',
'',
'\tswitch typedObject := obj.(type) {'
])
Expand All @@ -195,23 +198,42 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks
'\t\tif b.modifier != nil {',
'\t\t\tb.modifier(typedObject)',
'\t\t}',
'\t\tif deleteReq, err := resourcedelete.Delete{}{}(ctx, b.{}, typedObject,'.format(type_name, version, client_prop_name),
'\t\t\tupdatingMode); err != nil {',
'\t\t\treturn err',
'\t\t} else if !deleteReq {',
])

type_key = (package, type_name)
modifier = modifiers.get(type_key)
if modifier:
lines.extend([
'\t\tif err := {}(ctx, typedObject); err != nil {{'.format(modifier),
'\t\t\treturn err',
'\t\t}',
'\t\t\tif err := {}(ctx, typedObject); err != nil {{'.format(modifier),
'\t\t\t\treturn err',
'\t\t\t}',
])

health_check = health_checks.get(type_key)
actual = '_'
if health_check:
actual = 'actual'

lines.extend([
'\t\tif _, _, err := resourceapply.Apply{}{}(ctx, b.{}, typedObject); err != nil {{'.format(type_name, version, client_prop_name),
'\t\t\treturn err',
'\t\t}',
'\t\t\tif {}, _, err := resourceapply.Apply{}{}(ctx, b.{}, typedObject, reconcilingMode); err != nil {{'.format(actual, type_name, version, client_prop_name),
'\t\t\t\treturn err',
])
health_check = health_checks.get(type_key)

if health_check:
lines.append('\t\treturn {}(ctx, typedObject)'.format(health_check))
lines.extend([
'\t\t\t} else if actual != nil {',
'\t\t\t\treturn {}(ctx, actual)'.format(health_check),
])

lines.extend([
'\t\t\t}',
'\t\t}',
])


lines.extend([
'\tdefault:',
Expand Down Expand Up @@ -284,9 +306,10 @@ def scheme_group_versions(types):
}

health_checks = {
('k8s.io/api/apps/v1', 'Deployment'): 'b.checkDeploymentHealth',
('k8s.io/api/apps/v1', 'DaemonSet'): 'b.checkDaemonSetHealth',
('k8s.io/api/apps/v1', 'Deployment'): 'b.checkDeploymentHealth',
('k8s.io/api/batch/v1', 'Job'): 'b.checkJobHealth',
('k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1', 'CustomResourceDefinition'): 'b.checkCustomResourceDefinitionHealth',
}

generate_lib_resources(directory='lib', types=types, clients=clients, modifiers=modifiers, health_checks=health_checks)
25 changes: 25 additions & 0 deletions lib/resourcebuilder/apiext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package resourcebuilder

import (
"context"
"fmt"

apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
)

func (b *builder) checkCustomResourceDefinitionHealth(ctx context.Context, crd *apiextv1.CustomResourceDefinition) error {
if b.mode == InitializingMode {
return nil
}

for _, condition := range crd.Status.Conditions {
if condition.Type == apiextv1.Established {
if condition.Status == apiextv1.ConditionTrue {
return nil
}
return fmt.Errorf("CustomResourceDefinition %s Established=%s, %s: %s", crd.Name, condition.Status, condition.Reason, condition.Message)
}
}

return fmt.Errorf("CustomResourceDefinition %s does not declare an Established status condition: %v", crd.Name, crd.Status.Conditions)
}
19 changes: 12 additions & 7 deletions lib/resourcebuilder/resourcebuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,11 @@ func (b *builder) Do(ctx context.Context) error {
if err := b.modifyDaemonSet(ctx, typedObject); err != nil {
return err
}
if _, _, err := resourceapply.ApplyDaemonSetv1(ctx, b.appsClientv1, typedObject, reconcilingMode); err != nil {
if actual, _, err := resourceapply.ApplyDaemonSetv1(ctx, b.appsClientv1, typedObject, reconcilingMode); err != nil {
return err
} else if actual != nil {
return b.checkDaemonSetHealth(ctx, actual)
}
return b.checkDaemonSetHealth(ctx, typedObject)
}
case *appsv1.Deployment:
if b.modifier != nil {
Expand All @@ -130,10 +131,11 @@ func (b *builder) Do(ctx context.Context) error {
if err := b.modifyDeployment(ctx, typedObject); err != nil {
return err
}
if _, _, err := resourceapply.ApplyDeploymentv1(ctx, b.appsClientv1, typedObject, reconcilingMode); err != nil {
if actual, _, err := resourceapply.ApplyDeploymentv1(ctx, b.appsClientv1, typedObject, reconcilingMode); err != nil {
return err
} else if actual != nil {
Copy link
Contributor

@jottofar jottofar May 9, 2022

Choose a reason for hiding this comment

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

So before your change if a resource was IsCreateOnly this would have segfault'ed? Seems like a pretty big hole.

Should the nil check be in each of the functions instead?

Copy link
Member Author

@wking wking May 9, 2022

Choose a reason for hiding this comment

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

no, before my change we were passing typedObject through. That manifest content wasn't nil, but it didn't have anything useful in status either. I'm agnostic about nil checks or panics inside the check*Health functions; either way it would be a pretty serious bug to pass nil through to the health checker, and either way I expect we'll hear about it.

Copy link
Contributor

@jottofar jottofar May 9, 2022

Choose a reason for hiding this comment

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

ACK. Did not notice that param change.

return b.checkDeploymentHealth(ctx, actual)
}
return b.checkDeploymentHealth(ctx, typedObject)
}
case *batchv1.Job:
if b.modifier != nil {
Expand All @@ -143,10 +145,11 @@ func (b *builder) Do(ctx context.Context) error {
updatingMode); err != nil {
return err
} else if !deleteReq {
if _, _, err := resourceapply.ApplyJobv1(ctx, b.batchClientv1, typedObject, reconcilingMode); err != nil {
if actual, _, err := resourceapply.ApplyJobv1(ctx, b.batchClientv1, typedObject, reconcilingMode); err != nil {
return err
} else if actual != nil {
return b.checkJobHealth(ctx, actual)
}
return b.checkJobHealth(ctx, typedObject)
}
case *corev1.ConfigMap:
if b.modifier != nil {
Expand Down Expand Up @@ -252,8 +255,10 @@ func (b *builder) Do(ctx context.Context) error {
updatingMode); err != nil {
return err
} else if !deleteReq {
if _, _, err := resourceapply.ApplyCustomResourceDefinitionv1(ctx, b.apiextensionsClientv1, typedObject, reconcilingMode); err != nil {
if actual, _, err := resourceapply.ApplyCustomResourceDefinitionv1(ctx, b.apiextensionsClientv1, typedObject, reconcilingMode); err != nil {
return err
} else if actual != nil {
return b.checkCustomResourceDefinitionHealth(ctx, actual)
}
}
default:
Expand Down
39 changes: 0 additions & 39 deletions lib/resourcedelete/apireg.go
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions lib/resourceread/resourceread.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
)

var (
Expand All @@ -26,9 +25,6 @@ func init() {
if err := apiextensionsv1.AddToScheme(scheme); err != nil {
panic(err)
}
if err := apiregistrationv1.AddToScheme(scheme); err != nil {
panic(err)
}
if err := appsv1.AddToScheme(scheme); err != nil {
panic(err)
}
Expand All @@ -49,7 +45,6 @@ func init() {
}
decoder = codecs.UniversalDecoder(
apiextensionsv1.SchemeGroupVersion,
apiregistrationv1.SchemeGroupVersion,
appsv1.SchemeGroupVersion,
batchv1.SchemeGroupVersion,
corev1.SchemeGroupVersion,
Expand Down