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.

Sign up for GitHub

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

fix(kubernetes): support public labels #452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(kubernetes): support public labels #452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions app/discovery/autostop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,13 @@ import (
func StopAllUnregisteredInstances(ctx context.Context, provider providers.Provider, registered []string) error {
log.Info("Stopping all unregistered running instances")

log.Tracef("Retrieving all instances with label [%v=true]", LabelEnable)
instances, err := provider.InstanceList(ctx, providers.InstanceListOptions{
All: false, // Only running containers
Labels: []string{LabelEnable},
})
log.Trace("Retrieving all registered instances")
instances, err := provider.List(ctx)
if err != nil {
return err
}

log.Tracef("Found %v instances with label [%v=true]", len(instances), LabelEnable)
log.Tracef("Found %v instances", len(instances))
names := make([]string, 0, len(instances))
for _, instance := range instances {
names = append(names, instance.Name)
Expand Down
15 changes: 4 additions & 11 deletions app/discovery/autostop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"github.com/sablierapp/sablier/app/discovery"
"github.com/sablierapp/sablier/app/providers"
"github.com/sablierapp/sablier/app/providers/mock"
"github.com/sablierapp/sablier/app/types"
"testing"
Expand All @@ -22,11 +21,8 @@ func TestStopAllUnregisteredInstances(t *testing.T) {
}
registered := []string{"instance1"}

// Set up expectations for InstanceList
mockProvider.On("InstanceList", ctx, providers.InstanceListOptions{
All: false,
Labels: []string{discovery.LabelEnable},
}).Return(instances, nil)
// Set up expectations for List
mockProvider.On("List", ctx).Return(instances, nil)

// Set up expectations for Stop
mockProvider.On("Stop", ctx, "instance2").Return(nil)
Expand Down Expand Up @@ -54,11 +50,8 @@ func TestStopAllUnregisteredInstances_WithError(t *testing.T) {
}
registered := []string{"instance1"}

// Set up expectations for InstanceList
mockProvider.On("InstanceList", ctx, providers.InstanceListOptions{
All: false,
Labels: []string{discovery.LabelEnable},
}).Return(instances, nil)
// Set up expectations for List
mockProvider.On("List", ctx).Return(instances, nil)

// Set up expectations for Stop with error
mockProvider.On("Stop", ctx, "instance2").Return(errors.New("stop error"))
Expand Down
10 changes: 3 additions & 7 deletions app/providers/docker/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,16 @@ import (
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/filters"
"github.com/sablierapp/sablier/app/discovery"
"github.com/sablierapp/sablier/app/providers"
"github.com/sablierapp/sablier/app/types"
"strings"
)

func (provider *DockerClassicProvider) InstanceList(ctx context.Context, options providers.InstanceListOptions) ([]types.Instance, error) {
func (provider *DockerClassicProvider) List(ctx context.Context) ([]types.Instance, error) {
args := filters.NewArgs()
for _, label := range options.Labels {
args.Add("label", label)
args.Add("label", fmt.Sprintf("%s=true", label))
}
args.Add("label", fmt.Sprintf("%s=true", discovery.LabelEnable))

containers, err := provider.Client.ContainerList(ctx, container.ListOptions{
All: options.All,
All: true,
Filters: args,
})

Expand Down
8 changes: 2 additions & 6 deletions app/providers/dockerswarm/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,14 @@ import (
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/swarm"
"github.com/sablierapp/sablier/app/discovery"
"github.com/sablierapp/sablier/app/providers"
"github.com/sablierapp/sablier/app/types"
log "github.com/sirupsen/logrus"
"strconv"
)

func (provider *DockerSwarmProvider) InstanceList(ctx context.Context, options providers.InstanceListOptions) ([]types.Instance, error) {
func (provider *DockerSwarmProvider) List(ctx context.Context) ([]types.Instance, error) {
args := filters.NewArgs()
for _, label := range options.Labels {
args.Add("label", label)
args.Add("label", fmt.Sprintf("%s=true", label))
}
args.Add("label", fmt.Sprintf("%s=true", discovery.LabelEnable))

services, err := provider.Client.ServiceList(ctx, dockertypes.ServiceListOptions{
Filters: args,
Expand Down
70 changes: 49 additions & 21 deletions app/providers/kubernetes/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,51 @@ package kubernetes
import (
"context"
"github.com/sablierapp/sablier/app/discovery"
"github.com/sablierapp/sablier/app/providers"
"github.com/sablierapp/sablier/app/types"
log "github.com/sirupsen/logrus"
v1 "k8s.io/api/apps/v1"
core_v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"
"strconv"
"strings"
)

func (provider *KubernetesProvider) InstanceList(ctx context.Context, options providers.InstanceListOptions) ([]types.Instance, error) {
deployments, err := provider.deploymentList(ctx, options)
const (
LabelEnable = "sablierapp.dev/enable"
LabelGroup = "sablierapp.dev/group"
LabelGroupDefaultValue = "default"
LabelReplicas = "sablierapp.dev/replicas"
LabelReplicasDefaultValue uint64 = 1
)

func (provider *KubernetesProvider) List(ctx context.Context) ([]types.Instance, error) {
deployments, err := provider.deploymentList(ctx)
if err != nil {
return nil, err
}

statefulSets, err := provider.statefulSetList(ctx, options)
statefulSets, err := provider.statefulSetList(ctx)
if err != nil {
return nil, err
}

return append(deployments, statefulSets...), nil
}

func (provider *KubernetesProvider) deploymentList(ctx context.Context, options providers.InstanceListOptions) ([]types.Instance, error) {
func (provider *KubernetesProvider) deploymentList(ctx context.Context) ([]types.Instance, error) {
requirement, err := labels.NewRequirement(LabelEnable, selection.Equals, []string{"true"})
if err != nil {
return nil, err
}
requirementDeprecated, err := labels.NewRequirement(discovery.LabelEnable, selection.Equals, []string{"true"})
if err != nil {
return nil, err
}
selector := labels.NewSelector()
selector = selector.Add(*requirement, *requirementDeprecated)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you merge the 2 selector type here, the deployments will need to have both legacy and new labels because the selector would result in sablier.enable=true,sablierapp.dev/enable=true.

I don't know how to handle this, either deprecate old selectors or add a flag to toggle legacy/new (or make it customizable?)

Copy link
Contributor

Choose a reason for hiding this comment

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

After a bit more tweaking I got it to work, I added a bunch of Trace logging to help me find the root cause and it was mainly due to old discovery.LabelGroup usage.

I rebased main onto my branch (449-xxx) so I cannot open a PR on yours but you can see my diff here: main...BapRx:sablier:449-kubernetes-labels-not-public

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, I will check it out.

deployments, err := provider.Client.AppsV1().Deployments(core_v1.NamespaceAll).List(ctx, metav1.ListOptions{
LabelSelector: strings.Join(options.Labels, ","),
LabelSelector: selector.String(),
})

if err != nil {
Expand All @@ -49,23 +67,23 @@ func (provider *KubernetesProvider) deploymentToInstance(d v1.Deployment) types.
var group string
var replicas uint64

if _, ok := d.Labels[discovery.LabelEnable]; ok {
if g, ok := d.Labels[discovery.LabelGroup]; ok {
if _, ok := d.Labels[LabelEnable]; ok {
if g, ok := d.Labels[LabelGroup]; ok {
group = g
} else {
group = discovery.LabelGroupDefaultValue
group = LabelGroupDefaultValue
}

if r, ok := d.Labels[discovery.LabelReplicas]; ok {
if r, ok := d.Labels[LabelReplicas]; ok {
atoi, err := strconv.Atoi(r)
if err != nil {
log.Warnf("Defaulting to default replicas value, could not convert value \"%v\" to int: %v", r, err)
replicas = discovery.LabelReplicasDefaultValue
replicas = LabelReplicasDefaultValue
} else {
replicas = uint64(atoi)
}
} else {
replicas = discovery.LabelReplicasDefaultValue
replicas = LabelReplicasDefaultValue
}
}

Expand All @@ -82,9 +100,19 @@ func (provider *KubernetesProvider) deploymentToInstance(d v1.Deployment) types.
}
}

func (provider *KubernetesProvider) statefulSetList(ctx context.Context, options providers.InstanceListOptions) ([]types.Instance, error) {
func (provider *KubernetesProvider) statefulSetList(ctx context.Context) ([]types.Instance, error) {
requirement, err := labels.NewRequirement(LabelEnable, selection.Equals, []string{"true"})
if err != nil {
return nil, err
}
requirementDeprecated, err := labels.NewRequirement(discovery.LabelEnable, selection.Equals, []string{"true"})
if err != nil {
return nil, err
}
selector := labels.NewSelector()
selector = selector.Add(*requirement, *requirementDeprecated)
statefulSets, err := provider.Client.AppsV1().StatefulSets(core_v1.NamespaceAll).List(ctx, metav1.ListOptions{
LabelSelector: strings.Join(options.Labels, ","),
LabelSelector: selector.String(),
})

if err != nil {
Expand All @@ -104,23 +132,23 @@ func (provider *KubernetesProvider) statefulSetToInstance(ss v1.StatefulSet) typ
var group string
var replicas uint64

if _, ok := ss.Labels[discovery.LabelEnable]; ok {
if g, ok := ss.Labels[discovery.LabelGroup]; ok {
if _, ok := ss.Labels[LabelEnable]; ok {
if g, ok := ss.Labels[LabelGroup]; ok {
group = g
} else {
group = discovery.LabelGroupDefaultValue
group = LabelGroupDefaultValue
}

if r, ok := ss.Labels[discovery.LabelReplicas]; ok {
if r, ok := ss.Labels[LabelReplicas]; ok {
atoi, err := strconv.Atoi(r)
if err != nil {
log.Warnf("Defaulting to default replicas value, could not convert value \"%v\" to int: %v", r, err)
replicas = discovery.LabelReplicasDefaultValue
replicas = LabelReplicasDefaultValue
} else {
replicas = uint64(atoi)
}
} else {
replicas = discovery.LabelReplicasDefaultValue
replicas = LabelReplicasDefaultValue
}
}

Expand Down
4 changes: 2 additions & 2 deletions app/providers/mock/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func (m *ProviderMock) GetGroups(ctx context.Context) (map[string][]string, erro
args := m.Called(ctx)
return args.Get(0).(map[string][]string), args.Error(1)
}
func (m *ProviderMock) InstanceList(ctx context.Context, options providers.InstanceListOptions) ([]types.Instance, error) {
args := m.Called(ctx, options)
func (m *ProviderMock) List(ctx context.Context) ([]types.Instance, error) {
args := m.Called(ctx)
return args.Get(0).([]types.Instance), args.Error(1)
}

Expand Down
2 changes: 1 addition & 1 deletion app/providers/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ type Provider interface {
Stop(ctx context.Context, name string) error
GetState(ctx context.Context, name string) (instance.State, error)
GetGroups(ctx context.Context) (map[string][]string, error)
InstanceList(ctx context.Context, options InstanceListOptions) ([]types.Instance, error)
List(ctx context.Context) ([]types.Instance, error)

NotifyInstanceStopped(ctx context.Context, instance chan<- string)
}
3 changes: 1 addition & 2 deletions app/providers/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package providers

type InstanceListOptions struct {
All bool
Labels []string
All bool
}
10 changes: 5 additions & 5 deletions docs/providers/kubernetes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,17 @@ kind: Deployment
metadata:
name: whoami
labels:
app: whoami
sablier.enable: "true"
sablier.group: mygroup
app.kubernetes.io/name: whoami
sablierapp.dev/enable: "true"
sablierapp.dev/group: mygroup
spec:
selector:
matchLabels:
app: whoami
app.kubernetes.io/name: whoami
template:
metadata:
labels:
app: whoami
app.kubernetes.io/name: whoami
spec:
containers:
- name: whoami
Expand Down
26 changes: 13 additions & 13 deletions plugins/traefik/e2e/kubernetes/manifests/deployment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: whoami-deployment
name: whoami
labels:
app: whoami
sablier.enable: "true"
sablier.group: "E2E"
app.kubernetes.io/name: whoami
sablierapp.dev/enable: "true"
sablierapp.dev/group: "E2E"
spec:
replicas: 0
selector:
matchLabels:
app: whoami
app.kubernetes.io/name: whoami
template:
metadata:
labels:
app: whoami
app.kubernetes.io/name: whoami
spec:
containers:
- name: whoami
Expand All @@ -36,7 +36,7 @@ spec:
targetPort: 80
port: 80
selector:
app: whoami
app.kubernetes.io/name: whoami
---
apiVersion: traefik.io/v1alpha1
kind: Middleware
Expand Down Expand Up @@ -160,18 +160,18 @@ kind: Deployment
metadata:
name: nginx-deployment
labels:
app: nginx
sablier.enable: "true"
sablier.group: "E2E"
app.kubernetes.io/name: nginx
sablierapp.dev/enable: "true"
sablierapp.dev/group: "E2E"
spec:
replicas: 0
selector:
matchLabels:
app: nginx
app.kubernetes.io/name: nginx
template:
metadata:
labels:
app: nginx
app.kubernetes.io/name: nginx
spec:
containers:
- name: nginx
Expand All @@ -195,7 +195,7 @@ spec:
targetPort: 80
port: 80
selector:
app: nginx
app.kubernetes.io/name: nginx
---
apiVersion: traefik.io/v1alpha1
kind: Middleware
Expand Down
Loading