reduce endpoint name length limitation to 15 chars, validate on port duplication

[yangcao77]

Signed-off-by: Stephanie [email protected]

What does this PR do?:

as discussed in cabel call, reduce endpoint name length limitation to 15 chars
update the validation rule and validation functions, disallow port duplication in same container component.

Which issue(s) this PR fixes:

api side change for #700

PR acceptance criteria:

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

• Open new test/doc issues under the devfile/api repo
• Check each criteria if:
• There is a separate tracking issue. Add the issue link under the criteria
  or
• test/doc updates are made as part of this PR
• If unchecked, explain why it's not needed

• Unit/Functional tests

• QE Integration test

• Documentation

• Client Impact

How to test changes / Special notes to the reviewer:

[amisevsk]

Before merging this PR, we need to make sure it can safely update through this version in OLM -- I suspect reducing the length might cause issues. I'll check it out when I have a moment.

[yangcao77]

@amisevsk Any updates?

[amisevsk]

Tested updating from a current DevWorkspace spec to one including these changes through OLM:

1. OLM upgrade completes without issue
2. Invalid resources that exist on the cluster remain as-is, unmodified
3. Editing DevWorkspaces/DevWorkspaceTemplates with endpoint names longer than 15 characters is not possible unless the endpoint name is also lengthened.

While 1 and 2 are good news, 3 is concerning as it impacts apps and tools. While a user can read the message and update their DevWorkspace appropriately, anything that updates DevWorkspaces will have trouble:

• The DevWorkspace Operator will enter a tight loop trying to update an invalid DevWorkspace without additional workarounds (and those workarounds may be invalid). This extends further as the DevWorkspace cannot be failed.
• Tooling such as web-terminal-exec and Theia will fail to idle/shut down workspaces

If the restriction was on editing specific fields (i.e. if patches that don't touch endpoints were valid) then I would say this PR is fine. As it stands, this change should ideally go into v1alpha3 DevWorkspaces. We can still go ahead with it now, but we expose the DevWorkspace Operator, Web Terminal, and existing users to complicated issues.

Another alternative would be to set the reduced name length limit in the Devfile spec only (and leave DevWorkspaces at 63 characters).

cc @l0rd WDYT?

To test the upgrade process

1. Apply the following yamls to an OpenShift cluster

   oc new-project devworkspace-operator-test
   cat <<EOF | oc apply -f -
   apiVersion: operators.coreos.com/v1
   kind: OperatorGroup
   metadata:
     name: dwo-test
   EOF
   
   cat <<EOF | oc apply -f - 
   apiVersion: operators.coreos.com/v1alpha1
   kind: CatalogSource
   metadata:
     name: custom-devworkspace-operator-catalog
     namespace: devworkspace-operator-test
   spec:
     sourceType: grpc
     image: quay.io/amisevsk/devworkspace-operator-index-test:endpoint-name-length
     publisher: Red Hat
     displayName: DevWorkspace Operator Catalog
   EOF
   
   cat <<EOF | oc apply -f -
   apiVersion: operators.coreos.com/v1alpha1
   kind: Subscription
   metadata:
     name: devworkspace-operator
   spec:
     channel: fast
     name: devworkspace-operator
     source: custom-devworkspace-operator-catalog
     sourceNamespace: devworkspace-operator-test
     installPlanApproval: Manual
     startingCSV: devworkspace-operator.v0.90.0
   EOF

2. In the OpenShift Operators UI, approve the v0.90.0 install (uses current limits)
3. Create DevWorkspaces as appropriate
4. In Operators UI, approve the v0.91.0 install (uses 15 char endpoint name limit)

[yangcao77]

any potential inconsistent behavior might be introduced if we only change the length limitation in devfile not in DevWorkspaces? @kadel @valaparthvi

[valaparthvi]

@yangcao77 I had discussion with Tomas, and he is of the opinion that there shouldn't be any inconsistent behavior, so we're good.

[yangcao77]

@l0rd Any thoughts on Angel's suggestion? Only make the change in devfile spec, and leave devworkspace still supporting 63 chars?

[valaparthvi]

Another alternative would be to set the reduced name length limit in the Devfile spec only (and leave DevWorkspaces at 63 characters).

How will this work? Wouldn't DevWorkspace run into error while trying to parse a devfile with the 63 char limit?

[l0rd]

@l0rd Any thoughts on Angel's suggestion? Only make the change in devfile spec, and leave devworkspace still supporting 63 chars?

@yangcao77 @amisevsk why Che / DevWorkspace / WebTerminal aren't affected original odo problem?

[amisevsk]

why Che / DevWorkspace / WebTerminal aren't affected original odo problem?

Since the original limit was 63 chars, DWO does what Che does and names the ports <port-number>-<protocol>.

[yangcao77]

why Che / DevWorkspace / WebTerminal aren't affected original odo problem?

Since the original limit was 63 chars, DWO does what Che does and names the ports <port-number>-<protocol>.

This is how devfile library currently behaves: https://github.com/devfile/library/blob/fe639c227027582c450c6d1c723a9755dafb5e9e/pkg/devfile/generator/utils.go#L49
And this is the issue Odo customer raises up, that the container port name does not match the endpoint name. So DWO doea have the same issue. The only thing is if we want to include the fix in current v1alpha2, or wait for v1alpha3 for the change to be adopted.

[yangcao77]

Another alternative would be to set the reduced name length limit in the Devfile spec only (and leave DevWorkspaces at 63 characters).

How will this work? Wouldn't DevWorkspace run into error while trying to parse a devfile with the 63 char limit?

only devfile spec will be updated, and will leave 63 chars limit in devworkspace template spec. it wouldn't fail if devworkspace try to parse a 15-char limit field to a 63-char limit field

[amisevsk]

With changes in the devfile spec, DWO can optionally choose to use the endpoint name as the port name if it is less than 15 chars long, fixing this issue for DWO as well (assuming the workspace is coming from a devfile and not created as a DevWorkspace directly).

Edit: PR devfile/devworkspace-operator#736 makes DWO use the endpoint name as the container port name if it is short enough.

[l0rd]

Edit: PR devfile/devworkspace-operator#736 makes DWO use the endpoint name as the container port name if it is short enough.

That means that if Che and odo use the same devfile the resulting container port should have the same name right? It's not a big deal if the DevWorkspace allow longer endpoint names, but we should make the devfile to kubernetes objects transformation as consistent as possible among our tools.

[yangcao77]

With changes in the devfile spec, DWO can optionally choose to use the endpoint name as the port name if it is less than 15 chars long, fixing this issue for DWO as well (assuming the workspace is coming from a devfile and not created as a DevWorkspace directly).

Edit: PR devfile/devworkspace-operator#736 makes DWO use the endpoint name as the container port name if it is short enough.

Library would also do that to support backward compatibility, in case any existing customers with devfile 2.1.0/2.2.0 has an endpoint name > 15 chars. @valaparthvi

[yangcao77]

@amisevsk I have updated the PR to only shorten the endpoint name limit for devfile spec. can you help review when you have time?

[amisevsk]

LGTM, thanks @yangcao77!

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, yangcao77

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment