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.

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

Add api to generate endpoints #207

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Add api to generate endpoints #207

merged 1 commit into from
Sep 26, 2024

Conversation

maksymvavilov
Copy link
Contributor

@maksymvavilov maksymvavilov commented Aug 8, 2024

Moving logic that generates endpoints from the kuadrant-operator repo to the DNS-operator.
Also transfer applicable test cases and extend them a bit to cover a few edge cases since it is a unit testing and it is cheap.
There is a bump in the GO version - gatewayAPI requires it. It is debatable if we should be using Gateways in our API. An alternative would be to use addresses and labels in our "custom" struct and have a parser on the kuadrant-operator side. Just like there is a Routing struct to avoid policy usage.

@maksymvavilov maksymvavilov force-pushed the gh-187 branch 6 times, most recently from 4aca26e to 9aa7b99 Compare August 14, 2024 12:26
@maksymvavilov maksymvavilov changed the title [wip] add api tpo generate endpoints Add api tpo generate endpoints Aug 14, 2024
@maksymvavilov maksymvavilov marked this pull request as ready for review August 14, 2024 12:29
@maksymvavilov maksymvavilov changed the title Add api tpo generate endpoints Add api to generate endpoints Aug 14, 2024
@maksymvavilov maksymvavilov force-pushed the gh-187 branch 5 times, most recently from 2a61424 to df09647 Compare August 27, 2024 09:28
@maksymvavilov maksymvavilov linked an issue Sep 6, 2024 that may be closed by this pull request
Copy link
Member

@mikenairn mikenairn left a comment

Choose a reason for hiding this comment

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

IMO, the builder pattern would be better adopted if you use it to build the more complicated resources we actually care about (endpoints and/or dnsrecords) rather than just a fairly simple resource(It's just adding simple values to the struct) to pass to another function. This can also be done without modifying so much of the existing code if you stick to the idea of wrapping a kubernetes resource that represents the target which will have the metadata attributes for geo and weight applied, and is used to generated the likes of names and hashes that are used throughout the generation of loadbalanced endpoints.

Added an example of what a builder API for the endpoints could look like here, and also maybe how it could be used for a dnsrecord builder here .
Considering we might want to change the GVK of the record being generated by the kuadrant operator in the future, the endpoint builder is the most important one, and needs to be something we can use on it's own i.e. Not just part of a dnsrecord builder.

@maksymvavilov maksymvavilov force-pushed the gh-187 branch 2 times, most recently from 9adfafb to aa71478 Compare September 11, 2024 12:15
@maksymvavilov
Copy link
Contributor Author

I also moved integration tests from controller to test/integration to avoid import cycles.

@mikenairn
Copy link
Member

I also moved integration tests from controller to test/integration to avoid import cycles.

What exactly was causing a cyclic import? I can't see any obvious reason why you would get one, but if you are could it be a sign of a bad API design somewhere(packages that depends on the wrong things)?

I'm not completely against the idea of moving the current controller tests to an integration pkg under tests for the right reasons, we have done this in other repos, but i would do that in it's own PR if at all possible.

@maksymvavilov
Copy link
Contributor Author

I also moved integration tests from controller to test/integration to avoid import cycles.

What exactly was causing a cyclic import? I can't see any obvious reason why you would get one, but if you are could it be a sign of a bad API design somewhere(packages that depends on the wrong things)?

I'm not completely against the idea of moving the current controller tests to an integration pkg under tests for the right reasons, we have done this in other repos, but i would do that in it's own PR if at all possible.

It was due to the generated deep copy in the API package. My mistake, corrected and moved the tests back. Will be more attentive next time

@maksymvavilov maksymvavilov force-pushed the gh-187 branch 4 times, most recently from 1c7a55e to 0082de7 Compare September 16, 2024 14:08
@maksymvavilov
Copy link
Contributor Author

maksymvavilov commented Sep 16, 2024

@mikenairn I've changed the PR to accommodate the DNS Policy rework. Now there should be no need to do anything after the simplification of the policy

@maksymvavilov maksymvavilov force-pushed the gh-187 branch 2 times, most recently from eaeb0f8 to 3e66109 Compare September 18, 2024 10:13
@maksymvavilov maksymvavilov force-pushed the gh-187 branch 2 times, most recently from 0ee142b to 59c0ea7 Compare September 19, 2024 14:39
Copy link
Member

@mikenairn mikenairn left a comment

Choose a reason for hiding this comment

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

I think this looks fine, and looking through the code and comparing with the current kuadrant operator (getSimpleEndpoints and getLoadBalancedEndpoints) looks like it should work as expected.

@maksymvavilov
Copy link
Contributor Author

I think this looks fine, and looking through the code and comparing with the current kuadrant operator (getSimpleEndpoints and getLoadBalancedEndpoints) looks like it should work as expected.

Thanks!

Signed-off-by: Maskym Vavilov <mvavilov@redhat.com>
@maksymvavilov maksymvavilov added this pull request to the merge queue Sep 26, 2024
Merged via the queue into main with commit 2e24974 Sep 26, 2024
15 checks passed
@mikenairn mikenairn deleted the gh-187 branch September 26, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants