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

connectivity: improve handling existing cilium-test namespace #119

Closed
errordeveloper opened this issue Mar 11, 2021 · 4 comments · Fixed by #127
Closed

connectivity: improve handling existing cilium-test namespace #119

errordeveloper opened this issue Mar 11, 2021 · 4 comments · Fixed by #127

Comments

@errordeveloper
Copy link
Contributor

The original reason for opening #115 was that I noticed an issue with cilium connectivity check.

When cilium-test namespace exists, srcDeploymentNeeded remains false and the deploy function returns without doing anything else.

func (k *K8sConnectivityCheck) deploy(ctx context.Context) error {
var srcDeploymentNeeded, dstDeploymentNeeded bool
if k.params.ForceDeploy {
if err := k.deleteDeployments(ctx, k.clients.src); err != nil {
return err
}
}
_, err := k.clients.src.GetNamespace(ctx, k.params.TestNamespace, metav1.GetOptions{})
if err != nil {
srcDeploymentNeeded = true
// In a single cluster environment, the source client is also
// responsibel for destination deployments
if k.params.MultiCluster == "" {
dstDeploymentNeeded = true
}
k.Log("✨ [%s] Creating namespace for connectivity check...", k.clients.src.ClusterName())
_, err = k.clients.src.CreateNamespace(ctx, k.params.TestNamespace, metav1.CreateOptions{})
if err != nil {
return fmt.Errorf("unable to create namespace %s: %s", k.params.TestNamespace, err)
}
}
if k.params.MultiCluster != "" {
if k.params.ForceDeploy {
if err := k.deleteDeployments(ctx, k.clients.dst); err != nil {
return err
}
}
_, err = k.clients.dst.GetNamespace(ctx, k.params.TestNamespace, metav1.GetOptions{})
if err != nil {
dstDeploymentNeeded = true
k.Log("✨ [%s] Creating namespace for connectivity check...", k.clients.dst.ClusterName())
_, err = k.clients.dst.CreateNamespace(ctx, k.params.TestNamespace, metav1.CreateOptions{})
if err != nil {
return fmt.Errorf("unable to create namespace %s: %s", k.params.TestNamespace, err)
}
}
}
if srcDeploymentNeeded {
k.Log("✨ [%s] Deploying echo-same-node service...", k.clients.src.ClusterName())
svc := newService(echoSameNodeDeploymentName, map[string]string{"name": echoSameNodeDeploymentName}, serviceLabels, "http", 8080)
_, err = k.clients.src.CreateService(ctx, k.params.TestNamespace, svc, metav1.CreateOptions{})
if err != nil {
return err
}
if k.params.MultiCluster != "" {
k.Log("✨ [%s] Deploying echo-other-node service...", k.clients.src.ClusterName())
svc := newService(echoOtherNodeDeploymentName, map[string]string{"name": echoOtherNodeDeploymentName}, serviceLabels, "http", 8080)
svc.ObjectMeta.Annotations = map[string]string{}
svc.ObjectMeta.Annotations["io.cilium/global-service"] = "true"
_, err = k.clients.src.CreateService(ctx, k.params.TestNamespace, svc, metav1.CreateOptions{})
if err != nil {
return err
}
}
echoDeployment := newDeployment(deploymentParameters{
Name: echoSameNodeDeploymentName,
Kind: kindEchoName,
Port: 8080,
Image: "quay.io/cilium/json-mock:1.2",
Affinity: &corev1.Affinity{
PodAffinity: &corev1.PodAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: []corev1.PodAffinityTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{
{Key: "name", Operator: metav1.LabelSelectorOpIn, Values: []string{clientDeploymentName}},
},
},
TopologyKey: "kubernetes.io/hostname",
},
},
},
},
ReadinessProbe: newLocalReadinessProbe(8080, "/"),
})
_, err = k.clients.src.CreateDeployment(ctx, k.params.TestNamespace, echoDeployment, metav1.CreateOptions{})
if err != nil {
return fmt.Errorf("unable to create deployment %s: %s", echoSameNodeDeploymentName, err)
}
k.Log("✨ [%s] Deploying client service...", k.clients.src.ClusterName())
clientDeployment := newDeployment(deploymentParameters{Name: clientDeploymentName, Kind: kindClientName, Port: 8080, Image: "quay.io/cilium/alpine-curl:1.0", Command: []string{"/bin/ash", "-c", "sleep 10000000"}})
_, err = k.clients.src.CreateDeployment(ctx, k.params.TestNamespace, clientDeployment, metav1.CreateOptions{})
if err != nil {
return fmt.Errorf("unable to create deployment %s: %s", clientDeploymentName, err)
}
}
if dstDeploymentNeeded {
if !k.params.SingleNode || k.params.MultiCluster != "" {
k.Log("✨ [%s] Deploying echo-other-node service...", k.clients.dst.ClusterName())
svc := newService(echoOtherNodeDeploymentName, map[string]string{"name": echoOtherNodeDeploymentName}, serviceLabels, "http", 8080)
if k.params.MultiCluster != "" {
svc.ObjectMeta.Annotations = map[string]string{}
svc.ObjectMeta.Annotations["io.cilium/global-service"] = "true"
}
_, err = k.clients.dst.CreateService(ctx, k.params.TestNamespace, svc, metav1.CreateOptions{})
if err != nil {
return err
}
echoOtherNodeDeployment := newDeployment(deploymentParameters{
Name: echoOtherNodeDeploymentName,
Kind: kindEchoName,
Port: 8080,
Image: "quay.io/cilium/json-mock:1.2",
Affinity: &corev1.Affinity{
PodAntiAffinity: &corev1.PodAntiAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: []corev1.PodAffinityTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{
{Key: "name", Operator: metav1.LabelSelectorOpIn, Values: []string{clientDeploymentName}},
},
},
TopologyKey: "kubernetes.io/hostname",
},
},
},
},
ReadinessProbe: newLocalReadinessProbe(8080, "/"),
})
_, err = k.clients.dst.CreateDeployment(ctx, k.params.TestNamespace, echoOtherNodeDeployment, metav1.CreateOptions{})
if err != nil {
return fmt.Errorf("unable to create deployment %s: %s", echoOtherNodeDeploymentName, err)
}
}
}
return nil
}

Getting #115 done maybe be a little more involved than I hoped, so a simpler fixes will be needed.

@tgraf
Copy link
Member

tgraf commented Mar 12, 2021

@errordeveloper So you need the ability for the namespace to exist but not the deployments inside?

@errordeveloper
Copy link
Contributor Author

@tgraf exactly :)

@errordeveloper
Copy link
Contributor Author

The use-case I have right is for OpenShift, where use of hostNetwork: true is gated by SecurityContextConstraints.
So one alternatively would be to add platform-specific initialisation logic in the connectivity check, which I thought was said to be undesirable. I could take a look and see if a PSP can be used instead of SecurityContextConstraints, in my understanding the API serve the same purpose, if we could use a PSP, we could add it for all platforms.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Mar 12, 2021

OpenShift doesn't actually support PSPs.

tgraf added a commit that referenced this issue Mar 16, 2021
Don't assume presence of namespace indicates presence of deployments.

Fixes: #119

Signed-off-by: Thomas Graf <thomas@cilium.io>
tgraf added a commit that referenced this issue Mar 17, 2021
Don't assume presence of namespace indicates presence of deployments.

Fixes: #119

Signed-off-by: Thomas Graf <thomas@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants