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

Refactor GS API bindings #352

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Refactor GS API bindings #352

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 19, 2024

Description

Checklist

  • added release notes to Unreleased section in CHANGELOG.md, if user facing change

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@ghost ghost force-pushed the SYSENG-1741 branch 2 times, most recently from c711676 to efb22e4 Compare March 20, 2024 06:43
ghost
ghost commented Mar 20, 2024
View reviewed changes
pkg/apis/kubernetes/v1/cluster_genclient.go
Comment on lines -20 to -37

// explicitlyFalse returns true if the value of the provided
// bool pointer is set to false, nil and true pointer return false
func explicitlyFalse(b *bool) bool {
return b != nil && !pointer.BoolVal(b)
}

// prefixConfigurationValidCreate validates that prefixes configured as managed
// are not set. This is only relevant for `Create` operations.
func prefixConfigurationValidCreate(c *Cluster) bool {
var (
internalV4 = c.InternalIPv4Prefix == nil || explicitlyFalse(c.ManageInternalIPv4Prefix)
externalV4 = c.ExternalIPv4Prefix == nil || explicitlyFalse(c.ManageExternalIPv4Prefix)
externalV6 = c.ExternalIPv6Prefix == nil || explicitlyFalse(c.ManageExternalIPv6Prefix)
)

return internalV4 && externalV4 && externalV6
}
Copy link
Author

Choose a reason for hiding this comment

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

TODO: should probably go into a different hook (maybe a new RequestValidationHook)

ghost
ghost commented Mar 20, 2024
View reviewed changes
pkg/apis/kubernetes/v1/cluster_test.go
Comment on lines -71 to -101
var _ = Describe("Create Cluster", func() {
DescribeTable("prefix configurations", func(cluster Cluster, expected error) {
_, err := cluster.FilterAPIRequestBody(types.ContextWithOperation(context.TODO(), types.OperationCreate))
if expected == nil {
Expect(err).ToNot(HaveOccurred())
} else {
Expect(err).To(MatchError(expected))
}
},
Entry("all prefixes implicitly managed", Cluster{}, nil),
Entry("all prefixes explicitly managed", Cluster{ManageInternalIPv4Prefix: pointer.Bool(true), ManageExternalIPv4Prefix: pointer.Bool(true), ManageExternalIPv6Prefix: pointer.Bool(true)}, nil),
Entry("internal v4 prefix implicitly managed and explicitly provided", Cluster{InternalIPv4Prefix: &common.PartialResource{Identifier: "foo"}}, ErrManagedPrefixSet),
Entry("internal v4 prefix explicitly managed and explicitly provided", Cluster{ManageInternalIPv4Prefix: pointer.Bool(true), InternalIPv4Prefix: &common.PartialResource{Identifier: "foo"}}, ErrManagedPrefixSet),
Entry("internal v4 prefix explicitly unmanaged and provided", Cluster{ManageInternalIPv4Prefix: pointer.Bool(false), InternalIPv4Prefix: &common.PartialResource{Identifier: "foo"}}, nil),
Entry("external v4 prefix implicitly managed and explicitly provided", Cluster{ExternalIPv4Prefix: &common.PartialResource{Identifier: "foo"}}, ErrManagedPrefixSet),
Entry("external v4 prefix explicitly managed and explicitly provided", Cluster{ManageExternalIPv4Prefix: pointer.Bool(true), ExternalIPv4Prefix: &common.PartialResource{Identifier: "foo"}}, ErrManagedPrefixSet),
Entry("external v4 prefix explicitly unmanaged and provided", Cluster{ManageExternalIPv4Prefix: pointer.Bool(false), ExternalIPv4Prefix: &common.PartialResource{Identifier: "foo"}}, nil),
Entry("external v6 prefix implicitly managed and explicitly provided", Cluster{ExternalIPv6Prefix: &common.PartialResource{Identifier: "foo"}}, ErrManagedPrefixSet),
Entry("external v6 prefix explicitly managed and explicitly provided", Cluster{ManageExternalIPv6Prefix: pointer.Bool(true), ExternalIPv6Prefix: &common.PartialResource{Identifier: "foo"}}, ErrManagedPrefixSet),
Entry("external v6 prefix explicitly unmanaged and provided", Cluster{ManageExternalIPv6Prefix: pointer.Bool(false), ExternalIPv6Prefix: &common.PartialResource{Identifier: "foo"}}, nil),
)
})

var _ = DescribeTable("explicitlyFalse",
func(b *bool, expected bool) {
Expect(explicitlyFalse(b)).To(Equal(expected))
},
Entry("explicitly false", pointer.Bool(false), true),
Entry("explicitly true", pointer.Bool(true), false),
Entry("implicitly false (nil)", nil, false),
)
Copy link
Author

Choose a reason for hiding this comment

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

TODO: restore with new hook

@ghost ghost force-pushed the SYSENG-1741 branch 6 times, most recently from 9931a46 to 3bef5d0 Compare March 20, 2024 09:04
@LittleFox94
Copy link
Contributor

I don't like how we have a ticket to plan this and then there's lots of code already that I have to parse again ^^`

Short look only rn, as I actually tried to get another hour of sleep but had another idea to solve this, but looks like you add code to the generic client to handle specific cases? I think we talked about this before.. it's not gonna be maintainable with all the different behaviors we have in different APIs, that's why it was hooks in the first place

Our problem is the hooks being attached to the objects and working on the Object they get via the receiver, right?

What if we moved all the hooks away from the object types itself and have them optionally implement an interface "client overrides" where we have those hooks instead?

  • they would get the object via an argument, so extending objects via embedding still works
  • we still have custom logic per object type if needed but not in the generic client itself
  • we can share lots more code since e.g. all of GS has a single "client override" class they can all use
  • it makes the object types a bit cleaner conceptually - they are what's transferred via API request, not the logic for that
  • existing objects could just return themselves as their "client overrides" object to still have the hooks directly on themselves (with slightly changed interface), easing migration to this pattern

@LittleFox94
Copy link
Contributor

Ah so you are essentially doing those things here:

  • add a way to flatten linked objects to their identifier
    • implemented here with reflection in the generic client
    • implementing this with (generated) code in the Objects would be better imho, reflection can get slow and this is something we should be able to do at build-time rather easily?
  • remove the need for hooks for GS State
  • remove the need for hooks to deal with GS APIs returning the object data on DELETE
    • I still think this does not belong into the generic client and should work good with a "client override" class instead of hooks on all the affected Object types

Maybe we should just have Encode and Decode methods on the "client overrides" interface, getting a context.Context, io.Reader and Object to decode into - they could have different encoding/decoding logic per operation and even call out to the Object's Encode/Decode which is generated to handle e.g. this identifier flattening?

@ghost
Copy link
Author

ghost commented Mar 22, 2024

implementing this with (generated) code in the Objects would be better imho, reflection can get slow and this is something we should be able to do at build-time rather easily?

I did a benchmark and while flattening has a relative high overhead compared to just json encoding, considering the unit of the extra time needed is not a valid concern for an API client IMHO.

BenchmarkEncodeWithFlatten-8              134679              9044 ns/op
BenchmarkEncodeWithoutFlatten-8          5170754               247.3 ns/op

Implementing this with generated code would probably not be as trivial as it appears with the needed support for embedded structs.

remove the need for hooks to deal with GS APIs returning the object data on DELETE

  • I still think this does not belong into the generic client and should work good with a "client override" class instead of hooks on all the affected Object types

With the plan to eventually move everything to GS I think it'd be better located in the client core than overriding it for every resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@LittleFox94