Added EnableIPTargetType feature gate to controller #9

thejasn · 2022-04-07T13:01:01Z

Signed-off-by: thejasn [email protected]

Issue

Description

Added a controller flag to toggle AWS VPC CNI dependent features. Currently, can be used to enforce the correct target-type annotation on Ingress type resources.

-->

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

arjunrn · 2022-04-07T14:27:11Z

/assign @alebedev87
/lgtm
/retest

arjunrn · 2022-04-07T16:09:43Z

/retest

alebedev87 · 2022-04-07T20:10:39Z

pkg/config/feature_gates.go

@@ -14,6 +14,7 @@ const (
 	WeightedTargetGroups        Feature = "WeightedTargetGroups"
 	ServiceTypeLoadBalancerOnly Feature = "ServiceTypeLoadBalancerOnly"
 	EndpointsFailOpen           Feature = "EndpointsFailOpen"
+	DisableAWSVpcCNI            Feature = "DisableAWSVpcCNI"


Doesn't it sound a little misleading? Like if ALBC would have a power to disable the CNI.
Also, it mentions the CNI plugin name, I didn't see any other CNI plugins which would have the directly accessible POD IPs but maybe there are some in the wild (or will come).

How about EnableIPTargetType similar the service controller feature gate from another PR?

I agree the name sounds misleading. Basically, we wanted to introduce a flag that toggles all features dependent on AWS VPC CNI, currently it's just ip target-type. How about AWSVpcCNIAvailable?

I think EnableIPTargetType is a more appropriate name. That fact that it is dependent on the VPC CNI being available is an internal detail.

alebedev87 · 2022-04-07T20:25:17Z

pkg/ingress/model_builder.go

@@ -40,7 +40,8 @@ func NewDefaultModelBuilder(k8sClient client.Client, eventRecorder record.EventR
 	authConfigBuilder AuthConfigBuilder, enhancedBackendBuilder EnhancedBackendBuilder,
 	trackingProvider tracking.Provider, elbv2TaggingManager elbv2deploy.TaggingManager,
 	vpcID string, clusterName string, defaultTags map[string]string, externalManagedTags []string, defaultSSLPolicy string,
-	backendSGProvider networkingpkg.BackendSGProvider, enableBackendSG bool, disableRestrictedSGRules bool, logger logr.Logger) *defaultModelBuilder {
+	backendSGProvider networkingpkg.BackendSGProvider, enableBackendSG bool, disableRestrictedSGRules bool, disableAWSVpcCNIflag bool,
+	logger logr.Logger) *defaultModelBuilder {


I don't see the change in the ingress group reconcile which is supposed to pass the boolean from the feature gate.

Fixed, had missed that commit.

alebedev87 · 2022-04-07T20:30:17Z

pkg/ingress/model_build_target_group.go

@@ -214,6 +214,9 @@ func (t *defaultModelBuildTask) buildTargetGroupTargetType(_ context.Context, sv
 	case string(elbv2model.TargetTypeInstance):
 		return elbv2model.TargetTypeInstance, nil
 	case string(elbv2model.TargetTypeIP):
+		if t.disableAWSVpcCNIFlag {


Why the feature gate is only applicable for Ingresses? The service type can also have IP target type.

This was a miss from my side, fixed it.

arjunrn · 2022-04-13T07:57:42Z

pkg/ingress/model_builder_test.go

+					},
+				},
+			},
+			wantErr: errors.New("ingress: ns-1/ing-1: unsupported targetType: ip when enableIPTargetType is false"),


Suggested change

wantErr: errors.New("ingress: ns-1/ing-1: unsupported targetType: ip when enableIPTargetType is false"),

wantErr: errors.New("ingress: ns-1/ing-1: unsupported targetType: ip when EnableIPTargetType is false"),

arjunrn · 2022-04-13T07:58:05Z

pkg/service/model_build_target_group.go

@@ -339,6 +339,9 @@ func (t *defaultModelBuildTask) buildTargetType(_ context.Context, port corev1.S
 	var lbTargetType string
 	lbTargetType = string(t.defaultTargetType)
 	_ = t.annotationParser.ParseStringAnnotation(annotations.SvcLBSuffixTargetType, &lbTargetType, t.service.Annotations)
+	if lbTargetType == LoadBalancerTargetTypeIP && !t.enableIPTargetType {
+		return "", errors.Errorf("unsupported targetType: %v when enableIPTargetType is %v", lbTargetType, t.enableIPTargetType)


Suggested change

return "", errors.Errorf("unsupported targetType: %v when enableIPTargetType is %v", lbTargetType, t.enableIPTargetType)

return "", errors.Errorf("unsupported targetType: %v when EnableIPTargetType is %v", lbTargetType, t.enableIPTargetType)

review changes and tests Signed-off-by: thejasn <[email protected]>

alebedev87 · 2022-04-21T12:35:44Z

/lgtm

alebedev87 · 2022-04-22T07:05:15Z

/approve

openshift-ci · 2022-04-22T07:06:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87, thejasn

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:

~~DOWNSTREAM_OWNERS~~ [alebedev87,thejasn]

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

thejasn · 2022-04-22T07:06:35Z

/label docs-approved
/label qe-approved
/label px-approved

openshift-ci · 2022-04-22T07:13:00Z

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

thejasn changed the title ~~added DisableAWSVpcCNI feature gate to controller~~ Added DisableAWSVpcCNI feature gate to controller Apr 7, 2022

openshift-ci bot requested review from arjunrn and Miciah April 7, 2022 13:01

thejasn force-pushed the ocp/feature/toggle-vpc-cni-options branch from 403bdb1 to 53d630b Compare April 7, 2022 14:06

openshift-ci bot assigned alebedev87 and arjunrn Apr 7, 2022

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

alebedev87 reviewed Apr 7, 2022

View reviewed changes

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Apr 9, 2022

thejasn force-pushed the ocp/feature/toggle-vpc-cni-options branch from 9aa7f82 to 8cfec35 Compare April 11, 2022 05:15

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2022

thejasn force-pushed the ocp/feature/toggle-vpc-cni-options branch 3 times, most recently from 8c03fb8 to 8038108 Compare April 13, 2022 07:48

arjunrn reviewed Apr 13, 2022

View reviewed changes

thejasn changed the title ~~Added DisableAWSVpcCNI feature gate to controller~~ Added EnableIPTargetType feature gate to controller Apr 13, 2022

UPSTREAM: 2587: added EnableIPTargetType feature to controller

be3bcd3

review changes and tests Signed-off-by: thejasn <[email protected]>

thejasn force-pushed the ocp/feature/toggle-vpc-cni-options branch from 8038108 to be3bcd3 Compare April 13, 2022 10:10

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

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

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR qe-approved Signifies that QE has signed off on this PR labels Apr 22, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Apr 22, 2022

openshift-merge-robot merged commit d943007 into openshift:main Apr 22, 2022

	wantErr: errors.New("ingress: ns-1/ing-1: unsupported targetType: ip when enableIPTargetType is false"),
	wantErr: errors.New("ingress: ns-1/ing-1: unsupported targetType: ip when EnableIPTargetType is false"),

	return "", errors.Errorf("unsupported targetType: %v when enableIPTargetType is %v", lbTargetType, t.enableIPTargetType)
	return "", errors.Errorf("unsupported targetType: %v when EnableIPTargetType is %v", lbTargetType, t.enableIPTargetType)

Added EnableIPTargetType feature gate to controller #9

Added EnableIPTargetType feature gate to controller #9

Conversation

thejasn commented Apr 7, 2022

Issue

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

arjunrn commented Apr 7, 2022

Uh oh!

arjunrn commented Apr 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alebedev87 commented Apr 21, 2022

Uh oh!

alebedev87 commented Apr 22, 2022

Uh oh!

openshift-ci bot commented Apr 22, 2022

Uh oh!

thejasn commented Apr 22, 2022

Uh oh!

openshift-ci bot commented Apr 22, 2022

Uh oh!

Uh oh!