From 17bd3eecfa23790980aeddbc5a808ff295106d22 Mon Sep 17 00:00:00 2001 From: Joel Diaz Date: Fri, 17 Jul 2020 13:16:52 -0400 Subject: [PATCH] handle bootstrap pod user-defined mode - [x] update Makefile to copy operator config from vendored openshift/api - [x] copy config CRD into bindata so that render subcommand has access to the file - [x] update render subcommand to check the operator config CR to determine static Pod rendering - [x] render will now also create the operator config CRD and a operator config CR (reflecting legacy configmap and/or operator config) - [x] update disabled detection util to error if there is no operator config CR present - [x] remove legacy default operator configmap (set to disabled: "false") as it is now deprecated - [x] add default operator config CR with default behavior (where credentialsMode is "") - [x] update test cases so that an operator config resource does exist - [x] add test cases to cover the various render subcommand behaviors - [x] add test cases for passthrough mode where the permissions simulation should not happen New operator behavior will now refuse to take actions until the operator config CR (named "cluster") is present. --- Makefile | 8 +- ...credential_v1_credentialsrequest_crd.yaml} | 0 ...loudcredential_v1_operator_config_crd.yaml | 155 ++++++++++++++ manifests/01-operator-config.yaml | 6 + manifests/01-operator_configmap.yaml | 9 - pkg/assets/bootstrap/bindata.go | 195 +++++++++++++++++- pkg/aws/actuator/actuator.go | 55 +++-- pkg/cmd/render/render.go | 195 +++++++++++++++--- pkg/cmd/render/render_test.go | 184 +++++++++++++++++ .../credentialsrequest_controller_gcp_test.go | 12 ++ .../credentialsrequest_controller_test.go | 135 +++++++++++- ...dentialsrequest_controller_vsphere_test.go | 5 + .../credentialsrequest/status_test.go | 3 +- .../secretannotator/aws/reconciler_test.go | 76 +++++-- .../secretannotator/azure/reconciler_test.go | 17 +- .../secretannotator/gcp/reconciler_test.go | 61 ++++-- .../vsphere/reconciler_test.go | 35 +++- pkg/operator/utils/utils.go | 55 ++--- 18 files changed, 1052 insertions(+), 154 deletions(-) rename bindata/bootstrap/{cloudcredential_v1_credentialsrequest.yaml => cloudcredential_v1_credentialsrequest_crd.yaml} (100%) create mode 100644 bindata/bootstrap/cloudcredential_v1_operator_config_crd.yaml create mode 100644 manifests/01-operator-config.yaml delete mode 100644 manifests/01-operator_configmap.yaml create mode 100644 pkg/cmd/render/render_test.go diff --git a/Makefile b/Makefile index 7d24eedae..0c1ca008d 100644 --- a/Makefile +++ b/Makefile @@ -76,7 +76,13 @@ $(call build-image,ocp-cloud-credential-operator,$(IMAGE_REGISTRY)/ocp/4.5:cloud $(call add-crd-gen,cloudcredential-manifests,./pkg/apis/cloudcredential/v1,./manifests,./manifests) $(call add-crd-gen,cloudcredential-bindata,./pkg/apis/cloudcredential/v1,./bindata/bootstrap,./bindata/bootstrap) -update: update-codegen +update: update-vendored-crds update-codegen update-bindata + +update-vendored-crds: + # copy config CRD from openshift/api + cp vendor/github.com/openshift/api/operator/v1/0000_40_cloud-credential-operator_00_config.crd.yaml ./manifests/00-config-crd.yaml + # ...and into where we generate bindata from + cp vendor/github.com/openshift/api/operator/v1/0000_40_cloud-credential-operator_00_config.crd.yaml ./bindata/bootstrap/cloudcredential_v1_operator_config_crd.yaml update-codegen: update-codegen-crds ./hack/update-codegen.sh diff --git a/bindata/bootstrap/cloudcredential_v1_credentialsrequest.yaml b/bindata/bootstrap/cloudcredential_v1_credentialsrequest_crd.yaml similarity index 100% rename from bindata/bootstrap/cloudcredential_v1_credentialsrequest.yaml rename to bindata/bootstrap/cloudcredential_v1_credentialsrequest_crd.yaml diff --git a/bindata/bootstrap/cloudcredential_v1_operator_config_crd.yaml b/bindata/bootstrap/cloudcredential_v1_operator_config_crd.yaml new file mode 100644 index 000000000..1723c2337 --- /dev/null +++ b/bindata/bootstrap/cloudcredential_v1_operator_config_crd.yaml @@ -0,0 +1,155 @@ +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: cloudcredentials.operator.openshift.io +spec: + scope: Cluster + preserveUnknownFields: false + group: operator.openshift.io + names: + kind: CloudCredential + listKind: CloudCredentialList + plural: cloudcredentials + singular: cloudcredential + subresources: + status: {} + versions: + - name: v1 + served: true + storage: true + validation: + openAPIV3Schema: + description: CloudCredential provides a means to configure an operator to manage + CredentialsRequests. + type: object + required: + - spec + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: CloudCredentialSpec is the specification of the desired behavior + of the cloud-credential-operator. + type: object + properties: + credentialsMode: + description: CredentialsMode allows informing CCO that it should not + attempt to dynamically determine the root cloud credentials capabilities, + and it should just run in the specified mode. It also allows putting + the operator into "manual" mode if desired. Leaving the field in default + mode runs CCO so that the cluster's cloud credentials will be dynamically + probed for capabilities (on supported clouds/platforms). + type: string + enum: + - "" + - Manual + - Mint + - Passthrough + logLevel: + description: logLevel is an intent based logging for an overall component. It + does not give fine grained control, but it is a simple way to manage + coarse grained logging choices that operators have to interpret for + their operands. + type: string + managementState: + description: managementState indicates whether and how the operator + should manage the component + type: string + pattern: ^(Managed|Unmanaged|Force|Removed)$ + observedConfig: + description: observedConfig holds a sparse config that controller has + observed from the cluster state. It exists in spec because it is + an input to the level for the operator + type: object + nullable: true + x-kubernetes-preserve-unknown-fields: true + operatorLogLevel: + description: operatorLogLevel is an intent based logging for the operator + itself. It does not give fine grained control, but it is a simple + way to manage coarse grained logging choices that operators have to + interpret for themselves. + type: string + unsupportedConfigOverrides: + description: 'unsupportedConfigOverrides holds a sparse config that + will override any previously set options. It only needs to be the + fields to override it will end up overlaying in the following order: + 1. hardcoded defaults 2. observedConfig 3. unsupportedConfigOverrides' + type: object + nullable: true + x-kubernetes-preserve-unknown-fields: true + status: + description: CloudCredentialStatus defines the observed status of the cloud-credential-operator. + type: object + properties: + conditions: + description: conditions is a list of conditions and their status + type: array + items: + description: OperatorCondition is just the standard condition fields. + type: object + properties: + lastTransitionTime: + type: string + format: date-time + message: + type: string + reason: + type: string + status: + type: string + type: + type: string + generations: + description: generations are used to determine when an item needs to + be reconciled or has changed in a way that needs a reaction. + type: array + items: + description: GenerationStatus keeps track of the generation for a + given resource so that decisions about forced updates can be made. + type: object + properties: + group: + description: group is the group of the thing you're tracking + type: string + hash: + description: hash is an optional field set for resources without + generation that are content sensitive like secrets and configmaps + type: string + lastGeneration: + description: lastGeneration is the last generation of the workload + controller involved + type: integer + format: int64 + name: + description: name is the name of the thing you're tracking + type: string + namespace: + description: namespace is where the thing you're tracking is + type: string + resource: + description: resource is the resource type of the thing you're + tracking + type: string + observedGeneration: + description: observedGeneration is the last generation change you've + dealt with + type: integer + format: int64 + readyReplicas: + description: readyReplicas indicates how many replicas are ready and + at the desired state + type: integer + format: int32 + version: + description: version is the level this availability applies to + type: string diff --git a/manifests/01-operator-config.yaml b/manifests/01-operator-config.yaml new file mode 100644 index 000000000..80644ebd8 --- /dev/null +++ b/manifests/01-operator-config.yaml @@ -0,0 +1,6 @@ +apiVersion: operator.openshift.io/v1 +kind: CloudCredential +metadata: + name: cluster + annotations: + release.openshift.io/create-only: "true" diff --git a/manifests/01-operator_configmap.yaml b/manifests/01-operator_configmap.yaml deleted file mode 100644 index 92d68f041..000000000 --- a/manifests/01-operator_configmap.yaml +++ /dev/null @@ -1,9 +0,0 @@ -apiVersion: v1 -kind: ConfigMap -metadata: - name: cloud-credential-operator-config - namespace: openshift-cloud-credential-operator - annotations: - release.openshift.io/create-only: "true" -data: - disabled: "false" diff --git a/pkg/assets/bootstrap/bindata.go b/pkg/assets/bootstrap/bindata.go index 21df9df9b..a8caab0f7 100644 --- a/pkg/assets/bootstrap/bindata.go +++ b/pkg/assets/bootstrap/bindata.go @@ -1,6 +1,7 @@ // Code generated for package bootstrap by go-bindata DO NOT EDIT. (@generated) // sources: -// bindata/bootstrap/cloudcredential_v1_credentialsrequest.yaml +// bindata/bootstrap/cloudcredential_v1_credentialsrequest_crd.yaml +// bindata/bootstrap/cloudcredential_v1_operator_config_crd.yaml // bindata/bootstrap/namespace.yaml package bootstrap @@ -55,7 +56,7 @@ func (fi bindataFileInfo) Sys() interface{} { return nil } -var _bootstrapCloudcredential_v1_credentialsrequestYaml = []byte(`apiVersion: apiextensions.k8s.io/v1beta1 +var _bootstrapCloudcredential_v1_credentialsrequest_crdYaml = []byte(`apiVersion: apiextensions.k8s.io/v1beta1 kind: CustomResourceDefinition metadata: name: credentialsrequests.cloudcredential.openshift.io @@ -203,17 +204,189 @@ status: storedVersions: [] `) -func bootstrapCloudcredential_v1_credentialsrequestYamlBytes() ([]byte, error) { - return _bootstrapCloudcredential_v1_credentialsrequestYaml, nil +func bootstrapCloudcredential_v1_credentialsrequest_crdYamlBytes() ([]byte, error) { + return _bootstrapCloudcredential_v1_credentialsrequest_crdYaml, nil } -func bootstrapCloudcredential_v1_credentialsrequestYaml() (*asset, error) { - bytes, err := bootstrapCloudcredential_v1_credentialsrequestYamlBytes() +func bootstrapCloudcredential_v1_credentialsrequest_crdYaml() (*asset, error) { + bytes, err := bootstrapCloudcredential_v1_credentialsrequest_crdYamlBytes() if err != nil { return nil, err } - info := bindataFileInfo{name: "bootstrap/cloudcredential_v1_credentialsrequest.yaml", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} + info := bindataFileInfo{name: "bootstrap/cloudcredential_v1_credentialsrequest_crd.yaml", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} + a := &asset{bytes: bytes, info: info} + return a, nil +} + +var _bootstrapCloudcredential_v1_operator_config_crdYaml = []byte(`apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: cloudcredentials.operator.openshift.io +spec: + scope: Cluster + preserveUnknownFields: false + group: operator.openshift.io + names: + kind: CloudCredential + listKind: CloudCredentialList + plural: cloudcredentials + singular: cloudcredential + subresources: + status: {} + versions: + - name: v1 + served: true + storage: true + validation: + openAPIV3Schema: + description: CloudCredential provides a means to configure an operator to manage + CredentialsRequests. + type: object + required: + - spec + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: CloudCredentialSpec is the specification of the desired behavior + of the cloud-credential-operator. + type: object + properties: + credentialsMode: + description: CredentialsMode allows informing CCO that it should not + attempt to dynamically determine the root cloud credentials capabilities, + and it should just run in the specified mode. It also allows putting + the operator into "manual" mode if desired. Leaving the field in default + mode runs CCO so that the cluster's cloud credentials will be dynamically + probed for capabilities (on supported clouds/platforms). + type: string + enum: + - "" + - Manual + - Mint + - Passthrough + logLevel: + description: logLevel is an intent based logging for an overall component. It + does not give fine grained control, but it is a simple way to manage + coarse grained logging choices that operators have to interpret for + their operands. + type: string + managementState: + description: managementState indicates whether and how the operator + should manage the component + type: string + pattern: ^(Managed|Unmanaged|Force|Removed)$ + observedConfig: + description: observedConfig holds a sparse config that controller has + observed from the cluster state. It exists in spec because it is + an input to the level for the operator + type: object + nullable: true + x-kubernetes-preserve-unknown-fields: true + operatorLogLevel: + description: operatorLogLevel is an intent based logging for the operator + itself. It does not give fine grained control, but it is a simple + way to manage coarse grained logging choices that operators have to + interpret for themselves. + type: string + unsupportedConfigOverrides: + description: 'unsupportedConfigOverrides holds a sparse config that + will override any previously set options. It only needs to be the + fields to override it will end up overlaying in the following order: + 1. hardcoded defaults 2. observedConfig 3. unsupportedConfigOverrides' + type: object + nullable: true + x-kubernetes-preserve-unknown-fields: true + status: + description: CloudCredentialStatus defines the observed status of the cloud-credential-operator. + type: object + properties: + conditions: + description: conditions is a list of conditions and their status + type: array + items: + description: OperatorCondition is just the standard condition fields. + type: object + properties: + lastTransitionTime: + type: string + format: date-time + message: + type: string + reason: + type: string + status: + type: string + type: + type: string + generations: + description: generations are used to determine when an item needs to + be reconciled or has changed in a way that needs a reaction. + type: array + items: + description: GenerationStatus keeps track of the generation for a + given resource so that decisions about forced updates can be made. + type: object + properties: + group: + description: group is the group of the thing you're tracking + type: string + hash: + description: hash is an optional field set for resources without + generation that are content sensitive like secrets and configmaps + type: string + lastGeneration: + description: lastGeneration is the last generation of the workload + controller involved + type: integer + format: int64 + name: + description: name is the name of the thing you're tracking + type: string + namespace: + description: namespace is where the thing you're tracking is + type: string + resource: + description: resource is the resource type of the thing you're + tracking + type: string + observedGeneration: + description: observedGeneration is the last generation change you've + dealt with + type: integer + format: int64 + readyReplicas: + description: readyReplicas indicates how many replicas are ready and + at the desired state + type: integer + format: int32 + version: + description: version is the level this availability applies to + type: string +`) + +func bootstrapCloudcredential_v1_operator_config_crdYamlBytes() ([]byte, error) { + return _bootstrapCloudcredential_v1_operator_config_crdYaml, nil +} + +func bootstrapCloudcredential_v1_operator_config_crdYaml() (*asset, error) { + bytes, err := bootstrapCloudcredential_v1_operator_config_crdYamlBytes() + if err != nil { + return nil, err + } + + info := bindataFileInfo{name: "bootstrap/cloudcredential_v1_operator_config_crd.yaml", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} a := &asset{bytes: bytes, info: info} return a, nil } @@ -296,8 +469,9 @@ func AssetNames() []string { // _bindata is a table, holding each asset generator, mapped to its name. var _bindata = map[string]func() (*asset, error){ - "bootstrap/cloudcredential_v1_credentialsrequest.yaml": bootstrapCloudcredential_v1_credentialsrequestYaml, - "bootstrap/namespace.yaml": bootstrapNamespaceYaml, + "bootstrap/cloudcredential_v1_credentialsrequest_crd.yaml": bootstrapCloudcredential_v1_credentialsrequest_crdYaml, + "bootstrap/cloudcredential_v1_operator_config_crd.yaml": bootstrapCloudcredential_v1_operator_config_crdYaml, + "bootstrap/namespace.yaml": bootstrapNamespaceYaml, } // AssetDir returns the file names below a certain @@ -342,7 +516,8 @@ type bintree struct { var _bintree = &bintree{nil, map[string]*bintree{ "bootstrap": {nil, map[string]*bintree{ - "cloudcredential_v1_credentialsrequest.yaml": {bootstrapCloudcredential_v1_credentialsrequestYaml, map[string]*bintree{}}, + "cloudcredential_v1_credentialsrequest_crd.yaml": {bootstrapCloudcredential_v1_credentialsrequest_crdYaml, map[string]*bintree{}}, + "cloudcredential_v1_operator_config_crd.yaml": {bootstrapCloudcredential_v1_operator_config_crdYaml, map[string]*bintree{}}, "namespace.yaml": {bootstrapNamespaceYaml, map[string]*bintree{}}, }}, }} diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index 200765a3f..2cd2736ff 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -24,21 +24,13 @@ import ( log "github.com/sirupsen/logrus" - minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" - ccaws "github.com/openshift/cloud-credential-operator/pkg/aws" - minteraws "github.com/openshift/cloud-credential-operator/pkg/aws" - "github.com/openshift/cloud-credential-operator/pkg/operator/constants" - actuatoriface "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator" - awsannotator "github.com/openshift/cloud-credential-operator/pkg/operator/secretannotator/aws" - "github.com/openshift/cloud-credential-operator/pkg/operator/utils" - awsutils "github.com/openshift/cloud-credential-operator/pkg/operator/utils/aws" - - configv1 "github.com/openshift/api/config/v1" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/iam" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -46,6 +38,15 @@ import ( "k8s.io/apimachinery/pkg/types" utilrand "k8s.io/apimachinery/pkg/util/rand" "sigs.k8s.io/controller-runtime/pkg/client" + + minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" + ccaws "github.com/openshift/cloud-credential-operator/pkg/aws" + minteraws "github.com/openshift/cloud-credential-operator/pkg/aws" + "github.com/openshift/cloud-credential-operator/pkg/operator/constants" + actuatoriface "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator" + awsannotator "github.com/openshift/cloud-credential-operator/pkg/operator/secretannotator/aws" + "github.com/openshift/cloud-credential-operator/pkg/operator/utils" + awsutils "github.com/openshift/cloud-credential-operator/pkg/operator/utils/aws" ) const ( @@ -225,19 +226,31 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR } else { // for passthrough creds, just see if we have the permissions requested in the credentialsrequest - region, err := awsutils.LoadInfrastructureRegion(a.Client, logger) + + // but for the case where the operator mode is non-default, then we will avoid performing any + // policy simulations and assume that the passthrough creds must be good enough + mode, _, err := utils.GetOperatorConfiguration(a.Client, logger) if err != nil { return true, err } - simParams := &ccaws.SimulateParams{ - Region: region, - } - goodEnough, err := ccaws.CheckPermissionsUsingQueryClient(readAWSClient, awsClient, awsSpec.StatementEntries, simParams, logger) - if err != nil { - return true, fmt.Errorf("error validating whether current creds are good enough: %v", err) - } - if !goodEnough { - return true, nil + if mode == operatorv1.CloudCredentialsModePassthrough { + logger.Debug("will not perform permissions simulation because operator in mode %q", mode) + } else { + region, err := awsutils.LoadInfrastructureRegion(a.Client, logger) + if err != nil { + return true, err + } + simParams := &ccaws.SimulateParams{ + Region: region, + } + + goodEnough, err := ccaws.CheckPermissionsUsingQueryClient(readAWSClient, awsClient, awsSpec.StatementEntries, simParams, logger) + if err != nil { + return true, fmt.Errorf("error validating whether current creds are good enough: %v", err) + } + if !goodEnough { + return true, nil + } } } diff --git a/pkg/cmd/render/render.go b/pkg/cmd/render/render.go index 207480301..b34cc5583 100644 --- a/pkg/cmd/render/render.go +++ b/pkg/cmd/render/render.go @@ -17,17 +17,22 @@ limitations under the License. package render import ( + "bytes" "fmt" "io/ioutil" "os" "path/filepath" + "text/template" + "github.com/pkg/errors" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" yaml "k8s.io/apimachinery/pkg/util/yaml" + operatorv1 "github.com/openshift/api/operator/v1" + minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" assets "github.com/openshift/cloud-credential-operator/pkg/assets/bootstrap" "github.com/openshift/cloud-credential-operator/pkg/operator/constants" @@ -35,6 +40,8 @@ import ( ) const ( + podYamlFilename = "cloud-credential-operator-pod.yaml" + podTemplate = `apiVersion: v1 kind: Pod metadata: @@ -66,11 +73,25 @@ const ( manifestsDir = "manifests" bootstrapManifestsDir = "bootstrap-manifests" defaultLogLevel = "info" + + installConfigNamespace = "kube-system" + installConfigName = "cluster-config-v1" + installConfigKeyName = "install-config" + + operatorConfigFilename = "cco-operator-config.yaml" ) var ( + operatorConfigTemplate = template.Must(template.New("operatorConfig").Parse(`apiVersion: operator.openshift.io/v1 +kind: CloudCredential +metadata: + name: cluster +spec: + credentialsMode: "{{ .CredentialsMode }}"`)) + renderAssets = []string{ - "bootstrap/cloudcredential_v1_credentialsrequest.yaml", + "bootstrap/cloudcredential_v1_operator_config_crd.yaml", + "bootstrap/cloudcredential_v1_credentialsrequest_crd.yaml", "bootstrap/namespace.yaml", } @@ -89,6 +110,10 @@ var ( } ) +type operatorTemplateVars struct { + CredentialsMode operatorv1.CloudCredentialsMode +} + func NewRenderCommand() *cobra.Command { renderCmd.Flags().StringVar(&renderOpts.manifestsDir, "manifests-dir", "", "The directory where the install-time manifests are located.") renderCmd.Flags().StringVar(&renderOpts.destinationDir, "dest-dir", "", "The destination directory where CCO writes the manifests.") @@ -109,54 +134,110 @@ func runRenderCmd(cmd *cobra.Command, args []string) { log.SetLevel(level) log.Debug("debug logging enabled") - operatorDisabled := isDisabled() + err = render() + if err != nil { + log.WithError(err).Fatal("failed while rendering bootstrap assets") + } +} + +func render() error { + operatorDisabledViaConfigmap := isDisabledViaConfigmap() + + installConfigMode, err := getModeFromInstallConfig() + if err != nil { + return err + } + if !isValidMode(installConfigMode) { + return fmt.Errorf("invalid mode defined: %s", installConfigMode) + } + + effectiveMode, conflict := utils.GetEffectiveOperatorMode(operatorDisabledViaConfigmap, installConfigMode) + + if conflict { + return fmt.Errorf("config map asking for CCO to be disabled, and install-config asking for %q mode", installConfigMode) + } log.Infof("Rendering files to %s", renderOpts.destinationDir) ccoRenderDir := renderOpts.destinationDir - // render manifests + // render manifests from bindata if err := os.MkdirAll(filepath.Join(ccoRenderDir, manifestsDir), 0775); err != nil { - log.WithError(err).Fatal("error creating manifests directory") + return errors.Wrap(err, "error creating manifests directory") } for _, assetName := range renderAssets { asset, err := assets.Asset(assetName) if err != nil { - log.WithError(err).Fatal("failed to read static asset") + return errors.Wrap(err, "failed to read static asset") } - assetRenderPath := filepath.Join(ccoRenderDir, "manifests", "cco-"+filepath.Base(assetName)) - log.Infof("Writing file: %s", assetRenderPath) - err = ioutil.WriteFile(assetRenderPath, asset, 0644) - if err != nil { - log.WithError(err).Fatal("failed to write file") + assetRenderPath := filepath.Join(ccoRenderDir, manifestsDir, "cco-"+filepath.Base(assetName)) + if err := writeFile(assetRenderPath, asset); err != nil { + return err } } + // render operator config + var operatorConfig bytes.Buffer + templateVars := operatorTemplateVars{ + CredentialsMode: effectiveMode, + } + + if err := operatorConfigTemplate.Execute(&operatorConfig, templateVars); err != nil { + return errors.Wrap(err, "failed to execute operator config template") + } + assetRenderPath := filepath.Join(ccoRenderDir, manifestsDir, operatorConfigFilename) + if err := writeFile(assetRenderPath, operatorConfig.Bytes()); err != nil { + return err + } + // need at least the empty dir so the installer bootkube.sh script works as expected if err := os.Mkdir(filepath.Join(ccoRenderDir, bootstrapManifestsDir), 0775); err != nil { - log.WithError(err).Fatal("error creating bootstrap-manifests directory") + errors.Wrap(err, "error creating bootstrap-manifests directory") } - if !operatorDisabled { + if effectiveMode != operatorv1.CloudCredentialsModeManual { log.Info("Rendering static pod") - podPath := filepath.Join(ccoRenderDir, bootstrapManifestsDir, "cloud-credential-operator-pod.yaml") + podPath := filepath.Join(ccoRenderDir, bootstrapManifestsDir, podYamlFilename) podContent := fmt.Sprintf(podTemplate, renderOpts.ccoImage) log.Infof("writing file: %s", podPath) - err = ioutil.WriteFile(podPath, []byte(podContent), 0644) + err := ioutil.WriteFile(podPath, []byte(podContent), 0644) if err != nil { - log.WithError(err).Fatal("failed to write file") + return errors.Wrap(err, "failed to write file") } } else { log.Info("CCO disabled, skipping static pod manifest.") } + + return nil +} + +func writeFile(filePath string, fileData []byte) error { + log.Infof("Writing file: %s", filePath) + err := ioutil.WriteFile(filePath, fileData, 0644) + if err != nil { + return errors.Wrap(err, "failed to write file") + } + return nil +} + +func isValidMode(mode operatorv1.CloudCredentialsMode) bool { + switch mode { + case operatorv1.CloudCredentialsModeDefault, + operatorv1.CloudCredentialsModeManual, + operatorv1.CloudCredentialsModeMint, + operatorv1.CloudCredentialsModePassthrough: + return true + default: + return false + } } // isDisabled will search through all the files in destinationDir (which also contains -// the source manifests) for a configmap indicating whether or not CCO is disabled. In +// the source manifests) for the deprecated configmap indicating whether or not CCO is disabled. In // the absence of any configmap, it will return the default disabled setting (which is // that the operator is enabled by default) for the operator. -func isDisabled() bool { +func isDisabledViaConfigmap() bool { // if were were not provided a place to search for the install-time manifests, // just return the default CCO operator enabled/disabled value @@ -164,25 +245,80 @@ func isDisabled() bool { return utils.OperatorDisabledDefault } - files, err := ioutil.ReadDir(renderOpts.manifestsDir) + cm, err := getConfigMap(renderOpts.manifestsDir, minterv1.CloudCredOperatorNamespace, constants.CloudCredOperatorConfigMap) + if err != nil { + log.WithError(err).Warnf("errored while searching for ConfigMap %s/%s, using defaults", minterv1.CloudCredOperatorNamespace, constants.CloudCredOperatorConfigMap) + return utils.OperatorDisabledDefault + } + if cm == nil { + log.Debugf("configmap %s/%s not found, using default mode", minterv1.CloudCredOperatorNamespace, constants.CloudCredOperatorConfigMap) + return utils.OperatorDisabledDefault + } + + disabled, err := utils.CCODisabledCheck(cm, log.WithFields(nil)) if err != nil { - log.WithError(err).Errorf("failed to list files in %s, using defualt operator settings", renderOpts.destinationDir) return utils.OperatorDisabledDefault } + return disabled +} + +type basicInstallConfig struct { + CredentialsMode operatorv1.CloudCredentialsMode `json:"credentialsMode"` +} + +func getModeFromInstallConfig() (operatorv1.CloudCredentialsMode, error) { + + // if we were not provided a place to search for the install-time manifests, + // just return the default cloudCredentialsMode (empty string) + if renderOpts.manifestsDir == "" { + return "", nil + } + + cm, err := getConfigMap(renderOpts.manifestsDir, installConfigNamespace, installConfigName) + if err != nil { + return "", errors.Wrapf(err, "failed to find configmap %s/%s in manifests", installConfigNamespace, installConfigName) + } + if cm == nil { + return "", fmt.Errorf("failed to find configmap %s/%s in manifests", installConfigNamespace, installConfigName) + } + + data, ok := cm.Data[installConfigKeyName] + if !ok { + return "", fmt.Errorf("did not find key %s in configmap %s/%s", installConfigKeyName, installConfigNamespace, installConfigName) + } + + decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader([]byte(data)), 4096) + instConf := &basicInstallConfig{} + if err := decoder.Decode(instConf); err != nil { + return "", errors.Wrap(err, "failed to decode install config") + } + log.Debugf("install-config contains CredentialsMode: %s", instConf.CredentialsMode) + return instConf.CredentialsMode, nil +} + +func getConfigMap(manifestsDir, namespace, name string) (*corev1.ConfigMap, error) { + + files, err := ioutil.ReadDir(manifestsDir) + if err != nil { + log.WithError(err).Errorf("failed to list files in %s", manifestsDir) + return nil, err + } + for _, fInfo := range files { - // non-recursive checking of all files where the the source manifests are located + // non-recursive checking of all files where the source manifests are located if fInfo.IsDir() { continue } - fullPath := filepath.Join(renderOpts.manifestsDir, fInfo.Name()) + fullPath := filepath.Join(manifestsDir, fInfo.Name()) log.Debugf("checking file: %s", fullPath) file, err := os.Open(fullPath) if err != nil { log.WithError(err).Warn("failed to open file while searching for configmap") continue } + decoder := yaml.NewYAMLOrJSONDecoder(file, 4096) configMap := &corev1.ConfigMap{} if err := decoder.Decode(configMap); err != nil { @@ -190,17 +326,14 @@ func isDisabled() bool { continue } - if configMap.Namespace == minterv1.CloudCredOperatorNamespace && configMap.Name == constants.CloudCredOperatorConfigMap { - logger := log.New() - logger.SetLevel(log.GetLevel()) - disabled, err := utils.CCODisabledCheck(configMap, logger) - if err != nil { - return utils.OperatorDisabledDefault - } - - return disabled + if configMap.APIVersion == corev1.SchemeGroupVersion.Identifier() && + configMap.Kind == "ConfigMap" && + configMap.Namespace == namespace && + configMap.Name == name { + return configMap, nil } } - return utils.OperatorDisabledDefault + // if we made it this far, then the file was not found + return nil, nil } diff --git a/pkg/cmd/render/render_test.go b/pkg/cmd/render/render_test.go new file mode 100644 index 000000000..497487240 --- /dev/null +++ b/pkg/cmd/render/render_test.go @@ -0,0 +1,184 @@ +package render + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "testing" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/yaml" +) + +const ( + testDestinationDirName = "renderHere" +) + +type testFile struct { + name string + data string +} + +func TestRender(t *testing.T) { + tests := []struct { + name string + existingFiles []*testFile + expectPodFile bool + expectMode string + expectError bool + }{ + { + name: "install config default", + existingFiles: []*testFile{ + testInstallConfig(""), + }, + expectPodFile: true, + // default is no setting in the config + expectMode: "", + }, + { + name: "install config manual mode", + existingFiles: []*testFile{ + testInstallConfig(string(operatorv1.CloudCredentialsModeManual)), + }, + expectPodFile: false, + expectMode: string(operatorv1.CloudCredentialsModeManual), + }, + { + name: "deprecated configmap disables cco", + existingFiles: []*testFile{ + testInstallConfig(""), + testConfigMap("true"), + }, + expectPodFile: false, + expectMode: string(operatorv1.CloudCredentialsModeManual), + }, + { + name: "install config mint mode", + existingFiles: []*testFile{ + testInstallConfig(string(operatorv1.CloudCredentialsModeMint)), + }, + expectPodFile: true, + expectMode: string(operatorv1.CloudCredentialsModeMint), + }, + { + name: "configmap and installconfig conflict", + existingFiles: []*testFile{ + testInstallConfig(string(operatorv1.CloudCredentialsModeMint)), + testConfigMap("true"), + }, + expectError: true, + }, + { + name: "configmap and installconfig concur", + existingFiles: []*testFile{ + testInstallConfig(string(operatorv1.CloudCredentialsModeManual)), + testConfigMap("true"), + }, + expectPodFile: false, + expectMode: string(operatorv1.CloudCredentialsModeManual), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + manifestsDir, err := ioutil.TempDir("/tmp", "rendertestmanifests") + require.NoError(t, err, "errored setting up test") + defer os.RemoveAll(manifestsDir) + + for _, file := range test.existingFiles { + filePath := filepath.Join(manifestsDir, file.name) + err := ioutil.WriteFile(filePath, []byte(file.data), 0644) + require.NoError(t, err, "failed writing out manifests for test") + } + destDir, err := ioutil.TempDir("/tmp", "rendertestdestination") + require.NoError(t, err, "errored setting up test") + defer os.RemoveAll(destDir) + + destDirRenderPath := filepath.Join(destDir, testDestinationDirName) + + renderOpts.manifestsDir = manifestsDir + renderOpts.destinationDir = destDirRenderPath + renderOpts.ccoImage = "testCCOImage" + renderOpts.logLevel = "debug" + + err = render() + + if test.expectError { + require.Error(t, err, "expected error for test case") + } else { + require.NoError(t, err, "unexpected error") + + verifyPodFile(t, destDirRenderPath, test.expectPodFile) + + verifyConfigMode(t, destDirRenderPath, test.expectMode) + } + }) + } +} + +func testInstallConfig(credMode string) *testFile { + instConfFile := testFile{ + name: "install-config.yaml", + } + + installConfigData := `apiVersion: v1 +kind: ConfigMap +metadata: + name: cluster-config-v1 + namespace: kube-system +data: + install-config: | + baseDomain: test.openshift.io + credentialsMode: %s` + + instConfFile.data = fmt.Sprintf(installConfigData, credMode) + + return &instConfFile +} + +func verifyPodFile(t *testing.T, destDir string, expectFileToExist bool) { + podFilePath := filepath.Join(destDir, bootstrapManifestsDir, podYamlFilename) + _, err := os.Stat(podFilePath) + + if expectFileToExist { + assert.NoError(t, err, "expected pod yaml to be rendered") + } else { + assert.True(t, os.IsNotExist(err), "expect pod yaml to not be rendered") + } +} + +func verifyConfigMode(t *testing.T, destDir, expectMode string) { + configFilePath := filepath.Join(destDir, manifestsDir, operatorConfigFilename) + + file, err := os.Open(configFilePath) + require.NoError(t, err, "error reading in rendered config file") + + conf := operatorv1.CloudCredential{} + decoder := yaml.NewYAMLOrJSONDecoder(file, 4096) + err = decoder.Decode(&conf) + require.NoError(t, err, "error decoding rendered config") + + assert.Equal(t, expectMode, string(conf.Spec.CredentialsMode), "config file has unexpected mode set") +} + +func testConfigMap(disabled string) *testFile { + configMapFile := testFile{ + name: "configmap.yaml", + } + + data := `apiVersion: v1 +kind: ConfigMap +metadata: + name: cloud-credential-operator-config + namespace: openshift-cloud-credential-operator +data: + disabled: "%s"` + + configMapFile.data = fmt.Sprintf(data, disabled) + + return &configMapFile +} diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go index 40917b368..a93088d8f 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go @@ -109,6 +109,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "new credential", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), @@ -164,6 +165,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "new credential cluster has no infra name", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), @@ -219,6 +221,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "new credential no root creds available", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), @@ -250,6 +253,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "cred missing access key exists", // expect old key(s) deleted, new key created/saved existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), @@ -290,6 +294,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "cred exists access key missing", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), @@ -330,6 +335,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "cred deletion", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequestWithDeletionTimestamp(t), @@ -356,6 +362,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "failed to mint condition", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), @@ -389,6 +396,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "cred deletion failure condition", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequestWithDeletionTimestamp(t), testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), @@ -414,6 +422,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "new cred passthrough", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), @@ -447,6 +456,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "new cred passthrough fail permissions", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), @@ -483,6 +493,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "existing cr up to date", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPPassthroughCredentialsRequest(t), testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), @@ -515,6 +526,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { { name: "existing secret has old secret content", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPPassthroughCredentialsRequest(t), testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go index 85ab1e3f9..532a734df 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go @@ -28,6 +28,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/awserr" + "github.com/aws/aws-sdk-go/service/iam" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -49,10 +53,6 @@ import ( "github.com/openshift/cloud-credential-operator/pkg/operator/utils" schemeutils "github.com/openshift/cloud-credential-operator/pkg/util" "github.com/openshift/cloud-credential-operator/pkg/util/clusteroperator" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" - "github.com/aws/aws-sdk-go/service/iam" ) var c client.Client @@ -131,6 +131,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "add finalizer", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), func() *minterv1.CredentialsRequest { @@ -157,6 +158,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "new credential", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -212,6 +214,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "new credential cluster has no infra name", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -268,6 +271,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { // This tests the case where we create our own read only creds initially: name: "new credential no read-only creds available", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -317,6 +321,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { // This indicates an error state. name: "new credential no root creds available", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -359,6 +364,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred and secret exist user tagged", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -394,6 +400,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred and secret exist user missing tag", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -434,6 +441,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred and secret exist no root creds", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -482,6 +490,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred missing access key exists", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -519,6 +528,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred exists access key missing", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -557,6 +567,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred deletion", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequestWithDeletionTimestamp(t), @@ -596,6 +607,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "new passthrough credential", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), @@ -623,9 +635,86 @@ func TestCredentialsRequestReconcile(t *testing.T) { assert.True(t, cr.Status.Provisioned) }, }, + { + name: "existing passthrough credential", + existing: []runtime.Object{ + testOperatorConfig(""), + createTestNamespace(testNamespace), + testInfrastructure(testInfraName), + createTestNamespace(testSecretNamespace), + testPassthroughCredentialsRequest(t), + testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), + testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), + }, + mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + return mockAWSClient + }, + mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + mockGetUser(mockAWSClient) + // will simulate to check that the creds in the target secret actually satisfy the requested perms + mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient) + return mockAWSClient + }, + mockSecretAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + mockGetUser(mockAWSClient) + return mockAWSClient + }, + validate: func(c client.Client, t *testing.T) { + targetSecret := getSecret(c) + if assert.NotNil(t, targetSecret) { + assert.Equal(t, testAWSAccessKeyID, + string(targetSecret.Data["aws_access_key_id"])) + assert.Equal(t, testAWSSecretAccessKey, + string(targetSecret.Data["aws_secret_access_key"])) + } + cr := getCR(c) + assert.True(t, cr.Status.Provisioned) + }, + }, + { + name: "existing passthrough credential bypassing simulations", + existing: []runtime.Object{ + testOperatorConfig(operatorv1.CloudCredentialsModePassthrough), // indicates that CCO should not perform permissions simulations + createTestNamespace(testNamespace), + testInfrastructure(testInfraName), + createTestNamespace(testSecretNamespace), + testPassthroughCredentialsRequest(t), + testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), + testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), + }, + mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + return mockAWSClient + }, + mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + mockGetUser(mockAWSClient) + return mockAWSClient + }, + mockSecretAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + mockGetUser(mockAWSClient) + return mockAWSClient + }, + validate: func(c client.Client, t *testing.T) { + targetSecret := getSecret(c) + if assert.NotNil(t, targetSecret) { + assert.Equal(t, testAWSAccessKeyID, + string(targetSecret.Data["aws_access_key_id"])) + assert.Equal(t, testAWSSecretAccessKey, + string(targetSecret.Data["aws_secret_access_key"])) + } + cr := getCR(c) + assert.True(t, cr.Status.Provisioned) + }, + }, { name: "passthrough cred deletion", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), @@ -642,6 +731,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "no namespace condition", existing: []runtime.Object{ + testOperatorConfig(""), testInfrastructure(testInfraName), testCredentialsRequest(t), }, @@ -660,6 +750,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "insufficient creds", existing: []runtime.Object{ + testOperatorConfig(""), testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -681,6 +772,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "failed to mint condition", existing: []runtime.Object{ + testOperatorConfig(""), testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -709,6 +801,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "cred deletion failure condition", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testInfrastructure(testInfraName), testCredentialsRequestWithDeletionTimestamp(t), @@ -737,6 +830,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "skip AWS if recently synced", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), @@ -755,6 +849,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "regenerate secret if missing", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), testInfrastructure(testInfraName), testClusterVersion(), @@ -786,6 +881,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { name: "recently synced but modified", existing: func() []runtime.Object { objects := []runtime.Object{} + objects = append(objects, testOperatorConfig("")) objects = append(objects, createTestNamespace(testNamespace)) objects = append(objects, createTestNamespace(testSecretNamespace)) @@ -820,6 +916,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "skip nonAWS credreq", existing: []runtime.Object{ + testOperatorConfig(""), testGCPCredentialsRequest(t), }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { @@ -840,6 +937,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "clear conditions when ignoring cred request", existing: []runtime.Object{ + testOperatorConfig(""), func() runtime.Object { cr := testGCPCredentialsRequest(t) for _, cond := range minterv1.FailureConditionTypes { @@ -869,6 +967,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "pass along any existing permissions boundary", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), @@ -911,6 +1010,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "new credential but operator disabled via configmap", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testOperatorConfigMap("true"), @@ -1011,6 +1111,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "recently synced but with error condition set", existing: []runtime.Object{ + testOperatorConfig(""), func() *minterv1.CredentialsRequest { cr := testCredentialsRequestWithRecentLastSync(t) cr.Status.Conditions = utils.SetCredentialsRequestCondition(cr.Status.Conditions, @@ -1043,6 +1144,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "recently synced but not provisioned", existing: []runtime.Object{ + testOperatorConfig(""), func() *minterv1.CredentialsRequest { cr := testCredentialsRequestWithRecentLastSync(t) cr.Status.Provisioned = false @@ -1070,6 +1172,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "provision with aws policy condition", existing: []runtime.Object{ + testOperatorConfig(""), func() *minterv1.CredentialsRequest { cr := testCredentialsRequest(t) awsProvSpec, err := codec.EncodeProviderSpec( @@ -1135,6 +1238,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { { name: "already provisioned with aws policy condition", existing: []runtime.Object{ + testOperatorConfig(""), func() *minterv1.CredentialsRequest { cr := testCredentialsRequest(t) awsProvSpec, err := codec.EncodeProviderSpec( @@ -1210,6 +1314,17 @@ func TestCredentialsRequestReconcile(t *testing.T) { assert.NotNil(t, cr.Status.LastSyncTimestamp) }, }, + { + name: "error reconcile without operator config", + existing: []runtime.Object{ + testCredentialsRequest(t), + }, + mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + return mockAWSClient + }, + expectErr: true, + }, } for _, test := range tests { @@ -1263,7 +1378,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { } if err != nil && !test.expectErr { - t.Errorf("Unexpected error: %v", err) + require.NoError(t, err, "Unexpected error: %v", err) } if err == nil && test.expectErr { t.Errorf("Expected error but got none") @@ -1272,14 +1387,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { cr := getCR(fakeClient) for _, condition := range test.expectedConditions { foundCondition := utils.FindCredentialsRequestCondition(cr.Status.Conditions, condition.conditionType) - assert.NotNil(t, foundCondition) + require.NotNil(t, foundCondition, "unexpected unable to find condition") assert.Exactly(t, condition.status, foundCondition.Status) assert.Exactly(t, condition.reason, foundCondition.Reason) } for _, condition := range test.expectedCOConditions { co := getClusterOperator(fakeClient) - assert.NotNil(t, co) + require.NotNil(t, co) foundCondition := findClusterOperatorCondition(co.Status.Conditions, condition.conditionType) assert.NotNil(t, foundCondition) assert.Equal(t, string(condition.status), string(foundCondition.Status), "condition %s had unexpected status", condition.conditionType) @@ -1301,7 +1416,7 @@ const ( testSecretName = "test-secret" testSecretNamespace = "myproject" testAWSUser = "mycluster-test-aws-user" - testAWSARN = "some:fake:ARN:1234" + testAWSARN = "arn:aws:iam::1234:user/testuser" testAWSUserID = "FAKEAWSUSERID" testAWSAccessKeyID = "FAKEAWSACCESSKEYID" testAWSAccessKeyID2 = "FAKEAWSACCESSKEYID2" @@ -1662,6 +1777,10 @@ func testClusterVersion() *configv1.ClusterVersion { } } +func mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient *mockaws.MockClient) { + mockAWSClient.EXPECT().SimulatePrincipalPolicyPages(gomock.Any(), gomock.Any()).Return(nil) +} + func mockSimulatePrincipalPolicySuccess(mockAWSClient *mockaws.MockClient) { mockAWSClient.EXPECT().SimulatePrincipalPolicy(gomock.Any()).Return(&iam.SimulatePolicyResponse{ EvaluationResults: []*iam.EvaluationResult{ diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go index 580bec249..f0510e588 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go @@ -78,6 +78,7 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { { name: "new credentialsrequest passthrough", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testVSphereCredsSecretPassthrough(), @@ -113,6 +114,7 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { { name: "new credential no root creds available", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), @@ -138,6 +140,7 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { { name: "cred deletion", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequestWithDeletionTimestamp(t), @@ -152,6 +155,7 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { { name: "existing cr up to date", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), testVSphereCredsSecretPassthrough(), @@ -172,6 +176,7 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { { name: "existing secret has old secret content", existing: []runtime.Object{ + testOperatorConfig(""), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), testVSphereCredsSecretPassthrough(), diff --git a/pkg/operator/credentialsrequest/status_test.go b/pkg/operator/credentialsrequest/status_test.go index 32c896bef..d61f9bf9d 100644 --- a/pkg/operator/credentialsrequest/status_test.go +++ b/pkg/operator/credentialsrequest/status_test.go @@ -264,7 +264,8 @@ func TestClusterOperatorVersion(t *testing.T) { t.Run(test.name, func(t *testing.T) { existingCO := testClusterOperator("4.0.0-5", twentyHoursAgo) - existing := []runtime.Object{existingCO} + operatorConfig := testOperatorConfig("") + existing := []runtime.Object{existingCO, operatorConfig} fakeClient := fake.NewFakeClient(existing...) rcr := &ReconcileCredentialsRequest{ diff --git a/pkg/operator/secretannotator/aws/reconciler_test.go b/pkg/operator/secretannotator/aws/reconciler_test.go index 2fc0528bc..fb5490ded 100644 --- a/pkg/operator/secretannotator/aws/reconciler_test.go +++ b/pkg/operator/secretannotator/aws/reconciler_test.go @@ -94,8 +94,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue string }{ { - name: "cred minter mode", - existing: []runtime.Object{testSecret()}, + name: "cred minter mode", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -106,8 +109,12 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.MintAnnotation, }, { - name: "operator disabled via configmap", - existing: []runtime.Object{testSecret(), testOperatorConfigMap("true")}, + name: "operator disabled via configmap", + existing: []runtime.Object{ + testSecret(), + testOperatorConfigMap("true"), + testOperatorConfig(""), + }, }, { name: "operator disabled", @@ -117,8 +124,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { }, }, { - name: "detect root user creds", - existing: []runtime.Object{testSecret()}, + name: "detect root user creds", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetRootUser(mockAWSClient) @@ -128,8 +138,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { expectErr: true, }, { - name: "cred passthrough mode", - existing: []runtime.Object{testSecret()}, + name: "cred passthrough mode", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -143,8 +156,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.PassthroughAnnotation, }, { - name: "cred passthrough mode wrong region permission", - existing: []runtime.Object{testSecret()}, + name: "cred passthrough mode wrong region permission", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -158,8 +174,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.InsufficientAnnotation, }, { - name: "useless creds", - existing: []runtime.Object{testSecret()}, + name: "useless creds", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -173,22 +192,28 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.InsufficientAnnotation, }, { - name: "missing secret", + name: "missing secret", + existing: []runtime.Object{ + testOperatorConfig(""), + }, expectErr: true, }, { name: "secret missing key", expectErr: true, - existing: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: testSecretName, - Namespace: testNamespace, - }, - Data: map[string][]byte{ - annaws.AwsAccessKeyName: []byte(testAWSAccessKeyID), - "not_aws_secret_access_key": []byte(testAWSSecretAccessKey), + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testNamespace, + }, + Data: map[string][]byte{ + annaws.AwsAccessKeyName: []byte(testAWSAccessKeyID), + "not_aws_secret_access_key": []byte(testAWSSecretAccessKey), + }, }, - }}, + testOperatorConfig(""), + }, }, { name: "annotation matches forced mode", @@ -206,6 +231,13 @@ func TestSecretAnnotatorReconcile(t *testing.T) { }, expectErr: true, }, + { + name: "error on missing config CR", + existing: []runtime.Object{ + testSecret(), + }, + expectErr: true, + }, } for _, test := range tests { diff --git a/pkg/operator/secretannotator/azure/reconciler_test.go b/pkg/operator/secretannotator/azure/reconciler_test.go index a334483f9..a7c77ac13 100644 --- a/pkg/operator/secretannotator/azure/reconciler_test.go +++ b/pkg/operator/secretannotator/azure/reconciler_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -83,8 +84,9 @@ func TestAzureSecretAnnotatorReconcile(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { base := getInputSecret() + operatorConfig := testOperatorConfig("") infra := &configv1.Infrastructure{ObjectMeta: metav1.ObjectMeta{Name: "cluster"}} - fakeClient := fake.NewFakeClient(base, infra) + fakeClient := fake.NewFakeClient(base, operatorConfig, infra) mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() mockAdalClient := mock.NewMockAdalService(mockCtrl) @@ -142,6 +144,19 @@ func getInputSecret() *corev1.Secret { } +func testOperatorConfig(mode operatorv1.CloudCredentialsMode) *operatorv1.CloudCredential { + conf := &operatorv1.CloudCredential{ + ObjectMeta: metav1.ObjectMeta{ + Name: constants.CloudCredOperatorConfig, + }, + Spec: operatorv1.CloudCredentialSpec{ + CredentialsMode: mode, + }, + } + + return conf +} + func setupAdalMock(r *mock.MockAdalServiceMockRecorder, roles []string) { gomock.InOrder( // these methdods are extensivly tested in azure codebase diff --git a/pkg/operator/secretannotator/gcp/reconciler_test.go b/pkg/operator/secretannotator/gcp/reconciler_test.go index 08f249a8d..96b1096bc 100644 --- a/pkg/operator/secretannotator/gcp/reconciler_test.go +++ b/pkg/operator/secretannotator/gcp/reconciler_test.go @@ -85,8 +85,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue string }{ { - name: "cred minter mode", - existing: []runtime.Object{testSecret()}, + name: "cred minter mode", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -98,8 +101,12 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.MintAnnotation, }, { - name: "operator disabled via configmap", - existing: []runtime.Object{testSecret(), testOperatorConfigMap("true")}, + name: "operator disabled via configmap", + existing: []runtime.Object{ + testSecret(), + testOperatorConfigMap("true"), + testOperatorConfig(""), + }, mockGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) return mockGCPClient @@ -113,8 +120,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { }, }, { - name: "cred passthrough mode", - existing: []runtime.Object{testSecret()}, + name: "cred passthrough mode", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -128,8 +138,11 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.PassthroughAnnotation, }, { - name: "useless creds", - existing: []runtime.Object{testSecret()}, + name: "useless creds", + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, mockGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -142,7 +155,10 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue: constants.InsufficientAnnotation, }, { - name: "missing secret", + name: "missing secret", + existing: []runtime.Object{ + testOperatorConfig(""), + }, expectErr: true, }, { @@ -157,6 +173,7 @@ func TestSecretAnnotatorReconcile(t *testing.T) { "not-the-right-key": []byte(testGCPAuthJSON), }, }, + testOperatorConfig(""), }, validateAnnotationValue: constants.InsufficientAnnotation, }, @@ -272,6 +289,19 @@ func testOperatorConfigMap(disabled string) *corev1.ConfigMap { } } +func testOperatorConfig(mode operatorv1.CloudCredentialsMode) *operatorv1.CloudCredential { + conf := &operatorv1.CloudCredential{ + ObjectMeta: metav1.ObjectMeta{ + Name: constants.CloudCredOperatorConfig, + }, + Spec: operatorv1.CloudCredentialSpec{ + CredentialsMode: mode, + }, + } + + return conf +} + func mockGetProjectName(mockGCPClient *mockgcp.MockClient) { mockGCPClient.EXPECT().GetProjectName().AnyTimes().Return("test-GCP-project") } @@ -306,16 +336,3 @@ func mockListMintServicesEnabledSuccess(mockGCPClient *mockgcp.MockClient) { func mockListPassthroughServicesEnabledSuccess(mockGCPClient *mockgcp.MockClient) { mockGCPClient.EXPECT().ListServicesEnabled().Return(passthroughServicesEnabledMap, nil) } - -func testOperatorConfig(mode operatorv1.CloudCredentialsMode) *operatorv1.CloudCredential { - conf := &operatorv1.CloudCredential{ - ObjectMeta: metav1.ObjectMeta{ - Name: constants.CloudCredOperatorConfig, - }, - Spec: operatorv1.CloudCredentialSpec{ - CredentialsMode: mode, - }, - } - - return conf -} diff --git a/pkg/operator/secretannotator/vsphere/reconciler_test.go b/pkg/operator/secretannotator/vsphere/reconciler_test.go index bad7442c2..79a41764d 100644 --- a/pkg/operator/secretannotator/vsphere/reconciler_test.go +++ b/pkg/operator/secretannotator/vsphere/reconciler_test.go @@ -31,6 +31,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -61,17 +62,28 @@ func TestSecretAnnotatorReconcile(t *testing.T) { validateAnnotationValue string }{ { - name: "operator disabled", - existing: []runtime.Object{testSecret(), testOperatorConfigMap("true")}, + name: "operator disabled", + existing: []runtime.Object{ + testSecret(), + testOperatorConfigMap("true"), + testOperatorConfig(""), + }, }, { - name: "operator enabled", - existing: []runtime.Object{testSecret(), testOperatorConfigMap("false")}, + name: "operator enabled", + existing: []runtime.Object{ + testSecret(), + testOperatorConfigMap("false"), + testOperatorConfig(""), + }, }, { name: "annotate passthrough mode", // right now only passthrough mode is supported so any secret works - existing: []runtime.Object{testSecret()}, + existing: []runtime.Object{ + testSecret(), + testOperatorConfig(""), + }, validateAnnotationValue: constants.PassthroughAnnotation, }, { @@ -148,6 +160,19 @@ func testOperatorConfigMap(disabled string) *corev1.ConfigMap { } } +func testOperatorConfig(mode operatorv1.CloudCredentialsMode) *operatorv1.CloudCredential { + conf := &operatorv1.CloudCredential{ + ObjectMeta: metav1.ObjectMeta{ + Name: constants.CloudCredOperatorConfig, + }, + Spec: operatorv1.CloudCredentialSpec{ + CredentialsMode: mode, + }, + } + + return conf +} + func validateSecretAnnotation(c client.Client, t *testing.T, value string) { secret := getCredSecret(c) validateAnnotation(t, secret, value) diff --git a/pkg/operator/utils/utils.go b/pkg/operator/utils/utils.go index ef44ef8c2..6b3f0318e 100644 --- a/pkg/operator/utils/utils.go +++ b/pkg/operator/utils/utils.go @@ -149,10 +149,9 @@ func GenerateNameWithFieldLimits(infraName string, infraNameMaxLen int, crName s return fmt.Sprintf("%s%s", infraPrefix, crName), nil } -// IsOperatorDisabledViaConfigmap checks the cloud-credential-operator-config ConfigMap for a -// "disabled" property set to true. If the configmap or property do not exist, we assume -// false and continue normal operation. This should be used in all controllers to shutdown -// functionality in environments where admins want to manage their credentials themselves. +// isOperatorDisabledViaConfigmap checks the cloud-credential-operator-config ConfigMap for a +// "disabled" property set to true. If the configmap or property does not exist, we assume +// false and continue normal operation. // DEPRECATED (and unexported), use GetOperatorConfiguration to determine disabled state. func isOperatorDisabledViaConfigmap(kubeClient client.Client, logger log.FieldLogger) (bool, error) { cm, err := GetLegacyConfigMap(kubeClient) @@ -181,14 +180,14 @@ func GetLegacyConfigMap(kubeClient client.Client) (*corev1.ConfigMap, error) { // and whether there is a conflict between the legacy ConfigMap and CCO config (in the even of a conflict, the // operator mode will be reported to reflect the actual value in the operator config). func GetOperatorConfiguration(kubeClient client.Client, logger log.FieldLogger) ( - operatorMode operatorv1.CloudCredentialsMode, + effectiveOperatorMode operatorv1.CloudCredentialsMode, configurationConflict bool, err error) { + var operatorMode operatorv1.CloudCredentialsMode operatorMode, err = getOperatorMode(kubeClient, logger) if err != nil { return } - disabledViaOperatorConfig := operatorMode == operatorv1.CloudCredentialsModeManual var disabledViaConfigMap bool disabledViaConfigMap, err = isOperatorDisabledViaConfigmap(kubeClient, logger) @@ -196,26 +195,35 @@ func GetOperatorConfiguration(kubeClient client.Client, logger log.FieldLogger) return } - if operatorMode == operatorv1.CloudCredentialsModeDefault { - // if no mode is set, then only the value in the configmap can end up disabling CCO - // so report back that the operator mode is manual (via the configmap setting) - if disabledViaConfigMap { - operatorMode = operatorv1.CloudCredentialsModeManual + effectiveOperatorMode, configurationConflict = GetEffectiveOperatorMode(disabledViaConfigMap, operatorMode) + if configurationConflict { + log.Errorf("legacy configmap disabled set to %v conflicts with operator CR mode of %s", + disabledViaConfigMap, operatorMode) + } + + return +} + +// GetEffectiveOperatorMode will take the legacy configmap and the value in the operator config, and return +// the effective CCO mode and whether there is a conflict between the legacy and operator config values. +func GetEffectiveOperatorMode(configMapDisabledValue bool, operatorConfigMode operatorv1.CloudCredentialsMode) (operatorv1.CloudCredentialsMode, bool) { + + // if no mode is set, then only the value in the configmap can end up disabling CCO + if operatorConfigMode == "" { + if configMapDisabledValue { + return operatorv1.CloudCredentialsModeManual, false } - return + return operatorConfigMode, false } // else see if there is a disconnect between the operator mode and the // opt-in value of 'disabled: "true"' in the ConfigMap - if disabledViaConfigMap && disabledViaConfigMap != disabledViaOperatorConfig { - log.Errorf("legacy configmap disabled set to %v conflict with operator CR mode of %s", - disabledViaConfigMap, operatorMode) - // Allow the actual CCO config to be returned as that should take - // precedence for the purpose of reporting metrics data, but indicate the conflict. - configurationConflict = true + disabledViaOperatorConfig := operatorConfigMode == operatorv1.CloudCredentialsModeManual + if configMapDisabledValue && !disabledViaOperatorConfig { + return operatorConfigMode, true } - return + return operatorConfigMode, false } @@ -227,14 +235,15 @@ func getOperatorMode(kubeClient client.Client, logger log.FieldLogger) (operator Name: constants.CloudCredOperatorConfig, }, conf) if err != nil { - // SHOULD WE BE WATCHING FOR THIS ERROR??? + // TODO: is it valuable to watch for this error, or just return the error + // at the bottom of this block??? if metaerrors.IsNoMatchError(err) { logger.WithError(err).Debug("no config CRD found") - return operatorv1.CloudCredentialsModeDefault, nil + return "", err } if errors.IsNotFound(err) { - logger.Debugf("%s CCO operator config does not exist, assuming default behavior", constants.CloudCredOperatorConfig) - return operatorv1.CloudCredentialsModeDefault, nil + logger.Debugf("%s CCO operator config does not exist", constants.CloudCredOperatorConfig) + return "", err } return "", err }