servicemeshinterface / smi-sdk-go Goto Github PK

Hi, Could we define a sample program? maybe just *.yaml resources, so that SMI providers can use it for testing, verifying and authenticating the implement, maybe transplant the Istio BookInfo app is a good choise

SMI could provide a conversion webhook that will automate the upgrade process of SMI custom resources.

For example, the TrafficSplit v1alpha2 comes with a breaking change as the backend weight type changed from resource.Quantity to int in #24. The webhook would make the conversion between versions and update the TrafficSplit objects in-cluster without manual intervention by the cluster admins.

The spec shows a selector field for v1alpha1 and v1alpha2 TrafficMetricsList: https://github.com/servicemeshinterface/smi-spec/blob/e59f6e61ecc6528897b0c6c97b26ee9e89617970/apis/traffic-metrics/v1alpha1/traffic-metrics.md#trafficmetricslist

The corresponding field in the TrafficMetricsList type definition is missing from the SDK (from both v1alpha1 and v1alpha2):

Implement TrafficSplit v1alpha3 according to servicemeshinterface/smi-spec#104

We also need to tag 0.4.0 of the specification

I noticed that the validation for TrafficSplit objects for versions v1alpha2 and v1alpha3 does not match their respective specs.

In the spec for v1alpha2 and v1alpha3, for TrafficSplitBackend, the Weight field has the JSON Go tag omitempty.

But in the CRD, the OpenApiValidation does not allow the Weight field to be empty (or a value of 0).

When I use the TrafficSplit client to create a TrafficSplit with a TrafficSplitBackend that has a Weight field of 0, I get a validation error: TrafficSplit.split.smi-spec.io "split" is invalid: spec.backends.weight: Required value

I believe the spec should be modified to remove omitempty from the Weight field, thus making it required.

I also think that the same issue applies for the Service field as well.

When the script is run I get following errors:

$ hack/update-codegen.sh 
###### Generating Traffic Split Client ######
hack/update-codegen.sh: line 14: cd: too many arguments

I edited the file to run with -x and here is the output:

$ hack/update-codegen.sh 
+ ROOT_PACKAGE=github.com/deislabs/smi-sdk-go
+ CUSTOM_RESOURCE_VERSION=v1alpha1
+ echo '###### Generating Traffic Split Client ######'
###### Generating Traffic Split Client ######
+ generate_client split
+++ dirname hack/update-codegen.sh
++ cd hack
++ pwd
+ SCRIPT_ROOT='/home/surajssd/go/src/github.com/deislabs/smi-sdk-go/hack
/home/surajssd/go/src/github.com/deislabs/smi-sdk-go/hack'
++ cd /home/surajssd/go/src/github.com/deislabs/smi-sdk-go/hack /home/surajssd/go/src/github.com/deislabs/smi-sdk-go/hack/..
hack/update-codegen.sh: line 16: cd: too many arguments
+ ROOT_DIR=

As of Kubernetes 1.16, CRDs have been promoted to GA.
SMI currently uses apiextensions.k8s.io/v1beta1, which is deprecated in 1.19 and will be removed in 1.22: kubernetes/kubernetes#82022.

• Update the CRD yaml
• Update docs with supported Kubernetes version 1.16+, so we can default to apiextensions.k8s.io/v1
• Need to document known issues/workaround if there are existing apiextensions.k8s.io/v1beta1 CRDs for upgrade scenarios

The spec shows a backend field in v1alpha1 of TrafficMetrics: https://github.com/servicemeshinterface/smi-spec/blob/e59f6e61ecc6528897b0c6c97b26ee9e89617970/apis/traffic-metrics/v1alpha1/traffic-metrics.md#traffic-splits

That same field is missing from the v1alpha1 TrafficMetrics type definition in the SDK:

Kubernetes v1.18.0 has been released we should update the SDK.

having some issues with TCPRoute Informers not Syncing.

The following code:

package main

import (
	"context"
	"fmt"
	"time"

	specsv1alpha1 "github.com/deislabs/smi-sdk-go/pkg/apis/specs/v1alpha1"
	fakeSMISpecs "github.com/deislabs/smi-sdk-go/pkg/gen/client/specs/clientset/versioned/fake"
	specsInformer "github.com/deislabs/smi-sdk-go/pkg/gen/client/specs/informers/externalversions"
	"k8s.io/client-go/kubernetes/scheme"
	"k8s.io/client-go/tools/cache"
)

func init() {
	err := specsv1alpha1.AddToScheme(scheme.Scheme)
	if err != nil {
		panic(err)
	}
}

func main() {
	client := fakeSMISpecs.NewSimpleClientset()

	specsInformerFactory := specsInformer.NewSharedInformerFactory(client, 0)

	httpRouteGroupInformer := specsInformerFactory.Specs().V1alpha1().HTTPRouteGroups().Informer()
	tcpRouteInformer := specsInformerFactory.Specs().V1alpha1().TCPRoutes().Informer()

	httpRouteGroupInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{})
	tcpRouteInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{})

	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
	defer cancel()

	specsInformerFactory.Start(ctx.Done())

	for t, ok := range specsInformerFactory.WaitForCacheSync(ctx.Done()) {
		if !ok {
			fmt.Printf("timed out waiting for controller caches to sync: %s", t.String())
		}
	}
}

When run:

% go run test.go
timed out waiting for controller caches to sync: *v1alpha1.TCPRoute

With the TCPRoute informer commented out:

% go run test.go
%

and with the HTTPRouteGroup informer commented out:

% go run test.go
timed out waiting for controller caches to sync: *v1alpha1.TCPRoute

Not sure where to go from here.

The spec shows an edge.side field in v1alpha1 of TrafficMetrics: https://github.com/servicemeshinterface/smi-spec/blob/e59f6e61ecc6528897b0c6c97b26ee9e89617970/apis/traffic-metrics/v1alpha1/traffic-metrics.md#specification

That same field is missing from the v1alpha1 TrafficMetrics type definition in the SDK:

Audit current list of owners and update accordingly. Reminder to add existing OWNERS to emeritus.

includes the http header matching feature and some bug fixes

Hi 👋 I was recently working on adding OpenAPI validation to existing crds in linkerd. For TrafficSplit I thought I'd come straight to the source so I took the validation schema as is from this repository (made sure to credit everything ofc 😄 ).

In linkerd v1alpha1 is still in use. Throughout the code base (including tests) the weight field is parsed as a quantity string (e.g 200m, 300m, etc.). Because the weight field is a number type in the schema, it breaks backwards compatibility with older versions that also allow quantity strings as a type. This made the integration test suite fail in this case.

I thought I would let you know about this, just in case. I fixed it by turning the field type into x-kubernetes-int-or-string (this still keeps the schema as structural afaik). I realise you'll be moving to v1 soon so it might not be worth it to change it now, but perhaps it might be helpful when doing per-version schemas.

It's a small change so I'd also be happy to do it myself depending on what your chosen course of action is. Thank you!

Here the TrafficMetrics CRD yaml file is just a placeholder add configs in this file.

https://github.com/deislabs/smi-sdk-go/blob/86042c47396f92f00ad451a030b498bb033277b3/crds/metrics.yaml

Since the traffic is quite low on this repo, would it be possible to setup a nightly build to ensure that dependencies and builds are functional.

It should be possible to configure this in the CIrcle CI setup if you have org admin.

The SMI CRDs should have their schema validated. At the bare minimum, this should be added to the default definitions in deislabs/smi-sdk-go. It would be nice to improve on that schema validation by providing a ValidatingAdmissionWebhook that projects can install which provides extensive validation and returns explanations on how to fix the schemas.

I noticed that there is a discrepancy between the documentation for the Matches field and the spec for v1alpha3.

In the documentation, the Matches field is underneath the spec field:

kind: TrafficSplit
metadata:
name: ab-test
spec:
service: website
matches:

• kind: HTTPRouteGroup
  name: ab-test
  backends:
• service: website-v1
  weight: 0
• service: website-v2
  weight: 100

But in the spec, the Matches field is defined underneath TrafficSplitBackend instead of TrafficSplitSpec.

I believe that the v1alpha3 spec should be updated to match the usage described in the documentation.

The status sub-resource should be part of the SMI specification, I propose we remove it from the SDK, decide on a status specification for each API object then implement it in the SDK.

Objects in this library need to be renamed from v1beta1 to v1alpha1.

@adleong if I do this will it mess up any of your references?

In order to facilitate migrations between versions, the SDK should contain all API versions. We could define a deprecation policy in the future and decide based on that when to remove a version from the SDK.

Of the resources supported by the TrafficMetrics API according to the spec, only a subset are defined in the SDK (for both v1alpha1 and v1alpha2).

Spec: https://github.com/servicemeshinterface/smi-spec/blob/e59f6e61ecc6528897b0c6c97b26ee9e89617970/apis/traffic-metrics/v1alpha1/traffic-metrics.md#kubernetes-api

SDK:

We should migrate to GitHub Actions, doing so would break Kubernetes codegen as the go action doesn't set GOPATH the way codegen expects. Kubernetes 1.18 codegen doesn't require GOPATH so moving to GH Actions depends on #64

Hi everyone 👋

While updating the smi-sdk-go to v0.4 on Maesh I noticed an inconsistency with the specification.

The TrafficTarget v1alpha2 type defines an attribute Spec as a []TrafficTargetSpec but according to the specification it shouldn't be a slice.

When applying a TrafficTarget as defined in the spec I get the following error:

W0729 08:41:55.191388       1 reflector.go:404] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:125: watch of
*v1alpha2.TrafficTarget ended with: an error on the server ("unable to decode an event from the watch stream: unable to decode
watch event: v1alpha2.TrafficTarget.Spec: []v1alpha2.TrafficTargetSpec: decode slice: expect [ or n, but found {, error found in
#10 byte of ...|},\"spec\":{\"destinati|..., bigger context ...|\":\"368b7feb-efb3-4b1c-bbe6-315ffa27bb34\"},\"spec\":{\"destination\"
{\"kind\":\"ServiceAccount\",\"name\":\"ser|...") has prevented the request from succeeding

I'm going to open a PR shortly to fix this.

Matches for TrafficTarget is of type []string.

Matches for TrafficSplit is of type []corev1.TypedLocalObjectReference

The two types should match.

Why are the matches for TrafficSplit in TrafficSplitBackend and not TrafficSplitSpec? My understanding from the documentation was that the TrafficSpecs would apply to all of the backends. Is that assumption incorrect?

Also is there a reason the type is different than Matches for TrafficTarget? Will the TrafficTarget Matches type change in future iterations?

The spec outlines that another way to define traffic (in a Traffic Spec) is through HTTP headers, which the smi sdk does not currently support. Are there plans to add this?

since we've updated trafficsplit to v1alpha2

Is there anything stopping us from doing a release/tag for v1alpha1 for this sdk? do we need to define the TrafficMetrics CRD (#16) before doing a release?

cc/ @grampelberg @Pothulapati @lachie83 @slack