fix: skip access provider population in cluster profiles if no endpoint/CA data exists - #849
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #761 by changing the ClusterProfile controller’s status population so it only publishes a status.accessProviders entry when required cluster access properties are available, preventing consumers from receiving an AccessProvider with an empty cluster.server. It also updates unit/integration tests to reflect the new behavior and includes minor generated-file import formatting changes.
Changes:
- Update ClusterProfile status population to conditionally set
Status.AccessProvidersonly when both the cluster entrypoint and CA data properties exist. - Adjust unit/integration tests to stop expecting an AccessProvider when required properties are missing, and to supply the entrypoint in integration tests.
- Normalize import aliases in several controller-gen generated deepcopy files.
Reviewed changes
Copilot reviewed 3 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/controllers/clusterinventory/clusterprofile/controller.go | Changes AccessProvider population logic to be conditional on entrypoint + CA data existence. |
| pkg/controllers/clusterinventory/clusterprofile/controller_test.go | Updates unit tests to align with new conditional AccessProvider behavior. |
| pkg/controllers/clusterinventory/clusterprofile/controller_integration_test.go | Updates integration test fixtures to include the cluster entrypoint property so AccessProvider population occurs. |
| apis/cluster/v1beta1/zz_generated.deepcopy.go | Generated import alias formatting change. |
| apis/placement/v1beta1/zz_generated.deepcopy.go | Generated import alias formatting change. |
| apis/placement/v1alpha1/zz_generated.deepcopy.go | Generated import alias formatting change. |
| apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go | Generated import alias formatting change. |
| test/apis/v1alpha1/zz_generated.deepcopy.go | Generated import alias formatting change. |
Files not reviewed (5)
- apis/cluster/v1beta1/zz_generated.deepcopy.go: Generated file
- apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
- apis/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
- apis/placement/v1beta1/zz_generated.deepcopy.go: Generated file
- test/apis/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 8 changed files in this pull request and generated 2 comments.
Files not reviewed (5)
- apis/cluster/v1beta1/zz_generated.deepcopy.go: Generated file
- apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
- apis/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
- apis/placement/v1beta1/zz_generated.deepcopy.go: Generated file
- test/apis/v1alpha1/zz_generated.deepcopy.go: Generated file
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Simon Waight (sjwaight)
left a comment
Simon Waight (sjwaight)
left a comment
There was a problem hiding this comment.
Assuming E2Es pass, LGTM.
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
|
||
| // Add cluster access provider, if and only if a cluster entry point and the CA data exist as part of the | ||
| // cluster properties. | ||
| clusterEntrypoint, entryPtExists := mc.Status.Properties[propertyprovider.ClusterEntryPointProperty] |
There was a problem hiding this comment.
Can mc.Status.Properties[propertyprovider.ClusterEntryPointProperty] ever throw nil pointer exception?
There was a problem hiding this comment.
Hi Yetkin! mc.Status is a struct (empty if not written yet, but never nil), and nil maps (.status.properties) can be read as empty maps with no panic errors, so we should be good.
| // Add cluster access provider, if and only if a cluster entry point and the CA data exist as part of the | ||
| // cluster properties. | ||
| clusterEntrypoint, entryPtExists := mc.Status.Properties[propertyprovider.ClusterEntryPointProperty] | ||
| caData, caDataExists := mc.Status.Properties[propertyprovider.ClusterCertificateAuthorityProperty] |
There was a problem hiding this comment.
Same question for this one too.
There was a problem hiding this comment.
Hi Yetkin! mc.Status is a struct (empty if not written yet, but never nil), and nil maps (.status.properties) can be read as empty maps with no panic errors, so we should be good.
| // Note: at this moment the Azure property provider does not expose cluster FQDNs, and | ||
| // as a result no access provider will be populated. |
There was a problem hiding this comment.
If case checks if AccessProviders array has items, right? The comment sounds a bit off to me. Am I missing something?
There was a problem hiding this comment.
Hi Yetkin! Yeah, about this one: the access provider, as the issuer contributor mentioned, needs to have a valid cluster entrypoint (API server URL) set. However, at this moment, even though we do have the cluster entrypoint property listed, it is never written/used; as a result, all the cluster profiles we produce have invalid access providers with no entrypoints.
With this PR we have added the check as requested, i.e., if there is no cluster entrypoint, we do not populate access providers; until we add support for the cluster entrypoint property, the expected behavior onwards is that all the cluster profiles hub agent produces will have no access providers, hence the updated check.
|
Hi all! I am merging this to unblock progress; if there is any question/concern, please let me know. Will address them in separate PRs. |
898e0bb
into
kubefleet-dev:main
Description of your changes
This pull request primarily refactors import statements for consistency and improves the logic for populating the
AccessProvidersfield in theClusterProfilecontroller. It ensures that the cluster access provider is only set when both the cluster entry point and CA data are available, and updates related tests accordingly.Fixes #761
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
N/A