CCO-366 Add ability to detect AWS STS and behave accordingly #542

bentito · 2023-05-19T19:50:35Z

Adds capability to detect AWS STS enabled clusters and behave a bit like Mint mode when detected.

There is now a small unit test suite. And a single e2e test that uses an STS workflow.

Notes:

Will need changes to allow for move of the TAT data to AWSProviderSpec;
Is the e2e really working? Lack of "STS detection" related logging and lack of created Secret in the must-gather make me unsure:

$  yq '.items[].metadata.name' /tmp/artifacts/registry*/namespaces/default/core/secrets.yaml
builder-dockercfg-l6mps
builder-token-6skc2
default-dockercfg-28hgj
default-token-pr62d
deployer-dockercfg-p248v
deployer-token-djlzm

Ah, it seems to be working, see inline comment with log lines showing Secret creation/deletion on CredentialsRequest
addition in the e2e test.

openshift-ci · 2023-05-19T19:50:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-05-19T22:21:51Z

Codecov Report

Merging #542 (a3b71ca) into master (ee67cc6) will decrease coverage by 0.23%.
The diff coverage is 44.72%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   48.06%   47.84%   -0.23%     
==========================================
  Files          93       93              
  Lines       11305    11488     +183     
==========================================
+ Hits         5434     5496      +62     
- Misses       5251     5359     +108     
- Partials      620      633      +13

Impacted Files	Coverage Δ
pkg/assets/bootstrap/bindata.go	`23.85% <ø> (ø)`
pkg/kubevirt/actuator.go	`67.60% <0.00%> (-0.97%)`	⬇️
pkg/openstack/actuator.go	`0.00% <0.00%> (ø)`
...awspodidentity/awspodidentitywebhook_controller.go	`32.21% <0.00%> (-0.22%)`	⬇️
pkg/operator/cleanup/cleanup_controller.go	`52.32% <0.00%> (ø)`
...g/operator/credentialsrequest/actuator/actuator.go	`39.13% <0.00%> (-3.73%)`	⬇️
pkg/operator/loglevel/controller.go	`49.15% <0.00%> (ø)`
pkg/operator/platform/platform.go	`11.11% <0.00%> (-19.66%)`	⬇️
pkg/operator/secretannotator/aws/reconciler.go	`45.29% <0.00%> (ø)`
pkg/operator/secretannotator/azure/reconciler.go	`35.52% <0.00%> (ø)`
... and 12 more

bentito · 2023-06-11T07:52:04Z

pkg/aws/actuator/actuator.go

