Skip to content

Commit

Permalink
adds NewUnionRevisionLabelPodDeployer that combines a standard OpenSh…
Browse files Browse the repository at this point in the history
…iftAPIServer deployer and a temporal OpenSiftAuthAPI deployer into one

OpenSiftAuthAPI is on/off based on the annotation of the openshift-oauth-apiserver-encryption-cofngi secret. It will be removed in future releases ( >= 4.7),
when OpenSiftAuthAPI deployer is on encryption controllers for this operator will wait until OpenShiftAuthAPI servers converge onto a single revision.

it also starts OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver in openshift-config-managed namespace as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md
  • Loading branch information
p0lyn0mial committed Jul 27, 2020
1 parent f3b2cad commit f4f436e
Show file tree
Hide file tree
Showing 7 changed files with 433 additions and 36 deletions.
50 changes: 32 additions & 18 deletions pkg/operator/encryptionprovider/encryptionprovider.go
Expand Up @@ -14,13 +14,10 @@ import (
)

type encryptionProvider struct {
oauthAPIServerTargetNamespace string
oauthEncryptionCfgAnnotationKey string

allEncryptedGRs []schema.GroupResource
encryptedGRsManagedByExternalServer sets.String

secretLister corev1listers.SecretNamespaceLister
isOAuthEncryptionConfigManagedByThisOperator func() bool
}

var _ controllers.Provider = &encryptionProvider{}
Expand All @@ -32,11 +29,13 @@ func New(
encryptedGRsManagedByExternalServer sets.String,
kubeInformersForNamespaces operatorv1helpers.KubeInformersForNamespaces) *encryptionProvider {
return &encryptionProvider{
oauthAPIServerTargetNamespace: oauthAPIServerTargetNamespace,
oauthEncryptionCfgAnnotationKey: oauthEncryptionCfgAnnotationKey,
allEncryptedGRs: allEncryptedGRs,
encryptedGRsManagedByExternalServer: encryptedGRsManagedByExternalServer,
secretLister: kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace).Core().V1().Secrets().Lister().Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
isOAuthEncryptionConfigManagedByThisOperator: IsOAuthEncryptionConfigManagedByThisOperator(
kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace).Core().V1().Secrets().Lister().Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
oauthAPIServerTargetNamespace,
oauthEncryptionCfgAnnotationKey,
),
}
}

