Skip to content

fix: skip access provider population in cluster profiles if no endpoint/CA data exists #849

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

Merged
Chen Yu (michaelawyu) merged 5 commits into from
Aug 26, 2026
Merged

fix: skip access provider population in cluster profiles if no endpoint/CA data exists #849

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8c3484
Updated the logic
michaelawyu Aug 25, 2026
e829e1d
Minor fixes
michaelawyu Aug 26, 2026
24bb270
Minor fixes
michaelawyu Aug 26, 2026
9b9f585
Minor fixes
michaelawyu Aug 26, 2026
ab71a30
Merge branch 'main' into fix/cluster-profile-access-provider
sjwaight Aug 26, 2026
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
2 changes: 1 addition & 1 deletion apis/cluster/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apis/placement/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apis/placement/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 22 additions & 22 deletions pkg/controllers/clusterinventory/clusterprofile/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
clientcmdv1 "k8s.io/client-go/tools/clientcmd/api/v1"
"k8s.io/client-go/util/workqueue"
"k8s.io/klog/v2"
clusterinventory "sigs.k8s.io/cluster-inventory-api/apis/v1alpha1"
Expand Down Expand Up @@ -180,8 +181,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, nil
}

// fillInClusterStatus fills in the ClusterProfile status fields from the MemberCluster status.
// Currently, it only fills in the Kubernetes version field.
// fillInClusterStatus fills in the ClusterProfile status fields from the MemberCluster status,
// including the Kubernetes version and, when available, the cluster access provider.
func (r *Reconciler) fillInClusterStatus(mc *clusterv1beta1.MemberCluster, cp *clusterinventory.ClusterProfile) {
clusterPropertyCondition := meta.FindStatusCondition(mc.Status.Conditions, string(clusterv1beta1.ConditionTypeClusterPropertyCollectionSucceeded))
if !condition.IsConditionStatusTrue(clusterPropertyCondition, mc.Generation) {
Expand All @@ -196,27 +197,26 @@ func (r *Reconciler) fillInClusterStatus(mc *clusterv1beta1.MemberCluster, cp *c
Kubernetes: k8sversion.Value,
}
}
// Add the class access provider, we only have one so far
cp.Status.AccessProviders = []clusterinventory.AccessProvider{
{
Name: controller.ClusterManagerName,
},
}
// TODO throw and unexpected error if clusterEntryPoint is not found
// We don't have a way to get it yet
clusterEntry, exists := mc.Status.Properties[propertyprovider.ClusterEntryPointProperty]
if exists {
klog.V(3).InfoS("Get Kubernetes cluster entry point from member cluster status", "clusterEntryPoint", clusterEntry.Value, "clusterProfile", klog.KObj(cp))
cp.Status.AccessProviders[0].Cluster.Server = clusterEntry.Value
}
// Get the CA Data
certificateAuthorityData, exists := mc.Status.Properties[propertyprovider.ClusterCertificateAuthorityProperty]
if exists {
klog.V(3).InfoS("Get Kubernetes cluster certificate authority data from member cluster status", "clusterProfile", klog.KObj(cp))
cp.Status.AccessProviders[0].Cluster.CertificateAuthorityData = []byte(certificateAuthorityData.Value)

// 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]

@ytimocin Yetkin Timocin (ytimocin) Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can mc.Status.Properties[propertyprovider.ClusterEntryPointProperty] ever throw nil pointer exception?

@michaelawyu Chen Yu (michaelawyu) Aug 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

caData, caDataExists := mc.Status.Properties[propertyprovider.ClusterCertificateAuthorityProperty]

@ytimocin Yetkin Timocin (ytimocin) Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same question for this one too.

@michaelawyu Chen Yu (michaelawyu) Aug 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

if entryPtExists && caDataExists && len(clusterEntrypoint.Value) > 0 && len(caData.Value) > 0 {
Comment thread
sjwaight marked this conversation as resolved.
cp.Status.AccessProviders = []clusterinventory.AccessProvider{
{
Name: controller.ClusterManagerName,
Cluster: clientcmdv1.Cluster{
Server: clusterEntrypoint.Value,
CertificateAuthorityData: []byte(caData.Value),
},
},
}
} else {
// throw an alert
_ = controller.NewUnexpectedBehaviorError(fmt.Errorf("cluster certificate authority data not found in member cluster %s status", mc.Name))
cp.Status.AccessProviders = nil
klog.V(2).InfoS("Cluster entry point and/or CA data is missing or empty; reset cluster access provider to cluster profile status",
"memberCluster", klog.KObj(mc), "clusterProfile", klog.KObj(cp),
"clusterEntryPointExists", entryPtExists, "caDataExists", caDataExists)
}
Comment thread
michaelawyu marked this conversation as resolved.
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/controllers/clusterinventory/clusterprofile/controller_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,10 @@ var _ = Describe("Test ClusterProfile Controller", func() {
},
}
mc.Status.Properties = map[clusterv1beta1.PropertyName]clusterv1beta1.PropertyValue{
propertyprovider.ClusterEntryPointProperty: {
Value: "https://dummy-cluster-endpoint",
ObservationTime: metav1.Time{Time: time.Now()},
},
propertyprovider.ClusterCertificateAuthorityProperty: {
Value: "dummy-ca-data",
ObservationTime: metav1.Time{Time: time.Now()},
Expand Down
80 changes: 76 additions & 4 deletions pkg/controllers/clusterinventory/clusterprofile/controller_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/google/go-cmp/cmp"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
clientcmdv1 "k8s.io/client-go/tools/clientcmd/api/v1"
clusterinventory "sigs.k8s.io/cluster-inventory-api/apis/v1alpha1"

clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
Expand Down Expand Up @@ -84,7 +85,7 @@ func TestFillInClusterStatus(t *testing.T) {
},
clusterProfile: &clusterinventory.ClusterProfile{},
expectVersion: false,
expectAccessProvider: true,
expectAccessProvider: false,
},
{
name: "Cluster property collection succeeded with k8s version only",
Expand All @@ -111,7 +112,7 @@ func TestFillInClusterStatus(t *testing.T) {
clusterProfile: &clusterinventory.ClusterProfile{},
expectVersion: true,
expectedK8sVersion: "v1.28.0",
expectAccessProvider: true,
expectAccessProvider: false,
},
{
name: "Cluster property collection succeeded with all properties",
Expand Down Expand Up @@ -176,8 +177,79 @@ func TestFillInClusterStatus(t *testing.T) {
clusterProfile: &clusterinventory.ClusterProfile{},
expectVersion: true,
expectedK8sVersion: "v1.27.5",
expectAccessProvider: true,
expectedServer: "https://api.partial-cluster.example.com:6443",
expectAccessProvider: false,
},
{
name: "Cluster property collection succeeded but access provider properties are empty",
memberCluster: &clusterv1beta1.MemberCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Generation: 1,
},
Status: clusterv1beta1.MemberClusterStatus{
Conditions: []metav1.Condition{
{
Type: string(clusterv1beta1.ConditionTypeClusterPropertyCollectionSucceeded),
Status: metav1.ConditionTrue,
ObservedGeneration: 1,
},
},
Properties: map[clusterv1beta1.PropertyName]clusterv1beta1.PropertyValue{
propertyprovider.K8sVersionProperty: {
Value: "v1.30.0",
},
propertyprovider.ClusterEntryPointProperty: {
Value: "",
},
propertyprovider.ClusterCertificateAuthorityProperty: {
Value: "",
},
},
},
},
clusterProfile: &clusterinventory.ClusterProfile{},
expectVersion: true,
expectedK8sVersion: "v1.30.0",
expectAccessProvider: false,
},
{
name: "Access provider properties missing resets a previously populated access provider",
memberCluster: &clusterv1beta1.MemberCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Generation: 1,
},
Status: clusterv1beta1.MemberClusterStatus{
Conditions: []metav1.Condition{
{
Type: string(clusterv1beta1.ConditionTypeClusterPropertyCollectionSucceeded),
Status: metav1.ConditionTrue,
ObservedGeneration: 1,
},
},
Properties: map[clusterv1beta1.PropertyName]clusterv1beta1.PropertyValue{
propertyprovider.K8sVersionProperty: {
Value: "v1.31.0",
},
},
},
},
clusterProfile: &clusterinventory.ClusterProfile{
Status: clusterinventory.ClusterProfileStatus{
AccessProviders: []clusterinventory.AccessProvider{
{
Name: controller.ClusterManagerName,
Cluster: clientcmdv1.Cluster{
Server: "https://api.stale-cluster.example.com:6443",
CertificateAuthorityData: []byte("c3RhbGUtY2EtZGF0YQ=="),
},
},
},
},
},
expectVersion: true,
expectedK8sVersion: "v1.31.0",
expectAccessProvider: false,
},
}

