Added support for annotation substitutions

[dfulgham]

Added websocket annotation for nginx ingress k8s

Description

added the nginx annotation for websocket support in k8s

Related Issue(s)

(https://github.com/flowforge/flowforge-driver-k8s/issues/83)

Checklist

• I have read the contribution guidelines
• Suitable unit/system level tests have been added and they pass Not needed

Labels

• Backport needed? -> add the backport label
• Includes a DB migration? -> add the area:migration label

[hardillb]

Hi,

As mentioned in the issue you have referenced, this should be already possible by passing the annotations to the helm chart when installing.

see:

https://github.com/flowforge/flowforge-driver-k8s/blob/a3b3a44e96df18ab3f40ac4c35cf3b4874fe2696/kubernetes.js#L160

This change would always add the annotation no matter what ingress controller was used. Any solution needs to work for all possible installations and not add unnecessary annotations.

If the proposed solution mentioned in #83 please add a comment explaining why.

[hardillb]

Sorry, I'd forgotten that the annotation needs the service name, which can't be set globally with that annotations from the helm chart.

I will try and dig at the nginx ingress docs to see if there is more global solution, I know that the kubernetes version of the nginx ingress controller doesn't need these flags so hopefully there will be something we can set at a wider scope.

[dfulgham]

I scoured the nginx docs (for a long while) and could not find a way to globally turn on websocket support, this is why I had to add it here.

[dfulgham]

@hardillb
If a string replacement method could be implemented, then we could pass something like

annotations:
"nginx.org/websocket-services": "{{safeName}}"

[hardillb]

Hi,

Nice idea, unfortunately it's not quiet that simple.

The service name is not always just the safeName. Kubernetes has slightly different rules for something, e.g. service names are a stricter version of the hostname rules and can't start with a number, where as the project name is allowed to start with a number, so we have the srv- prefix that gets applied if the name starts with a number. (This ended up being a fix when we ran into the problem, if we had known we would have added a prefix to all).

https://github.com/flowforge/flowforge-driver-k8s/blob/a3b3a44e96df18ab3f40ac4c35cf3b4874fe2696/kubernetes.js#L335C1-L347C2

Lines 336 and 344

So it can't just be a simple substitution, I'll have a look to see if we can expand on this. e.g. we could define a list of available substitutions and then we can do the specific mapping.

[dfulgham]

We can change this to

annotations:
"nginx.org/websocket-services": "{{prefix}}{{safeName}}"

[hardillb]

So it can't just be a simple substitution, I'll have a look to see if we can expand on this. e.g. we could define a list of available substitutions and then we can do the specific mapping.

Sorry, I should have been clearer here, I think rather than expose the whole project object and expect users to know about the prefix, it would be better to just have a list of available options e.g.

• serviceName
• instanceURL

We can document this simpler list in the helm chart README.md

That way if/when we rename things like project (which user facing is called an instance now) it doesn't break things.

[dfulgham]

I've updated the PR with specific properties that would potentially be usable in annotations.

const exposedData = {
        serviceName: `${prefix}${project.safeName}`,
        instanceURL: url.href,
        instanceHost: url.host,
        instanceProtocol: url.protocol,
    }

Which can then be used:

...
   annotations:
      "nginx.org/websocket-services": "{{serviceName}}"
      ...

[hardillb]

I think those are a sensible start to the token names.

If you can fix up the lint errors (which should be flagged on the "Files Changed" page or the details page of the failed tests we can get this merged

[hardillb]

Need to make sure this gets documented in flowforge/helm before release

[dfulgham]

Need to make sure this gets documented in flowforge/helm before release

Will also need to ensure the {{ replacements }} are accepted in the FlowForge Container as well.

[hardillb]

Ahhh, that is going to be a problem, as it will have to be done in the helm chart templating and I'm not sure we can do replacements there