Expand All @@ -47,18 +46,11 @@ func New(
// case 2 otherwise reduce the authoritative list and let CAO manage its own encryption configuration
//
// TODO:
// - change the code in 4.6 so that it only returns a static list (https://bugzilla.redhat.com/show_bug.cgi?id=1819723)
// - change the code in 4.7 so that it only returns a static list (https://bugzilla.redhat.com/show_bug.cgi?id=1819723)
func (p *encryptionProvider) EncryptedGRs() []schema.GroupResource {
oauthAPIServerEncryptionCfg, err := p.secretLister.Get(fmt.Sprintf("%s-%s", encryptionconfig.EncryptionConfSecretName, p.oauthAPIServerTargetNamespace))
if err != nil {
// note that it's okay to return the authoritative list on an error because:
// - the list is static most of the time it only changes on a downgrade (4.6 -> 4.5)
// - the only type of error we can get here (cache) is NotFound which means that the encryption is off
return p.allEncryptedGRs // case 1 - we are in charge
}

if _, exist := oauthAPIServerEncryptionCfg.Annotations[p.oauthEncryptionCfgAnnotationKey]; exist {
return p.allEncryptedGRs // case 1 - we are in charge
// case 1 - we are in charge
if p.isOAuthEncryptionConfigManagedByThisOperator() {
return p.allEncryptedGRs
}

// case 2 - CAO is in charge, reduce the list
Expand All @@ -76,3 +68,25 @@ func (p *encryptionProvider) EncryptedGRs() []schema.GroupResource {
func (p *encryptionProvider) ShouldRunEncryptionControllers() (bool, error) {
return true, nil // always ready
}

// IsOAuthEncryptionConfigManagedByThisOperator determines whether this operator is in charge of encryption-config-openshift-oauth-apiserver
//
// case 1 encryption off or the secret was annotated - we are in charge
// case 2 otherwise let CAO manage its own encryption configuration
// TODO:
// - change case 1 in in 4.7 so that this operator doesn't manage CAO's encryption config when encryption is off
func IsOAuthEncryptionConfigManagedByThisOperator(secretLister corev1listers.SecretNamespaceLister, oauthAPIServerTargetNamespace string, oauthEncryptionCfgAnnotationKey string) func() bool {
return func() bool {
oauthAPIServerEncryptionCfg, err := secretLister.Get(fmt.Sprintf("%s-%s", encryptionconfig.EncryptionConfSecretName, oauthAPIServerTargetNamespace))
if err != nil {
// note that it's okay to return true on an error because:
// - the only type of error we can get here (cache) is NotFound which means that the encryption is off
return true // case 1 - we are in charge
}

if _, exist := oauthAPIServerEncryptionCfg.Annotations[oauthEncryptionCfgAnnotationKey]; exist {
return true // case 1 - we are in charge
}
return false // case 2 - CAO is in charge
}
}
8 changes: 5 additions & 3 deletions pkg/operator/encryptionprovider/encryptionprovider_test.go
Expand Up @@ -70,11 +70,13 @@ func TestEncryptionProvider(t *testing.T) {

// act
target := encryptionProvider{
oauthAPIServerTargetNamespace: "openshift-apiserver",
oauthEncryptionCfgAnnotationKey: encryptionCfgAnnotationKey,
allEncryptedGRs: scenario.defaultEncryptedGRs,
encryptedGRsManagedByExternalServer: grsManagedByExternalServer,
secretLister: fakeSecretsLister.Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
isOAuthEncryptionConfigManagedByThisOperator: IsOAuthEncryptionConfigManagedByThisOperator(
fakeSecretsLister.Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
"openshift-apiserver",
encryptionCfgAnnotationKey,
),
}

actualEncryptedGRs := target.EncryptedGRs()
Expand Down
@@ -1,4 +1,4 @@
package oauthapiencryptioncontroller
package oauthapiencryption

import (
"context"
Expand Down Expand Up @@ -26,24 +26,24 @@ const (
This annotation indicates that OAS-O manages this secret.`
)

type oauthAPIServerController struct {
type oauthEncryptionConfigSyncController struct {
oauthAPIServerTargetNamespace string

secretLister corev1listers.SecretNamespaceLister
secretClient corev1client.SecretInterface
}

// New creates OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver in openshift-config-managed namespace as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md
// Note that this code will be removed in the future release (4.6)
func New(
// NewEncryptionConfigSyncController creates OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver in openshift-config-managed namespace as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md
// Note that this code will be removed in the future release (4.7)
func NewEncryptionConfigSyncController(
name string,
oauthAPIServerTargetNamespace string,
secretClient corev1client.SecretsGetter,
kubeInformersForNamespaces operatorv1helpers.KubeInformersForNamespaces,
eventRecorder events.Recorder) factory.Controller {

controllerFactory := factory.New()
target := &oauthAPIServerController{
target := &oauthEncryptionConfigSyncController{
oauthAPIServerTargetNamespace: oauthAPIServerTargetNamespace,
secretLister: kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace).Core().V1().Secrets().Lister().Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
secretClient: secretClient.Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
Expand All @@ -62,13 +62,13 @@ func New(
// case 2: if the secret exists and it is annotated
// - it will simply start synchronisation
//
// case 3: no-op: when the secret exits but it doesn't have the annotation - that means it was created by CAO in 4.6 and this is downgrade
// case 3: no-op: when the secret exits but it doesn't have the annotation - that means it was created by CAO in 4.7 and this is downgrade
// case 4: no-op: when the secret doesn't exist and encryption is off
//
// drawbacks:
// - it will not recover when the annotation was manually removed by a user,
// to recover we would have to put a value in the annotation instead and coordinate OAS-A and CAO but then we would have to remember to add it in CAO (4.6) as well
func (c *oauthAPIServerController) sync(ctx context.Context, controllerContext factory.SyncContext) error {
func (c *oauthEncryptionConfigSyncController) sync(ctx context.Context, controllerContext factory.SyncContext) error {
openshiftAPIServerEncryptionCfg, err := c.secretLister.Get(fmt.Sprintf("%s-%s", encryptionconfig.EncryptionConfSecretName, operatorclient.TargetNamespace))
if apierrors.IsNotFound(err) {
return nil // case 4: encryption off
Expand Down
@@ -1,4 +1,4 @@
package oauthapiencryptioncontroller
package oauthapiencryption

import (
"context"
Expand Down Expand Up @@ -137,7 +137,7 @@ func TestOAuthAPIServerController(t *testing.T) {
}
fakeKubeClient := fake.NewSimpleClientset(rawSecrets...)

target := oauthAPIServerController{
target := oauthEncryptionConfigSyncController{
oauthAPIServerTargetNamespace: "oauth-apiserver",
secretLister: fakeSecretsLister.Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
secretClient: fakeKubeClient.CoreV1().Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace),
Expand Down
151 changes: 151 additions & 0 deletions pkg/operator/revisionpoddeployer/deployers.go
@@ -0,0 +1,151 @@
package revisionpoddeployer

import (
"fmt"
"reflect"

"github.com/openshift/library-go/pkg/operator/encryption/encryptionconfig"
"github.com/openshift/library-go/pkg/operator/encryption/statemachine"
corev1 "k8s.io/api/core/v1"
"k8s.io/client-go/tools/cache"
)

type MaybeDisabledDeployer interface {
statemachine.Deployer
Disabled() bool
}

// UnionDeployer provides unified state from multiple distinct deployers.
// It's converged if all delegates are converged. Each deployer can be disabled.
type UnionDeployer struct {
delegates []MaybeDisabledDeployer
hasSynced []cache.InformerSynced
}

var _ statemachine.Deployer = &UnionDeployer{}

// NewUnionDeployer creates a deployer that returns a unified state from multiple distinct deployers.
// That means:
// - none has reported an error
// - all have converged
// - all have observed exactly the same encryption configuration otherwise it returns converged=false
func NewUnionDeployer(delegates ...MaybeDisabledDeployer) (*UnionDeployer, error) {
if len(delegates) == 0 {
return nil, fmt.Errorf("no deployers were configured")
}
return &UnionDeployer{delegates: delegates}, nil
}

// DeployedEncryptionConfigSecret returns the actual encryption configuration across multiple deployers if they all agree.
func (d *UnionDeployer) DeployedEncryptionConfigSecret() (secret *corev1.Secret, converged bool, err error) {
seenSecrets := []*corev1.Secret{}

for _, delegate := range d.delegates {
if delegate.Disabled() {
continue
}
secret, converged, err := delegate.DeployedEncryptionConfigSecret()
if !converged || err != nil {
return nil, converged, err
}

seenSecrets = append(seenSecrets, secret)
}

// at this point an empty secret (nil) can actually mean two things
// 1. encryption is off
// 2. a replica hasn't converged, it's been stuck, it's been slow etc
//
// in either of this case we should report !converged and let the encryption state machine handle it
potentiallyConvergedSecrets := []*corev1.Secret{}
for _, secret := range seenSecrets {
if secret != nil {
potentiallyConvergedSecrets = append(potentiallyConvergedSecrets, secret)
}
}

if len(potentiallyConvergedSecrets) == 0 {
return nil, true, nil // encryption is off
}
if len(potentiallyConvergedSecrets) != len(seenSecrets) {
return nil, false, nil // not all replicas have converged
}

// we need to check that the encryption configuration is exactly the same among deployers
// so we promote the fist secret and compare it with the rest
goldenSecret := potentiallyConvergedSecrets[0]
potentiallyConvergedSecrets = potentiallyConvergedSecrets[1:]

goldenEncryptionCfg, err := encryptionconfig.FromSecret(goldenSecret)
if err != nil {
return nil, false, err
}

for _, secret := range potentiallyConvergedSecrets {
currentEncryptionCfg, err := encryptionconfig.FromSecret(secret)
if err != nil {
return nil, false, err
}

if !reflect.DeepEqual(goldenEncryptionCfg.Resources, currentEncryptionCfg.Resources) {
return nil, false, nil
}
}

return goldenSecret, true, nil
}

func (d *UnionDeployer) HasSynced() bool {
for _, hasSynced := range d.hasSynced {
if !hasSynced() {
return false
}
}
return true
}

// AddEventHandler registers a event handler that might influence the result of DeployedEncryptionConfigSecret for all configured deployers.
func (d *UnionDeployer) AddEventHandler(handler cache.ResourceEventHandler) {
d.hasSynced = []cache.InformerSynced{}
for _, delegate := range d.delegates {
delegate.AddEventHandler(handler)
d.hasSynced = append(d.hasSynced, delegate.HasSynced)
}
}

type disabledByPredicateDeployer struct {
statemachine.Deployer
enabled func() bool
}

var _ MaybeDisabledDeployer = &disabledByPredicateDeployer{}

// NewDisabledByPredicateDeployer returns a deployer used by the encryption controllers.
// Whether this deployer is on/off is determined by enabled
// TODO: remove this deployer in 4.7
func NewDisabledByPredicateDeployer(
enabled func() bool,
delegate statemachine.Deployer) *disabledByPredicateDeployer {
return &disabledByPredicateDeployer{
Deployer: delegate,
enabled: enabled,
}
}

// Disabled indicates whether this deployer is disabled
// Note: OAuthAPIServer deployer changes its availability - see enabled implementation
func (d *disabledByPredicateDeployer) Disabled() bool {
return !d.enabled()
}

type AlwaysEnabledDeployer struct {
statemachine.Deployer
}

var _ MaybeDisabledDeployer = &AlwaysEnabledDeployer{}

// Disabled indicates whether this deployer is disabled
// Note: OpenShift deployer is always enabled
func (d *AlwaysEnabledDeployer) Disabled() bool {
return false
}

0 comments on commit f4f436e

Please sign in to comment.