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

MCO-483: generate machineconfiguration (MCO) clients as part of MCO API migration #243

Merged
merged 3 commits into from
Oct 5, 2023
Merged

MCO-483: generate machineconfiguration (MCO) clients as part of MCO API migration #243

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
b27fe8e
add mco applyconfiguration hack, mco to codegen
jkyros Aug 7, 2023
a11014f
vendor in mco api
jkyros Oct 2, 2023
c7f9314
make generate
jkyros Oct 2, 2023
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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/openshift/client-go
go 1.20

require (
github.com/openshift/api v0.0.0-20230915112357-693d4b64813c
github.com/openshift/api v0.0.0-20230929221546-73731d374fbf
github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d
github.com/spf13/pflag v1.0.5
k8s.io/api v0.28.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
github.com/onsi/ginkgo/v2 v2.9.4 h1:xR7vG4IXt5RWx6FfIjyAtsoMAtnc3C/rFXBBd2AjZwE=
github.com/onsi/gomega v1.27.6 h1:ENqfyGeS5AX/rlXDd/ETokDz93u0YufY1Pgxuy/PvWE=
github.com/openshift/api v0.0.0-20230915112357-693d4b64813c h1:ro/BvvpAikMoZc/fsxJN6jxmK+4uIbdNIK9nwaFQ5xo=
github.com/openshift/api v0.0.0-20230915112357-693d4b64813c/go.mod h1:NFgA+laiQtptmjsp1trDnGqjV62nYzlUfQ6P5I9oqXA=
github.com/openshift/api v0.0.0-20230929221546-73731d374fbf h1:V9YUWOXuMh8JXBbmaEJEGIBl21XLsvGSPBMgYb4rJzE=
github.com/openshift/api v0.0.0-20230929221546-73731d374fbf/go.mod h1:qNtV0315F+f8ld52TLtPvrfivZpdimOzTi3kn9IVbtU=
github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d h1:RR4ah7FfaPR1WePizm0jlrsbmPu91xQZnAsVVreQV1k=
github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
github.com/openshift/kube-openapi v0.0.0-20230816122517-ffc8f001abb0 h1:GPlAy197Jkr+D0T2FNWanamraTdzS/r9ZkT29lxvHaA=
Expand Down
25 changes: 23 additions & 2 deletions hack/update-codegen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,25 +55,46 @@ function generateApplyConfiguration(){
done
done

echo "Generating applyconfigurations"
if [ "$OUTPUT_PKG" == "github.com/openshift/client-go/machineconfiguration" ]
then
# TODO(jkyros): this is a temporary hack to ensure proper generation until the MCO can sort
# out their embedded corev1.ObjectReference in MachineConfigPoolStatusConfiguration, which needs
# to be resolved by the end of release-4.15 so this hack can be removed
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being tracked?

Copy link
Contributor Author

@jkyros jkyros Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I created MCO-726 for this purpose. Since we're in 4.15 now I moved it into our API migration epic, so it will be required to be completed in order for MCO to close the epic 😄

echo "Generating applyconfigurations specifically for MCO"
applyconfigurationgen_external_apis_csv="$(codegen::join , "${FQ_APIS[@]}")"
applyconfigurations_package="${OUTPUT_PKG}/${CLIENTSET_PKG_NAME:-applyconfigurations}"
${APPLYCONFIGURATION_GEN} \
--output-package "${applyconfigurations_package}" \
--input-dirs "${applyconfigurationgen_external_apis_csv}" \
--external-applyconfigurations k8s.io/api/core/v1.ObjectReference:k8s.io/client-go/applyconfigurations/core/v1 \
Comment on lines +63 to +69
Copy link
Contributor Author

@jkyros jkyros Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So the MCO embeds a corev1.ObjectReference here, which makes applyconfiguration-gen generate code that doesn't compile.

# github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:27:11: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:35:16: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:43:11: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:51:10: cannot use &value (value of type *types.UID) as type types.UID in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:59:17: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:67:22: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:75:16: cannot use &value (value of type *string) as type string in assignment
make: *** [Makefile:40: _build-machine-config-daemon] Error 2

It looks like applyconfiguration-gen includes the following as external by default (which I believe is why this is an issue with corev1.ObjectReference but not an issue with the metav1.TypeMeta/metav1.ObjectMeta embedded fields that seem to work:

k8s.io/apimachinery/pkg/apis/meta/v1.TypeMeta:k8s.io/client-go/applyconfigurations/meta/v1,

k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta:k8s.io/client-go/applyconfigurations/meta/v1,

k8s.io/apimachinery/pkg/apis/meta/v1.OwnerReference:k8s.io/client-go/applyconfigurations/meta/v1

If I add corev1.ObjectReference as external, and point it to its appyconfiguration:

k8s.io/api/core/v1.ObjectReference:k8s.io/client-go/applyconfigurations/core/v1

Everything generates right for the MCO, but it changes a bunch of the other packages that were originally generated without the presence of v1.ObjectReferenceApplyConfiguration , and that seems...not good...because they've already been working well for quite some time.

So...I only added it when we're generating for the MCO and left it out for everyone else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the "additional damage" in other packages if I let them generate with corev1 as external: 3367954

Copy link
Contributor

Choose a reason for hiding this comment

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

It's curious that the MCO blows up but others haven't, is there any difference in how the object reference is being used in the other APIs? Pointer vs reference per chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are examples in the API of both pointer/non-pointer usages, but ours is the only one that is embedded:

apps/v1/types.go:       To corev1.ObjectReference `json:"to" protobuf:"bytes,2,opt,name=to"`
apps/v1/types.go:       From corev1.ObjectReference `json:"from" protobuf:"bytes,3,opt,name=from"`
apps/v1/types.go:       From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
apps/v1/types.go:       From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
authorization/v1/types.go:      Subjects []corev1.ObjectReference `json:"subjects" protobuf:"bytes,4,rep,name=subjects"`
authorization/v1/types.go:      RoleRef corev1.ObjectReference `json:"roleRef" protobuf:"bytes,5,opt,name=roleRef"`
authorization/v1/types.go:      Subjects []corev1.ObjectReference `json:"subjects" protobuf:"bytes,4,rep,name=subjects"`
authorization/v1/types.go:      RoleRef corev1.ObjectReference `json:"roleRef" protobuf:"bytes,5,opt,name=roleRef"`
build/v1/types.go:      FromRef *corev1.ObjectReference `json:"fromRef,omitempty" protobuf:"bytes,2,opt,name=fromRef"`
build/v1/types.go:      Config *corev1.ObjectReference `json:"config,omitempty" protobuf:"bytes,9,opt,name=config"`
build/v1/types.go:      From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go:      From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go:      From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go:      From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go:      To *corev1.ObjectReference `json:"to,omitempty" protobuf:"bytes,1,opt,name=to"`
build/v1/types.go:      From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,2,opt,name=from"`
build/v1/types.go:      TriggeredByImage *corev1.ObjectReference `json:"triggeredByImage,omitempty" protobuf:"bytes,3,opt,name=triggeredByImage"`
build/v1/types.go:      From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,4,opt,name=from"`
image/v1/types.go:      From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,3,opt,name=from"`
image/v1/types.go:      From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
image/v1/types.go:      From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
machine/v1beta1/types_machinehealthcheck.go:    RemediationTemplate *corev1.ObjectReference `json:"remediationTemplate,omitempty"`
machine/v1beta1/types_machine.go:       NodeRef *corev1.ObjectReference `json:"nodeRef,omitempty"`
security/v1/types.go:   AllowedBy *corev1.ObjectReference `json:"allowedBy,omitempty" protobuf:"bytes,1,opt,name=allowedBy"`
template/v1/types.go:   Ref corev1.ObjectReference `json:"ref,omitempty" protobuf:"bytes,1,opt,name=ref"`
template/v1/types.go:   TemplateInstance corev1.ObjectReference `json:"templateInstance" protobuf:"bytes,1,opt,name=templateInstance"`
template/v1/types.go:   Secret corev1.ObjectReference `json:"secret" protobuf:"bytes,2,opt,name=secret"`
user/v1/types.go:       User corev1.ObjectReference `json:"user" protobuf:"bytes,4,opt,name=user"`
user/v1/types.go:       Identity corev1.ObjectReference `json:"identity,omitempty" protobuf:"bytes,2,opt,name=identity"`
user/v1/types.go:       User corev1.ObjectReference `json:"user,omitempty" protobuf:"bytes,3,opt,name=user"`

vs

machineconfiguration/v1/types.go:       PullSecret *corev1.ObjectReference `json:"pullSecret,omitempty" protobuf:"bytes,9,opt,name=pullSecret"`
---> machineconfiguration/v1/types.go:       corev1.ObjectReference `json:",inline"  protobuf:"bytes,1,rep,name=objectReference"`
machineconfiguration/v1/types.go:       Source []corev1.ObjectReference `json:"source,omitempty" protobuf:"bytes,2,rep,name=source"`

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, right, in general for APIs we try to avoid embedding structs like that, but, it's done now.

In theory you could change the go type without causing serialization errors, copying the fields from the object reference into the parent struct, that would mean you don't have this hack any more.

Also, we should remove the protobuf tags, they aren't used for CRDs

Copy link
Contributor Author

@jkyros jkyros Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In theory you could change the go type without causing serialization errors, copying the fields from the object reference into the parent struct, that would mean you don't have this hack any more.

Something like this? https://gist.github.com/jkyros/db4d27fb28b38fa2ea170164ebfabae9#file-hypothetical_atrocity-go That feels like it should be a crime 😛 I think it would work, but there is stuff in the MCO that cares that it's an ObjectReference (mostly tests) and we'd need to defuse all of that.

Would you entertain letting us keep this PR's MCO-only applyconfig hack for now (or something similar -- I don't want to make your update script ugly) until we can do some rearrangement? The applyconfigurations generated by this hack do seem to be proper and work, e.g. https://gist.github.com/jkyros/db4d27fb28b38fa2ea170164ebfabae9#file-main-go

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the gist looks like what I meant. Since the serialization is the same, it's fine for a CRD to make a change like that to the go type, I got second opinions on that and a few of us are in agreement 😅

I'm happy to let you carry on like this, but, can we stick a pin in this to fix it before the end of 4.15? I'd like a TODO to remove this hack

--external-applyconfigurations github.com/openshift/api/operator/v1.OperatorSpec:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
--external-applyconfigurations github.com/openshift/api/operator/v1.OperatorStatus:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
--external-applyconfigurations github.com/openshift/api/operator/v1.OperatorCondition:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
--external-applyconfigurations github.com/openshift/api/operator/v1.GenerationStatus:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
"$@"
else
echo "Generating applyconfigurations for $OUTPUT_PKG"
applyconfigurationgen_external_apis_csv="$(codegen::join , "${FQ_APIS[@]}")"
applyconfigurations_package="${OUTPUT_PKG}/${CLIENTSET_PKG_NAME:-applyconfigurations}"
${APPLYCONFIGURATION_GEN} \
--output-package "${applyconfigurations_package}" \
--input-dirs "${applyconfigurationgen_external_apis_csv}" \
--external-applyconfigurations github.com/openshift/api/operator/v1.OperatorSpec:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
--external-applyconfigurations github.com/openshift/api/operator/v1.OperatorStatus:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
--external-applyconfigurations github.com/openshift/api/operator/v1.OperatorCondition:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
--external-applyconfigurations github.com/openshift/api/operator/v1.GenerationStatus:github.com/openshift/client-go/operator/applyconfigurations/operator/v1 \
"$@"
fi


}

# Until we get https://github.com/kubernetes/kubernetes/pull/120877 merged we need to
# explicitly set these two variables which are not defaulted properly in generate-internal-groups.sh
export CLIENTSET_PKG=clientset
export CLIENTSET_NAME=versioned

for group in apiserver apps authorization build cloudnetwork image imageregistry network oauth project quota route samples security securityinternal template user; do
for group in apiserver apps authorization build cloudnetwork image imageregistry machineconfiguration network oauth project quota route samples security securityinternal template user; do
bash ${CODEGEN_PKG}/generate-groups.sh "lister,informer" \
github.com/openshift/client-go/${group} \
github.com/openshift/api \
Expand Down
96 changes: 96 additions & 0 deletions machineconfiguration/applyconfigurations/internal/internal.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 45 additions & 0 deletions machineconfiguration/applyconfigurations/machineconfiguration/v1/certexpiry.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading