Bug 2035903: handle resources with feature-gate annotation #444

joelddiaz · 2022-01-17T22:40:58Z

First commit disables processing of resources with feature-gate annotation.

Second commit adds new --enable-tech-preview parameter which allows tech-preview-annotated resources through. Adds several test cases to cover it.

openshift-ci · 2022-01-17T22:41:54Z

@joelddiaz: This pull request references Bugzilla bug 2035903, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @lwan-wanglin

In response to this:

Bug 2035903: ignore resources with feature-gate annotation

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.

joelddiaz · 2022-01-17T22:44:28Z

/assign @akhil-rane
FWIW, we'll ignore-by-default (given the present uncertainty with how to future-proof our CLI parameters), but someone really motivated will always be able to remove the annotation before asking ccoctl to process the extracted CredentialsRequests.

codecov · 2022-01-17T23:01:18Z

Codecov Report

Merging #444 (4a2c157) into master (238bd0e) will decrease coverage by 0.04%.
The diff coverage is 43.90%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage   45.79%   45.74%   -0.05%     
==========================================
  Files          92       92              
  Lines        9314     9336      +22     
==========================================
+ Hits         4265     4271       +6     
- Misses       4541     4556      +15     
- Partials      508      509       +1

Impacted Files	Coverage Δ
pkg/cmd/provisioning/alibabacloud/alibaba.go	`0.00% <ø> (ø)`
pkg/cmd/provisioning/aws/aws.go	`0.00% <ø> (ø)`
pkg/cmd/provisioning/aws/create_all.go	`0.00% <0.00%> (ø)`
pkg/cmd/provisioning/gcp/create_all.go	`0.00% <0.00%> (ø)`
pkg/cmd/provisioning/gcp/gcp.go	`0.00% <ø> (ø)`
pkg/cmd/provisioning/ibmcloud/ibmcloud.go	`0.00% <ø> (ø)`
...kg/cmd/provisioning/gcp/create_service_accounts.go	`52.97% <33.33%> (-0.88%)`	⬇️
.../cmd/provisioning/alibabacloud/create-ram-users.go	`38.58% <40.00%> (-0.43%)`	⬇️
pkg/cmd/provisioning/aws/create-iam-roles.go	`61.32% <40.00%> (-0.59%)`	⬇️
pkg/cmd/provisioning/ibmcloud/create_service_id.go	`43.75% <50.00%> (-0.56%)`	⬇️
... and 4 more

wking · 2022-01-17T23:29:08Z

pkg/cmd/provisioning/constants.go

+	// featureGateAnnotation is the annotation used to indicate that a specific manifest is hidden behind a feature gate.
+	featureGateAnnotation = "release.openshift.io/feature-gate"
+	// validTechPreviewAnnotationValue is the only valid value for the feature-gate annoation
+	validTechPreviewAnnotationValue = "TechPreviewNoUpgrade"


You can use configv1.TechPreviewNoUpgrade instead of defining your own variable for this.

cloud-credential-operator $ git --no-pager grep TechPreviewNoUpgrade vendor/github.com/openshift/api/config/v1/types_feature.go: // TechPreviewNoUpgrade turns on tech preview features that are not part of the normal supported platform. Turning vendor/github.com/openshift/api/config/v1/types_feature.go: TechPreviewNoUpgrade FeatureSet = "TechPreviewNoUpgrade" vendor/github.com/openshift/api/config/v1/types_feature.go: TechPreviewNoUpgrade: newDefaultFeatures().

lwan-wanglin · 2022-01-18T04:53:53Z

@joelddiaz Whether there is a way to not extract such CredentialsRequests when running oc adm release extract --credentials-requests ? otherwise we need to document such case, we support both manual and manual+sts mode, if we only fix ccoctl, we need to tell the customer don't create secret manifests for such CRs when cco in manual mode, am I right?

JoelSpeed · 2022-01-18T09:42:06Z

pkg/cmd/provisioning/utils.go

+					log.Printf("CredentialsRequests %s/%s has unexpected feature-gate value", cr.Namespace, cr.Name)
+
+				}
+				log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview annotation", cr.Namespace, cr.Name)


This will make it impossible for users to bootstrap a cluster as a TechPreviewNoUpgrade cluster on certain platforms right? I think this will break a number of release informing jobs and QE scenarios as we, as an org, decided a while back, bootstrapping as a TPNU cluster should be supported.

