Add Support for DNS Config on GKE #470
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Upgraded google.golang.org/api v0.52.0 to google.golang.org/api v0.97.0 sigs.k8s.io/controller-tools v0.8.0 to sigs.k8s.io/controller-tools v0.9.2 (fixes kubernetes-sigs/controller-tools#613 which occurs because google.golang.org/grpc is upgraded by the google.golang.org/api upgrade) The "DesiredAutopilot' field no longer exists in the newer version of the client. In the documentation, it says that converting between Standard and Autopilot is not supported: https://cloud.google.com/kubernetes-engine/docs/concepts/autopilot-overview#no_conversion Signed-off-by: Jensen Lo <54557407+jensentanlo@users.noreply.github.com> Signed-off-by: Jensen Lo <jtlo@alumni.princeton.edu>
Signed-off-by: Jensen Lo <54557407+jensentanlo@users.noreply.github.com> Signed-off-by: Jensen Lo <jtlo@alumni.princeton.edu>
Signed-off-by: Jensen Lo <54557407+jensentanlo@users.noreply.github.com> Signed-off-by: Jensen Lo <jtlo@alumni.princeton.edu>
jensentanlo
force-pushed
the
add-DNS-config-to-gke
branch
from
f4f0190
to
010a6c9
Compare
|
@Feggah Thanks for the help yesterday with testing. Would it be possible to kick off the pre-merge checks for me? |
Feggah
requested changes
Oct 5, 2022
There was a problem hiding this comment.
Thanks for your effort, @jensentanlo !
Added some small coments in the code.
I think you also need to add the DNSConfig logic for the LateInitializeSpec method. This method is responsible for importing the field value when it isn't specified in the MR manifest. This is important when we are importing an existing GKE.
Lastly, I think you should change the TestLateInitializeSpec and TestIsUpToDate tests to cover the modifications you made in this PR.
Added relevant annotations Changed optional fields to pointers Added late initialization logic (and manually tested) Added more tests Signed-off-by: Jensen Lo <jtlo@alumni.princeton.edu>
|
Thanks for the detailed review comments, I think I've addressed everything + manually tested again as well. |
Feggah
approved these changes
Oct 6, 2022
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
In the GKE API and gcloud, there is the option to select CloudDNS instead of relying on the regular kube-dns. This adds those fields to the GKE Cluster CRD.
In order to do that, I had to upgrade some dependencies to deal with some issues as detailed here:
Upgraded google.golang.org/api v0.52.0 to google.golang.org/api v0.97.0
sigs.k8s.io/controller-tools v0.8.0 to sigs.k8s.io/controller-tools v0.9.2 (fixes kubernetes-sigs/controller-tools#613 which occurs because google.golang.org/grpc is upgraded by the google.golang.org/api upgrade)
The "DesiredAutopilot' field no longer exists in the newer version of the client. In the documentation, it says that converting between Standard and Autopilot is not supported: https://cloud.google.com/kubernetes-engine/docs/concepts/autopilot-overview#no_conversion
Fixes #464
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
I created a GKE cluster and verified with gcloud that its dnsConfig was correctly set.