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.

Sign up for GitHub

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

removed optional annotation from non-optional fields #427

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

removed optional annotation from non-optional fields #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ test-e2e: docker-build-e2e cluster-e2e-templates cluster-templates ## Run the en
mkdir -p $(ARTIFACTS)
NUTANIX_LOG_LEVEL=debug ginkgo -v \
--trace \
--progress \
--show-node-events \
--tags=e2e \
--label-filter=$(LABEL_FILTER_ARGS) \
$(_SKIP_ARGS) \
Expand All @@ -345,7 +345,7 @@ test-e2e: docker-build-e2e cluster-e2e-templates cluster-templates ## Run the en
--output-dir="$(ARTIFACTS)" \
--junit-report=${JUNIT_REPORT_FILE} \
--timeout="24h" \
--always-emit-ginkgo-writer \
-v \
$(GINKGO_ARGS) ./test/e2e -- \
-e2e.artifacts-folder="$(ARTIFACTS)" \
-e2e.config="$(E2E_CONF_FILE)" \
Expand All @@ -357,7 +357,7 @@ test-e2e-no-kubeproxy: docker-build-e2e cluster-e2e-templates-no-kubeproxy clust
mkdir -p $(ARTIFACTS)
NUTANIX_LOG_LEVEL=debug ginkgo -v \
--trace \
--progress \
--show-node-events \
--tags=e2e \
--label-filter=$(LABEL_FILTER_ARGS) \
$(_SKIP_ARGS) \
Expand All @@ -366,7 +366,7 @@ test-e2e-no-kubeproxy: docker-build-e2e cluster-e2e-templates-no-kubeproxy clust
--output-dir="$(ARTIFACTS)" \
--junit-report=${JUNIT_REPORT_FILE} \
--timeout="24h" \
--always-emit-ginkgo-writer \
-v \
$(GINKGO_ARGS) ./test/e2e -- \
-e2e.artifacts-folder="$(ARTIFACTS)" \
-e2e.config="$(E2E_CONF_FILE)" \
Expand Down
10 changes: 4 additions & 6 deletions api/v1beta1/nutanixcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,12 @@ type NutanixClusterSpec struct {

// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
// host can be either DNS name or ip address
// +optional
ControlPlaneEndpoint capiv1.APIEndpoint `json:"controlPlaneEndpoint"`

// prismCentral holds the endpoint address and port to access the Nutanix Prism Central.
// When a cluster-wide proxy is installed, by default, this endpoint will be accessed via the proxy.
// Should you wish for communication with this endpoint not to be proxied, please add the endpoint to the
// proxy spec.noProxy list.
// +optional
PrismCentral *credentialTypes.NutanixPrismEndpoint `json:"prismCentral"`

// failureDomains configures failure domains information for the Nutanix platform.
Expand All @@ -64,7 +62,7 @@ type NutanixClusterSpec struct {
// +listType=map
// +listMapKey=name
// +optional
FailureDomains []NutanixFailureDomain `json:"failureDomains"`
FailureDomains []NutanixFailureDomain `json:"failureDomains,omitempty"`
}

// NutanixClusterStatus defines the observed state of NutanixCluster
Expand Down Expand Up @@ -152,13 +150,13 @@ func (ncl *NutanixCluster) SetConditions(conditions capiv1.Conditions) {
func (ncl *NutanixCluster) GetPrismCentralCredentialRef() (*credentialTypes.NutanixCredentialReference, error) {
prismCentralInfo := ncl.Spec.PrismCentral
if prismCentralInfo == nil {
return nil, nil
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
Comment on lines +153 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@thunderboltsid thunderboltsid May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@deepakm-ntnx deepakm-ntnx May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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
  1. One option: Here technically entire prismCentral section can be optional and we use endpoint and secret used by controller
  2. 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.
  3. 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?

Copy link
Contributor

@thunderboltsid thunderboltsid May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

  • cluster-api-provider-nutanix/test/e2e/nutanix_client_test.go

    Lines 75 to 129 in 74c5d33

    // credentialRef is a mandatory parameters for the prismCentral attribute
    It("Create a cluster without credentialRef (should fail)", func() {
    flavor = "no-nutanix-cluster"
    Expect(namespace).NotTo(BeNil())
    By("Creating NutanixCluster resource without credentialRef", func() {
    ntnxCluster := testHelper.createDefaultNutanixCluster(
    clusterName,
    namespace.Name,
    controlplaneEndpointIP,
    controlplaneEndpointPort,
    )
    ntnxCreds, err := getNutanixCredentials(*e2eConfig)
    Expect(err).ToNot(HaveOccurred())
    ntnxPort, err := strconv.Atoi(ntnxCreds.Port)
    Expect(err).ToNot(HaveOccurred())
    ntnxCluster.Spec.PrismCentral = &credentialTypes.NutanixPrismEndpoint{
    Address: ntnxCreds.Endpoint,
    Port: int32(ntnxPort),
    Insecure: ntnxCreds.Insecure,
    }
    testHelper.createCapiObject(ctx, createCapiObjectParams{
    creator: bootstrapClusterProxy.GetClient(),
    capiObject: ntnxCluster,
    })
    })
    By("Creating a workload cluster", func() {
    testHelper.deployCluster(
    deployClusterParams{
    clusterName: clusterName,
    namespace: namespace,
    flavor: flavor,
    clusterctlConfigPath: clusterctlConfigPath,
    artifactFolder: artifactFolder,
    bootstrapClusterProxy: bootstrapClusterProxy,
    }, clusterResources)
    })
    By("Checking CredentialRefSecretOwnerSet condition is false", func() {
    testHelper.verifyConditionOnNutanixCluster(verifyConditionParams{
    clusterName: clusterName,
    namespace: namespace,
    bootstrapClusterProxy: bootstrapClusterProxy,
    expectedCondition: clusterv1.Condition{
    Type: infrav1.CredentialRefSecretOwnerSetCondition,
    Status: corev1.ConditionFalse,
    Reason: infrav1.CredentialRefSecretOwnerSetFailed,
    Severity: clusterv1.ConditionSeverityError,
    },
    })
    })
    ensures 2 is explicitly not supported

  • cluster-api-provider-nutanix/test/e2e/nutanix_client_test.go

    Lines 134 to 176 in 74c5d33

    It("Create a cluster without prismCentral attribute (use default credentials)", func() {
    flavor = "no-nutanix-cluster"
    Expect(namespace).NotTo(BeNil())
    By("Creating NutanixCluster resource without credentialRef", func() {
    ntnxCluster := testHelper.createDefaultNutanixCluster(
    clusterName,
    namespace.Name,
    controlplaneEndpointIP,
    controlplaneEndpointPort,
    )
    testHelper.createCapiObject(ctx, createCapiObjectParams{
    creator: bootstrapClusterProxy.GetClient(),
    capiObject: ntnxCluster,
    })
    })
    By("Creating a workload cluster", func() {
    testHelper.deployCluster(
    deployClusterParams{
    clusterName: clusterName,
    namespace: namespace,
    flavor: flavor,
    clusterctlConfigPath: clusterctlConfigPath,
    artifactFolder: artifactFolder,
    bootstrapClusterProxy: bootstrapClusterProxy,
    }, clusterResources)
    })
    By("Checking cluster prism client init condition is true", func() {
    testHelper.verifyConditionOnNutanixCluster(verifyConditionParams{
    clusterName: clusterName,
    namespace: namespace,
    bootstrapClusterProxy: bootstrapClusterProxy,
    expectedCondition: clusterv1.Condition{
    Type: infrav1.PrismCentralClientCondition,
    Status: corev1.ConditionTrue,
    },
    })
    })
    By("PASSED!")
    })
    ensures 1 is explicitly supported

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 β.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first comment made by @thunderboltsid explains the initial intent of the code. The documentation can be modified to reflect the same.

}
if prismCentralInfo.CredentialRef.Kind != credentialTypes.SecretKind {
return nil, nil
return nil, fmt.Errorf("credentialRef should be of kind Secret")
}

return prismCentralInfo.CredentialRef, nil
Expand Down
22 changes: 18 additions & 4 deletions api/v1beta1/nutanixcluster_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"

"github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
)
Expand All @@ -43,6 +44,10 @@ func TestGetCredentialRefForCluster(t *testing.T) {
Namespace: corev1.NamespaceDefault,
},
Spec: NutanixClusterSpec{
ControlPlaneEndpoint: capiv1.APIEndpoint{
Host: "host",
Port: 6443,
},
PrismCentral: &credentials.NutanixPrismEndpoint{
Address: "address",
Port: 9440,
Expand All @@ -61,45 +66,54 @@ func TestGetCredentialRefForCluster(t *testing.T) {
},
},
{
name: "prismCentralInfo is nil, should not fail",
name: "prismCentralInfo is nil, should fail",
nutanixCluster: &NutanixCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: corev1.NamespaceDefault,
},
Spec: NutanixClusterSpec{},
},
expectedErr: fmt.Errorf("prismCentral info is not provided."),
},
{
name: "CredentialRef kind is not kind Secret, should not fail",
name: "CredentialRef kind is not kind Secret, should fail",
nutanixCluster: &NutanixCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: corev1.NamespaceDefault,
},
Spec: NutanixClusterSpec{
ControlPlaneEndpoint: capiv1.APIEndpoint{
Host: "host",
Port: 6443,
},
PrismCentral: &credentials.NutanixPrismEndpoint{
CredentialRef: &credentials.NutanixCredentialReference{
Kind: "unknown",
},
},
},
},
expectedErr: fmt.Errorf("credentialRef should be of kind Secret"),
},
{
name: "prismCentralInfo is not nil but CredentialRef is nil, should fail",
name: "prismCentralInfo is not nil but CredentialRef is nil, should not fail",
nutanixCluster: &NutanixCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: corev1.NamespaceDefault,
},
Spec: NutanixClusterSpec{
ControlPlaneEndpoint: capiv1.APIEndpoint{
Host: "host",
Port: 6443,
},
PrismCentral: &credentials.NutanixPrismEndpoint{
Address: "address",
},
},
},
expectedErr: fmt.Errorf("credentialRef must be set on prismCentral attribute for cluster test in namespace default"),
},
}
for _, tt := range tests {
Expand Down
2 changes: 2 additions & 0 deletions api/v1beta1/nutanixmachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ type NutanixMachineSpec struct {
Subnets []NutanixResourceIdentifier `json:"subnet"`
// List of categories that need to be added to the machines. Categories must already exist in Prism Central
// +kubebuilder:validation:Optional
// +optional
AdditionalCategories []NutanixCategoryIdentifier `json:"additionalCategories,omitempty"`
// Add the machine resources to a Prism Central project
// +optional
Expand All @@ -104,6 +105,7 @@ type NutanixMachineSpec struct {

// List of GPU devices that need to be added to the machines.
// +kubebuilder:validation:Optional
// +optional
GPUs []NutanixGPU `json:"gpus,omitempty"`
}

Expand Down
3 changes: 3 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_nutanixclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,9 @@ spec:
- address
- port
type: object
required:
- controlPlaneEndpoint
- prismCentral
type: object
status:
description: NutanixClusterStatus defines the observed state of NutanixCluster
Expand Down
3 changes: 3 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_nutanixclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,9 @@ spec:
- address
- port
type: object
required:
- controlPlaneEndpoint
- prismCentral
type: object
required:
- spec
Expand Down
24 changes: 13 additions & 11 deletions controllers/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,15 @@ import (
"testing"

"github.com/golang/mock/gomock"
credentialtypes "github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
credentialTypes "github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
prismclientv3 "github.com/nutanix-cloud-native/prism-go-client/v3"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/cluster-api/util"

infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
Expand Down Expand Up @@ -60,7 +61,8 @@ func TestControllerHelpers(t *testing.T) {
Namespace: "default",
},
Spec: infrav1.NutanixClusterSpec{
PrismCentral: &credentialtypes.NutanixPrismEndpoint{
ControlPlaneEndpoint: capiv1.APIEndpoint{},
PrismCentral: &credentialTypes.NutanixPrismEndpoint{
// Adding port info to override default value (0)
Port: 9440,
},
Expand Down Expand Up @@ -134,11 +136,11 @@ func TestGetPrismCentralClientForCluster(t *testing.T) {
ctx := context.Background()
cluster := &infrav1.NutanixCluster{
Spec: infrav1.NutanixClusterSpec{
PrismCentral: &credentialtypes.NutanixPrismEndpoint{
PrismCentral: &credentialTypes.NutanixPrismEndpoint{
Address: "prismcentral.nutanix.com",
Port: 9440,
CredentialRef: &credentialtypes.NutanixCredentialReference{
Kind: credentialtypes.SecretKind,
CredentialRef: &credentialTypes.NutanixCredentialReference{
Kind: credentialTypes.SecretKind,
Name: "test-credential",
Namespace: "test-ns",
},
Expand All @@ -164,9 +166,9 @@ func TestGetPrismCentralClientForCluster(t *testing.T) {
t.Run("GetOrCreate Fails", func(t *testing.T) {
ctrl := gomock.NewController(t)

creds := []credentialtypes.Credential{
creds := []credentialTypes.Credential{
{
Type: credentialtypes.BasicAuthCredentialType,
Type: credentialTypes.BasicAuthCredentialType,
Data: []byte(`{"prismCentral":{"username":"user","password":"password"}}`),
},
}
Expand All @@ -175,7 +177,7 @@ func TestGetPrismCentralClientForCluster(t *testing.T) {

secret := &corev1.Secret{
Data: map[string][]byte{
credentialtypes.KeyName: credsMarshal,
credentialTypes.KeyName: credsMarshal,
},
}

Expand All @@ -202,9 +204,9 @@ func TestGetPrismCentralClientForCluster(t *testing.T) {
// Create a new client cache with session auth disabled to avoid network calls in tests
nutanixclient.NutanixClientCache = prismclientv3.NewClientCache()

creds := []credentialtypes.Credential{
creds := []credentialTypes.Credential{
{
Type: credentialtypes.BasicAuthCredentialType,
Type: credentialTypes.BasicAuthCredentialType,
Data: []byte(`{"prismCentral":{"username":"user","password":"password"}}`),
},
}
Expand All @@ -213,7 +215,7 @@ func TestGetPrismCentralClientForCluster(t *testing.T) {
require.NoError(t, err)
secret := &corev1.Secret{
Data: map[string][]byte{
credentialtypes.KeyName: credsMarshal,
credentialTypes.KeyName: credsMarshal,
},
}

Expand Down
7 changes: 5 additions & 2 deletions controllers/nutanixcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,12 @@
log := ctrl.LoggerFrom(ctx)
credentialRef, err := getPrismCentralCredentialRefForCluster(nutanixCluster)
if err != nil {
log.Error(err, fmt.Sprintf("error occurred while getting credential ref for cluster %s", nutanixCluster.Name))
// do not use nutanixCluster in error case as it might be nil
log.Error(err, "Failed to get PC Creds")

Check warning on line 340 in controllers/nutanixcluster_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixcluster_controller.go#L340 

Added line #L340 was not covered by tests
return err
}
if credentialRef == nil {
log.V(1).Info(fmt.Sprintf("Credential ref is nil for cluster %s. Ignoring since object must be deleted", nutanixCluster.Name))
log.V(1).Info(fmt.Sprintf("Credential ref is nil for cluster %s. Ignoring since object must be deleted or was not overriden", nutanixCluster.Name))
return nil
}
log.V(1).Info(fmt.Sprintf("Credential ref is kind Secret for cluster %s. Continue with deletion of secret", nutanixCluster.Name))
Expand Down Expand Up @@ -377,6 +378,8 @@
log := ctrl.LoggerFrom(ctx)
credentialRef, err := getPrismCentralCredentialRefForCluster(nutanixCluster)
if err != nil {
// do not use nutanixCluster in error case as it might be nil
log.Error(err, "Failed to get PC Creds")
return err
}

Expand Down
Loading
Loading