API: Use new token generation SSP API and remove feature gate #3087

akrejcir · 2024-09-02T12:37:45Z

What this PR does / why we need it:
The token generation API was stabilized in the SSP, and feature gate was removed:
kubevirt/ssp-operator#1018

This PR removes the same feature gate from HCO, and adds a new field in the .spec to enable this feature.

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Jira Ticket:

https://issues.redhat.com/browse/CNV-45065

Release note:

- The feature gate "deployVmConsoleProxy" was deprecated and is now ignored.
- Added a new field ".spec.enableTokenGenerationApi" to enable the token generation API.

kubevirt-bot · 2024-09-02T12:37:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nunnatsa for approval. For more information see the Kubernetes Code Review Process.

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

coveralls · 2024-09-02T13:00:08Z

Pull Request Test Coverage Report for Build 11124276641

Details

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.007%) to 72.344%

Totals
Change from base Build 11091119977:	0.007%
Covered Lines:	5964
Relevant Lines:	8244

💛 - Coveralls

nunnatsa

Thanks for the PR @akrejcir

What will happen on upgrade, if the DeployVMConsoleProxy FG is set?

akrejcir · 2024-09-02T14:02:18Z

Thanks for the PR @akrejcir

What will happen on upgrade, if the DeployVMConsoleProxy FG is set?

Good point, I did not consider update.

Does HCO have functionality to run some logic only during update? It would be good, if we can copy the value of the feature gate to the new field in .spec. But only during update, because in the new version, the feature gate will be ignored.

nunnatsa · 2024-09-02T14:08:05Z

Thanks for the PR @akrejcir
What will happen on upgrade, if the DeployVMConsoleProxy FG is set?

Good point, I did not consider update.

Does HCO have functionality to run some logic only during update? It would be good, if we can copy the value of the feature gate to the new field in .spec. But only during update, because in the new version, the feature gate will be ignored.

HCO does have code that only run during upgrade, but we need to avoid modifying the HyperConverged spec. Another alternative is the mutation webhook. @tiraboschi WDYT?

hco-bot · 2024-09-02T15:42:53Z

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

kubevirt-bot · 2024-09-02T15:42:56Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tiraboschi · 2024-09-03T08:05:47Z

Good point, I did not consider update.
Does HCO have functionality to run some logic only during update? It would be good, if we can copy the value of the feature gate to the new field in .spec. But only during update, because in the new version, the feature gate will be ignored.

HCO does have code that only run during upgrade, but we need to avoid modifying the HyperConverged spec. Another alternative is the mutation webhook. @tiraboschi WDYT?

In assets/upgradePatches.json we are accumulating a set of jsonpatches to be executed during the upgrades (according to semver matching rules) on the spec stanza of the HCO CR.
move and copy operations are also supported. Please take a look at https://github.com/evanphx/json-patch/blob/master/v5/patch_test.go for examples.
I'd suggest to add there an upgradePatch to handle it during upgrades without the need for additional code.

akrejcir · 2024-09-09T10:41:21Z

...

HCO does have code that only run during upgrade, but we need to avoid modifying the HyperConverged spec. Another alternative is the mutation webhook. @tiraboschi WDYT?

In assets/upgradePatches.json we are accumulating a set of jsonpatches to be executed during the upgrades (according to semver matching rules) on the spec stanza of the HCO CR. move and copy operations are also supported. Please take a look at https://github.com/evanphx/json-patch/blob/master/v5/patch_test.go for examples. I'd suggest to add there an upgradePatch to handle it during upgrades without the need for additional code.

Thanks, I will add a patch there.
Which version is checked by the semverRange field? Is it the version from which we are upgrading, or to which we are upgrading? It was not clear from the code.

hco-bot · 2024-09-09T19:08:14Z

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

kubevirt-bot · 2024-09-09T19:08:17Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kubevirt-bot · 2024-09-10T12:46:52Z

@akrejcir: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-hyperconverged-cluster-operator-e2e-k8s-1.29	`25eac49`	link	true	`/test pull-hyperconverged-cluster-operator-e2e-k8s-1.29`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

hco-bot · 2024-09-10T16:53:12Z

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

kubevirt-bot · 2024-09-10T16:53:16Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tests/func-tests/defaults_test.go

The token generation API was stabilized in the SSP, and feature gate was removed: kubevirt/ssp-operator#1018 This commit removes the same feature gate from HCO, and adds a new field in the .spec to enable this feature. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

sonarcloud · 2024-10-01T11:22:24Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

hco-bot · 2024-10-01T13:20:49Z

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure
hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure
hco-e2e-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-operator-sdk-gcp

