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

Jump to bottom

allow to configure specific annotations #1070

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

allow to configure specific annotations #1070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1000cd3
feat: allow to configure specific annotations
gbonnefille Mar 21, 2023
b5023ec
fix: sync the routing.annotations property
gbonnefille Mar 22, 2023
4321e9c
fix: wording
gbonnefille Mar 22, 2023
240e0c9
doc: TODO
gbonnefille Mar 22, 2023
a8992c0
fix: improve tests
gbonnefille Mar 22, 2023
e28c740
fix: regenerate code and CRDs
gbonnefille Mar 22, 2023
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
5 changes: 5 additions & 0 deletions apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ type RoutingConfig struct {
// DevWorkspaces. However, changing the proxy configuration for the DevWorkspace Operator itself
// requires restarting the controller deployment.
ProxyConfig *Proxy `json:"proxyConfig,omitempty"`
// Annotations defines the map of annotations to add on to the routing object.
//
// Use this property to set the annotations expected by the routing framework used
// in your cluster (nginx, traefik, ...)
Annotations map[string]string `json:"annotations,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to find a more descriptive name here -- perhaps RoutingAnnotations? I feel that annotations is too generic here (though I'm open to being swayed on this one).

}

type Proxy struct {
Expand Down
9 changes: 8 additions & 1 deletion apis/controller/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 26 additions & 12 deletions controllers/controller/devworkspacerouting/solvers/basic_solver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,33 @@ import (
"github.com/devfile/devworkspace-operator/pkg/infrastructure"
)

var routeAnnotations = func(endpointName string) map[string]string {
return map[string]string{
"haproxy.router.openshift.io/rewrite-target": "/",
constants.DevWorkspaceEndpointNameAnnotation: endpointName,
func appendMap[K, V comparable](dst map[K]V, m map[K]V) {
for k, v := range m {
dst[k] = v
}
}

var nginxIngressAnnotations = func(endpointName string) map[string]string {
return map[string]string{
"kubernetes.io/ingress.class": "nginx",
"nginx.ingress.kubernetes.io/rewrite-target": "/",
"nginx.ingress.kubernetes.io/ssl-redirect": "false",
var routeAnnotations = map[string]string{
"haproxy.router.openshift.io/rewrite-target": "/",
}

var nginxIngressAnnotations = map[string]string{
"kubernetes.io/ingress.class": "nginx",
"nginx.ingress.kubernetes.io/rewrite-target": "/",
"nginx.ingress.kubernetes.io/ssl-redirect": "false",
}

func createAnnotations(endpointName string, routingAnnotations map[string]string, defaultAnnotations map[string]string) map[string]string {
annotations := map[string]string{
constants.DevWorkspaceEndpointNameAnnotation: endpointName,
}

if routingAnnotations == nil || len(routingAnnotations) == 0 {
appendMap(annotations, defaultAnnotations)
} else {
appendMap(annotations, routingAnnotations)
}
return annotations
Comment on lines -32 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

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

A better approach for setting a default value if the configuration is unspecified would be similar to what we do in the config package -- this avoids needing functions that take an optional value and a default.

}

// Basic solver exposes endpoints without any authentication
Expand Down Expand Up @@ -67,12 +80,13 @@ func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRou
services := getServicesForEndpoints(spec.Endpoints, workspaceMeta)
services = append(services, GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta)...)
routingObjects.Services = services
// TODO: Use workspace-scoped routing annotations to allow overriding
routingAnnotations := config.GetGlobalConfig().Routing.Annotations
gbonnefille marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +83 to +84
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will likely not be possible to get workspace-scoped annotations, as this is within the context of the DevWorkspaceRouting reconciler. In fact, we should avoid using config within controllers/controller/devworkspacerouting/... in general, as the routing reconciler may be run outside the DWO context (as it is in Eclipse Che, for example).

In an ideal world, the configuration of DWO is propagated into the DevWorkspaceRouting CR directly in some way. However, some choices made in the past make this very difficult -- likely the only way to do this is to serialize the map to JSON and add it as an annotation on the routing CR.

if infrastructure.IsOpenShift() {
routingObjects.Routes = getRoutesForSpec(routingSuffix, spec.Endpoints, workspaceMeta)
routingObjects.Routes = getRoutesForSpec(routingSuffix, spec.Endpoints, workspaceMeta, routingAnnotations)
} else {
routingObjects.Ingresses = getIngressesForSpec(routingSuffix, spec.Endpoints, workspaceMeta)
routingObjects.Ingresses = getIngressesForSpec(routingSuffix, spec.Endpoints, workspaceMeta, routingAnnotations)
}

return routingObjects, nil
}

Expand Down
16 changes: 8 additions & 8 deletions controllers/controller/devworkspacerouting/solvers/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,33 +153,33 @@ func getServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointLis
}
}

func getRoutesForSpec(routingSuffix string, endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata) []routeV1.Route {
func getRoutesForSpec(routingSuffix string, endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata, annotations map[string]string) []routeV1.Route {
var routes []routeV1.Route
for _, machineEndpoints := range endpoints {
for _, endpoint := range machineEndpoints {
if endpoint.Exposure != controllerv1alpha1.PublicEndpointExposure {
continue
}
routes = append(routes, getRouteForEndpoint(routingSuffix, endpoint, meta))
routes = append(routes, getRouteForEndpoint(routingSuffix, endpoint, meta, annotations))
}
}
return routes
}

func getIngressesForSpec(routingSuffix string, endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata) []networkingv1.Ingress {
func getIngressesForSpec(routingSuffix string, endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata, annotations map[string]string) []networkingv1.Ingress {
var ingresses []networkingv1.Ingress
for _, machineEndpoints := range endpoints {
for _, endpoint := range machineEndpoints {
if endpoint.Exposure != controllerv1alpha1.PublicEndpointExposure {
continue
}
ingresses = append(ingresses, getIngressForEndpoint(routingSuffix, endpoint, meta))
ingresses = append(ingresses, getIngressForEndpoint(routingSuffix, endpoint, meta, annotations))
}
}
return ingresses
}

func getRouteForEndpoint(routingSuffix string, endpoint controllerv1alpha1.Endpoint, meta DevWorkspaceMetadata) routeV1.Route {
func getRouteForEndpoint(routingSuffix string, endpoint controllerv1alpha1.Endpoint, meta DevWorkspaceMetadata, annotations map[string]string) routeV1.Route {
targetEndpoint := intstr.FromInt(endpoint.TargetPort)
endpointName := common.EndpointName(endpoint.Name)
return routeV1.Route{
Expand All @@ -189,7 +189,7 @@ func getRouteForEndpoint(routingSuffix string, endpoint controllerv1alpha1.Endpo
Labels: map[string]string{
constants.DevWorkspaceIDLabel: meta.DevWorkspaceId,
},
Annotations: routeAnnotations(endpointName),
Annotations: createAnnotations(endpoint.Name, annotations, routeAnnotations),
},
Spec: routeV1.RouteSpec{
Host: common.WorkspaceHostname(routingSuffix, meta.DevWorkspaceId),
Expand All @@ -209,7 +209,7 @@ func getRouteForEndpoint(routingSuffix string, endpoint controllerv1alpha1.Endpo
}
}

func getIngressForEndpoint(routingSuffix string, endpoint controllerv1alpha1.Endpoint, meta DevWorkspaceMetadata) networkingv1.Ingress {
func getIngressForEndpoint(routingSuffix string, endpoint controllerv1alpha1.Endpoint, meta DevWorkspaceMetadata, annotations map[string]string) networkingv1.Ingress {
endpointName := common.EndpointName(endpoint.Name)
hostname := common.EndpointHostname(routingSuffix, meta.DevWorkspaceId, endpointName, endpoint.TargetPort)
ingressPathType := networkingv1.PathTypeImplementationSpecific
Expand All @@ -220,7 +220,7 @@ func getIngressForEndpoint(routingSuffix string, endpoint controllerv1alpha1.End
Labels: map[string]string{
constants.DevWorkspaceIDLabel: meta.DevWorkspaceId,
},
Annotations: nginxIngressAnnotations(endpoint.Name),
Annotations: createAnnotations(endpoint.Name, annotations, nginxIngressAnnotations),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this approach; there's a pretty distant link between where annotations is defined and where it's used, and how it's defined completely changes the code significantly further down the call stack. The function createAnnotations behaves completely differently based on whether annotations is an empty map here:

	if routingAnnotations == nil || len(routingAnnotations) == 0 {
		appendMap(annotations, defaultAnnotations)
	} else {
		appendMap(annotations, routingAnnotations)
	}

},
Spec: networkingv1.IngressSpec{
Rules: []networkingv1.IngressRule{
Expand Down
113 changes: 113 additions & 0 deletions controllers/controller/devworkspacerouting/solvers/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package solvers

import (
"testing"

"github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
)

func TestGetRouteForEndpointAnnotations(t *testing.T) {
tests := []struct {
name string

annotations map[string]string

expectedAnnotations map[string]string
}{
{
name: "Gets default OpenShift route annotation when annotations aren't defined",

annotations: nil,

expectedAnnotations: map[string]string{
"controller.devfile.io/endpoint_name": "Endpoint",
"haproxy.router.openshift.io/rewrite-target": "/",
},
},
{
name: "Gets default OpenShift route annotation when annotations are empty",

annotations: map[string]string{},

expectedAnnotations: map[string]string{
"controller.devfile.io/endpoint_name": "Endpoint",
"haproxy.router.openshift.io/rewrite-target": "/",
},
},
{
name: "Gets default OpenShift route annotation when annotations are defined",

annotations: map[string]string{
"example.com/extra": "val",
},

expectedAnnotations: map[string]string{
"controller.devfile.io/endpoint_name": "Endpoint",
"example.com/extra": "val",
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
route := getRouteForEndpoint("routingSuffix", v1alpha1.Endpoint{Name: "Endpoint"}, DevWorkspaceMetadata{DevWorkspaceId: "WorkspaceTest"}, tt.annotations)
assert.Equal(t, tt.expectedAnnotations, route.Annotations, "Annotations should match: Diff: %s", cmp.Diff(tt.expectedAnnotations, route.Annotations))
})
}
}

func TestGetIngressForEndpointAnnotations(t *testing.T) {
gbonnefille marked this conversation as resolved.
Show resolved Hide resolved
tests := []struct {
name string

annotations map[string]string

expectedAnnotations map[string]string
}{
{
name: "Gets default Kubernetes ingress annotation when annotations aren't defined",

annotations: nil,

expectedAnnotations: map[string]string{
"controller.devfile.io/endpoint_name": "Endpoint",
"kubernetes.io/ingress.class": "nginx",
"nginx.ingress.kubernetes.io/rewrite-target": "/",
"nginx.ingress.kubernetes.io/ssl-redirect": "false",
},
},
{
name: "Gets default Kubernetes ingress annotation when annotations are empty",

annotations: map[string]string{},

expectedAnnotations: map[string]string{
"controller.devfile.io/endpoint_name": "Endpoint",
"kubernetes.io/ingress.class": "nginx",
"nginx.ingress.kubernetes.io/rewrite-target": "/",
"nginx.ingress.kubernetes.io/ssl-redirect": "false",
},
},
{
name: "Gets default Kubernetes ingress annotation when annotations are defined",

annotations: map[string]string{
"kubernetes.io/ingress.class": "traefik",
},

expectedAnnotations: map[string]string{
"controller.devfile.io/endpoint_name": "Endpoint",
"kubernetes.io/ingress.class": "traefik",
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ingress := getIngressForEndpoint("routingSuffix", v1alpha1.Endpoint{Name: "Endpoint"}, DevWorkspaceMetadata{DevWorkspaceId: "WorkspaceTest"}, tt.annotations)
assert.Equal(t, tt.expectedAnnotations, ingress.Annotations, "Annotations should match: Diff: %s", cmp.Diff(tt.expectedAnnotations, ingress.Annotations))
})
}
}
8 changes: 8 additions & 0 deletions deploy/deployment/kubernetes/combined.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions ...s/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions deploy/deployment/openshift/combined.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions ...t/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions pkg/config/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ func mergeConfig(from, to *controller.OperatorConfiguration) {
if from.Routing.DefaultRoutingClass != "" {
to.Routing.DefaultRoutingClass = from.Routing.DefaultRoutingClass
}
if from.Routing.Annotations != nil {
to.Routing.Annotations = from.Routing.Annotations
}
if from.Routing.ClusterHostSuffix != "" {
to.Routing.ClusterHostSuffix = from.Routing.ClusterHostSuffix
}
Expand Down Expand Up @@ -387,6 +390,9 @@ func GetCurrentConfigString(currConfig *controller.OperatorConfiguration) string
routing := currConfig.Routing
var config []string
if routing != nil {
if routing.Annotations != nil && len(routing.Annotations) > 0 {
config = append(config, fmt.Sprintf("routing.Annotations=%s", routing.Annotations))
}
if routing.ClusterHostSuffix != "" && routing.ClusterHostSuffix != defaultConfig.Routing.ClusterHostSuffix {
config = append(config, fmt.Sprintf("routing.clusterHostSuffix=%s", routing.ClusterHostSuffix))
}
Expand Down