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

clusterVersion analyzer does not detect 1.29 correctly in EKS #1441

Closed
xavpaice opened this issue Jan 31, 2024 · 8 comments · Fixed by #1449
Closed

clusterVersion analyzer does not detect 1.29 correctly in EKS #1441

xavpaice opened this issue Jan 31, 2024 · 8 comments · Fixed by #1449
Assignees

Comments

@xavpaice
Copy link
Member

xavpaice commented Jan 31, 2024

Bug Description

A report arrived stating that when the query when: "< 1.29.0" is used in the outcomes section of the clusterVersion analyzer, a cluster running 1.29 on EKS does not report correctly (i.e. it reports true for this condition).

Expected Behavior

EKS running k8s 1.29 should evaluate false for when: "< 1.29.0"

Steps To Reproduce

Create an EKS cluster with k8s 1.29
use the following preflight spec:

apiVersion: troubleshoot.sh/v1beta2
kind: Preflight
metadata:
  name: kubernetes-version
spec:
  collectors: []
  analyzers:
    - clusterVersion:
        outcomes:
          - fail:
              when: < 1.24.0
              message: >-
                The application requires at Kubernetes 1.24.0 or later, and
                recommends 1.29.0.
              uri: 'https://kubernetes.io'
          - warn:
              when: < 1.29.0
              message: >-
                Your cluster meets the minimum version of Kubernetes, but we
                recommend you update to 1.29.0 or later.
              uri: 'https://kubernetes.io'
          - pass:
              message: >-
                Your cluster meets the recommended and required versions of
                Kubernetes.

The output should be 'pass', but is 'warn'.

Additional Context

Include the following information.

  • Troubleshoot version. If you built from source, note that including the version of Go you used to build with.
  • Operating system
  • Operating system version
  • Other details that might be helpful in diagnosing the problem

Edited: corrected EBS -> EKS @adamancini

@gilvan-fernandes
Copy link

Notice that it is EKS, not EBS.
And, the problem happens on SupportBundle and Prefligh.

The additional information is on the support-bundle:

{
"info": {
"major": "1",
"minor": "29+",
"gitVersion": "v1.29.0-eks-c417bb3",
"gitCommit": "787475c0c70a6bc04f58927faf4d30969314cf59",
"gitTreeState": "clean",
"buildDate": "2023-12-13T19:28:31Z",
"goVersion": "go1.21.5",
"compiler": "gc",
"platform": "linux/amd64"
},
"string": "v1.29.0-eks-c417bb3"
}

kind: SupportBundle
spec:
versionNumber: v0.76.4-0.20231102041618-a7bb9ea31e61

@xavpaice xavpaice changed the title clusterVersion analyzer does not detect 1.29 correctly in EBS clusterVersion analyzer does not detect 1.29 correctly in EKS Feb 1, 2024
@xavpaice
Copy link
Member Author

xavpaice commented Feb 1, 2024

From my test EKS 1.29:

{
  "info": {
    "major": "1",
    "minor": "29+",
    "gitVersion": "v1.29.0-eks-c417bb3",
    "gitCommit": "787475c0c70a6bc04f58927faf4d30969314cf59",
    "gitTreeState": "clean",
    "buildDate": "2023-12-13T19:28:31Z",
    "goVersion": "go1.21.5",
    "compiler": "gc",
    "platform": "linux/amd64"
  },
  "string": "v1.29.0-eks-c417bb3"
}

From a k3s test cluster at 1.29:

{
  "info": {
    "major": "1",
    "minor": "29",
    "gitVersion": "v1.29.0+k3s1",
    "gitCommit": "3190a5faa28d7a0d428c756d67adcab7eb11e6a5",
    "gitTreeState": "clean",
    "buildDate": "2023-12-22T00:03:57Z",
    "goVersion": "go1.21.5",
    "compiler": "gc",
    "platform": "linux/amd64"
  },
  "string": "v1.29.0+k3s1"
}

I suspect the issue is with the minor having an extra +.

@nvanthao nvanthao self-assigned this Feb 1, 2024
@nvanthao nvanthao added the wontfix This will not be worked on label Feb 1, 2024
@nvanthao
Copy link
Member

nvanthao commented Feb 1, 2024

Hi @gilvan-fernandes,

Troubleshoot is parsing the K8S version using string field. For our case, EKS it is parsed asv1.29.0-eks-c417bb3.
Per SemVer syntax, -eks-c417bb3 is Pre-Release version. Which is why it falls under range < 1.29.0.

Other distributions do not have this issue as they has correct release version name.

I would suggest us updating the spec with < 1.29.0-0 to cater for checks in EKS.

Tested on my end

   --- PASS Required Kubernetes Version
      --- Your cluster meets the recommended and required versions of Kubernetes.
--- PASS   kubernetes-version
PASS

@xavpaice xavpaice closed this as completed Feb 1, 2024
@gilvan-fernandes
Copy link

Hello

Thanks for the quick solution. Which kots version will fix it? Do you have a release date?

@ajp-io
Copy link
Member

ajp-io commented Feb 2, 2024

@xavpaice Using < 1.29.0-0 is a workaround, but IMO this still needs a fix in Troubleshoot. If we know that EKS versions this way, we should account for it and do something like strip the pre-release version when we compare. Otherwise customers will keep running into this issue.

Happy to discuss this internally if you disagree, but I think we're kicking this can down the road instead of creating an experience that is easy for customers to use.

@xavpaice
Copy link
Member Author

xavpaice commented Feb 5, 2024

The semver comparison in Troubleshoot is correct, the version string that EKS supplies is a prerelease version which should be marked lower than the release version (see https://semver.org/#spec-item-11).

If we want to change our product to handle the incorrect string from EKS, we must ensure that this only affects EKS version strings and not others that are standards compliant, otherwise folks will see inconsistent results from the analyzer.

This is tracked in an Amazon issue, you can see the discussion in aws/containers-roadmap#1404. It does look like fixing it will cause their customers other problems so I understand the reluctance to change things at this stage.

We're left with two options:

  • have folks use the workaround for EKS clusters, or
  • modify Troubleshoot to detect EKS version strings and treat them specifically with some workaround (regex substitution?).

I'll reopen this while we discuss the optimal path.

FWIW, this is not currently on the Troubleshoot roadmap, and therefore there's no release date available for Troubleshoot, and KOTS will only see an update after a Troubleshoot release.

@xavpaice xavpaice reopened this Feb 5, 2024
xavpaice added a commit that referenced this issue Feb 5, 2024
The EKS version string returned is not semver compliant.  To work around this, we remove
the suffix for version strings that contain -eks-.

Fixes #1441
@DexterYan
Copy link
Member

osVersionMinor, _ = strconv.ParseInt(osVersionParts[1], 10, 64)

if the string is 29+, our osVersionMinor, _ = strconv.ParseInt(osVersionParts[1], 10, 64) will return 0
same issue with kots sc-98355

xavpaice added a commit that referenced this issue Feb 5, 2024
* Add workaround for EKS version string

The EKS version string returned is not semver compliant.  To work around this, we remove
the suffix for version strings that contain -eks-.

Fixes #1441

* Add parsing version test cases

* Rename function

---------

Co-authored-by: Evans Mungai <evans@replicated.com>
@xavpaice
Copy link
Member Author

xavpaice commented Feb 5, 2024

Change was released in Troubleshoot v0.82.0. This will need to be updated in KOTS to get released there.

@xavpaice xavpaice removed the wontfix This will not be worked on label Feb 5, 2024
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.

5 participants