kubevirt-bot · 2024-10-01T13:20:55Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to this:

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure
hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure
hco-e2e-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-operator-sdk-gcp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

hco-bot · 2024-10-01T14:10:42Z

hco-e2e-consecutive-operator-sdk-upgrades-azure lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws
hco-e2e-operator-sdk-sno-azure lane succeeded.
/override ci/prow/hco-e2e-operator-sdk-sno-aws

kubevirt-bot · 2024-10-01T14:10:48Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws, ci/prow/hco-e2e-operator-sdk-sno-aws

In response to this:

hco-e2e-consecutive-operator-sdk-upgrades-azure lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws
hco-e2e-operator-sdk-sno-azure lane succeeded.
/override ci/prow/hco-e2e-operator-sdk-sno-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2024-10-01T15:33:51Z

@akrejcir: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/hco-e2e-operator-sdk-gcp	`25f2d24`	link	true	`/test hco-e2e-operator-sdk-gcp`
ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws	`25f2d24`	link	true	`/test hco-e2e-consecutive-operator-sdk-upgrades-aws`
ci/prow/hco-e2e-operator-sdk-sno-aws	`25f2d24`	link	false	`/test hco-e2e-operator-sdk-sno-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

akrejcir · 2024-10-02T07:45:06Z

/retest

akrejcir · 2024-10-02T07:45:15Z

Can you please take another look?

hco-bot · 2024-10-02T10:44:39Z

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

kubevirt-bot · 2024-10-02T10:44:43Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

nunnatsa · 2024-10-10T08:17:49Z

assets/upgradePatches.json

+    },
+    {
+      "semverRange": "<1.14.0",
+      "jsonPatch": [
+        {
+          "op": "test",
+          "path": "/spec/featureGates/deployVmConsoleProxy",
+          "value": false
+        },
+        {
+          "op": "add",
+          "path": "/spec/enableTokenGenerationApi",
+          "value": false
+        }
+      ]


@tiraboschi @akrejcir - are we sure about this logic? the deprecated feature gate is false by default. And we want this field to be true by default.

I'm not sure I have good solution for this - we can't tell if the feature gate is false on purpose or was just left with its default value.

My concern we will carry this potential wrong value for all future upgrades.

I wanted to do this update with minimal surprise for the user.
The problem is, as you've written, that we don't know if user decided to disable the feature, or just left it default.
If it was intentionally disabled, it would be a surprise to enable it on update.

But maybe we can enable it by default, and add a note to the documentation.

nunnatsa · 2024-10-10T08:22:51Z

@akrejcir, @tiraboschi

I'm not sure about this PR.

Do we really need a new enabler field to replace the existing feature gate? The term "feature gate" is wrongly used in HCO anyway, and we need to treat the feature gates as "enablers". I think this change will just create mess.

We want to introduce a new API, to fix the wrong feature gate naming anyway.

I suggest to just replace the default value to true.

nunnatsa · 2024-10-10T08:23:09Z

And @akrejcir - sorry for the late response.

akrejcir · 2024-10-10T09:42:55Z

@akrejcir, @tiraboschi

I'm not sure about this PR.

Do we really need a new enabler field to replace the existing feature gate? The term "feature gate" is wrongly used in HCO anyway, and we need to treat the feature gates as "enablers". I think this change will just create mess.

We want to introduce a new API, to fix the wrong feature gate naming anyway.

I suggest to just replace the default value to true.

Thanks for the feedback. I don't have any opinion on HCO API change.
I think it is up to you to decide what should change. Then, I will update this PR.

tiraboschi · 2024-10-11T07:34:55Z

@akrejcir, @tiraboschi

I'm not sure about this PR.

Do we really need a new enabler field to replace the existing feature gate? The term "feature gate" is wrongly used in HCO anyway, and we need to treat the feature gates as "enablers". I think this change will just create mess.

We want to introduce a new API, to fix the wrong feature gate naming anyway.

I agree with this:
the name of the feature gate stanza is misleading but it's already used for other configuration knobs.
For the sake of consistency, let's continue using it until we will be able to move all of them with the next version of the API.

I suggest to just replace the default value to true.

We need to understand if we simply want to propose a new default for future fresh deployments or if we want also to "silently" change it for all the cluster on upgrades.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 2, 2024

kubevirt-bot requested review from machadovilaca and orenc1 September 2, 2024 12:37

kubevirt-bot added the size/L label Sep 2, 2024

akrejcir force-pushed the ssp-remove-vm-proxy-feature-gate branch from 33a9922 to 25eac49 Compare September 2, 2024 12:56

kubevirt-bot added size/M and removed size/L labels Sep 2, 2024

nunnatsa reviewed Sep 2, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2024

akrejcir force-pushed the ssp-remove-vm-proxy-feature-gate branch from 25eac49 to cf2b009 Compare September 9, 2024 14:36

kubevirt-bot added size/L and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M labels Sep 9, 2024

akrejcir force-pushed the ssp-remove-vm-proxy-feature-gate branch from cf2b009 to 4355973 Compare September 9, 2024 14:41

akrejcir force-pushed the ssp-remove-vm-proxy-feature-gate branch 3 times, most recently from e44cc2f to 7dc0c86 Compare September 10, 2024 12:31

akrejcir commented Sep 13, 2024

View reviewed changes

tests/func-tests/defaults_test.go Outdated Show resolved Hide resolved

akrejcir force-pushed the ssp-remove-vm-proxy-feature-gate branch from 7dc0c86 to 25f2d24 Compare October 1, 2024 11:21

nunnatsa reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Use new token generation SSP API and remove feature gate #3087

API: Use new token generation SSP API and remove feature gate #3087

akrejcir commented Sep 2, 2024

kubevirt-bot commented Sep 2, 2024

coveralls commented Sep 2, 2024 •

edited

Loading

nunnatsa left a comment

akrejcir commented Sep 2, 2024

nunnatsa commented Sep 2, 2024

hco-bot commented Sep 2, 2024

kubevirt-bot commented Sep 2, 2024

tiraboschi commented Sep 3, 2024

akrejcir commented Sep 9, 2024

hco-bot commented Sep 9, 2024

kubevirt-bot commented Sep 9, 2024

kubevirt-bot commented Sep 10, 2024 •

edited

Loading

hco-bot commented Sep 10, 2024

kubevirt-bot commented Sep 10, 2024

sonarcloud bot commented Oct 1, 2024

hco-bot commented Oct 1, 2024

kubevirt-bot commented Oct 1, 2024

hco-bot commented Oct 1, 2024

kubevirt-bot commented Oct 1, 2024

openshift-ci bot commented Oct 1, 2024 •

edited

Loading

akrejcir commented Oct 2, 2024

akrejcir commented Oct 2, 2024

hco-bot commented Oct 2, 2024

kubevirt-bot commented Oct 2, 2024

nunnatsa Oct 10, 2024

akrejcir Oct 10, 2024 •

edited

Loading

nunnatsa commented Oct 10, 2024

nunnatsa commented Oct 10, 2024

akrejcir commented Oct 10, 2024

tiraboschi commented Oct 11, 2024

API: Use new token generation SSP API and remove feature gate #3087

Are you sure you want to change the base?

API: Use new token generation SSP API and remove feature gate #3087

Conversation

akrejcir commented Sep 2, 2024

kubevirt-bot commented Sep 2, 2024

coveralls commented Sep 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11124276641

Details

💛 - Coveralls

nunnatsa left a comment

Choose a reason for hiding this comment

akrejcir commented Sep 2, 2024

nunnatsa commented Sep 2, 2024

hco-bot commented Sep 2, 2024

kubevirt-bot commented Sep 2, 2024

tiraboschi commented Sep 3, 2024

akrejcir commented Sep 9, 2024

hco-bot commented Sep 9, 2024

kubevirt-bot commented Sep 9, 2024

kubevirt-bot commented Sep 10, 2024 • edited Loading

hco-bot commented Sep 10, 2024

kubevirt-bot commented Sep 10, 2024

sonarcloud bot commented Oct 1, 2024

Quality Gate passed

hco-bot commented Oct 1, 2024

kubevirt-bot commented Oct 1, 2024

hco-bot commented Oct 1, 2024

kubevirt-bot commented Oct 1, 2024

openshift-ci bot commented Oct 1, 2024 • edited Loading

akrejcir commented Oct 2, 2024

akrejcir commented Oct 2, 2024

hco-bot commented Oct 2, 2024

kubevirt-bot commented Oct 2, 2024

nunnatsa Oct 10, 2024

Choose a reason for hiding this comment

akrejcir Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

nunnatsa commented Oct 10, 2024

nunnatsa commented Oct 10, 2024

akrejcir commented Oct 10, 2024

tiraboschi commented Oct 11, 2024

coveralls commented Sep 2, 2024 •

edited

Loading

kubevirt-bot commented Sep 10, 2024 •

edited

Loading

openshift-ci bot commented Oct 1, 2024 •

edited

Loading

akrejcir Oct 10, 2024 •

edited

Loading