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.

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

Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() #225

Merged
merged 11 commits into from
Apr 26, 2023
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ help: ## Display this help.
run: ## Run in development mode
go run main.go


.PHONY: presubmit
presubmit: vet test ## Run all commands before submitting code

Expand Down
14 changes: 14 additions & 0 deletions docs/developer.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ GATEWAY_API_CONTROLLER_LOGLEVEL=debug make run
LATTICE_ENDPOINT=https://vpc-lattice.us-west-2.amazonaws.com/ make run
```

To easier load environment variables, if you hope to run the controller by GoLand IDE locally, you could run the `scripts/load_env_variables.sh`
And use "EnvFile" GoLand plugin to read the env variables from the generated `.env` file.

### End-to-End Testing

Run the following command to run the end-to-end tests against the Kubernetes cluster pointed to by `kubectl config current-context`:
Expand All @@ -36,6 +39,17 @@ You should set up the correct `REGION` env variable
export REGION=us-west-2
make e2etest
```

Pass `FOCUS` environment variable to run some specific test cases based on filter condition:
```bash
export REGION=us-west-2
export FOCUS="HTTPRoute should support multiple path matches"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a note on how to find out the this FOCUS string to a particular test. thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, Added more description in the next revision

make e2etest
```
For more detail on filter condition for ginkgo
https://onsi.github.io/ginkgo/#focused-specs
https://onsi.github.io/ginkgo/#description-based-filtering

Notice: the prerequisites for running the end-to-end tests success are:
- Current eks cluster don't have any k8s resource
- The vpc used by current eks cluster don't have any vpc service network association
Expand Down
46 changes: 46 additions & 0 deletions scripts/load_env_variables.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this mess that folks(most of us) doing development on clouddesktop ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Developer should run (or set it in GoLand manually) load_env_variables.sh manually, and no where refer it.

I have some comments in this script, if it is still confusing, I could remove this script

Copy link
Contributor

Choose a reason for hiding this comment

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

Today, Makefile takes care of all these environment variable setting. @ellistarn should we merge these into Makefile?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with golang env vars, but I'd avoid any ide specific configuration. At the end of the day, your team needs to choose the development practices that work best for you. I setup what works for me, and how I run other OSS projects. Feel free to change things.

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update docs/developer.md and mentioned that this can be used for when u want to run it inside golang IDE locally


# When Running the k8s Controller by GoLand IDE locally, it is required to set the environment variables, this script will generate a file called `envFile`
# that contains all needed environment variables, this `envFile` file can be use by GoLand "EnvFile" plugin
# When config the "green arrow"(how we `run` or `debug` this project) in GoLand, we could set a "before launch" task to run this script `load_env_variables.sh`, and set the "EnvFile" plugin to load the `envFile` file
# "EnvFile" is a plugin, you can install it from GoLand plugin marketplace

echo "Setting environment variables"

> envFile

# Set KUBEBUILDER_ASSETS if not set
if [ -z "$KUBEBUILDER_ASSETS" ]; then
KUBEBUILDER_ASSETS=${HOME}/.kubebuilder/bin
fi
echo "KUBEBUILDER_ASSETS=$KUBEBUILDER_ASSETS" >> envFile


# Set CLUSTER_NAME if not set
if [ -z "$CLUSTER_NAME" ]; then
CLUSTER_NAME=$(kubectl config view --minify -o jsonpath='{.clusters[].name}' | rev | cut -d"/" -f1 | rev | cut -d"." -f1)
fi
echo "CLUSTER_NAME=$CLUSTER_NAME" >> envFile


# Set CLUSTER_VPC_ID if not set
if [ -z "$CLUSTER_VPC_ID" ]; then
CLUSTER_VPC_ID=$(aws eks describe-cluster --name ${CLUSTER_NAME} | jq -r ".cluster.resourcesVpcConfig.vpcId")
fi
echo "CLUSTER_VPC_ID=$CLUSTER_VPC_ID" >> envFile

