Skip to content

fix: transfer resolveGranularity to ApisixUpstream to fix errors #18

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.

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

Merged
merged 13 commits into from
Jun 4, 2024
Merged
96 changes: 0 additions & 96 deletions .github/workflows/codeql-analysis.yml

This file was deleted.

1 change: 1 addition & 0 deletions .github/workflows/dependency-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2

permissions:
contents: read
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/e2e-test-ci-v2-cron-dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2

concurrency:
group: ${{ github.workflow }}-dev
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/e2e-test-ci-v2-cron.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}-v2
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/e2e-test-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/goimports-reviser.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2
jobs:
changes:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/golangci-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2

jobs:
changes:
Expand Down
39 changes: 0 additions & 39 deletions .github/workflows/license-checker.yml

This file was deleted.

1 change: 1 addition & 0 deletions .github/workflows/lint-checker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2
jobs:
changes:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/spell-checker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2
jobs:
misspell:
name: runner / misspell
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/unit-test-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2
jobs:
changes:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/verify-codegen.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2
jobs:
changes:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/yamllint-checker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ on:
branches:
- master
- 1.8.0
- release/1.8.2
jobs:
changes:
runs-on: ubuntu-latest
Expand Down
11 changes: 1 addition & 10 deletions pkg/kube/apisix/apis/config/v2/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ type ApisixRouteHTTPBackend struct {
ServiceName string `json:"serviceName" yaml:"serviceName"`
// The service port, could be the name or the port number.
ServicePort intstr.IntOrString `json:"servicePort" yaml:"servicePort"`
// The resolve granularity, can be "endpoints" or "service",
// when set to "endpoints", the pod ips will be used; other
// wise, the service ClusterIP or ExternalIP will be used,
// default is endpoints.
ResolveGranularity string `json:"resolveGranularity,omitempty" yaml:"resolveGranularity,omitempty"`
// Weight of this backend.
Weight *int `json:"weight" yaml:"weight"`
// Subset specifies a subset for the target Service. The subset should be pre-defined
Expand Down Expand Up @@ -257,11 +252,6 @@ type ApisixRouteStreamBackend struct {
ServiceName string `json:"serviceName" yaml:"serviceName"`
// The service port, could be the name or the port number.
ServicePort intstr.IntOrString `json:"servicePort" yaml:"servicePort"`
// The resolve granularity, can be "endpoints" or "service",
// when set to "endpoints", the pod ips will be used; other
// wise, the service ClusterIP or ExternalIP will be used,
// default is endpoints.
ResolveGranularity string `json:"resolveGranularity,omitempty" yaml:"resolveGranularity,omitempty"`
// Subset specifies a subset for the target Service. The subset should be pre-defined
// in ApisixUpstream about this service.
Subset string `json:"subset,omitempty" yaml:"subset,omitempty"`
Expand Down Expand Up @@ -515,6 +505,7 @@ type ApisixUpstreamSpec struct {
// ApisixUpstreamConfig contains rich features on APISIX Upstream, for instance
// load balancer, health check, etc.
type ApisixUpstreamConfig struct {
Granularity string `json:"granularity,omitempty" yaml:"scheme,omitempty"`
// LoadBalancer represents the load balancer configuration for Kubernetes Service.
// The default strategy is round robin.
// +optional
Expand Down
49 changes: 28 additions & 21 deletions pkg/providers/apisix/apisix_upstream.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package apisix

import (
"context"
"errors"
"fmt"
"reflect"
"strconv"
Expand Down Expand Up @@ -191,6 +192,12 @@ func (c *apisixUpstreamController) sync(ctx context.Context, ev *types.Event) er
if au.Spec == nil {
return nil
}
svc, err := c.SvcLister.Services(namespace).Get(name)
if err != nil {
log.Errorf("failed to get service %s: %s", key, err)
errRecord = err
goto updateStatus
}

// We will prioritize ExternalNodes and Discovery.
if len(au.Spec.ExternalNodes) != 0 || au.Spec.Discovery != nil {
Expand Down Expand Up @@ -221,7 +228,7 @@ func (c *apisixUpstreamController) sync(ctx context.Context, ev *types.Event) er
}
// updateUpstream for real
upsName := apisixv1.ComposeExternalUpstreamName(au.Namespace, au.Name)
errRecord = c.updateUpstream(ctx, upsName, &au.Spec.ApisixUpstreamConfig, ev.Type.IsSyncEvent())
errRecord = c.updateUpstream(ctx, upsName, &au.Spec.ApisixUpstreamConfig, ev.Type.IsSyncEvent(), svc.Spec.ClusterIP)
if err == apisix.ErrNotFound {
errRecord = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
}
Expand All @@ -235,14 +242,6 @@ func (c *apisixUpstreamController) sync(ctx context.Context, ev *types.Event) er
portLevelSettings[port.Port] = port.ApisixUpstreamConfig
}
}

svc, err := c.SvcLister.Services(namespace).Get(name)
if err != nil {
log.Errorf("failed to get service %s: %s", key, err)
errRecord = err
goto updateStatus
}

var subsets []configv2.ApisixUpstreamSubset
subsets = append(subsets, configv2.ApisixUpstreamSubset{})
if len(au.Spec.Subsets) > 0 {
Expand All @@ -258,16 +257,7 @@ func (c *apisixUpstreamController) sync(ctx context.Context, ev *types.Event) er
cfg = au.Spec.ApisixUpstreamConfig
}
}
err := c.updateUpstream(ctx, apisixv1.ComposeUpstreamName(namespace, name, subset.Name, port.Port, types.ResolveGranularity.Endpoint), &cfg, ev.Type.IsSyncEvent())
if err != nil {
if err == apisix.ErrNotFound {
errRecord = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
} else {
errRecord = err
}
goto updateStatus
}
err = c.updateUpstream(ctx, apisixv1.ComposeUpstreamName(namespace, name, subset.Name, port.Port, types.ResolveGranularity.Service), &cfg, ev.Type.IsSyncEvent())
err := c.updateUpstream(ctx, apisixv1.ComposeUpstreamName(namespace, name, subset.Name, port.Port), &cfg, ev.Type.IsSyncEvent(), svc.Spec.ClusterIP)
Copy link
Contributor

@AlinsRan AlinsRan Jun 3, 2024

Choose a reason for hiding this comment

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

pls ref: https://kubernetes.io/docs/concepts/services-networking/service/#externalname.

This type of service is only for DNS mapping. Are you sure you can use clusterip.

For accessing external services, the lack of SNI prevents TLS handshake.

Copy link
Contributor Author

@Revolyssup Revolyssup Jun 3, 2024

Choose a reason for hiding this comment

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

But the upstream controller is triggered and uses the service even if the service type is not external name. Am I missing something? @AlinsRan

Copy link
Contributor

@AlinsRan AlinsRan Jun 3, 2024

Choose a reason for hiding this comment

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

I mean that services of externalName type are not compatible here.

Because ExternalName services do not have endpoints, the main purpose of resolveGranularity is to enable gateways to access external services.

Similarly, it does not have clusterIP.

For external services, simply use the domain name.

Copy link
Contributor Author

@Revolyssup Revolyssup Jun 3, 2024

Choose a reason for hiding this comment

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

Okay so the logic should be that when service is of type external name then even if granularity was passed in the ApisixUpstream, it should be ignored. Because if granularity is not passed then by default, ApisixUpstream doesn't change the value of nodes that was created in upstream when ApisixRoute was created. So no change required on ApisixUpstream sync.

And currently the TranslateService which creates upstream for k8s service doesn't take into account. So this issue already exists,right? I just need to add logic in translate service that when service type is ExternalName, just use externalName as node value instead of getting endpoints.
@AlinsRan Right?

Copy link
Contributor Author

@Revolyssup Revolyssup Jun 3, 2024

Choose a reason for hiding this comment

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

@AlinsRan Since thats an existing issue and not something introduced by this PR. Should I create another PR for it. Because even in current scenario before my change this problem exists.

Copy link
Contributor

@AlinsRan AlinsRan Jun 3, 2024

Choose a reason for hiding this comment

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

I just need to add logic in translate service that when service type is ExternalName, just use externalName as node value instead of getting endpoints.

Yes.

The current PR does not need to solve this problem, I just mentioned the possible issues that this feature may encounter.

if err != nil {
if err == apisix.ErrNotFound {
errRecord = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
Expand Down Expand Up @@ -335,7 +325,7 @@ func (c *apisixUpstreamController) updateStatus(obj kube.ApisixUpstream, statusE
}
}

func (c *apisixUpstreamController) updateUpstream(ctx context.Context, upsName string, cfg *configv2.ApisixUpstreamConfig, shouldCompare bool) error {
func (c *apisixUpstreamController) updateUpstream(ctx context.Context, upsName string, cfg *configv2.ApisixUpstreamConfig, shouldCompare bool, svcClusterIP string) error {
// TODO: multi cluster
clusterName := c.Config.APISIX.DefaultClusterName

Expand All @@ -358,7 +348,24 @@ func (c *apisixUpstreamController) updateUpstream(ctx context.Context, upsName s
}

newUps.Metadata = ups.Metadata
newUps.Nodes = ups.Nodes

if cfg.Granularity == types.ResolveGranularity.Service {
if svcClusterIP == "" {
log.Errorw("ApisixRoute refers to a headless service but want to use the service level resolve granularity",
zap.String("ApisixUpstream name", upsName),
)
return errors.New("conflict headless service and backend resolve granularity")
}
for _, node := range ups.Nodes {
newUps.Nodes = append(newUps.Nodes, apisixv1.UpstreamNode{
Host: svcClusterIP,
Port: node.Port,
Weight: node.Weight,
})
}
} else {
newUps.Nodes = ups.Nodes
}
log.Debugw("updating upstream since ApisixUpstream changed",
zap.Any("upstream", newUps),
zap.String("ApisixUpstream name", upsName),
Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/apisix/translation/apisix_plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (t *translator) translateTrafficSplitPlugin(ctx *translation.TranslateConte
if err != nil {
return nil, err
}
ups, err := t.translateService(ns, backend.ServiceName, backend.Subset, backend.ResolveGranularity, svcClusterIP, svcPort)
ups, err := t.translateService(ns, backend.ServiceName, backend.Subset, svcClusterIP, svcPort)
if err != nil {
return nil, err
}
Expand Down
Loading
Loading