-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Platform] Add API for Kubernetes provider configuration discovery #8371
Conversation
This adds the two new configuration options required by the Kubernetes discovery API which is introduced in yugabyte/yugabyte-db#8371 Test plan: - Upgraded existing Helm release, the API works as expected. Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Do update the Test Plan for the PR description with a sample response and the test environment.
Most of the comments are about code organization.
managed/src/main/java/com/yugabyte/yw/controllers/CloudProviderController.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/controllers/CloudProviderController.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/controllers/CloudProviderController.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/common/KubernetesManager.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/common/KubernetesManager.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/controllers/CloudProviderController.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/controllers/CloudProviderController.java
Outdated
Show resolved
Hide resolved
managed/src/main/java/com/yugabyte/yw/controllers/CloudProviderController.java
Outdated
Show resolved
Hide resolved
cf46eb4
to
fa0d1d3
Compare
fa0d1d3
to
2f3463c
Compare
- This API tries to fetch the details which can be used to pre-fill the data during Kubernetes provider creation (should work on OpenShift or Tanzu as well). The returned JSON is similar to what we use for the create call. Please refer this issue for more details: yugabyte#7394 - Use blank string for KUBECONFIG if kubeconfig file is not provided. This allows us to in-cluster credentials (ServiceAccount) when running in the same cluster as of the target cluster. Can be used to simplify the current flow later, we won't need a separate kubeconfig in that case. - Finds out the region and AZ based on node annotations. - Takes the pull secret name and the storage class name from app config. Scenarios tested: - Ran platform inside Kubernetes, curl the API, it returns the expected JSON. - Tested all the failure scenarios like missing config, no permissions to get secret, nodes etc. This PR is backend part for the yugabyte#7394 Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
2f3463c
to
c0fcd57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Sorry for the delay, github PRs don't get put back in the "review" list unless you re-request the review.
@@ -168,7 +168,8 @@ libraryDependencies ++= Seq( | |||
"com.fasterxml.jackson.core" % "jackson-core" % "2.10.5", | |||
"com.jayway.jsonpath" % "json-path" % "2.4.0", | |||
"commons-io" % "commons-io" % "2.8.0", | |||
"commons-codec" % "commons-codec" % "1.15" | |||
"commons-codec" % "commons-codec" % "1.15", | |||
"org.apache.commons" % "commons-collections4" % "4.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already use guava that has all the necessary collection required by this change.
Can you please use guava and not add yet another dependency?
@bhavin192
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I wasn't aware of that. I will work on removing that and using guava. I will create a new PR shortly.
This adds the two new configuration options required by the Kubernetes discovery API which is introduced in yugabyte/yugabyte-db#8371 Test plan: - Upgraded existing Helm release, the API works as expected. Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
This is a follow up change from yugabyte#8371 Scenarios tested: - Ran platform inside Kubernetes, curl the API, it returns the expected JSON. Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
This is a follow up change from #8371 Scenarios tested: - Ran platform inside Kubernetes, curl the API, it returns the expected JSON. Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
c0b9786 changed the behavior of az.setConfig and added az.updateConfig. This change uses the correct method to add `{"KUBECONFIG": ""}` to the existing configuration instead of overwriting the whole AZ configuration. This was missed when I rebased yugabyte#8371 against latest changes. Fixes yugabyte#10860 Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
…10928) c0b9786 changed the behavior of az.setConfig and added az.updateConfig. This change uses the correct method to add `{"KUBECONFIG": ""}` to the existing configuration instead of overwriting the whole AZ configuration. This was missed when I rebased #8371 against latest changes. Fixes #10860 Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
…ugabyte#10928) c0b9786 changed the behavior of az.setConfig and added az.updateConfig. This change uses the correct method to add `{"KUBECONFIG": ""}` to the existing configuration instead of overwriting the whole AZ configuration. This was missed when I rebased yugabyte#8371 against latest changes. Fixes yugabyte#10860 Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
Scenarios tested:
A sample output:
Open Questions
/customers/:cUUID/providers/kubernetes_discovery
?This PR is backend part for the #7394