# Set AWS_ACCOUNT_ID if not set
if [ -z "$AWS_ACCOUNT_ID" ]; then
AWS_ACCOUNT_ID=$(aws sts get-caller-identity --query Account --output text)
fi
echo "AWS_ACCOUNT_ID=$AWS_ACCOUNT_ID" >> envFile

if [ -z "$REGION" ]; then
REGION=us-west-2
fi
echo "REGION=$REGION" >> envFile


GATEWAY_API_CONTROLLER_LOGLEVEL=debug
echo "GATEWAY_API_CONTROLLER_LOGLEVEL=$GATEWAY_API_CONTROLLER_LOGLEVEL" >> envFile

109 changes: 78 additions & 31 deletions test/pkg/test/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type Framework struct {
TestCasesCreatedServiceNetworkNames map[string]bool //key: ServiceNetworkName; value: not in use, meaningless
TestCasesCreatedServiceNames map[string]bool //key: ServiceName; value not in use, meaningless
TestCasesCreatedTargetGroupNames map[string]bool //key: TargetGroupName; value: not in use, meaningless
TestCasesCreatedK8sResource []client.Object
}

func NewFramework(ctx context.Context) *Framework {
Expand All @@ -82,10 +83,8 @@ func NewFramework(ctx context.Context) *Framework {

SetDefaultEventuallyTimeout(180 * time.Second)
SetDefaultEventuallyPollingInterval(10 * time.Second)
AfterEach(func() { framework.ExpectToBeClean(ctx) })
AfterSuite(func() { framework.CleanTestEnvironment(ctx) })
// TODO: in BeforeSuite(), need to make sure current cluster's VPC don't have any Vpc ServiceNetwork association,
// and current k8s cluster is clean (no existing service, deployment(pods), gateway, httproute)
BeforeEach(func() { framework.ExpectToBeClean(ctx) })
AfterEach(func() { framework.CleanTestEnvironment(ctx) })
return framework
}

Expand All @@ -98,6 +97,11 @@ func (env *Framework) ExpectToBeClean(ctx context.Context) {
})

currentClusterVpcId := os.Getenv("CLUSTER_VPC_ID")
retrievedServiceNetworkVpcAssociations, _ := env.LatticeClient.ListServiceNetworkVpcAssociationsAsList(ctx, &vpclattice.ListServiceNetworkVpcAssociationsInput{
VpcIdentifier: aws.String(currentClusterVpcId),
})
Logger(ctx).Infof("Expect VPC used by current cluster don't have any ServiceNetworkVPCAssociation, if it has you should manually delete it")
Expect(len(retrievedServiceNetworkVpcAssociations)).To(Equal(0))
Eventually(func(g Gomega) {
retrievedServiceNetworks, _ := env.LatticeClient.ListServiceNetworksAsList(ctx, &vpclattice.ListServiceNetworksInput{})
for _, sn := range retrievedServiceNetworks {
Expand Down Expand Up @@ -144,41 +148,49 @@ func (env *Framework) ExpectToBeClean(ctx context.Context) {
//}
}

func (env *Framework) CleanServiceNetworkVpcAssociationForCurrentK8sClusterVpc() {
currentClusterVpcId := os.Getenv("CLUSTER_VPC_ID")
snvas, err := env.LatticeClient.ListServiceNetworkVpcAssociationsAsList(env.ctx, &vpclattice.ListServiceNetworkVpcAssociationsInput{
VpcIdentifier: aws.String(currentClusterVpcId),
})
Expect(err).To(BeNil())
if len(snvas) == 0 {
return
}
for _, snva := range snvas {
_, err := env.LatticeClient.DeleteServiceNetworkVpcAssociationWithContext(env.ctx, &vpclattice.DeleteServiceNetworkVpcAssociationInput{
ServiceNetworkVpcAssociationIdentifier: snva.Id,
})
Expect(err).To(BeNil())
}
Eventually(func(g Gomega) {
snvas, err := env.LatticeClient.ListServiceNetworkVpcAssociationsAsList(env.ctx, &vpclattice.ListServiceNetworkVpcAssociationsInput{
VpcIdentifier: aws.String(currentClusterVpcId),
})
g.Expect(err).To(BeNil())
g.Expect(snvas).To(BeEmpty())
}).Should(Succeed())
}

func (env *Framework) CleanTestEnvironment(ctx context.Context) {
defer GinkgoRecover()
Logger(ctx).Info("Cleaning the test environment")
// Kubernetes API Objects
namespaces := &v1.NamespaceList{}
Expect(env.List(ctx, namespaces)).WithOffset(1).To(Succeed())
Eventually(func(g Gomega) {
for _, namespace := range namespaces.Items {
if namespace.Name == "kube-system" {
continue
}
parallel.ForEach(TestObjects, func(testObject TestObject, _ int) {
log.Println("ExpectDeleteAllToSucceed for testObject.Type", testObject.Type, "in namespace", namespace.Name)
env.ExpectDeleteAllToSucceed(ctx, testObject.Type, namespace.Name)
})
}
}).Should(Succeed())
for _, object := range env.TestCasesCreatedK8sResource {
Logger(ctx).Infof("Deleting k8s resource %s %s/%s", reflect.TypeOf(object), object.GetNamespace(), object.GetName())
env.Delete(ctx, object)
//Ignore resource-not-found error here, as the test case logic itself could already clear the resources
}

//Theoretically, Deleting all k8s resource by `env.ExpectDeleteAllToSucceed()`, will make controller delete all related VPC Lattice resource,
//but the controller is still developing in the progress and may leaking some vPCLattice resource, need to invoke vpcLattice api to double confirm and delete leaking resource.
env.DeleteAllFrameworkTracedServiceNetworks(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's NOT do these automatically. The test should failed and stopped if this is the case, and let developer debug why it is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, actually now no where invoke the CleanTestEnvironment(), we could remove this method, but maybe we could keep it in case we use it in the future?

env.DeleteAllFrameworkTracedVpcLatticeServices(ctx)
env.DeleteAllFrameworkTracedTargetGroups(ctx)
Eventually(func(g Gomega) {
for _, namespace := range namespaces.Items {
if namespace.Name == "kube-system" {
continue
}
parallel.ForEach(TestObjects, func(testObject TestObject, _ int) {
log.Println("EventuallyExpectNoneFound for testObject.Type", testObject.Type, "in namespace", namespace.Name)
defer GinkgoRecover()
env.EventuallyExpectNoneFound(ctx, testObject.ListType)
})
}
}).Should(Succeed())
env.EventuallyExpectNotFound(ctx, env.TestCasesCreatedK8sResource...)
env.TestCasesCreatedK8sResource = nil

}

Expand Down Expand Up @@ -210,9 +222,10 @@ func (env *Framework) ExpectDeleteAllToSucceed(ctx context.Context, object clien
func (env *Framework) EventuallyExpectNotFound(ctx context.Context, objects ...client.Object) {
Eventually(func(g Gomega) {
for _, object := range objects {
Logger(ctx).Infof("Checking whether %s %s %s is not found", reflect.TypeOf(object), object.GetNamespace(), object.GetName())
g.Expect(errors.IsNotFound(env.Get(ctx, client.ObjectKeyFromObject(object), object))).To(BeTrue())
}
}).Should(Succeed())
}).WithOffset(1).Should(Succeed())
}

func (env *Framework) EventuallyExpectNoneFound(ctx context.Context, objectList client.ObjectList) {
Expand Down Expand Up @@ -277,8 +290,15 @@ func (env *Framework) GetTargets(ctx context.Context, targetGroup *vpclattice.Ta
var found []*vpclattice.TargetSummary
Eventually(func(g Gomega) {
log.Println("Trying to retrieve registered targets for targetGroup", targetGroup)
log.Println("deployment.Spec.Selector.MatchLabels:", deployment.Spec.Selector.MatchLabels)
podList := &v1.PodList{}
g.Expect(env.List(ctx, podList, client.MatchingLabels(deployment.Spec.Selector.MatchLabels))).To(Succeed())
expectedMatchingLables := make(map[string]string, len(deployment.Spec.Selector.MatchLabels))
for k, v := range deployment.Spec.Selector.MatchLabels {
expectedMatchingLables[k] = v
}
expectedMatchingLables[DiscoveryLabel] = "true"
log.Println("Expected matching labels:", expectedMatchingLables)
g.Expect(env.List(ctx, podList, client.MatchingLabels(expectedMatchingLables))).To(Succeed())
g.Expect(podList.Items).To(HaveLen(int(*deployment.Spec.Replicas)))
retrievedTargets, err := env.LatticeClient.ListTargetsAsList(ctx, &vpclattice.ListTargetsInput{TargetGroupIdentifier: targetGroup.Id})
g.Expect(err).To(BeNil())
Expand Down Expand Up @@ -324,7 +344,25 @@ func (env *Framework) DeleteAllFrameworkTracedServiceNetworks(ctx aws.Context) {
}
}

//TODO: check and delete serviceNetwork VPC associations
var allServiceNetworkVpcAssociationIdsToBeDeleted []*string
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use test code to cleanup the resource for now. Let developer debug why it is leaked and manually clean them up.

Also, if these are not cleanup, the test should stop

for _, snIdWithRemainingAssociations := range serviceNetworkIdsWithRemainingAssociations {
associations, err := env.LatticeClient.ListServiceNetworkVpcAssociationsAsList(ctx, &vpclattice.ListServiceNetworkVpcAssociationsInput{
ServiceNetworkIdentifier: snIdWithRemainingAssociations,
})
Expect(err).ToNot(HaveOccurred())

snvaIds := lo.Map(associations, func(association *vpclattice.ServiceNetworkVpcAssociationSummary, _ int) *string {
return association.Id
})
allServiceNetworkVpcAssociationIdsToBeDeleted = append(allServiceNetworkVpcAssociationIdsToBeDeleted, snvaIds...)
}

for _, snvaId := range allServiceNetworkVpcAssociationIdsToBeDeleted {
_, err := env.LatticeClient.DeleteServiceNetworkVpcAssociationWithContext(ctx, &vpclattice.DeleteServiceNetworkVpcAssociationInput{
ServiceNetworkVpcAssociationIdentifier: snvaId,
})
Expect(err).ToNot(HaveOccurred())
}

var allServiceNetworkServiceAssociationIdsToBeDeleted []*string

Expand All @@ -348,12 +386,20 @@ func (env *Framework) DeleteAllFrameworkTracedServiceNetworks(ctx aws.Context) {
}

Eventually(func(g Gomega) {
for _, snvaId := range allServiceNetworkVpcAssociationIdsToBeDeleted {
_, err := env.LatticeClient.GetServiceNetworkVpcAssociationWithContext(ctx, &vpclattice.GetServiceNetworkVpcAssociationInput{
ServiceNetworkVpcAssociationIdentifier: snvaId,
})
if err != nil {
g.Expect(err.(awserr.Error).Code()).To(Equal(vpclattice.ErrCodeResourceNotFoundException))
}
}
for _, snsaId := range allServiceNetworkServiceAssociationIdsToBeDeleted {
_, err := env.LatticeClient.GetServiceNetworkServiceAssociationWithContext(ctx, &vpclattice.GetServiceNetworkServiceAssociationInput{
ServiceNetworkServiceAssociationIdentifier: snsaId,
})
if err != nil {
Expect(err.(awserr.Error).Code()).To(Equal(vpclattice.ErrCodeResourceNotFoundException))
g.Expect(err.(awserr.Error).Code()).To(Equal(vpclattice.ErrCodeResourceNotFoundException))
}
}
}).Should(Succeed())
Expand Down Expand Up @@ -492,6 +538,7 @@ func (env *Framework) DeleteAllFrameworkTracedTargetGroups(ctx aws.Context) {
}

func (env *Framework) GetVpcLatticeServiceDns(httpRouteName string, httpRouteNamespace string) string {
log.Println("GetVpcLatticeServiceDns: ", httpRouteName, httpRouteNamespace)
httproute := v1beta1.HTTPRoute{}
env.Get(env.ctx, types.NamespacedName{Name: httpRouteName, Namespace: httpRouteNamespace}, &httproute)
vpcLatticeServiceDns := httproute.Annotations[controllers.LatticeAssignedDomainName]
Expand Down
18 changes: 13 additions & 5 deletions test/pkg/test/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,23 @@ func (env *Framework) NewGateway() *v1beta1.Gateway {
},
Spec: v1beta1.GatewaySpec{
GatewayClassName: "amazon-vpc-lattice",
Listeners: []v1beta1.Listener{{
Name: "http",
Protocol: v1beta1.HTTPProtocolType,
Port: 80,
}},
Listeners: []v1beta1.Listener{
{
Name: "http",
Protocol: v1beta1.HTTPProtocolType,
Port: 80,
},
{
Name: "https",
Protocol: v1beta1.HTTPSProtocolType,
Port: 443,
},
},
},
Status: v1beta1.GatewayStatus{},
},
)
env.TestCasesCreatedServiceNetworkNames[gateway.Name] = true
env.TestCasesCreatedK8sResource = append(env.TestCasesCreatedK8sResource, gateway)
return gateway
}
1 change: 1 addition & 0 deletions test/pkg/test/header_match_httproute.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,6 @@ func (env *Framework) NewHeaderMatchHttpRoute(parentRefsGateway *v1beta1.Gateway
})

env.TestCasesCreatedServiceNames[latticestore.AWSServiceName(httpRoute.Name, httpRoute.Namespace)] = true
env.TestCasesCreatedK8sResource = append(env.TestCasesCreatedK8sResource, httpRoute)
return httpRoute
}
8 changes: 6 additions & 2 deletions test/pkg/test/httpapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ func (env *Framework) NewHttpApp(options HTTPAppOptions) (*appsv1.Deployment, *v
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
"app": options.Name,
"app": options.Name,
DiscoveryLabel: "true",
},
},
Spec: v1.PodSpec{
Expand All @@ -53,6 +54,9 @@ func (env *Framework) NewHttpApp(options HTTPAppOptions) (*appsv1.Deployment, *v
}, options.MergeFromDeployment...)

service := New(&v1.Service{
TypeMeta: metav1.TypeMeta{
Kind: "Service",
},
Spec: v1.ServiceSpec{
Selector: map[string]string{
"app": options.Name,
Expand All @@ -65,7 +69,7 @@ func (env *Framework) NewHttpApp(options HTTPAppOptions) (*appsv1.Deployment, *v
},
}, options.MergeFromService...)
env.TestCasesCreatedTargetGroupNames[latticestore.TargetGroupName(service.Name, service.Namespace)] = true

env.TestCasesCreatedK8sResource = append(env.TestCasesCreatedK8sResource, service, deployment)
return deployment, service

}
10 changes: 7 additions & 3 deletions test/pkg/test/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@ var (
)

func New[T client.Object](t T, mergeFrom ...T) T {
t.SetName(RandomName())
t.SetNamespace("default")
if t.GetName() == "" {
t.SetName(RandomName())
}
if t.GetNamespace() == "" {
t.SetNamespace("default")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, we also need to test things in non-default namespace, e.g.

  • httproute
  • gateway

}
t.SetLabels(map[string]string{DiscoveryLabel: "true"})
return MustMerge(t, mergeFrom...)
}
Expand All @@ -39,5 +43,5 @@ func RandomName() string {
sequentialNumberLock.Lock()
defer sequentialNumberLock.Unlock()
sequentialNumber++
return strings.ToLower(fmt.Sprintf("%s-%d-%s", randomdata.SillyName(), sequentialNumber, randomdata.Alphanumeric(10)))
return strings.ToLower(fmt.Sprintf("%s-%d-%s", randomdata.SillyName()[:5], sequentialNumber, randomdata.Alphanumeric(10)))
}
Loading