CCO-318: Read feature gates for future usage #908

deads2k · 2023-04-11T15:23:35Z

This is needed for the AzureWorkloadIdentity work (see #906), so adding the core featuregate access here.

When we eventually read featuregates from the cluster itself, this golang construct will remain the same.

/cc @abutcher @jstuever
/assign @Miciah

deads2k · 2023-04-12T12:52:21Z

/test all

jstuever · 2023-04-17T22:46:55Z

/retest-required

jstuever · 2023-04-18T17:59:50Z

/lgtm
I was able to use this PR in addition to #906 to read the AzureWorkloadIdentity featureGate and toggle relevant code paths as intended.

Miciah · 2023-04-18T19:25:17Z

/retitle CCO-318: Read feature gates for future usage

openshift-ci-robot · 2023-04-18T19:25:43Z

@deads2k: This pull request references CCO-318 which is a valid jira issue.

In response to this:

This is needed for the AzureWorkloadIdentity work (see #906), so adding the core featuregate access here.

When we eventually read featuregates from the cluster itself, this golang construct will remain the same.

/cc @abutcher @jstuever
/assign @Miciah

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.

jstuever · 2023-04-19T22:43:20Z

/retest-required

deads2k · 2023-04-24T15:07:25Z

/retest

Miciah

Would you mind mentioning the Jira issue in the Git commit message?

Miciah · 2023-04-24T17:02:17Z

manifests/00-cluster-role.yaml

+  - clusterversions
  verbs:
  - get
+  - list
+  - watch


I suppose this permission would be needed in the future for versioned featuregates, but why is it needed today?

I suppose this permission would be needed in the future for versioned featuregates, but why is it needed today?

Because the informer is started to be code compatible with the update comes.

Miciah · 2023-04-24T17:07:30Z

pkg/operator/controller/gatewayapi/controller.go

-	var fg configv1.FeatureGate
-	if err := r.cache.Get(ctx, request.NamespacedName, &fg); err != nil {
-		if apierrors.IsNotFound(err) {
-			log.Info("featuregate not found; reconciliation will be skipped", "request", request)
-			return reconcile.Result{}, nil
-		}
-		return reconcile.Result{}, err
-	}
-
-	if !featureIsEnabled(featureGateName, &fg) {
+	if !r.config.GatewayAPIEnabled {


With this change, there isn't really a good reason for this controller to watch featuregates at all. However, it does make sense to have a control loop to retry creating a CRD when a create fails. In addition, we really do want to have the controller but have it reconcile customresourcedefinitions if they change (see https://issues.redhat.com/browse/NE-1273), so let's leave it as a followup to modify the controller to watch and reconcile customresourcedefinitions.

Miciah · 2023-04-24T17:11:28Z

pkg/operator/controller/gatewayapi/controller.go

-// featureIsEnabled takes a feature name and a featuregate config API object and
-// returns a Boolean indicating whether the named feature is enabled.
-//
-// This function determines whether a named feature is enabled as follows:
-//
-//   - First, if the featuregate's spec.featureGateSelection.featureSet field is
-//     set to "CustomNoUpgrade", then the feature is enabled if, and only if, it
-//     is specified in spec.featureGateSelection.customNoUpgrade.enabled.
-//
-//   - Second, if spec.featureGateSelection.featureSet is set to a value that
-//     isn't defined in configv1.FeatureSets, then the feature is *not* enabled.
-//
-//   - Finally, the feature is enabled if, and only if, the feature is specified
-//     in configv1.FeatureSets[spec.featureGateSelection.featureSet].enabled.


Could this comment be preserved somewhere? It took me a bit of effort to work out from the features API documentation and Slack conversations what the logic is for enabling a feature.

Could this comment be preserved somewhere? It took me a bit of effort to work out from the features API documentation and Slack conversations what the logic is for enabling a feature.

Spoke on slack. With the just-today-merged FeatureGate.Status, this comment is no longer pertinent. We'll update in two stages so ingress isn't a guinea pig, but we won't preserve this comment.

@Miciah I use featureIsEnabled in my E2E tests and can preserve it there.

Miciah · 2023-04-24T17:12:54Z

pkg/operator/operator.go

+	desiredVersion := config.OperatorReleaseVersion
+	missingVersion := statuscontroller.UnknownVersionValue
+
+	// by default, this will exit(0) the process if the featuregates ever change to a different set of values.


I usually ask people to write full sentences in comments.

Suggested change

// by default, this will exit(0) the process if the featuregates ever change to a different set of values.

// By default, this will exit(0) the process if the featuregates ever change to a different set of values.

Miciah · 2023-04-24T17:15:56Z

pkg/operator/operator.go

+		klog.Infof("FeatureGates initialized: enabled=%v  disabled=%v", enabled, disabled)
+	case <-time.After(1 * time.Minute):
+		klog.Errorf("timed out waiting for FeatureGate detection")


Is there a reason to use klog?

Suggested change

klog.Infof("FeatureGates initialized: enabled=%v disabled=%v", enabled, disabled)

case <-time.After(1 * time.Minute):

klog.Errorf("timed out waiting for FeatureGate detection")

log.Infof("FeatureGates initialized", "enabled", enabled, "disabled", disabled)

case <-time.After(1 * time.Minute):

log.Error(nil, "timed out waiting for FeatureGate detection")

deads2k · 2023-04-24T19:39:52Z

comments updated to be full sentences, switch to log, updated commit message.

Miciah · 2023-04-25T21:55:07Z

Looks like you need a go mod tidy to remove the addition of k8s.io/klog/v2 to go.mod etc. that your latest changes obviated.

jstuever

For this to be usable by #906, it will need to be updated to be compatible with a newer openshift/api to include the AzureWorkloadIdentity commit. As it stands now, #906 cannot use the newer api due to the addition of the featureGateName type. As a result, the changes in this PR are no longer compatible.

makes https://issues.redhat.com//browse/CCO-318 easier

deads2k · 2023-04-27T20:16:40Z

For this to be usable by #906, it will need to be updated to be compatible with a newer openshift/api to include the AzureWorkloadIdentity commit. As it stands now, #906 cannot use the newer api due to the addition of the featureGateName type. As a result, the changes in this PR are no longer compatible.

updated. The kcm-o and kas-o have already switched. The code remained the same, but the library-go changed underneath it.

Miciah · 2023-04-27T23:43:34Z

pkg/operator/operator.go

+	}
+	// example of future featuregate read and usage to set a variable to pass to a controller
+	_ = sets.New[configv1.FeatureGateName](enabled...).Has("AzureWorkloadIdentity")
+	gatewayAPIEnabled := sets.New[configv1.FeatureGateName](enabled...).Has("GatewayAPI")


Ooh, generics!

Miciah · 2023-04-27T23:43:53Z

/approve

openshift-ci · 2023-04-27T23:44:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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~~ [Miciah]

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

deads2k · 2023-05-03T15:25:25Z

requires openshift/cluster-config-operator#301

deads2k · 2023-05-03T16:28:22Z

/test all

deads2k · 2023-05-03T19:48:05Z

infra Error: creating EC2 Instance: InsufficientInstanceCapacity: We currently do not have sufficient m6a.xlarge capacity in t

/retest

deads2k · 2023-05-03T22:43:28Z

/retest

jstuever · 2023-05-04T21:06:32Z

/retest

jstuever · 2023-05-04T23:51:41Z

/lgtm
This works as expected for #906

deads2k · 2023-05-08T14:41:18Z

related to the external CCM flow, so labeling so it'll merge.

openshift-ci · 2023-05-08T17:17:54Z

@deads2k: 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 bot assigned Miciah Apr 11, 2023

openshift-ci bot requested review from abutcher and jstuever April 11, 2023 15:23

deads2k force-pushed the use-feature-gates branch from 8c71879 to 771cd17 Compare April 11, 2023 16:42

jstuever mentioned this pull request Apr 11, 2023

CCO-318: Enable Azure Workload Identity authentication. #906

Merged

deads2k force-pushed the use-feature-gates branch from 338cd18 to e51a8ae Compare April 11, 2023 20:45

openshift-ci bot assigned jstuever Apr 18, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2023

openshift-ci bot changed the title ~~read feature gates for future usage~~ CCO-318: Read feature gates for future usage Apr 18, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 18, 2023

jstuever removed their assignment Apr 18, 2023

dlom mentioned this pull request Apr 19, 2023

CCO-346: Add support for Azure workload identity tokens openshift/machine-api-provider-azure#55

Merged

Miciah reviewed Apr 24, 2023

View reviewed changes

deads2k force-pushed the use-feature-gates branch from 62206bb to 1f94fb3 Compare April 24, 2023 19:39

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023

jstuever suggested changes Apr 27, 2023

View reviewed changes

deads2k added 2 commits April 27, 2023 16:09

add library-go featuregate usage functions

50de0ea

update to honor featuregates

624c2dc

makes https://issues.redhat.com//browse/CCO-318 easier

deads2k force-pushed the use-feature-gates branch from 1f94fb3 to dde23cc Compare April 27, 2023 20:14

Miciah reviewed Apr 27, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2023

deads2k mentioned this pull request May 2, 2023

add GatewayAPI featureset openshift/api#1452

Merged

deads2k added 5 commits May 3, 2023 16:16

switch to library-go featuregate evaluation

34dc8bd

expand read permissions to allow standard informers

6085461

generated

9e2ed37

update library-go to get more restrictive featuregate reader

8dab23e

use the featuregate value from openshift/api

3d26626

deads2k force-pushed the use-feature-gates branch from 03ac3d9 to 3d26626 Compare May 3, 2023 20:16

openshift-ci bot assigned jstuever May 4, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2023

jstuever approved these changes May 4, 2023

View reviewed changes

jstuever removed their assignment May 4, 2023

deads2k added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 8, 2023

openshift-merge-robot merged commit 6d9676e into openshift:master May 8, 2023

Miciah mentioned this pull request May 9, 2023

OCPBUGS-13190: Avoid spurious updates for internalTrafficPolicy #927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCO-318: Read feature gates for future usage #908

CCO-318: Read feature gates for future usage #908

deads2k commented Apr 11, 2023

deads2k commented Apr 12, 2023

jstuever commented Apr 17, 2023

jstuever commented Apr 18, 2023 •

edited

Loading

Miciah commented Apr 18, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Apr 18, 2023 •

edited by openshift-ci bot

Loading

jstuever commented Apr 19, 2023

deads2k commented Apr 24, 2023

Miciah left a comment

Miciah Apr 24, 2023

deads2k Apr 24, 2023

Miciah Apr 24, 2023

Miciah Apr 24, 2023

deads2k Apr 24, 2023

candita Apr 25, 2023

Miciah Apr 24, 2023

Miciah Apr 24, 2023

deads2k commented Apr 24, 2023

Miciah commented Apr 25, 2023

jstuever left a comment

deads2k commented Apr 27, 2023

Miciah Apr 27, 2023

Miciah commented Apr 27, 2023

openshift-ci bot commented Apr 27, 2023

deads2k commented May 3, 2023

deads2k commented May 3, 2023

deads2k commented May 3, 2023

deads2k commented May 3, 2023

jstuever commented May 4, 2023

jstuever commented May 4, 2023

deads2k commented May 8, 2023

openshift-ci bot commented May 8, 2023

	// by default, this will exit(0) the process if the featuregates ever change to a different set of values.
	// By default, this will exit(0) the process if the featuregates ever change to a different set of values.

CCO-318: Read feature gates for future usage #908

CCO-318: Read feature gates for future usage #908

Conversation

deads2k commented Apr 11, 2023

deads2k commented Apr 12, 2023

jstuever commented Apr 17, 2023

jstuever commented Apr 18, 2023 • edited Loading

Miciah commented Apr 18, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Apr 18, 2023 • edited by openshift-ci bot Loading

jstuever commented Apr 19, 2023

deads2k commented Apr 24, 2023

Miciah left a 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

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

deads2k commented Apr 24, 2023

Miciah commented Apr 25, 2023

jstuever left a comment

Choose a reason for hiding this comment

deads2k commented Apr 27, 2023

Choose a reason for hiding this comment

Miciah commented Apr 27, 2023

openshift-ci bot commented Apr 27, 2023

deads2k commented May 3, 2023

deads2k commented May 3, 2023

deads2k commented May 3, 2023

deads2k commented May 3, 2023

jstuever commented May 4, 2023

jstuever commented May 4, 2023

deads2k commented May 8, 2023

openshift-ci bot commented May 8, 2023

jstuever commented Apr 18, 2023 •

edited

Loading

Miciah commented Apr 18, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Apr 18, 2023 •

edited by openshift-ci bot

Loading