From e4f648a60f739ef674e041b86f65847b3751d6d6 Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Sat, 18 Jun 2022 09:10:19 +0800 Subject: [PATCH 1/2] little refactor for provisioner/service Signed-off-by: Gang Liu --- .../provisioner/objects/service/service.go | 77 +++++++++++-------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/internal/provisioner/objects/service/service.go b/internal/provisioner/objects/service/service.go index 87d835108e6..834338a06b3 100644 --- a/internal/provisioner/objects/service/service.go +++ b/internal/provisioner/objects/service/service.go @@ -231,11 +231,14 @@ func DesiredEnvoyService(contour *model.Contour) *corev1.Service { }, } + providerParams := &contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters + // Add AWS LB annotations based on the network publishing strategy and provider type. if contour.Spec.NetworkPublishing.Envoy.Type == model.LoadBalancerServicePublishingType && - contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AWSLoadBalancerProvider { + providerParams.Type == model.AWSLoadBalancerProvider { + // Add the TCP backend protocol annotation for AWS classic load balancers. - if isELB(&contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters) { + if isELB(providerParams) { svc.Annotations[awsLbBackendProtoAnnotation] = "tcp" svc.Annotations[awsLBProxyProtocolAnnotation] = "*" } else { @@ -246,29 +249,29 @@ func DesiredEnvoyService(contour *model.Contour) *corev1.Service { // Add the AllocationIDs annotation if specified by AWS provider parameters. if allocationIDsNeeded(&contour.Spec) { - svc.Annotations[awsLBAllocationIDsAnnotation] = strings.Join(contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.AWS.AllocationIDs, ",") + svc.Annotations[awsLBAllocationIDsAnnotation] = strings.Join(providerParams.AWS.AllocationIDs, ",") } // Add the ResourceGroup annotation if specified by Azure provider parameters. if resourceGroupNeeded(&contour.Spec) { - svc.Annotations[azureLBResourceGroupAnnotation] = *contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure.ResourceGroup + svc.Annotations[azureLBResourceGroupAnnotation] = *providerParams.Azure.ResourceGroup } // Add the Subnet annotation if specified by provider parameters. if subnetNeeded(&contour.Spec) { - if contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AzureLoadBalancerProvider { - svc.Annotations[azureLBSubnetAnnotation] = *contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure.Subnet - } else if contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.GCPLoadBalancerProvider { - svc.Annotations[gcpLBSubnetAnnotation] = *contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.GCP.Subnet + if providerParams.Type == model.AzureLoadBalancerProvider { + svc.Annotations[azureLBSubnetAnnotation] = *providerParams.Azure.Subnet + } else if providerParams.Type == model.GCPLoadBalancerProvider { + svc.Annotations[gcpLBSubnetAnnotation] = *providerParams.GCP.Subnet } } // Add LoadBalancerIP parameter if specified by provider parameters. if loadBalancerAddressNeeded(&contour.Spec) { - if contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AzureLoadBalancerProvider { - svc.Spec.LoadBalancerIP = *contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure.Address - } else if contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.GCPLoadBalancerProvider { - svc.Spec.LoadBalancerIP = *contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.GCP.Address + if providerParams.Type == model.AzureLoadBalancerProvider { + svc.Spec.LoadBalancerIP = *providerParams.Azure.Address + } else if providerParams.Type == model.GCPLoadBalancerProvider { + svc.Spec.LoadBalancerIP = *providerParams.GCP.Address } } @@ -282,7 +285,7 @@ func DesiredEnvoyService(contour *model.Contour) *corev1.Service { svc.Spec.Type = corev1.ServiceTypeLoadBalancer isInternal := contour.Spec.NetworkPublishing.Envoy.LoadBalancer.Scope == model.InternalLoadBalancer if isInternal { - provider := contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type + provider := providerParams.Type internalAnnotations := InternalLBAnnotations[provider] for name, value := range internalAnnotations { svc.Annotations[name] = value @@ -372,8 +375,10 @@ func updateEnvoyServiceIfNeeded(ctx context.Context, cli client.Client, contour switch contour.Spec.NetworkPublishing.Envoy.Type { case model.NodePortServicePublishingType: updated, needed = equality.NodePortServiceChanged(current, desired) + case model.ClusterIPServicePublishingType: updated, needed = equality.ClusterIPServiceChanged(current, desired) + // Add additional network publishing types as they are introduced. default: // LoadBalancerService is the default network publishing type. @@ -396,46 +401,54 @@ func isELB(params *model.ProviderLoadBalancerParameters) bool { // allocationIDsNeeded returns true if "service.beta.kubernetes.io/aws-load-balancer-eip-allocations" // annotation is needed based on the provided spec. func allocationIDsNeeded(spec *model.ContourSpec) bool { + providerParams := &spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters + return spec.NetworkPublishing.Envoy.Type == model.LoadBalancerServicePublishingType && spec.NetworkPublishing.Envoy.LoadBalancer.Scope == "External" && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AWSLoadBalancerProvider && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.AWS != nil && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.AWS.Type == model.AWSNetworkLoadBalancer && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.AWS.AllocationIDs != nil + providerParams.Type == model.AWSLoadBalancerProvider && + providerParams.AWS != nil && + providerParams.AWS.Type == model.AWSNetworkLoadBalancer && + providerParams.AWS.AllocationIDs != nil } // resourceGroupNeeded returns true if "service.beta.kubernetes.io/azure-load-balancer-resource-group" // annotation is needed based on the provided spec. func resourceGroupNeeded(spec *model.ContourSpec) bool { + providerParams := &spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters + return spec.NetworkPublishing.Envoy.Type == model.LoadBalancerServicePublishingType && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AzureLoadBalancerProvider && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure != nil && + providerParams.Type == model.AzureLoadBalancerProvider && + providerParams.Azure != nil && spec.NetworkPublishing.Envoy.LoadBalancer.Scope == "External" && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure.ResourceGroup != nil + providerParams.Azure.ResourceGroup != nil } // subnetNeeded returns true if "service.beta.kubernetes.io/azure-load-balancer-internal-subnet" or // "networking.gke.io/internal-load-balancer-subnet" annotation is needed based // on the provided spec. func subnetNeeded(spec *model.ContourSpec) bool { + providerParams := &spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters + return spec.NetworkPublishing.Envoy.Type == model.LoadBalancerServicePublishingType && spec.NetworkPublishing.Envoy.LoadBalancer.Scope == "Internal" && - ((spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AzureLoadBalancerProvider && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure != nil && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure.Subnet != nil) || - (spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.GCPLoadBalancerProvider && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.GCP != nil && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.GCP.Subnet != nil)) + ((providerParams.Type == model.AzureLoadBalancerProvider && + providerParams.Azure != nil && + providerParams.Azure.Subnet != nil) || + (providerParams.Type == model.GCPLoadBalancerProvider && + providerParams.GCP != nil && + providerParams.GCP.Subnet != nil)) } // loadBalancerAddressNeeded returns true if LoadBalancerIP parameter of service // is needed based on provided spec. func loadBalancerAddressNeeded(spec *model.ContourSpec) bool { + providerParams := &spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters + return spec.NetworkPublishing.Envoy.Type == model.LoadBalancerServicePublishingType && - ((spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.AzureLoadBalancerProvider && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure != nil && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Azure.Address != nil) || - (spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type == model.GCPLoadBalancerProvider && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.GCP != nil && - spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.GCP.Address != nil)) + ((providerParams.Type == model.AzureLoadBalancerProvider && + providerParams.Azure != nil && + providerParams.Azure.Address != nil) || + (providerParams.Type == model.GCPLoadBalancerProvider && + providerParams.GCP != nil && + providerParams.GCP.Address != nil)) } From b2ada19e164fe48728fe9a54b9fd259077a97263 Mon Sep 17 00:00:00 2001 From: Gang Liu Date: Tue, 21 Jun 2022 09:47:55 +0800 Subject: [PATCH 2/2] add changelog and make lint happy Signed-off-by: Gang Liu --- changelogs/unreleased/4586-izturn-minor.md | 3 +++ internal/provisioner/objects/service/service.go | 2 +- test/e2e/deployment.go | 3 --- 3 files changed, 4 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/4586-izturn-minor.md diff --git a/changelogs/unreleased/4586-izturn-minor.md b/changelogs/unreleased/4586-izturn-minor.md new file mode 100644 index 00000000000..0f37759648e --- /dev/null +++ b/changelogs/unreleased/4586-izturn-minor.md @@ -0,0 +1,3 @@ +## Use local variable to replace the long access chain of fields + +The access chain of fields is too long, so use local variable to replace them. diff --git a/internal/provisioner/objects/service/service.go b/internal/provisioner/objects/service/service.go index 834338a06b3..8c83c5b4166 100644 --- a/internal/provisioner/objects/service/service.go +++ b/internal/provisioner/objects/service/service.go @@ -443,7 +443,7 @@ func subnetNeeded(spec *model.ContourSpec) bool { // is needed based on provided spec. func loadBalancerAddressNeeded(spec *model.ContourSpec) bool { providerParams := &spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters - + return spec.NetworkPublishing.Envoy.Type == model.LoadBalancerServicePublishingType && ((providerParams.Type == model.AzureLoadBalancerProvider && providerParams.Azure != nil && diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go index f572366bb5e..6c71e18a30e 100644 --- a/test/e2e/deployment.go +++ b/test/e2e/deployment.go @@ -339,9 +339,6 @@ func (d *Deployment) WaitForContourDeploymentUpdated() error { if err := d.client.List(context.TODO(), pods, labelSelectAppContour); err != nil { return false, err } - if pods == nil { - return false, errors.New("failed to fetch Contour Deployment pods") - } updatedPods := 0 for _, pod := range pods.Items {