-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
Implement first set of federated service e2e tests. #26636
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,283 @@ | ||
/* | ||
Copyright 2016 The Kubernetes Authors All rights reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package e2e | ||
|
||
import ( | ||
"fmt" | ||
"os" | ||
"time" | ||
|
||
"k8s.io/kubernetes/federation/apis/federation" | ||
"k8s.io/kubernetes/federation/client/clientset_generated/federation_internalclientset" | ||
"k8s.io/kubernetes/pkg/api" | ||
"k8s.io/kubernetes/pkg/api/v1" | ||
"k8s.io/kubernetes/pkg/client/clientset_generated/release_1_3" | ||
"k8s.io/kubernetes/pkg/client/restclient" | ||
"k8s.io/kubernetes/pkg/client/unversioned/clientcmd" | ||
clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api" | ||
"k8s.io/kubernetes/pkg/util/intstr" | ||
"k8s.io/kubernetes/pkg/util/wait" | ||
"k8s.io/kubernetes/test/e2e/framework" | ||
|
||
. "github.com/onsi/ginkgo" | ||
. "github.com/onsi/gomega" | ||
) | ||
|
||
const ( | ||
UserAgentName = "federation-e2e-service-controller" | ||
// TODO(madhusudancs): Using the same values as defined in the federated | ||
// service controller. Replace it with the values from the e2e framework. | ||
KubeAPIQPS = 20.0 | ||
KubeAPIBurst = 30 | ||
|
||
FederatedServiceTimeout = 5 * time.Minute | ||
|
||
FederatedServiceName = "federated-service" | ||
FederatedServicePod = "federated-service-test-pod" | ||
|
||
// TODO: Only suppoprts IPv4 addresses. Also add a regexp for IPv6 addresses. | ||
FederatedIPAddrRegexp = `(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Wouldn't it be better to use a simpler regex like [0-9]{1,3}.){4} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This is not specific to anything federated. Would it not be better to call it something like IPv4Regexp and put it in the utils library for other people to also use? |
||
FederatedDNS1123Regexp = `([a-z0-9]([-a-z0-9]*[a-z0-9])?\.)*([a-z0-9]([-a-z0-9]*[a-z0-9])?)` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Similar comment to above. |
||
) | ||
|
||
var _ = framework.KubeDescribe("Service [Feature:Federation]", func() { | ||
var clusterClientSets []*release_1_3.Clientset | ||
var federationName string | ||
f := framework.NewDefaultFederatedFramework("service") | ||
|
||
BeforeEach(func() { | ||
framework.SkipUnlessFederated(f.Client) | ||
|
||
// TODO: Federation API server should be able to answer this. | ||
if federationName = os.Getenv("FEDERATION_NAME"); federationName == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No one is setting this env var anywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I wasn't expecting this PR to merge before waking up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have #27333 to fix this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
// Tests cannot proceed without this value, so fail early here. | ||
framework.Failf("FEDERATION_NAME environment variable must be set") | ||
} | ||
|
||
contexts := f.GetUnderlyingFederatedContexts() | ||
|
||
for _, context := range contexts { | ||
framework.Logf("Creating cluster object: %s (%s)", context.Name, context.Cluster.Cluster.Server) | ||
cluster := federation.Cluster{ | ||
ObjectMeta: api.ObjectMeta{ | ||
Name: context.Name, | ||
}, | ||
Spec: federation.ClusterSpec{ | ||
ServerAddressByClientCIDRs: []federation.ServerAddressByClientCIDR{ | ||
{ | ||
ClientCIDR: "0.0.0.0/0", | ||
ServerAddress: context.Cluster.Cluster.Server, | ||
}, | ||
}, | ||
}, | ||
} | ||
_, err := f.FederationClientset.Federation().Clusters().Create(&cluster) | ||
framework.ExpectNoError(err, "Creating cluster") | ||
} | ||
|
||
var clusterList *federation.ClusterList | ||
By("Obtaining a list of all the clusters") | ||
if err := wait.PollImmediate(framework.Poll, FederatedServiceTimeout, func() (bool, error) { | ||
var err error | ||
clusterList, err = f.FederationClientset.Federation().Clusters().List(api.ListOptions{}) | ||
if err != nil { | ||
return false, err | ||
} | ||
framework.Logf("%d clusters registered, waiting for %d", len(clusterList.Items), len(contexts)) | ||
if len(clusterList.Items) == len(contexts) { | ||
return true, nil | ||
} | ||
return false, nil | ||
}); err != nil { | ||
framework.Failf("Failed to list registered clusters: %+v", err) | ||
} | ||
|
||
for _, cluster := range clusterList.Items { | ||
framework.Logf("Creating a clientset for the cluster %s", cluster.Name) | ||
|
||
Expect(framework.TestContext.KubeConfig).ToNot(Equal(""), "KubeConfig must be specified to load clusters' client config") | ||
kubecfg, err := clientcmd.LoadFromFile(framework.TestContext.KubeConfig) | ||
framework.ExpectNoError(err, "error loading KubeConfig: %v", err) | ||
|
||
cfgOverride := &clientcmd.ConfigOverrides{ | ||
ClusterInfo: clientcmdapi.Cluster{ | ||
Server: cluster.Spec.ServerAddressByClientCIDRs[0].ServerAddress, | ||
}, | ||
} | ||
ccfg := clientcmd.NewNonInteractiveClientConfig(*kubecfg, cluster.Name, cfgOverride, clientcmd.NewDefaultClientConfigLoadingRules()) | ||
cfg, err := ccfg.ClientConfig() | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
cfg.QPS = KubeAPIQPS | ||
cfg.Burst = KubeAPIBurst | ||
clset := release_1_3.NewForConfigOrDie(restclient.AddUserAgent(cfg, UserAgentName)) | ||
clusterClientSets = append(clusterClientSets, clset) | ||
} | ||
}) | ||
|
||
AfterEach(func() { | ||
framework.SkipUnlessFederated(f.Client) | ||
|
||
// Delete the registered clusters in the federation API server. | ||
clusterList, err := f.FederationClientset.Federation().Clusters().List(api.ListOptions{}) | ||
Expect(err).NotTo(HaveOccurred()) | ||
for _, cluster := range clusterList.Items { | ||
err := f.FederationClientset.Federation().Clusters().Delete(cluster.Name, &api.DeleteOptions{}) | ||
Expect(err).NotTo(HaveOccurred()) | ||
} | ||
}) | ||
|
||
It("should be able to discover a federated service", func() { | ||
framework.SkipUnlessFederated(f.Client) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also skip all of the BeforeEach() stuff above (cluster registration etc) unless we're federated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
createService(f.FederationClientset, clusterClientSets, f.Namespace.Name) | ||
|
||
svcDNSNames := []string{ | ||
FederatedServiceName, | ||
fmt.Sprintf("%s.%s", FederatedServiceName, f.Namespace.Name), | ||
fmt.Sprintf("%s.%s.svc.cluster.local.", FederatedServiceName, f.Namespace.Name), | ||
fmt.Sprintf("%s.%s.%s", FederatedServiceName, f.Namespace.Name, federationName), | ||
fmt.Sprintf("%s.%s.%s.svc.cluster.local.", FederatedServiceName, f.Namespace.Name, federationName), | ||
} | ||
for _, name := range svcDNSNames { | ||
discoverService(f, name, true) | ||
} | ||
}) | ||
|
||
It("should be able to discover a non-local federated service", func() { | ||
framework.SkipUnlessFederated(f.Client) | ||
|
||
createService(f.FederationClientset, clusterClientSets, f.Namespace.Name) | ||
|
||
// Delete a federated service shard in the default e2e Kubernetes cluster. | ||
err := f.Clientset_1_3.Core().Services(f.Namespace.Name).Delete(FederatedServiceName, &api.DeleteOptions{}) | ||
Expect(err).NotTo(HaveOccurred()) | ||
waitForFederatedServiceShard(f.Clientset_1_3, f.Namespace.Name, nil, 0) | ||
|
||
localSvcDNSNames := []string{ | ||
FederatedServiceName, | ||
fmt.Sprintf("%s.%s", FederatedServiceName, f.Namespace.Name), | ||
fmt.Sprintf("%s.%s.svc.cluster.local.", FederatedServiceName, f.Namespace.Name), | ||
} | ||
for _, name := range localSvcDNSNames { | ||
discoverService(f, name, false) | ||
} | ||
|
||
svcDNSNames := []string{ | ||
fmt.Sprintf("%s.%s.%s", FederatedServiceName, f.Namespace.Name, federationName), | ||
fmt.Sprintf("%s.%s.%s.svc.cluster.local.", FederatedServiceName, f.Namespace.Name, federationName), | ||
} | ||
for _, name := range svcDNSNames { | ||
discoverService(f, name, true) | ||
} | ||
}) | ||
}) | ||
|
||
// waitForFederatedServiceShard waits until the number of shards of a given federated | ||
// service reaches the expected value, i.e. numSvcs in the given individual Kubernetes | ||
// cluster. If the shard count, i.e. numSvcs is expected to be at least one, then | ||
// it also checks if the first shard's name and spec matches that of the given service. | ||
func waitForFederatedServiceShard(cs *release_1_3.Clientset, namespace string, service *api.Service, numSvcs int) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor nit: Probably worth adding a brief comment describing what this function does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I don't understand how one could ever have more than 1 instance of the service in a given cluster at the same time. There would be a name clash, right? In which case, is the numSvcs parameter essentially a boolean? Would it ever make sense to call this function with that parameter not equal to either 0 or 1? |
||
By("Fetching a federated service shard") | ||
var clSvcList *v1.ServiceList | ||
if err := wait.PollImmediate(framework.Poll, FederatedServiceTimeout, func() (bool, error) { | ||
var err error | ||
clSvcList, err = cs.Core().Services(namespace).List(api.ListOptions{}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you only want to list the service with the given name (service.Name()) here? I realize that this test should run in it's own namespace, and that there should be no other services in that namespace, but the logic in this function still seems overly complex and confusing to me. Should you not simply be listing the services with the given name, and confirming that the length of the returned list is either 0 or 1 (depending on the value of numServices, which I suspect should be a bool, not an int). |
||
if err != nil { | ||
return false, err | ||
} | ||
n := len(clSvcList.Items) | ||
if n == numSvcs { | ||
return true, nil | ||
} | ||
framework.Logf("%d services found, waiting for %d, trying again in %s", n, numSvcs, framework.Poll) | ||
return false, nil | ||
}); err != nil { | ||
framework.Failf("Failed to list registered clusters: %+v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're listing services here, not clusters. Cut-n-paste error? |
||
} | ||
|
||
if numSvcs > 0 && service != nil { | ||
// Renaming for clarity/readability | ||
clSvc := clSvcList.Items[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only seems to work for numSvcs == 1 ? The semantics of this function are rather confusing. As far as I can tell it will wait until the number of services in the specifed namespace reaches numSvcs, and (presumably this only works if 'service' is not nil) if numSvcs >0, that the first listed service matches the one passed in? You should definitely document the semantics in the header, and also check for service != nil before dereferencing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's right and that's intentional. It only makes sense to execute the block inside the
Thanks for catching that. Added a check for service != nil. Also added the doc string for the function. |
||
Expect(clSvc.Name).To(Equal(service.Name)) | ||
Expect(clSvc.Spec).To(Equal(service.Spec)) | ||
} | ||
} | ||
|
||
func createService(fcs *federation_internalclientset.Clientset, clusterClientSets []*release_1_3.Clientset, namespace string) { | ||
By("Creating a federated service") | ||
labels := map[string]string{ | ||
"foo": "bar", | ||
} | ||
|
||
svc1port := "svc1" | ||
svc2port := "svc2" | ||
|
||
service := &api.Service{ | ||
ObjectMeta: api.ObjectMeta{ | ||
Name: FederatedServiceName, | ||
}, | ||
Spec: api.ServiceSpec{ | ||
Selector: labels, | ||
Ports: []api.ServicePort{ | ||
{ | ||
Name: "portname1", | ||
Port: 80, | ||
TargetPort: intstr.FromString(svc1port), | ||
}, | ||
{ | ||
Name: "portname2", | ||
Port: 81, | ||
TargetPort: intstr.FromString(svc2port), | ||
}, | ||
}, | ||
}, | ||
} | ||
_, err := fcs.Core().Services(namespace).Create(service) | ||
Expect(err).NotTo(HaveOccurred()) | ||
for _, cs := range clusterClientSets { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to add a deferred function here to delete the created service. |
||
waitForFederatedServiceShard(cs, namespace, service, 1) | ||
} | ||
} | ||
|
||
func discoverService(f *framework.Framework, name string, exists bool) { | ||
pod := &api.Pod{ | ||
ObjectMeta: api.ObjectMeta{ | ||
Name: FederatedServicePod, | ||
Labels: map[string]string{"name": FederatedServicePod}, | ||
}, | ||
Spec: api.PodSpec{ | ||
Containers: []api.Container{ | ||
{ | ||
Name: "federated-service-discovery-container", | ||
Image: "gcr.io/google_containers/busybox:1.24", | ||
Command: []string{"sh", "-c", "nslookup", name}, | ||
}, | ||
}, | ||
RestartPolicy: api.RestartPolicyNever, | ||
}, | ||
} | ||
if exists { | ||
f.TestContainerOutputRegexp("federated service discovery", pod, 0, []string{ | ||
`Name:\s+` + FederatedDNS1123Regexp + `\nAddress \d+:\s+` + FederatedIPAddrRegexp + `\s+` + FederatedDNS1123Regexp, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Surely you want the actual name of the service in FederatedDNS1123Regexp? Otherwise you'll match any DNS name that comes back. I guess that doesn't really matter in practise. To make this test more robust I think it would be better to only distinguish between DNS lookup success and failure. The exact output and format of the nslookup tool is mostly irrelevant, and might change over time and/or platform. I checked the nslookup tool and unfortunately it returns zero on both success and failure. But the 'host' tool is a little better for this. It is pretty ubiquitous, and prints the name and address on success, but prints nothing on failure. That might be a simpler, more robust test.
|
||
}) | ||
} else { | ||
f.TestContainerOutputRegexp("federated service discovery", pod, 0, []string{ | ||
`nslookup: can't resolve '` + name + `'`, | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name this federation-service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be renamed to "federation-service" like the way we call other items that belong to the federation. This service is "federated" across a federation, a verb, in addition to being part of the federation.