[WIP] kms: block revisions until encryption config and kms sidecar are in sync #2102
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,119 @@ | ||||||||||||||||
| package kms | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "context" | ||||||||||||||||
| "encoding/json" | ||||||||||||||||
| "fmt" | ||||||||||||||||
| "slices" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/openshift/api/features" | ||||||||||||||||
| "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/operatorclient" | ||||||||||||||||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||||||||||||||||
| "github.com/openshift/library-go/pkg/operator/revisioncontroller" | ||||||||||||||||
| corev1 "k8s.io/api/core/v1" | ||||||||||||||||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||||||||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||
| "k8s.io/apimachinery/pkg/runtime" | ||||||||||||||||
| "k8s.io/apimachinery/pkg/runtime/serializer" | ||||||||||||||||
| utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||||||||||||||||
| apiserverv1 "k8s.io/apiserver/pkg/apis/apiserver/v1" | ||||||||||||||||
| "k8s.io/client-go/kubernetes" | ||||||||||||||||
| "k8s.io/klog/v2" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| var ( | ||||||||||||||||
| apiserverScheme = runtime.NewScheme() | ||||||||||||||||
| apiserverCodecs = serializer.NewCodecFactory(apiserverScheme) | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| func init() { | ||||||||||||||||
| utilruntime.Must(apiserverv1.AddToScheme(apiserverScheme)) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // TODO: this needs to be moved to library-go | ||||||||||||||||
|
|
||||||||||||||||
| func KMSRevisionPostCheck( | ||||||||||||||||
| kubeClient kubernetes.Interface, | ||||||||||||||||
| featureGateAccessor featuregates.FeatureGateAccess, | ||||||||||||||||
| ) revisioncontroller.RevisionPostCheckFunc { | ||||||||||||||||
| return func(ctx context.Context, revision int32) error { | ||||||||||||||||
| return validateKMSRevision(ctx, kubeClient, featureGateAccessor, revision) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func validateKMSRevision(ctx context.Context, kubeClient kubernetes.Interface, featureGateAccessor featuregates.FeatureGateAccess, revision int32) error { | ||||||||||||||||
| if !featureGateAccessor.AreInitialFeatureGatesObserved() { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| featureGates, err := featureGateAccessor.CurrentFeatureGates() | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return nil | ||||||||||||||||
|
Comment on lines
+48
to
+50
There was a problem hiding this comment. Fail closed when feature-gate state can't be read. Line 50 returns Suggested fix featureGates, err := featureGateAccessor.CurrentFeatureGates()
if err != nil {
- return nil
+ return fmt.Errorf("kms revision post-check: failed to read feature gates: %w", err)
}📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (2.11.4)[error] 50-50: error is not nil (line 48) but it returns nil (nilerr) 🤖 Prompt for AI Agents |
||||||||||||||||
| } | ||||||||||||||||
| if !featureGates.Enabled(features.FeatureGateKMSEncryption) { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| revSuffix := fmt.Sprintf("-%d", revision) | ||||||||||||||||
|
|
||||||||||||||||
| encryptionSecret, err := kubeClient.CoreV1().Secrets(operatorclient.TargetNamespace).Get(ctx, "encryption-config"+revSuffix, metav1.GetOptions{}) | ||||||||||||||||
| if apierrors.IsNotFound(err) { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return fmt.Errorf("kms revision post-check: failed to get encryption-config for revision %d: %w", revision, err) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| podCM, err := kubeClient.CoreV1().ConfigMaps(operatorclient.TargetNamespace).Get(ctx, "kube-apiserver-pod"+revSuffix, metav1.GetOptions{}) | ||||||||||||||||
| if apierrors.IsNotFound(err) { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+58
to
+69
There was a problem hiding this comment. Don't treat missing revision resources as a successful check. Because A safer split is: treat a missing 🤖 Prompt for AI Agents |
||||||||||||||||
| if err != nil { | ||||||||||||||||
| return fmt.Errorf("kms revision post-check: failed to get kube-apiserver-pod for revision %d: %w", revision, err) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| kmsConfigured := hasKMSEncryptionProvider(encryptionSecret) | ||||||||||||||||
| // FIXME(bertinatto): plugin name is hardcoded here | ||||||||||||||||
| kmsInPod := podHasKMSSidecar(podCM, "kms-plugin") | ||||||||||||||||
| if kmsConfigured != kmsInPod { | ||||||||||||||||
| return fmt.Errorf("kms revision post-check: revision %d has mismatched KMS state: kmsConfigured=%v kmsInPod=%v", revision, kmsConfigured, kmsInPod) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| klog.V(4).Infof("kms revision post-check passed for revision %d: kmsConfigured=%v kmsInPod=%v", revision, kmsConfigured, kmsInPod) | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func podHasKMSSidecar(podCM *corev1.ConfigMap, sidecarName string) bool { | ||||||||||||||||
| podYAML, ok := podCM.Data["pod.yaml"] | ||||||||||||||||
| if !ok { | ||||||||||||||||
| return false | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| var pod corev1.Pod | ||||||||||||||||
| if err := json.Unmarshal([]byte(podYAML), &pod); err != nil { | ||||||||||||||||
| return false | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+91
to
+94
There was a problem hiding this comment. 🧩 Analysis chain🌐 Web query:
💡 Result: No, Go's encoding/json.Unmarshal accepts only JSON input. It explicitly parses "JSON-encoded data" and returns a SyntaxError for invalid JSON syntax, such as YAML-specific features like colons without quotes or indentation-based structures. Citations:
🏁 Script executed: cat -n pkg/operator/kms/postcheck.go | head -120 | tail -50Repository: openshift/cluster-kube-apiserver-operator Length of output: 2114 🏁 Script executed: cat -n pkg/operator/kms/postcheck.go | head -100Repository: openshift/cluster-kube-apiserver-operator Length of output: 3983 🏁 Script executed: head -30 pkg/operator/kms/postcheck.goRepository: openshift/cluster-kube-apiserver-operator Length of output: 964 Use Line 92 uses Suggested fix- if err := json.Unmarshal([]byte(podYAML), &pod); err != nil {
+ if err := yaml.Unmarshal([]byte(podYAML), &pod); err != nil {
return false
}Also update the imports from 🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| return slices.ContainsFunc(pod.Spec.Containers, func(c corev1.Container) bool { | ||||||||||||||||
| return c.Name == sidecarName | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func hasKMSEncryptionProvider(encryptionConfig *corev1.Secret) bool { | ||||||||||||||||
| encryptionConfigBytes, ok := encryptionConfig.Data["encryption-config"] | ||||||||||||||||
| if !ok { | ||||||||||||||||
| return false | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| gvk := apiserverv1.SchemeGroupVersion.WithKind("EncryptionConfiguration") | ||||||||||||||||
| obj, _, err := apiserverCodecs.UniversalDeserializer().Decode(encryptionConfigBytes, &gvk, nil) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return false | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| config := obj.(*apiserverv1.EncryptionConfiguration) | ||||||||||||||||
| return slices.ContainsFunc(config.Resources, func(resource apiserverv1.ResourceConfiguration) bool { | ||||||||||||||||
| return slices.ContainsFunc(resource.Providers, func(provider apiserverv1.ProviderConfiguration) bool { | ||||||||||||||||
| return provider.KMS != nil | ||||||||||||||||
| }) | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: openshift/cluster-kube-apiserver-operator
Length of output: 498
🏁 Script executed:
Repository: openshift/cluster-kube-apiserver-operator
Length of output: 171
🏁 Script executed:
Repository: openshift/cluster-kube-apiserver-operator
Length of output: 800
Replace directive references an inaccessible or non-existent repository.
The
replacedirective on line 140 redirectsgithub.com/openshift/library-gotogithub.com/bertinatto/library-go, which cannot be reached or does not exist. This will cause build failures. Additionally, redirecting to a personal fork creates supply-chain and reproducibility risk for the operator runtime and test utilities.Switch to an org-owned (or upstream) replacement before merge, and document removal criteria for this
replacedirective.🤖 Prompt for AI Agents