Skip to content

WIP: use server-side-apply to update client-ca bundle #1918

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

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

WIP: use server-side-apply to update client-ca bundle #1918

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
40 changes: 28 additions & 12 deletions pkg/operator/targetconfigcontroller/targetconfigcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/sets"
applycorev1 "k8s.io/client-go/applyconfigurations/core/v1"
"k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
coreclientv1 "k8s.io/client-go/kubernetes/typed/core/v1"
Expand Down Expand Up @@ -402,22 +403,37 @@ func ManageClientCABundle(ctx context.Context, lister corev1listers.ConfigMapLis
return nil, false, err
}

if creationRequired {
caBundleConfigMap, err = client.ConfigMaps(operatorclient.TargetNamespace).Create(ctx, requiredConfigMap, metav1.CreateOptions{})
resourcehelper.ReportCreateEvent(recorder, caBundleConfigMap, err)
if err != nil {
return nil, false, err
patch := applycorev1.ConfigMap(caBundleConfigMapName, operatorclient.TargetNamespace)
patch.WithData(requiredConfigMap.Data)
tmp := &metav1.ObjectMeta{}
additionalAnnotations.EnsureTLSMetadataUpdate(tmp)
patch.WithAnnotations(tmp.Annotations)
Comment on lines +408 to +410
Copy link
Contributor

Choose a reason for hiding this comment

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

We're likely to make a mistake eventually if we need to keep this in sync with CombineCABundleConfigMapsOptimistically. Can we build the apply configuration purely from requiredConfigMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I would image we would refactor the helpers to be a more appropriate, but for this POC just 'hacked' it in.


if creationRequired || updateRequired {

result, err := client.ConfigMaps(operatorclient.TargetNamespace).Apply(ctx, patch, metav1.ApplyOptions{Force: true, FieldManager: "cluster-kube-apiserver-operator"})

// emit events
if (result != nil && result.ResourceVersion == "1") || creationRequired {
// either the configmap was created or we expected it to be created and it failed
resourcehelper.ReportCreateEvent(recorder, caBundleConfigMap, err)
} else {
// an existing configmap was updated, even if we expected it to be created
resourcehelper.ReportUpdateEvent(recorder, caBundleConfigMap, err)
}
Comment on lines +417 to 423
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC apply responses will have HTTP status 201 if it caused the resource to be created and 200 otherwise. That would be 100% correct for cases where you get a response vs. a prediction based on the cached state.

klog.V(2).Infof("Created client CA bundle configmap %s/%s", caBundleConfigMap.Namespace, caBundleConfigMap.Name)
return caBundleConfigMap, true, nil
} else if updateRequired {
caBundleConfigMap, err = client.ConfigMaps(operatorclient.TargetNamespace).Update(ctx, requiredConfigMap, metav1.UpdateOptions{})
resourcehelper.ReportUpdateEvent(recorder, caBundleConfigMap, err)

// handle the err from the Apply call
if err != nil {
return nil, false, err
}
klog.V(2).Infof("Updated client CA bundle configmap %s/%s", caBundleConfigMap.Namespace, caBundleConfigMap.Name)
return caBundleConfigMap, true, nil

if result.ResourceVersion == "1" {
klog.V(2).Infof("Created client CA bundle configmap %s/%s", result.Namespace, result.Name)
} else {
klog.V(2).Infof("Updated client CA bundle configmap %s/%s", result.Namespace, result.Name)
}

return result, true, nil
}

return caBundleConfigMap, false, nil
Expand Down
15 changes: 12 additions & 3 deletions pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,7 @@ func TestManageClientCABundle(t *testing.T) {
name: "create new client-ca configmap when none exists",
existingConfigMaps: []*corev1.ConfigMap{},
expectedConfigMap: &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand Down Expand Up @@ -661,6 +662,7 @@ func TestManageClientCABundle(t *testing.T) {
},
},
expectedConfigMap: &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand Down Expand Up @@ -698,6 +700,7 @@ func TestManageClientCABundle(t *testing.T) {
},
},
expectedConfigMap: &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand All @@ -715,6 +718,7 @@ func TestManageClientCABundle(t *testing.T) {
name: "annotations update",
existingConfigMaps: []*corev1.ConfigMap{
{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand All @@ -737,6 +741,7 @@ func TestManageClientCABundle(t *testing.T) {
},
},
expectedConfigMap: &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand Down Expand Up @@ -787,6 +792,7 @@ func TestManageClientCABundle(t *testing.T) {
},
},
expectedConfigMap: &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand Down Expand Up @@ -826,6 +832,7 @@ func TestManageClientCABundle(t *testing.T) {
},
},
expectedConfigMap: &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"},
ObjectMeta: metav1.ObjectMeta{
Name: "client-ca",
Namespace: operatorclient.TargetNamespace,
Expand All @@ -843,7 +850,7 @@ func TestManageClientCABundle(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
client := fake.NewSimpleClientset()
client := fake.NewClientset()

// Create existing configmaps
for _, cm := range test.existingConfigMaps {
Expand All @@ -867,15 +874,17 @@ func TestManageClientCABundle(t *testing.T) {
// Assert change expectations
require.Equal(t, test.expectedChanged, changed, "Expected changed=%v, got changed=%v", test.expectedChanged, changed)

// Compare with expected configmap
// Compare with expected configmap, ignoring managed fields
resultConfigMap.ManagedFields = nil
require.Equal(t, test.expectedConfigMap, resultConfigMap)

// Verify the configmap exists in the cluster
storedConfigMap, err := client.CoreV1().ConfigMaps(operatorclient.TargetNamespace).Get(context.Background(), "client-ca", metav1.GetOptions{})
require.NoError(t, err)
require.NotNil(t, storedConfigMap)

// Ensure the returned configmap matches what's stored in the cluster
// Ensure the returned configmap matches what's stored in the cluster, ignoring managed fields
storedConfigMap.ManagedFields = nil
require.Equal(t, storedConfigMap, resultConfigMap, "returned configmap should match stored configmap")

// Verify events were recorded if changes were made
Expand Down