We can just take a new CLI parameter to opt into processing these annotated manifests.

openshift-ci · 2022-01-19T13:10:01Z

@joelddiaz: This pull request references Bugzilla bug 2035903, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @lwan-wanglin

In response to this:

Bug 2035903: handle resources with feature-gate annotation

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.

akhil-rane · 2022-01-19T16:19:18Z

pkg/cmd/provisioning/utils.go

 			}
+
+			// Ingore CredentialsRequest with the feature-gate annotation


Suggested change

// Ingore CredentialsRequest with the feature-gate annotation

// Ignore CredentialsRequest with the feature-gate annotation

akhil-rane · 2022-01-19T16:28:09Z

pkg/cmd/provisioning/ibmcloud/delete_service_id.go

@@ -59,7 +59,8 @@ func deleteServiceIDCmd(cmd *cobra.Command, args []string) error {

 func deleteServiceIDs(client ibmcloud.Client, accountID, name, credReqDir string, force bool) error {
 	// Process directory
-	credReqs, err := provisioning.GetListOfCredentialsRequests(credReqDir)
+	// (always tech-preview==true because we should do a full cleanup to be on the safe side)


why brackets () ?

Suggested change

// (always tech-preview==true because we should do a full cleanup to be on the safe side)

// always tech-preview==true because we should do a full cleanup to be on the safe side

akhil-rane · 2022-01-19T16:35:46Z

pkg/cmd/provisioning/utils_test.go

+		if credreq.Annotations == nil {
+			credreq.Annotations = map[string]string{}
+		}
+		credreq.Annotations[featureGateAnnotation] = "TechPreviewNoUpgrade"


Suggested change

credreq.Annotations[featureGateAnnotation] = "TechPreviewNoUpgrade"

credreq.Annotations[featureGateAnnotation] = configv1.TechPreviewNoUpgrade

akhil-rane · 2022-01-19T16:49:29Z

pkg/cmd/provisioning/utils.go

+			if value, ok := cr.Annotations[featureGateAnnotation]; ok {
+				if !enableTechPreview {
+					log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview annotation", cr.Namespace, cr.Name)
+					continue
+				}
+				if value != string(configv1.TechPreviewNoUpgrade) {
+					log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview value %s", cr.Namespace, cr.Name, value)
+					continue
+				} // else allow it to be added it to the list of CredReqs to process
+			}
+
+			credRequests = append(credRequests, cr)


Do we ignore a CredentialsRequest if featureGateAnnotation value is not equal to TechPreviewNoUpgrade?

That's how I wrote it up. That is supposed to be an invalid value for the annotation and undefined. So just log it and move on...

akhil-rane · 2022-01-19T16:54:24Z

pkg/cmd/provisioning/utils.go

+			// Handle CredentialsRequest with the feature-gate annotation
+			if value, ok := cr.Annotations[featureGateAnnotation]; ok {
+				if !enableTechPreview {
+					log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview annotation", cr.Namespace, cr.Name)
+					continue
+				}
+				if value != string(configv1.TechPreviewNoUpgrade) {
+					log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview value %s", cr.Namespace, cr.Name, value)
+					continue
+				} // else allow it to be added it to the list of CredReqs to process
+			}


IIUC, first we should check if featureGateAnnotation value is equal to TechPreviewNoUpgrade and then check if tech preview is enabled?

Suggested change

// Handle CredentialsRequest with the feature-gate annotation

if value, ok := cr.Annotations[featureGateAnnotation]; ok {

if !enableTechPreview {

log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview annotation", cr.Namespace, cr.Name)

continue

}

if value != string(configv1.TechPreviewNoUpgrade) {

log.Printf("Ignoring CredentialsRequest %s/%s with tech-preview value %s", cr.Namespace, cr.Name, value)

continue

} // else allow it to be added it to the list of CredReqs to process

}

// Handle CredentialsRequest with the feature-gate annotation

if value, ok := cr.Annotations[featureGateAnnotation]; ok {

if value != string(configv1.TechPreviewNoUpgrade) {

log.Printf("Ignoring CredentialsRequest %s/%s with release.openshift.io/feature-gate annotation value %s", cr.Namespace, cr.Name, value)

continue

}

if !enableTechPreview {

log.Printf("Ignoring CredentialsRequest %s/%s with release.openshift.io/feature-gate annotation value %s", cr.Namespace, cr.Name, configv1.TechPreviewNoUpgrade)

continue

}

// else allow it to be added to the list of CredentialsRequests to process

}

Cannot assume that a feature is enabled, and should not process them by default. We can revisit properly handling them going forward, but for now keep ccoctl from processing optional resources by default.

joelddiaz · 2022-01-19T21:10:45Z

/test e2e-aws-manual-oidc-sts

openshift-ci · 2022-01-19T21:10:46Z

@joelddiaz: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test coverage
/test e2e-aws
/test e2e-upgrade
/test images
/test unit
/test verify
/test verify-deps

The following commands are available to trigger optional jobs:

/test e2e-aws-manual-oidc
/test e2e-azure
/test e2e-azure-upgrade
/test e2e-gcp
/test e2e-gcp-manual-oidc
/test e2e-openstack
/test e2e-openstack-parallel

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-cloud-credential-operator-master-coverage
pull-ci-openshift-cloud-credential-operator-master-e2e-aws
pull-ci-openshift-cloud-credential-operator-master-e2e-upgrade
pull-ci-openshift-cloud-credential-operator-master-images
pull-ci-openshift-cloud-credential-operator-master-unit
pull-ci-openshift-cloud-credential-operator-master-verify
pull-ci-openshift-cloud-credential-operator-master-verify-deps

In response to this:

/test e2e-aws-manual-oidc-sts

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.

joelddiaz · 2022-01-19T21:11:12Z

/test e2e-aws-manual-oidc

joelddiaz · 2022-01-19T21:30:25Z

/retest

akhil-rane · 2022-01-20T20:18:43Z

/retest

lwan-wanglin · 2022-01-21T09:29:15Z

pkg/cmd/provisioning/aws/create_all.go

@@ -103,6 +104,7 @@ func NewCreateAllCmd() *cobra.Command {
 	createAllCmd.PersistentFlags().StringVar(&CreateAllOpts.CredRequestDir, "credentials-requests-dir", "", "Directory containing files of CredentialsRequests to create IAM Roles for (can be created by running 'oc adm release extract --credentials-requests --cloud=aws' against an OpenShift release image)")
 	createAllCmd.MarkPersistentFlagRequired("credentials-requests-dir")
 	createAllCmd.PersistentFlags().StringVar(&CreateAllOpts.TargetDir, "output-dir", "", "Directory to place generated files (defaults to current directory)")
+	createAllCmd.PersistentFlags().BoolVar(&CreateAllOpts.EnableTechPreview, "--enable-tech-preview", false, "Opt into processing CredentialsRequests marked as tech-preview")


@joelddiaz "--enable-tech-preview" unnecessary --

and here is testcases for the PR https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitems?query=trello%3AOCPBUGSM%5C-38879, please help to review, thanks

Yes, the -- is unnecessary. Pushed an update to the PR. Thanks!

This new parameter will allow ccoctl to either include or exclude any CredentialsRequest that is is processing based on the presence/value of the tech-preview annotation. Add some sanity checking that will ignore a CredReq with the tech-preview annotation, but with an unexpected value. Add test cases to cover the GetListOfCredentialsRequests function, and introduce a new test pattern for building resources that are needed for testing.

akhil-rane

/lgtm

openshift-ci · 2022-01-21T16:02:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akhil-rane, joelddiaz

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~~ [akhil-rane,joelddiaz]

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

openshift-bot · 2022-01-21T18:41:34Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-21T20:29:37Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-01-21T20:47:54Z

@joelddiaz: 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.

openshift-ci · 2022-01-21T20:49:32Z

@joelddiaz: All pull requests linked via external trackers have merged:

openshift/cloud-credential-operator#444

Bugzilla bug 2035903 has been moved to the MODIFIED state.

In response to this:

Bug 2035903: handle resources with feature-gate annotation

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 requested review from akhil-rane and suhanime January 17, 2022 22:41

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2022

joelddiaz changed the title ~~ignore resources with feature-gate annotation~~ Bug 2035903: ignore resources with feature-gate annotation Jan 17, 2022

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 17, 2022

openshift-ci bot requested a review from lwan-wanglin January 17, 2022 22:41

openshift-ci bot assigned akhil-rane Jan 17, 2022

wking reviewed Jan 17, 2022

View reviewed changes

JoelSpeed reviewed Jan 18, 2022

View reviewed changes

joelddiaz force-pushed the ignore-tech-preview branch from a3ee7be to 098d68d Compare January 19, 2022 13:08

joelddiaz changed the title ~~Bug 2035903: ignore resources with feature-gate annotation~~ Bug 2035903: handle resources with feature-gate annotation Jan 19, 2022

joelddiaz force-pushed the ignore-tech-preview branch from 098d68d to 408bf05 Compare January 19, 2022 13:26

akhil-rane reviewed Jan 19, 2022

View reviewed changes

ignore resources with feature-gate annotation

c898af2

Cannot assume that a feature is enabled, and should not process them by default. We can revisit properly handling them going forward, but for now keep ccoctl from processing optional resources by default.

joelddiaz force-pushed the ignore-tech-preview branch from 408bf05 to 88f718f Compare January 19, 2022 19:06

lwan-wanglin reviewed Jan 21, 2022

View reviewed changes

joelddiaz force-pushed the ignore-tech-preview branch from 88f718f to 4a2c157 Compare January 21, 2022 13:58

akhil-rane reviewed Jan 21, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2022

openshift-merge-robot merged commit 85a406b into openshift:master Jan 21, 2022

wking mentioned this pull request Mar 7, 2022

BZ2035903: Manually creating IAM instructions for tech preview CRs openshift/openshift-docs#42921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2035903: handle resources with feature-gate annotation #444

Bug 2035903: handle resources with feature-gate annotation #444

joelddiaz commented Jan 17, 2022 •

edited

Loading

openshift-ci bot commented Jan 17, 2022

joelddiaz commented Jan 17, 2022

codecov bot commented Jan 17, 2022 •

edited

Loading

wking Jan 17, 2022

lwan-wanglin commented Jan 18, 2022

JoelSpeed Jan 18, 2022

joelddiaz Jan 18, 2022

openshift-ci bot commented Jan 19, 2022

akhil-rane Jan 19, 2022

akhil-rane Jan 19, 2022

akhil-rane Jan 19, 2022

akhil-rane Jan 19, 2022

joelddiaz Jan 19, 2022

akhil-rane Jan 19, 2022

joelddiaz commented Jan 19, 2022

openshift-ci bot commented Jan 19, 2022

joelddiaz commented Jan 19, 2022

joelddiaz commented Jan 19, 2022

akhil-rane commented Jan 20, 2022

lwan-wanglin Jan 21, 2022

lwan-wanglin Jan 21, 2022

joelddiaz Jan 21, 2022

akhil-rane left a comment

openshift-ci bot commented Jan 21, 2022

openshift-bot commented Jan 21, 2022

openshift-bot commented Jan 21, 2022

openshift-ci bot commented Jan 21, 2022

openshift-ci bot commented Jan 21, 2022

		}

		// Ingore CredentialsRequest with the feature-gate annotation

	// Ingore CredentialsRequest with the feature-gate annotation
	// Ignore CredentialsRequest with the feature-gate annotation

	// (always tech-preview==true because we should do a full cleanup to be on the safe side)
	// always tech-preview==true because we should do a full cleanup to be on the safe side

	credreq.Annotations[featureGateAnnotation] = "TechPreviewNoUpgrade"
	credreq.Annotations[featureGateAnnotation] = configv1.TechPreviewNoUpgrade

Bug 2035903: handle resources with feature-gate annotation #444

Bug 2035903: handle resources with feature-gate annotation #444

Conversation

joelddiaz commented Jan 17, 2022 • edited Loading

openshift-ci bot commented Jan 17, 2022

joelddiaz commented Jan 17, 2022

codecov bot commented Jan 17, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lwan-wanglin commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelddiaz commented Jan 19, 2022

openshift-ci bot commented Jan 19, 2022

joelddiaz commented Jan 19, 2022

joelddiaz commented Jan 19, 2022

akhil-rane commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhil-rane left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 21, 2022

openshift-bot commented Jan 21, 2022

openshift-bot commented Jan 21, 2022

openshift-ci bot commented Jan 21, 2022

openshift-ci bot commented Jan 21, 2022

joelddiaz commented Jan 17, 2022 •

edited

Loading

codecov bot commented Jan 17, 2022 •

edited

Loading