-			ErrReason: minterv1.InsufficientCloudCredentials,
-			Message:   msg,
+	stsDetected, err := utils.IsTimedTokenCluster(a.Client, logger)
+	if stsDetected {


After this change e2e started working. I am kind of unsure about it though.

Okay, here is logging from a Clusterbot cluster launched with workflow-launch openshift-e2e-aws-manual-oidc-sts https://github.com/openshift/cloud-credential-operator/pull/542 and then having run make test-e2e-sts manually:

time="2023-06-12T13:40:45Z" level=info msg="reconciling clusteroperator status" time="2023-06-12T13:40:45Z" level=info msg="operator set to disabled / manual mode" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:45Z" level=info msg="syncing credentials request" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:45Z" level=info msg="adding finalizer: cloudcredential.openshift.io/deprovision" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req secret=default/test-sts-secret time="2023-06-12T13:40:45Z" level=info msg="operator set to disabled / manual mode" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:45Z" level=info msg="syncing credentials request" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:45Z" level=info msg="timed token access cluster detected: true, so not trying to provision with root secret" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req secret=default/test-sts-secret time="2023-06-12T13:40:45Z" level=info msg="actuator detected STS enabled cluster making secret" actuator=aws cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:45Z" level=info msg="creating secret" actuator=aws cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:45Z" level=info msg="clusteroperator status updated" controller=status time="2023-06-12T13:40:45Z" level=info msg="reconciling clusteroperator status" time="2023-06-12T13:40:45Z" level=info msg="secret created successfully" actuator=aws cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:40:56Z" level=info msg="calculating metrics for all CredentialsRequests" controller=metrics time="2023-06-12T13:40:56Z" level=info msg="reconcile complete" controller=metrics elapsed=4.220597ms time="2023-06-12T13:41:56Z" level=info msg="reconciling clusteroperator status" time="2023-06-12T13:42:56Z" level=info msg="calculating metrics for all CredentialsRequests" controller=metrics time="2023-06-12T13:42:56Z" level=info msg="reconcile complete" controller=metrics elapsed=4.133929ms time="2023-06-12T13:44:45Z" level=info msg="reconciling clusteroperator status" time="2023-06-12T13:44:45Z" level=info msg="operator set to disabled / manual mode" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:44:45Z" level=info msg="syncing credentials request" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:44:45Z" level=warning msg="no user name set on credentials being deleted, most likely were never provisioned or using passthrough creds" actuator=aws cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:44:45Z" level=info msg="target secret deleted successfully" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req secret=default/test-sts-secret targetSecret=default/test-sts-secret time="2023-06-12T13:44:45Z" level=info msg="actuator deletion complete, removing finalizer" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req secret=default/test-sts-secret time="2023-06-12T13:44:45Z" level=info msg="reconciling clusteroperator status" time="2023-06-12T13:44:45Z" level=info msg="operator set to disabled / manual mode" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:44:45Z" level=info msg="syncing credentials request" controller=credreq cr=openshift-cloud-credential-operator/test-sts-creds-req time="2023-06-12T13:44:45Z" level=info msg="clusteroperator status updated" controller=status time="2023-06-12T13:44:56Z" level=info msg="calculating metrics for all CredentialsRequests" controller=metrics time="2023-06-12T13:44:56Z" level=info msg="reconcile complete" controller=metrics elapsed=3.554297ms time="2023-06-12T13:46:56Z" level=info msg="calculating metrics for all CredentialsRequests" controller=metrics time="2023-06-12T13:46:56Z" level=info msg="reconcile complete" controller=metrics elapsed=4.054413ms time="2023-06-12T13:46:56Z" level=info msg="reconciling clusteroperator status"

So I feel more confident the test is really passing.

bentito · 2023-06-11T07:53:52Z

test/e2e/aws/sts/actutator_e2e_test.go

+
+func onAdd(t *testing.T, cfg envconf.Config, ctx context.Context) func(obj interface{}) {
+	time.Sleep(2 * time.Minute)
+	if err := cfg.Client().Resources().Get(ctx, secretName, namespace, secret); err != nil {


This is because of unsureness about Secret really existing here, despite this being a callback from the watch on the Secret.

The obj interface{} passed to this func is the object that was just added - perhaps use that? 2min wait should not be needed here

bentito · 2023-06-12T14:03:45Z

/assign @abutcher

abutcher

Couple small things. Wondering if some of the new Info logs should be moved to Debug based on their potential noise level but those can be moved or followed up on.

pkg/apis/cloudcredential/v1/types_aws.go

pkg/apis/cloudcredential/v1/types_credentialsrequest.go

Makefile

pkg/aws/actuator/actuator.go

pkg/aws/actuator/actuator_test.go

stevekuznetsov · 2023-06-23T13:36:39Z

pkg/operator/platform/platform.go

+}
+
+func computeClusterOperatorVersions() string {
+	currentVersion := os.Getenv("RELEASE_VERSION")


What if this is not set?

You don't get to use the feature gate? As featuregates.NewFeatureGateAccess likely doesn't do much for nil as the desired version.

A string cannot be nil in golang, it will instead be an empty string. So, in such a case it will be looking for the cluster version to be an empty string. In such a case, I expect a lot of other things will also be broken. The good news is RELEASE_VERSION is defined in the 03-deployment.yaml and should always be non-empty.

stevekuznetsov · 2023-06-23T13:37:51Z

pkg/operator/utils/utils.go

+		logger.WithError(err).Error("error loading CCO configuration to determine mode")
+		return false, err
+	}
+	if credentialsMode != "Manual" {


super-nit: is manual not a constant somewhere?

yikes! investigating I find that mode referred to as both "manual" and "Manual" in yamls, docs and code. Yes, there is a constant, that has it as "manual" (1b384cc#diff-f6d93feef6d59ca8ced14b54fe0058d41f14de44450baf6b723cc1c2ec280c54R25) but it doesn't seem to be used everywhere now....

So I'm a little leery to start using the constant version here until CCO team can review a bit more. I'd say it can wait until another day to get better?

GitOperatorConfiguration() returns an operatorv1.CloudCredentialsMode which has a constant CloudCredentialsModeManual = "Manual".

pkg/operator/utils/utils.go

stevekuznetsov · 2023-06-23T13:42:00Z

test/e2e/aws/sts/actutator_e2e_test.go

+
+func onAdd(t *testing.T, cfg envconf.Config, ctx context.Context) func(obj interface{}) {
+	time.Sleep(2 * time.Minute)
+	if err := cfg.Client().Resources().Get(ctx, secretName, namespace, secret); err != nil {


The obj interface{} passed to this func is the object that was just added - perhaps use that? 2min wait should not be needed here

pkg/operator/credentialsrequest/credentialsrequest_controller.go

pkg/operator/platform/platform.go

bentito · 2023-06-23T17:49:14Z

Couple small things. Wondering if some of the new Info logs should be moved to Debug based on their potential noise level but those can be moved or followed up on.

Changed all 3 instances to Debug in d6bb916

pkg/aws/actuator/actuator.go

abutcher · 2023-06-23T20:34:09Z

pkg/operator/credentialsrequest/credentialsrequest_controller.go

-		return reconcile.Result{}, err
+		if !stsDetected {
+			logger.Infof("operator detects STS enabled cluster")
+			return reconcile.Result{}, err


This return short circuits continuing through the controller to create the secret for the CredentialsRequest.

Worth adding a test to the credentials request controller tests to ensure we go through the motions.

It's the end of the day and it's been a long one for me, again, I'm not seeing a good way to create a test on the controller with STS enabled. There might be one, I'm just kind of lost at the moment. Maybe we should add a card to Jira for "Streamline Credentials Request Controller Unit Tests and add STS enabled and feature gate flags"?

abutcher · 2023-06-23T20:41:42Z

pkg/apis/cloudcredential/v1/types_aws.go

+	// The presence of an STSRoleARN within the AWSProviderSpec initiates creation of a secret containing IAM
+	// Role details necessary for assuming the IAM Role via Amazon's Secure Token Service.
+	// +optional
+	// +kubebuilder:validation:Pattern:="^arn:(?P<Partition>[^:\n]*):(?P<Service>[^:\n]*):(?P<Region>[^:\n]*):(?P<AccountID>[^:\n]*):(?P<Ignore>(?P<ResourceType>[^:\/\n]*)[:\/])?(?P<Resource>.*)$"


The role ARN validation is causing CRD generation to fail which for some reason fails silently when ran via make update. The silently failing part is possibly related to the openshift/api workaround.

'_output/tools/bin/controller-gen-v0.2.5' schemapatch:manifests="./manifests" paths="./pkg/apis/cloudcredential/v1" 'output:dir="./manifests"' /home/abutcher/go/src/github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1/types_aws.go:36:2: invalid char escape (at <input>:1:1) /home/abutcher/go/src/github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1/types_aws.go:36:2: invalid char escape (at <input>:1:1) /home/abutcher/go/src/github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1/types_aws.go:36:2: unable to parse string: invalid syntax (at <input>:1:1) /home/abutcher/go/src/github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1/types_aws.go:36:2: invalid char escape (at <input>:1:1) /home/abutcher/go/src/github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1/types_aws.go:36:2: invalid char escape (at <input>:1:1) /home/abutcher/go/src/github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1/types_aws.go:36:2: unable to parse string: invalid syntax (at <input>:1:1) Error: not all generators ran successfully

pkg/aws/actuator/actuator.go

jstuever

I have general concern about the logic defining what is STS enabled vs feature gate enabled (per platform). Otherwise, a few minor nitpicks.

hack/DEV_HOW_TO_OVERRIDE_CREDS_REQ_MGMT_BY_CVO.md

jstuever · 2023-06-27T16:20:18Z

pkg/operator/credentialsrequest/actuator/actuator.go

@@ -46,6 +46,8 @@ type Actuator interface {
 	Upgradeable(operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition
 	// GetCredentialsRootSecret returns the credentials root secret.
 	GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error)
+	// Query STSFeatureGateEnabled.
+	STSFeatureGateEnabled() bool


Nit: I'm a little concerned that feature gate enabled is determined per platform. I would instead expect the STSEnabled to be per platform and featureGateEnabled to simply reflect the status of the feature gate (platform agnostic). I don't think it's worth blocking on, but please make sure the functionality continues to work as expected when the STSFeatureGate bits are eventually removed.

yep, noted, I understand what you're saying, if you elaborate a little bit on your top level comment and make comment on the Jira about removing the FeatureGate that should do it for remembering to deal with this.

jstuever · 2023-06-27T16:36:00Z

pkg/operator/utils/utils.go

+		logger.WithError(err).Error("error loading CCO configuration to determine mode")
+		return false, err
+	}
+	if credentialsMode != "Manual" {


GitOperatorConfiguration() returns an operatorv1.CloudCredentialsMode which has a constant CloudCredentialsModeManual = "Manual".

pkg/operator/utils/utils.go

jstuever · 2023-06-27T17:06:05Z

pkg/operator/platform/platform.go

+	}
+	configInformers := configinformers.NewSharedInformerFactory(clientSet, 10*time.Minute)
+	desiredVersion := computeClusterOperatorVersions()
+	missingVersion := desiredVersion


I believe this should be missingVersion := "0.0.1-snapshot" to match what is in the 03-deployment.yaml

That seems wrong to hard code to a string which might end up different from YAML contents?

Possibly... I just noted the difference from many of the other operators.

cluster-ingress-operator

cloud-network-config-controller

azure-disk-csi-driver-operator

azure-file-csi-driver-operator

cluster-storage-operator

machine-api-provider-azure

I believe what exists today (missingVersion := desiredVersion) will cause it to treat all versions as if they are the missing version, thereby skipping the version check altogether.

Something to keep in mind, the purpose of missingVersion is to handle the situation where the 03-deployment.yaml is applied from locally compiled code, and where it is not going through the process of having a RELEASE_VERSION defined via our release process. Or, in other words, it is explicitly to enable locally compiled operator(s) to function as desired by ignoring the version check itself.

bentito · 2023-06-27T18:44:14Z

I have general concern about the logic defining what is STS enabled vs feature gate enabled (per platform). Otherwise, a few minor nitpicks.

@jstuever Could you elaborate a bit? Even given that we'll probably need to come back to fix the logic later, it would be helpful to put the specifcs here for a reference for that?

One thing I noticed as I try to add a unit test on the STS enabled path in credentialsrequest_controller.go, I am finding it easy to test the FeatureGate enabled part, but hard to fake the STS detected part, because that boolean is hard to reach from the tests.

…s for" This reverts commit 6561e65. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

``` $ make update ``` Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

For whatever reason, when the upstream target calls controller-gen, the tool reads and writes *every* YAML file in the `./manifests` directory, even files that the tool is not writing to during the generator step. We copy our `00-config-custresdef.yaml` from the OpenShift API repo, which uses a bsepoke generator with different formatting on the output. Therefore, when `controller-gen` is run on the YAML we copy from the API repo, it re-formats it and causes the `git diff` check to fail. In this override, we simply copy back in the YAMLs from the API repo after generating, overwriting anything `controller-gen` may have done, so we don't have this spurious failure. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

bentito · 2023-06-27T19:12:42Z

In reply to #542 (comment)
There is another constants file floating around this codebase and it's confusing:

pkg/operator/constants/constants.go has it as "manual"

bentito · 2023-06-27T19:19:42Z

/test e2e-aws-manual-oidc

jstuever · 2023-06-27T20:54:11Z

Responding to #542 (comment)

I agree, the constants file is confusing. I had to dig in to understand how it is used. It looks like they are used solely for keys (string) in a mapping in the metrics operator. The mapping appears to be more of a status use case than a desired configuration. There also appear to be additional status options compared to CloudCredentialsMode type definition. Why the statuses were selected to be lowercase, I do not know.

openshift-ci · 2023-06-27T21:18:54Z

@bentito: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

We spent some time in openshift#511 and again in openshift#542 trying to reconcile CRDs. The problem is that we want to *generate* the CredentialsRequest CRD from code in this repo, but *use* (copy) the CloudCredential CRD from openshift/api, which we vendor. But we invoke controller-gen through build-machinery-go, and it does unexpected things to the latter, which breaks validation. With this commit, we move the CredentialsRequest CRD to a `generated` subdirectory and the CloudCredential CRD to an `imported` subdirectory. This lets us go back to the simpler invocation of bmg's tooling while keeping everything in the shape we expect. One more quirk: Because build-machinery-go starts defining dependency chains for targets like `update`, we need to start defining that dependency chain *before* we import the bmg libs to ensure that we copy/generate CRDs *before* we include them in bindata.

stevekuznetsov · 2023-06-28T20:12:03Z

/lgtm

@abutcher gave his blessing last week, the new e2e are passing, @bentito let's do this!

2uasimojo · 2023-06-28T20:52:11Z

/approve

proxy for @abutcher

openshift-ci · 2023-06-28T20:52:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, bentito, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [2uasimojo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stevekuznetsov · 2023-07-10T17:37:10Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-07-10T17:37:59Z

@stevekuznetsov: #542 failed to apply on top of branch "release-4.13":

Applying: Add & logic - new token CredReq.spec.cred* fields
Applying: Add a timed access token detection capability
Applying: Add test suite and util funcs for detect STS
Applying: Add test - detect STS & new token fields present
Applying: Add e2e AWS STS Secret creation test
Using index info to reconstruct a base tree...
M	Makefile
A	test/e2e/aws/sts/actutator_e2e_test.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): test/e2e/aws/sts/actutator_e2e_test.go deleted in HEAD and modified in Add e2e AWS STS Secret creation test. Version Add e2e AWS STS Secret creation test of test/e2e/aws/sts/actutator_e2e_test.go left in tree.
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Add e2e AWS STS Secret creation test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 19, 2023

bentito force-pushed the CCO-366 branch from 7e52c3f to 813aa1f Compare May 26, 2023 13:53

bentito force-pushed the CCO-366 branch from f2671b5 to 007f928 Compare June 11, 2023 07:49

bentito commented Jun 11, 2023

View reviewed changes

bentito changed the title ~~WIP: CCO-366 Add ability to detect AWS STS and behave accordingly~~ CCO-366 Add ability to detect AWS STS and behave accordingly Jun 11, 2023

bentito marked this pull request as ready for review June 12, 2023 14:01

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2023

openshift-ci bot assigned abutcher Jun 12, 2023

openshift-ci bot requested review from dlom and lleshchi June 12, 2023 14:07

bentito force-pushed the CCO-366 branch from 727f17a to cd1de5e Compare June 22, 2023 13:12

stevekuznetsov mentioned this pull request Jun 22, 2023

OCPBUGS-15365: *: use a filtered LIST + WATCH on Secrets for AWS STS #545

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023

abutcher requested changes Jun 23, 2023

View reviewed changes

pkg/apis/cloudcredential/v1/types_aws.go Outdated Show resolved Hide resolved

pkg/apis/cloudcredential/v1/types_aws.go Outdated Show resolved Hide resolved

pkg/apis/cloudcredential/v1/types_credentialsrequest.go Outdated Show resolved Hide resolved

bentito force-pushed the CCO-366 branch from 62ac186 to 581ae33 Compare June 23, 2023 11:58

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023

stevekuznetsov reviewed Jun 23, 2023

View reviewed changes