-
Notifications
You must be signed in to change notification settings - Fork 28
Add conformance tests for scs-0210-v2 version policy #488
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
Conversation
Previously, they have been installed as transitive dependencies of the other listed packages and thus, have only been available by coincidence. Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
* Move to an importable Python module for tests * Add pytest and pytest-asyncio test requirements * Add K8sVersion and K8sRelease data classes instead of K8sVersionInfo * Convert CVEVersionInfo into a dataclass * Use the VersionRange concept * Compare based on release age with timedelta * Add support for kubeconfig contexts * Add EOL data for K8s versions * Update script name to reflect new standard name * Escape the dots in regular expressions * Retrieve the CVE id directly Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
b018fd3 to
902f2df
Compare
|
I will have a look. At first glance, it looks really good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it looks really good. Does it test though that the three contexts actually use different K8s versions? I missed that.
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
Yes, it does so by collecting the used "branches" in a set and checking them afterwards. (Release) Branch means tuple(major, minor) in K8s context. Remark: I used the word "branches" here for two reasons: the K8s release docs use it and there is no confusion with the minor version. |
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
|
I've finished the last few missing bits. The PR description is updated and the draft status removed. Please review. If someone other than me merges this, feel free to clean up the squash merge message. |
cah-hbaum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Tested at least with the "stable" part of the cluster and that worked well.
mbuechse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said, I like it very much. A few remarks here and there, which could be converted into an issue for later. Most importantly: try to find as many problems with the clusters as possible before returning to the caller.
|
@martinmo And one question that just occurred to me: are these hard-coded context names practicable? Or could we allow for a more sophisticated scheme? If we knew that three versions are officially supported (which is not always the case), then the command-line argument could look like the following: edit Addendum: maybe this scheme is not compatible with the variables mechanism in the certificate-scope yaml. But something like that. |
|
Please fix this bug that was taken over from the old program: #492 (comment) |
Thanks @cah-hbaum and @mbuechse! Co-authored-by: Hannes Baum <hannes.baum@cloudandheat.com> Co-authored-by: Matthias Büchse <matthias.buechse@cloudandheat.com> Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
I like your idea. We then could simplify some of the code, especially I could remove that "skipped because next context references a cluster that is already EOL" step. Do you intend to change the script call in |
@martinmo I think these specifics are best captured using a variable (just like the kubeconfig itself). We have one Zuul job per subject, and I would set the variable there (as a Zuul/Ansible variable that can then be passed on to the main check script, which in turn it passes it on to the individual script). We shouldn't forget though that in the future we are supposed to create the clusters ourselves. So this could already be fashioned with plugins in mind. So the actual syntax could be |
|
@martinmo One more thing: Should we just ignore any cluster whose branch is no longer supported? Maybe this could facilitate the transition periods. Otherwise people have to change the invocation of the script on the day of EOL. |
Instead of failing immediately, try to test *all* contexts/clusters before exiting the script. Only exit early in critical cases. Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
This is already done. One important caveat though is that the contexts need to be checked from newest to oldest for this detection to work properly. |
|
I incorporated your suggestions (thx @mbuechse and @cah-hbaum) and reworked the error handling, which now works in a similar fashion to the entropy check. But still, critical errors need to return early. Btw, TIL that it's also possible to be SCS-compliant with only two supported K8s versions, at least as of today: v1.27 and v1.28. This is because v1.26 is EOL and v1.29 is still in the 4 month cadence period. |
| - branch: '1.29' | ||
| end-of-life: '2025-02-28' | ||
| - branch: '1.28' | ||
| end-of-life: '2024-10-28' | ||
| - branch: '1.27' | ||
| end-of-life: '2024-06-28' | ||
| - branch: '1.26' | ||
| end-of-life: '2024-02-28' | ||
| - branch: '1.25' | ||
| end-of-life: '2023-10-28' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that bothers me: this file needs to be maintained from now on. I couldn't find a machine readable version upstream, just the Patch releases document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could assume that this rhythm of Oct 28, Feb 28, June 28 goes on, and then we only need to change anything if this rhythm breaks. Then of course we also need to know when these versions come out initially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. With this scheme, another assumption that can break is that only the minor branch increments. I would like to take that idea into a separate issue.
For this PR, what do you think of issuing a warning if the file seems to be out of date at the time of script execution. This is the case if the file lists less than 3 non-EOL releases. And bail out if there are less than 2. With that approach we would at least see it if we monitor the logs in the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issuing a warning if the file seems to be out of date at the time of script execution. This is the case if the file lists less than 3 non-EOL releases. And bail out if there are less than 2.
I've implemented this in the last commit. If you do not agree with that change, I can easily revert it, no problem :-)
That makes sense.
Did you mean |
|
@martinmo Yes, we could also use an URL syntax. I deliberately put v1.29 there instead of stable to indicate that providers could name their stuff whatever they like, and the meaning of |
Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>
This adds the missing conformance tests for scs-0210-v2. It has still some rough edges and some of the code doesn't look pretty, but I think it is good enough to be used.
One thing that needs to be maintained from now on, until we find a better solution, is the new
k8s-eol-data.ymlfile which contains the EOL data of the K8s release branches.To test it, provide a kubeconfig that has the contexts
stable,oldstableandoldoldstable. You can useminikubeto create clusters of different versions, e.g.,minikube create -p stable --kubernetes-version=...and provide-k ~/.kube/configas a script argument.