removed optional annotation from non-optional fields #427
Conversation
|
/test e2e-capx-controller-upgrade |
|
/retest |
|
CI is failing due to setup related timeout issues. make test-e2e-calico LABEL_FILTERS="quickstart && clusterclass" When following the Cluster API quick-start with ClusterClass Should create a workload cluster [quickstart, clusterclass, capx-feature-test] /Users/deepak.muley/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/e2e/quick_start.go:98 STEP: Creating a namespace for hosting the "quick-start" test spec @ 05/02/24 10:48:12.046 INFO: Creating namespace quick-start-muy2ho INFO: Creating event watcher for namespace "quick-start-muy2ho" STEP: Creating a workload cluster @ 05/02/24 10:48:12.055 INFO: Creating the workload cluster with name "quick-start-4a7dmo" using the "topology" template (Kubernetes v1.29.2, 1 control-plane machines, 1 worker machines) INFO: Getting the cluster template yaml INFO: clusterctl config cluster quick-start-4a7dmo --infrastructure (default) --kubernetes-version v1.29.2 --control-plane-machine-count 1 --worker-machine-count 1 --flavor topology INFO: Creating the workload cluster with name "quick-start-4a7dmo" from the provided yaml INFO: Applying the cluster template yaml of cluster quick-start-muy2ho/quick-start-4a7dmo Running kubectl apply --kubeconfig /var/folders/l3/8dxq0z892n992ffkmb33003r0000gn/T/e2e-kind4035023826 -f - stderr: Warning: Cluster refers to ClusterClass nutanix-quick-start but this object which hasn't yet been reconciled. Cluster topology has not been fully validated. The NutanixClusterTemplate "nutanix-quick-start-nct" is invalid: * spec.template.spec.controlPlaneEndpoint: Required value * spec.template.spec.prismCentral: Required value stdout: kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io/nutanix-quick-start-kcfg-0 created clusterclass.cluster.x-k8s.io/nutanix-quick-start created kubeadmcontrolplanetemplate.controlplane.cluster.x-k8s.io/nutanix-quick-start-kcpt created nutanixmachinetemplate.infrastructure.cluster.x-k8s.io/nutanix-quick-start-cp-nmt created nutanixmachinetemplate.infrastructure.cluster.x-k8s.io/nutanix-quick-start-md-nmt created configmap/quick-start-4a7dmo-pc-trusted-ca-bundle created configmap/cni-quick-start-4a7dmo-crs-cni created configmap/nutanix-ccm created secret/quick-start-4a7dmo-pc-creds created secret/nutanix-ccm-secret created clusterresourceset.addons.cluster.x-k8s.io/quick-start-4a7dmo-crs-cni created clusterresourceset.addons.cluster.x-k8s.io/nutanix-ccm-crs created cluster.cluster.x-k8s.io/quick-start-4a7dmo created Running kubectl apply --kubeconfig /var/folders/l3/8dxq0z892n992ffkmb33003r0000gn/T/e2e-kind4035023826 -f - stderr: Warning: Cluster refers to ClusterClass nutanix-quick-start but this object which hasn't yet been reconciled. Cluster topology has not been fully validated. The NutanixClusterTemplate "nutanix-quick-start-nct" is invalid: * spec.template.spec.controlPlaneEndpoint: Required value * spec.template.spec.prismCentral: Required value stdout: kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io/nutanix-quick-start-kcfg-0 unchanged clusterclass.cluster.x-k8s.io/nutanix-quick-start configured kubeadmcontrolplanetemplate.controlplane.cluster.x-k8s.io/nutanix-quick-start-kcpt unchanged nutanixmachinetemplate.infrastructure.cluster.x-k8s.io/nutanix-quick-start-cp-nmt unchanged nutanixmachinetemplate.infrastructure.cluster.x-k8s.io/nutanix-quick-start-md-nmt unchanged configmap/quick-start-4a7dmo-pc-trusted-ca-bundle unchanged configmap/cni-quick-start-4a7dmo-crs-cni unchanged configmap/nutanix-ccm unchanged secret/quick-start-4a7dmo-pc-creds configured secret/nutanix-ccm-secret configured clusterresourceset.addons.cluster.x-k8s.io/quick-start-4a7dmo-crs-cni unchanged clusterresourceset.addons.cluster.x-k8s.io/nutanix-ccm-crs unchanged cluster.cluster.x-k8s.io/quick-start-4a7dmo configured Running kubectl apply --kubeconfig /var/folders/l3/8dxq0z892n992ffkmb33003r0000gn/T/e2e-kind4035023826 -f - stderr: Warning: Cluster refers to ClusterClass nutanix-quick-start but this object which hasn't yet been reconciled. Cluster topology has not been fully validated. The NutanixClusterTemplate "nutanix-quick-start-nct" is invalid: * spec.template.spec.controlPlaneEndpoint: Required value * spec.template.spec.prismCentral: Required value |
If you read through the build-log.txt, CI fails for the same reason ^ |
deepakm-ntnx
force-pushed
the
jira/krbn-8094
branch
from
f3c7ef6 to
1bc0d50
Compare
|
/retest |
1 similar comment
|
/retest |
|
Did you also run |
will run next |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #427 +/- ##
==========================================
- Coverage 28.37% 28.12% -0.26%
==========================================
Files 14 14
Lines 1304 1305 +1
==========================================
- Hits 370 367 -3
- Misses 934 938 +4 ☔ View full report in Codecov by Sentry. |
deepakm-ntnx
force-pushed
the
jira/krbn-8094
branch
4 times, most recently
from
d2936df to
256be26
Compare
|
/retest |
deepakm-ntnx
force-pushed
the
jira/krbn-8094
branch
2 times, most recently
from
3ab1feb to
5d26af6
Compare
deepakm-ntnx
force-pushed
the
jira/krbn-8094
branch
from
5d26af6 to
85b6a78
Compare
|
/retest |
deepakm-ntnx
changed the title
| return nil, fmt.Errorf("prismCentral info is not provided.") | ||
| } | ||
| if prismCentralInfo.CredentialRef == nil { | ||
| return nil, fmt.Errorf("credentialRef must be set on prismCentral attribute for cluster %s in namespace %s", ncl.Name, ncl.Namespace) | ||
| return nil, nil // returning nil so that we can use default controller's secret |
There was a problem hiding this comment.
The original purpose was that if prismCentralInfo is not present, prismCentralInfo including credentials of the prism central on which management cluster is running will be used. If it is provided, the credentials were mandatory because you can't just expect credentials of one prism central to work on another. The switchover of these two conditions with that context is rather confusing to me and the motivation for doing so unclear.
There was a problem hiding this comment.
ok but per public docs, thats what is expected https://opendocs.nutanix.com/capx/v1.3.x/credential_management/. if you dont provide then use creds of controller and if given then use overridden ones.
also per type def https://github.com/nutanix-cloud-native/prism-go-client/blob/main/environment/credentials/types.go#L130 its specified as optional as well
we can debate separately on the confusing part of this from user perspective but this is the designed behavior at this time in my understanding. cc @yannickstruyf3
There was a problem hiding this comment.
IMO, in that case, it's the documentation that should be fixed; not the code to match unclear documentation. My understanding of what this change was originally supposed to be was to make credentials always mandatory and do away with optional credentials behavior. This PR is no longer doing what the original PR was doing.
There was a problem hiding this comment.
We have following cases:
Controller manager uses PC 1 with creds for user1/pwd1
- Workload Cluster1 need to be created on same pc PC 1 with same user as controller used
- One option: Here technically entire prismCentral section can be optional and we use endpoint and secret used by controller
- 2nd option (current implementation matching docs with mandatory prismCentral section fix): User specifies prismCentral with same endpoint and keeps CredentialRef empty assuming we use same secret as controller.
- 3rd option: User specifies prismCentral with same endpoint and same CredentialRef as controller even if we use same secret as controller.
-
Workload Cluster 2 needs to be created on same pc PC 1 with different user as controller used
Here prismCentral section is required with same pc endpoint but different user creds secret referred thru CredentialRef -
Workload Cluster 3 needs to be created on PC 2 with different user from that PC
Here prismCentral section is required with pc2 endpoint and different user from that PC
Are you proposing that we should not support bullet 1, option 1 and 2 and make option 3 available?
There was a problem hiding this comment.
For the sake of this comment, I'll refer to the three unnamed bullet cases above as a, b, and c (in the order they appear).
a. Current implementation entails either 1 or 3 can be used to satisfy this requirement.
b. Current implementation entails only 3 can be used to satisfy this requirement.
c. Current implementation entails only 3 can be used to satisfy this requirement.
To be more explicit, 2 has never been a supported mechanism. To that extent, we even have explicit E2E test cases to ensure new changes don't accidentally change the behavior to support 2 and remove support for 1.
- ensures 2 is explicitly not supported
cluster-api-provider-nutanix/test/e2e/nutanix_client_test.go
Lines 75 to 129 in 74c5d33
- ensures 1 is explicitly supported
cluster-api-provider-nutanix/test/e2e/nutanix_client_test.go
Lines 134 to 176 in 74c5d33
Now for the proposal of removing support for 1. I was under the impression that this was originally your proposal. This is what the original PR #400 was doing. It entailed removing the optionality of relying on "defaults" altogether. I highlighted this in a comment on the original PR #400 (comment). It was a breaking change but that was something I could get behind as it simplifies the controller by ensuring credentials and prism central are always required for each cluster.
I am advocating for one of these two (using alpha (ɑ) and beta (β) as we're now running out of bullet indexes):
ɑ. leave things as is: i.e. prism central info stays optional; if it's left blank, prism central info and credentials are taken from default CAPX config. If the prism central info is specified, credentials have to be explicitly provided.
β. make both mandatory: no more clusters relying on default prism central and credentials i.e. remove support for 1. This is what #400 was doing.
Between ɑ and β, I personally lean towards β.
There was a problem hiding this comment.
The first comment made by @thunderboltsid explains the initial intent of the code. The documentation can be modified to reflect the same.
What this PR does / why we need it:
CAPX API specifies certain fields as optional but some of them are actually mandatory to create a cluster. this PR resets them appropriately and fixes the tests which assume otherwise
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # https://jira.nutanix.com/browse/KRBN-8094
How Has This Been Tested?:
Also ran e2e tests which passed
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and test output
Per https://opendocs.nutanix.com/capx/v1.3.x/credential_management/ as well as existing api says Credentialref is optional and is only used when we want to override controller default creds to create cluster. but tests were assuming various cases. hence fixed the assumptions with this PR
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: