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.

Sign up for GitHub

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

Jump to bottom

API: Improve API and increase version to v1 #165

Merged
merged 4 commits into from
Jul 29, 2024
Merged

API: Improve API and increase version to v1 #165

merged 4 commits into from
Jul 29, 2024

Conversation

akrejcir
Copy link
Collaborator

What this PR does / why we need it:
This PR does these changes:

  • Adds a new convenience field to the struct returned by /token endpoint.
  • Removes dependency on OpenShift from the API
  • Increases the API version to v1

Which issue(s) this PR fixes:
Jira: https://issues.redhat.com/browse/CNV-45061

Release note:

- The /token endpoint returns a new field in the json:  expirationTimestamp.
- The format of the TLS profile in the ConfigMap changed to not depend on OpenShift struct.
- The version of the API was increased to v1.

@kubevirt-bot 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 Jul 26, 2024
@akrejcir
Copy link
Collaborator Author

/cc @0xFelix @jcanocan @ksimon1

@kubevirt-bot kubevirt-bot requested review from 0xFelix and jcanocan July 26, 2024 09:25
0xFelix
0xFelix reviewed Jul 26, 2024
View reviewed changes
manifests/config_map.yaml Outdated
tls-profile-v1alpha1.yaml: |
type: Intermediate
intermediate: {}
tls-profile-v1.yaml: ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example not required anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to specify any configuration, but at the same time I wanted to keep the file in ConfigMap, so it is clear what filename should be used.

IMHO it is better to use empty file, which will result in default TLS configuration, than to specify some configuration here. Otherwise we would need to keep it updated.

pkg/console/service/service.go Outdated Show resolved Hide resolved
pkg/console/service/service_test.go Show resolved Hide resolved
pkg/console/tlsconfig/tlsconfig.go Show resolved Hide resolved
tests/proxy_test.go Outdated Show resolved Hide resolved
@akrejcir akrejcir force-pushed the improve-api branch 3 times, most recently from fc25211 to 379805f Compare July 26, 2024 12:21
akrejcir added 4 commits July 26, 2024 14:31
@akrejcir
api: Return token expiration timestamp
e3a0433 
Added "expirationTimestamp" to the response of the /token
endpoint. This is for convenience, and to be similar to the response
from ServiceAccount TokenRequest API.

Signed-off-by: Andrej Krejcir <[email protected]>
@akrejcir
chore: Simplify tlsconfig unit tests
1e05da7 
Signed-off-by: Andrej Krejcir <[email protected]>
@akrejcir
api: Do not use OCP struct in the config file API
856175c 
The struct that is expected in the TLS config file
does not use OpenShift type. This is useful to remove
OpenShift dependency from the API.

Signed-off-by: Andrej Krejcir <[email protected]>
@akrejcir
api: Increase API version to v1
490ed97 
The current API is good enough to be v1.

Signed-off-by: Andrej Krejcir <[email protected]>
0xFelix
0xFelix reviewed Jul 26, 2024
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to deprecate the old API or ensure compatibility?

Otherwise looks fine

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2024
@akrejcir
Copy link
Collaborator Author

Do we need to deprecate the old API or ensure compatibility?

Otherwise looks fine

/approve

Previous API was alpha version, so in my opinion it's not needed.

@jcanocan
Copy link

Thanks for your efforts! Nice job!
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2024
@kubevirt-bot kubevirt-bot merged commit 4df4c85 into kubevirt:main Jul 29, 2024
5 checks passed
@akrejcir akrejcir deleted the improve-api branch July 29, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcanocan jcanocan jcanocan left review comments

@0xFelix 0xFelix 0xFelix left review comments

@ksimon1 ksimon1 Awaiting requested review from ksimon1

@opokornyy opokornyy Awaiting requested review from opokornyy

Assignees

@jcanocan jcanocan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akrejcir @kubevirt-bot @jcanocan @0xFelix