Expand Down
2 changes: 1 addition & 1 deletion test/apis/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 4 additions & 9 deletions test/e2e/join_and_leave_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
"github.com/kubefleet-dev/kubefleet/pkg/utils"
"github.com/kubefleet-dev/kubefleet/pkg/utils/controller"
)

const (
Expand Down Expand Up @@ -462,14 +461,10 @@ var _ = Describe("Test member cluster join and leave with clusterProfile", Label
if cp.Status.Version.Kubernetes == "" {
return fmt.Errorf("cluster profile %s Kubernetes version should not be empty", cp.Name)
}
if len(cp.Status.AccessProviders) != 1 {
return fmt.Errorf("cluster profile %s has no access providers %+v", cp.Name, cp.Status.AccessProviders)
}
if cp.Status.AccessProviders[0].Name != controller.ClusterManagerName {
return fmt.Errorf("cluster profile %s access provider name %s doesn't match expected %s", cp.Name, cp.Status.AccessProviders[0].Name, controller.ClusterManagerName)
}
if len(cp.Status.AccessProviders[0].Cluster.CertificateAuthorityData) == 0 {
return fmt.Errorf("cluster profile %s access provider certificate authority data should not be empty", allMemberClusterNames[idx])
if len(cp.Status.AccessProviders) != 0 {
// Note: at this moment the Azure property provider does not expose cluster FQDNs, and
// as a result no access provider will be populated.
Comment on lines +465 to +466

@ytimocin Yetkin Timocin (ytimocin) Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If case checks if AccessProviders array has items, right? The comment sounds a bit off to me. Am I missing something?

@michaelawyu Chen Yu (michaelawyu) Aug 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

return fmt.Errorf("cluster profile %s has access providers %+v", cp.Name, cp.Status.AccessProviders)
}
}
return nil
Expand Down
Loading