Defining new types for Cloudstack AvailabilityZone development #2412
Conversation
eks-distro-bot
added
the
size/L
Codecov Report
@@ Coverage Diff @@
## main #2412 +/- ##
==========================================
+ Coverage 57.21% 57.24% +0.02%
==========================================
Files 310 310
Lines 25308 25447 +139
==========================================
+ Hits 14481 14567 +86
- Misses 9505 9553 +48
- Partials 1322 1327 +5
Continue to review full report at Codecov.
|
|
/test eks-anywhere-e2e-presubmit |
maxdrib
changed the title
|
/test eks-anywhere-e2e-presubmit |
| v.Spec.AvailabilityZones = make([]CloudStackAvailabilityZone, 0, len(v.Spec.Zones)) | ||
| for index, csZone := range v.Spec.Zones { | ||
| az := CloudStackAvailabilityZone{ | ||
| Name: fmt.Sprintf("availability-zone-%d", index), |
There was a problem hiding this comment.
this is subject to change pending alignment with CAPC for backwards compatibility on existing clusters
| @@ -29,14 +31,15 @@ type CloudStackDatacenterConfigSpec struct { | |||
| // Domain contains a grouping of accounts. Domains usually contain multiple accounts that have some logical relationship to each other and a set of delegated administrators with some authority over the domain and its subdomains | |||
| // This field is considered as a fully qualified domain name which is the same as the domain path without "ROOT/" prefix. For example, if "foo" is specified then a domain with "ROOT/foo" domain path is picked. | |||
| // The value "ROOT" is a special case that points to "the" ROOT domain of the CloudStack. That is, a domain with a path "ROOT/ROOT" is not allowed. | |||
| // | |||
| Domain string `json:"domain"` | |||
| Domain string `json:"domain,omitempty"` | |||
There was a problem hiding this comment.
do we want to add the deprecated note to each field?
There was a problem hiding this comment.
We will add that after we've migrated EKS-A internally to using the new fields. Prior to that, the linter throws a ton of errors for using deprecated fields which will slow down our reviews :(
|
|
||
| To add more detail to the credential ref, we will continue to accept a base64 encoded ini file as input from the customer when running eks-a. However, | ||
| in order to avoid taking a dependency on a meaningless section of a transient file, we will modify EKS-A to also create the secrets to be used by CAPC. | ||
| These secrets will be generated from the ini file provided by customer and applied to the cluster. CAPC will then proceed to take ownership of them as |
There was a problem hiding this comment.
does it mean every time we pass an updated ini and run upgrade cmd, eks-a creates a new secret and update the CredentialRef to point to the new secret? what happens to the old unused secrets?
|
|
||
| In a multi-endpoint Cloudstack cluster, each endpoint may have its own credentials. We propose that Cloudstack credentials will be passed in via environment variable in the same way as they are currently, | ||
| only as a list corresponding to AvailabilityZones. Currently, these credentials are passed in via environment variable, which contains a base64 encoded .ini file that looks like | ||
| only as a list corresponding to some k8s secrets which will be generated. Currently, these credentials are passed in via environment variable, which contains a base64 encoded .ini file that looks like |
| We are also exploring converting the ini file to a yaml input file which contains a list of credentials and their associated endpoints. Either way, this environment variable would | ||
| be passed along to CAPC and used by the CAPC controller just like it is currently. | ||
| The secrets will be managed by EKS-A insofar that they can be created but not updated. If users want to update an existing secret, they will have to | ||
| do so manually both in the k8s secret object, as well as the local ini file prior to running upgrade. We can also add a warning if there is a discrepancy in secret contents between the k8s object and the ini file. |
There was a problem hiding this comment.
I'd prefer error initially and if that's a problem we can relax the constraint.
Co-authored-by: Guillermo Gaston <guillermogastonlorente@gmail.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: g-gaston The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available:
#2406
Description of changes:
This PR introduces the new types as described in the Design PR #2399 for supporting running Cloudstack clusters across availability zones. We will preserve the old types as well and introduce a transformer to convert the old types to the new ones once the internal logic is migrated to use the new fields.
Generated files with
make generate-manifests,make generate, andmake release-manifestsTesting (if applicable):
Unit tests all pass
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.