fix: transfer resolveGranularity to ApisixUpstream to fix errors #18
Conversation
Signed-off-by: Ashish Tiwari <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 561 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 400 | 2 | 159 | 0 |
Click to see the invalid file list
- samples/deploy/crd/v1/ApisixRoute.yaml
- samples/deploy/crd/v1/ApisixUpstream.yaml
| "fmt" | ||
| "net/http" | ||
| "time" | ||
| // var _ = ginkgo.Describe("suite-chore: ApisixRoute resolvegranularity Testing", func() { |
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But the upstream controller is triggered and uses the service even if the service type is not external name. Am I missing something? @AlinsRan
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
| @@ -317,9 +297,6 @@ spec: | |||
| - type: integer | |||
| - type: string | |||
| x-kubernetes-int-or-string: true | |||
| resolveGranularity: | |||
There was a problem hiding this comment.
The management method of the K8S API does not allow this. pls refs: https://kubernetes.io/docs/concepts/overview/kubernetes-api/#api-changes
I reserve my opinion on the enterprise version, but the open source version cannot do so.
There was a problem hiding this comment.
Yes the open source version cant do so but as discussed with @nic-6443 , compatibility is not an issue here.
There was a problem hiding this comment.
Sure, we confirmed with our enterprise user, It's ok to introduce break changes in the CRD
Type of change:
What this PR does / why we need it:
Pre-submission checklist: