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.

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

fix: validates CONTROL_PLANE_ENDPOINT_IP and NUTANIX_ENDPOINT are distinct #994

Closed

Conversation

manoj-nutanix
Copy link
Contributor

@manoj-nutanix manoj-nutanix commented Dec 9, 2024

What problem does this PR solve?:
api-server raises error if CONTROL_PLANE_ENDPOINT_IP and NUTANIX_ENDPOINT IP are same. It only implements dumb check which compares PC IP with control-plane IP.

Which issue(s) this PR fixes:
Fixes #
https://jira.nutanix.com/browse/NCN-102626

How Has This Been Tested?:

Locally tested on kind cluster

Special notes for your reviewer:

  1. Same IP for PC and control-plane -
clusterctl generate cluster ${CLUSTER_NAME} \
  --from ${CLUSTER_FILE} \
  --kubernetes-version ${KUBERNETES_VERSION} \
  --worker-machine-count 1 | \
kubectl apply --server-side -f -
secret/nutanix-cluster-cilium-helm-addonexport-dockerhub-credentials serverside-applied
secret/nutanix-cluster-cilium-helm-addonexport-pc-creds-for-csi serverside-applied
Warning: Detected changes to resource nutanix-cluster-cilium-helm-addonexport-pc-creds which is currently being deleted.
secret/nutanix-cluster-cilium-helm-addonexport-pc-creds serverside-applied
The Cluster "nutanix-cluster-cilium-helm-addonexport" is invalid: spec.topology.variables[clusterConfig].value.nutanix: Invalid value: "{\"addons\":{\"ccm\":{\"credentials\":{\"secretRef\":{\"name\":\"nutanix-cluster-cilium-helm-addonexport-pc-creds\"}},\"strategy\":\"HelmAddon\"},\"clusterAutoscaler\":{\"strategy\":\"HelmAddon\"},\"cni\":{\"provider\":\"Cilium\",\"strategy\":\"HelmAddon\"},\"csi\":{\"defaultStorage\":{\"provider\":\"nutanix\",\"storageClassConfig\":\"volume\"},\"providers\":{\"nutanix\":{\"credentials\":{\"secretRef\":{\"name\":\"nutanix-cluster-cilium-helm-addonexport-pc-creds-for-csi\"}},\"storageClassConfigs\":{\"volume\":{\"allowExpansion\":false,\"parameters\":{\"storageContainer\":\"default-container-32638919133770\"},\"reclaimPolicy\":\"Delete\",\"volumeBindingMode\":\"WaitForFirstConsumer\"}},\"strategy\":\"HelmAddon\"}},\"snapshotController\":{\"strategy\":\"HelmAddon\"}},\"nfd\":{\"strategy\":\"HelmAddon\"},\"serviceLoadBalancer\":{\"configuration\":{\"addressRanges\":[{\"end\":\"198.18.1.10\",\"start\":\"198.18.1.1\"},{\"end\":\"198.18.1.30\",\"start\":\"198.18.1.21\"}]},\"provider\":\"MetalLB\"}},\"controlPlane\":{\"nutanix\":{\"machineDetails\":{\"bootType\":\"uefi\",\"cluster\":{\"name\":\"auto_cluster_prod_manoj_surudwad_1a2aac0a51c7\",\"type\":\"name\"},\"image\":{\"name\":\"nkp-rocky-9.5-release-1.30.5-20241204003513.qcow2\",\"type\":\"name\"},\"memorySize\":\"4Gi\",\"subnets\":[{\"name\":\"vlan.155\",\"type\":\"name\"}],\"systemDiskSize\":\"40Gi\",\"vcpuSockets\":2,\"vcpusPerSocket\":1}}},\"dns\":{\"coreDNS\":{}},\"encryptionAtRest\":{\"providers\":[{\"aescbc\":{}}]},\"imageRegistries\":[{\"credentials\":{\"secretRef\":{\"name\":\"nutanix-cluster-cilium-helm-addonexport-dockerhub-credentials\"}},\"url\":\"https://docker.io\"}],\"nutanix\":{\"controlPlaneEndpoint\":{\"host\":\"10.47.10.4\",\"port\":6443,\"virtualIP\":{\"provider\":\"KubeVIP\"}},\"prismCentralEndpoint\":{\"credentials\":{\"secretRef\":{\"name\":\"nutanix-cluster-cilium-helm-addonexport-pc-creds\"}},\"insecure\":true,\"url\":\"https://10.47.10.4:9440\"}}}": Prism Central URL must not match the control plane endpoint IP
  1. Different IP for PC and control-plane -
clusterctl generate cluster ${CLUSTER_NAME} \         
  --from ${CLUSTER_FILE} \
  --kubernetes-version ${KUBERNETES_VERSION} \
  --worker-machine-count 1 | \
kubectl apply --server-side -f -
secret/nutanix-cluster-cilium-helm-addonexport-dockerhub-credentials serverside-applied
secret/nutanix-cluster-cilium-helm-addonexport-pc-creds-for-csi serverside-applied
Warning: Detected changes to resource nutanix-cluster-cilium-helm-addonexport-pc-creds which is currently being deleted.
secret/nutanix-cluster-cilium-helm-addonexport-pc-creds serverside-applied
cluster.cluster.x-k8s.io/nutanix-cluster-cilium-helm-addonexport serverside-applied

Copy link
Member

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

Can we do this via CEL on the API definitions instead? No need for a custom webhook for this I don't think.

@manoj-nutanix
Copy link
Contributor Author

Can we do this via CEL on the API definitions instead? No need for a custom webhook for this I don't think.

Thanks for the suggestion, will check on CEL usage and update here.

@manoj-nutanix
Copy link
Contributor Author

Can we do this via CEL on the API definitions instead? No need for a custom webhook for this I don't think.

@jimmidyson I realized, caren APIs do not persist in cluster as CRDs, the CEL validation with kubebuilder annotation will work only if CRD is present. Were you suggesting anything different than this? If no then, we have two choice -

  1. Move CEL validation with to CAPX CRDs.
  2. Use current PR's caren webhhok approach.

@jimmidyson
Copy link
Member

@manoj-nutanix I implemented CEL validation for CC variables in CAPI - no need for CRDs :) kubernetes-sigs/cluster-api#9239

@manoj-nutanix
Copy link
Contributor Author

@jimmidyson I've another PR which introduces webhook validations for nutanix, should we revert this PR to earlier webhook logic so changes can managed from single place as we want remove them in future anyways.

@jimmidyson
Copy link
Member

@jimmidyson I've another PR which introduces webhook validations for nutanix, should we revert this PR to earlier webhook logic so changes can managed from single place as we want remove them in future anyways.

Yes that would make sense. Sorry for messing you around! I think this has been useful though to understand the limitations of using CEL and has given me some thoughts on where we should and should not use it. One reason I like it is that it expresses the validation rules as part of the API rather than in opaque webhook logic. My other concern generally about using webhooks for validating resources that CAREN doesn't strictly own (i.e. CAPI Cluster resources) is that if this web is down then it blocks all Cluster mutations, not even just ones that we are trying to validate as part of this work.

Anyway, I agree we should move this back alongside the other webhook logic rather than use CEL so we are consistent.

@manoj-nutanix
Copy link
Contributor Author

Closing this PR in favor of #1002 as forked repos don't have access to github secrets so some e2e tests failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants