Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NE-1674 : Adds logic to add service annotation service.beta.kubernetes.io/aws-load-balancer-eip-allocations #1034

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

NE-1674 : Adds logic to add service annotation service.beta.kubernetes.io/aws-load-balancer-eip-allocations #1034

wants to merge 3 commits into from

Conversation

miheer
Copy link
Contributor

@miheer miheer commented Mar 24, 2024 •
edited

Adds logic to add service annotation service.beta.kubernetes.io/aws-load-balancer-eip-allocations to the load balancer service.

@openshift-ci openshift-ci bot requested review from gcs278 and knobunc March 24, 2024 03:39
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 24, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from miheer. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@miheer miheer force-pushed the eip branch 9 times, most recently from 5c0713c to a2dde18 Compare April 6, 2024 11:08
@miheer miheer force-pushed the eip branch 4 times, most recently from 8fa722a to 75b4632 Compare April 11, 2024 06:44
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2024
@miheer miheer force-pushed the eip branch 5 times, most recently from 3b44f35 to 3050641 Compare April 18, 2024 09:31
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 18, 2024
@miheer miheer force-pushed the eip branch 5 times, most recently from 2b3360d to 9eec6ac Compare April 21, 2024 02:44
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2024
@miheer miheer force-pushed the eip branch 6 times, most recently from 39a2c26 to e19d15f Compare April 25, 2024 08:36
@lihongan
Copy link
Contributor

The e2e "TestInvalidEIP" should be skipped on platforms other than AWS, right? seems it caused the job "e2e-azure-operator", "e2e-gcp-operator" failed.

lihongan
lihongan reviewed Apr 26, 2024
View reviewed changes
manifests/00-custom-resource-definition.yaml
@@ -280,7 +281,7 @@ spec:
format: duration
type: string
type: object
networkLoadBalancer:
nlbParameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see properties:eipAllocations: in the API PR, should we add same here?

Copy link
Contributor Author

@miheer miheer Apr 28, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally we had this but after the feature gate changes in the API, that file is no longer present so I modified the hack to have

# Can't rely on associative arrays for old Bash versions (e.g. OSX)
install_crd \
  "vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml" \
  "manifests/00-custom-resource-definition.yaml"

However the vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml does not have eipAllocations in it which might because I added featuregates on it.
Links:

In the 0000_50_ingress_00_ingresscontrollers-Default.crd.yaml file I don't see eipAllocations being added.

misalunk@misalunk-mac cluster-ingress-operator % cat vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml  | grep eipAllocation 
misalunk@misalunk-mac cluster-ingress-operator %

However, in the following files I do see them getting added.

misalunk@misalunk-mac cluster-ingress-operator % cat vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yaml  | grep eipAllocation
                                  eipAllocations:
                                  eipAllocations:
misalunk@misalunk-mac cluster-ingress-operator % 

misalunk@misalunk-mac cluster-ingress-operator % cat vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yaml | grep eipAllocation
                                  eipAllocations:
                                  eipAllocations:
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % cat vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml | grep eipAllocation
                                  eipAllocations:
                                  eipAllocations:
misalunk@misalunk-mac cluster-ingress-operator %

@Miciah do you think we shall change the filename present in update-generated-crd.sh to any one of the above 3 files which have eipAllocations available ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe update-generated-crd.sh needs to copy all CRD variants from vendor/ to manifests/. CVO should figure out which one of the variants to use based on their respective release.openshift.io/feature-set annotations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please note that you cannot change the name of a field that already exists in shipped releases, as I noted here: openshift/api#1826 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference is the between vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml . and the above mentioned 3 files is that it does not. have eipAllocations as mentioned in the following:

misalunk@misalunk-mac cluster-ingress-operator % diff vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml 
10c10
<     release.openshift.io/feature-set: CustomNoUpgrade
---
>     release.openshift.io/feature-set: Default
288,300d287
<                                 properties:
<                                   eipAllocations:
<                                     description: 'You can assign Elastic IP addresses
<                                       to the Network Load Balancer by adding the following
<                                       annotation. The number of Allocation IDs must
<                                       match the number of subnets that are used for
<                                       the load balancer. service.beta.kubernetes.io/aws-load-balancer-eip-allocations:
<                                       eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy
<                                       https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html
<                                       https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations'
<                                     items:
<                                       type: string
<                                     type: array
1945,1957d1931
<                                 properties:
<                                   eipAllocations:
<                                     description: 'You can assign Elastic IP addresses
<                                       to the Network Load Balancer by adding the following
<                                       annotation. The number of Allocation IDs must
<                                       match the number of subnets that are used for
<                                       the load balancer. service.beta.kubernetes.io/aws-load-balancer-eip-allocations:
<                                       eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy
<                                       https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html
<                                       https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations'
<                                     items:
<                                       type: string
<                                     type: array
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % diff vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade  vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml
diff: vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade: No such file or directory
misalunk@misalunk-mac cluster-ingress-operator % diff vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yaml  vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml  
10c10
<     release.openshift.io/feature-set: TechPreviewNoUpgrade
---
>     release.openshift.io/feature-set: Default
288,300d287
<                                 properties:
<                                   eipAllocations:
<                                     description: 'You can assign Elastic IP addresses
<                                       to the Network Load Balancer by adding the following
<                                       annotation. The number of Allocation IDs must
<                                       match the number of subnets that are used for
<                                       the load balancer. service.beta.kubernetes.io/aws-load-balancer-eip-allocations:
<                                       eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy
<                                       https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html
<                                       https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations'
<                                     items:
<                                       type: string
<                                     type: array
1945,1957d1931
<                                 properties:
<                                   eipAllocations:
<                                     description: 'You can assign Elastic IP addresses
<                                       to the Network Load Balancer by adding the following
<                                       annotation. The number of Allocation IDs must
<                                       match the number of subnets that are used for
<                                       the load balancer. service.beta.kubernetes.io/aws-load-balancer-eip-allocations:
<                                       eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy
<                                       https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html
<                                       https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations'
<                                     items:
<                                       type: string
<                                     type: array
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % 
misalunk@misalunk-mac cluster-ingress-operator % diff vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yaml  vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml  
10c10
<     release.openshift.io/feature-set: DevPreviewNoUpgrade
---
>     release.openshift.io/feature-set: Default
288,300d287
<                                 properties:
<                                   eipAllocations:
<                                     description: 'You can assign Elastic IP addresses
<                                       to the Network Load Balancer by adding the following
<                                       annotation. The number of Allocation IDs must
<                                       match the number of subnets that are used for
<                                       the load balancer. service.beta.kubernetes.io/aws-load-balancer-eip-allocations:
<                                       eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy
<                                       https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html
<                                       https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations'
<                                     items:
<                                       type: string
<                                     type: array
1945,1957d1931
<                                 properties:
<                                   eipAllocations:
<                                     description: 'You can assign Elastic IP addresses
<                                       to the Network Load Balancer by adding the following
<                                       annotation. The number of Allocation IDs must
<                                       match the number of subnets that are used for
<                                       the load balancer. service.beta.kubernetes.io/aws-load-balancer-eip-allocations:
<                                       eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy
<                                       https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/elastic-ip-addresses-eip.html
<                                       https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/guide/service/annotations/#eip-allocations'
<                                     items:
<                                       type: string
<                                     type: array
misalunk@misalunk-mac cluster-ingress-operator %

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or i think best is to add configv1.Default to openshift/api@24253ce#diff-a8b6135d50534471326ea7bcd20e0f5eae25353f7788338060f718128a6a0b34R78

@miheer miheer force-pushed the eip branch 6 times, most recently from 95cce44 to 9197dea Compare April 29, 2024 10:17
lihongan
lihongan reviewed Apr 30, 2024
View reviewed changes
pkg/operator/controller/ingress/load_balancer_service.go
var eipAllocationsOfTypeString []string
if ci.Spec.EndpointPublishingStrategy != nil {
lbSpec := ci.Spec.EndpointPublishingStrategy.LoadBalancer
if lbSpec.ProviderParameters.AWS.NetworkLoadBalancerParameters != nil || lbSpec.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations != nil || len(lbSpec.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if NetworkLoadBalancerParameters != nil then need to check if NetworkLoadBalancerParameters.EIPAllocations != nil right?

I see panic might be related here

2024-04-30T03:12:04.794Z	INFO	operator.init	runtime/panic.go:914	Observed a panic in reconciler: runtime error: invalid memory address or nil pointer dereference	{"controller": "ingress_controller", "object": {"name":"default","namespace":"openshift-ingress-operator"}, "namespace": "openshift-ingress-operator", "name": "default", "reconcileID": "4f2dc3b1-abb0-483a-9dd5-e7f21c381a54"}
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x18f614d]

goroutine 1413 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	/ingress-operator/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:116 +0x1e5
panic({0x25e1100?, 0x4609bb0?})
	/usr/lib/golang/src/runtime/panic.go:914 +0x21f
github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress.desiredLoadBalancerService(0xc0014c8600, {{0x2a3cab8, 0x7}, {0x2a42282, 0xa}, {0xc000ae7fd0, 0xe}, {0xc0017fe060, 0x24}, 0xc0011107ea, ...}, ...)
	/ingress-operator/pkg/operator/controller/ingress/load_balancer_service.go:422 +0x9ad

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lihongan I don't see these nil errors. Which test are you seeing this ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just set spec.endpointPublishingStrategy as below (note: do not set nlbParameters: {})

spec:
  endpointPublishingStrategy:
    loadBalancer:
      dnsManagementPolicy: Managed
      providerParameters:
        aws:
          type: NLB
        type: AWS
      scope: External
    type: LoadBalancerService

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems be an API related issue. I do have copied all the crds in the manifest dir. The CVO should pick the right one based on the release.openshift.io/feature-set annotation

After spinning the cluster from cluster-bot:

Run:
git clone my branch

oc scale --replicas 0 -n openshift-cluster-version deployments/cluster-version-operator

oc replace -f manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yaml

I use the above commands in my cluster and have not seen this issue.

miheer added 3 commits May 1, 2024 11:50
@miheer
This commit adds logic for adding the serive annotation for EIP alloc…
fba4380 
…ations.

`pkg/operator/controller/ingress/load_balancer_service.go` logic to add the service annotation "service.beta.kubernetes.io/aws-load-balancer-eip-allocations" with the EIP Allocations to the Load Balancer service.
`pkg/operator/controller/ingress/load_balancer_service_test.go` Unit tests for testing the logic written for adding eipAllocation in service annotation "service.beta.kubernetes.io/aws-load-balancer-eip-allocations"
`controller.go` : Sets the status w.r.t to the spec for eipAllocations
`controller_test`: Modifications in existing unit tests to adjust as per the logic written in controller.go
`pkg/operator/operator.go` : Allocating EIPs from the ingress.config.io/cluter set by installer to the default ingress controller.
`pkg/operator/controller/ingress/status.go` sets the status to the right EIPAllocations.
`pkg/operator/controller/ingress/status_test.go` modifed existing unit test as per new changes in the status.go
`pkg/operator/controller/names.go` helper function to get secrets to fetch cloud credentials.
`test/e2e/all_test.go` Add the references to the new e2e tests for EIP allocation.
`test/e2e/aws_eip_for_nlb_test.go` Add e2e tests for testing the EIP allocation while creating and updating the Ingress Controller CR.
@miheer
This commit consists of API bump and the vendored changes.
c9b03a2 
It also copies all crd yaml files related to Ingress Controller in the manifests directory.
@miheer
Modifies CGO_ENABLED=0 as the ingress-operator fails with the followi…
7297dd2 
…ng error when CGO_ENABLED=1

% oc logs ingress-operator-7fc8f75988-4kr79 -n openshift-ingress-operator
Defaulted container "ingress-operator" out of: ingress-operator, kube-rbac-proxy
ingress-operator: /lib64/libc.so.6: version `GLIBC_2.32' not found (required by ingress-operator)
ingress-operator: /lib64/libc.so.6: version `GLIBC_2.34' not found (required by ingress-operator)
@eggfoobar
Copy link

eggfoobar commented May 1, 2024 •
edited

/test e2e-aws-ovn-single-node

The install error should be resolved now

Edit:

Cluster install worked as expected, the current failure is just a regular test error.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 1, 2024

@miheer: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-hypershift 7297dd2 link true /test e2e-hypershift
ci/prow/e2e-aws-operator 7297dd2 link true /test e2e-aws-operator
ci/prow/e2e-gcp-operator 7297dd2 link true /test e2e-gcp-operator
ci/prow/e2e-aws-ovn-single-node 7297dd2 link false /test e2e-aws-ovn-single-node

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@lihongan lihongan lihongan left review comments

@gcs278 gcs278 Awaiting requested review from gcs278

@knobunc knobunc Awaiting requested review from knobunc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@miheer @lihongan @eggfoobar @Miciah @openshift-merge-robot