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

Jump to bottom

Deprecating/Replacing securityGroups field from OpenStackMachineTemplate ports #1251

Closed
Xenwar opened this issue May 25, 2022 · 17 comments · Fixed by #1516
Closed

Deprecating/Replacing securityGroups field from OpenStackMachineTemplate ports #1251

Xenwar opened this issue May 25, 2022 · 17 comments · Fixed by #1516
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API

Comments

@Xenwar
Copy link

Xenwar commented May 25, 2022

/kind feature

Describe the solution you'd like
With the PR #1246, we have introduced a new field
OpenStackMachineTemplate.spec.template.spec.ports.securityGroupsFilters.

Before that, there was only one such field, OpenStackMachineTemplate .spec.template.spec.ports.securityGroups which is a lit of UUIDs.

I would like to deprecate the older field for two reasons.

  1. It is restrictive in that allows only specifying UUIDs
  2. It is not the same as what is at intense level, shown below.
kubectl explain OpenStackMachineTemplate.spec.template.spec.securityGroups
KIND:     OpenStackMachineTemplate
VERSION:  infrastructure.cluster.x-k8s.io/v1alpha5
RESOURCE: securityGroups <[]Object>
DESCRIPTION:
     The names of the security groups to assign to the instance
FIELDS:
   filter       <Object>
     Filters used to query security groups in openstack
   name <string>
     Security Group name
   uuid <string>
     Security Group UID

In favour of uniformity, I intend to

  • Remove ports.SecurityGroups
  • Rename the new field ports.securityGroupsFilters to ports.securityGroups

Anything else you would like to add:

I would like to know if this breaks existing deployments and if it is ok to start coding and If there are any side effect this may introduce.
@Xenwar
Copy link
Author

Xenwar commented May 25, 2022

/assign @mdbooth

@jichenjc
Copy link
Contributor

given we are still alpha version, I think we didn't guarantee the version compatible (actually we already did so)
thus, I see no reason we can't do it given the previous PR is already merged and it covers the deprecated feature

@Xenwar
Copy link
Author

Xenwar commented May 27, 2022

@jichenjc Thanks.

@apricote
Copy link
Member

Making these changes to the CRD is breaking change which requires a new version of the CRD (probably v1alpha6?)

@mdbooth
Copy link
Contributor

mdbooth commented May 27, 2022

Making these changes to the CRD is breaking change which requires a new version of the CRD (probably v1alpha6?)

Yes, definitely!

@mdbooth
Copy link
Contributor

mdbooth commented May 27, 2022

However, I do think it's appropriate to make these consistent.

Xenwar pushed a commit to Nordix/cluster-api-provider-openstack that referenced this issue Jun 1, 2022
Deprecating and replacing securityGroups field from OpenStackMachineT…
6b1a197 
…emplate ports

fixes: kubernetes-sigs#1251
@Xenwar Xenwar mentioned this issue Jun 1, 2022
Closed
Xenwar pushed a commit to Nordix/cluster-api-provider-openstack that referenced this issue Jun 6, 2022
Deprecating and replacing securityGroups field from OpenStackMachineT…
bf212ec 
…emplate ports

fixes: kubernetes-sigs#1251
Xenwar pushed a commit to Nordix/cluster-api-provider-openstack that referenced this issue Jun 6, 2022
Deprecating and replacing securityGroups field from OpenStackMachineT…
04cc66b 
…emplate ports

fixes: kubernetes-sigs#1251
Xenwar pushed a commit to Nordix/cluster-api-provider-openstack that referenced this issue Jun 6, 2022
Deprecating and replacing securityGroups field from OpenStackMachineT…
9f64727 
…emplate ports

fixes: kubernetes-sigs#1251
Xenwar pushed a commit to Nordix/cluster-api-provider-openstack that referenced this issue Jun 6, 2022
Deprecating and replacing securityGroups field from OpenStackMachineT…
f699622 
…emplate ports

fixes: kubernetes-sigs#1251
@Xenwar Xenwar mentioned this issue Jun 6, 2022
Closed
@apricote
Copy link
Member

/kind api-change

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jun 15, 2022
@seanschneeweiss seanschneeweiss assigned mdbooth and unassigned mdbooth Jun 15, 2022
@lentzi90 lentzi90 mentioned this issue Jul 4, 2022
Closed
3 tasks
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 13, 2022
@jichenjc
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 13, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 13, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 12, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 11, 2023
@lentzi90
Copy link
Contributor

/reopen
I'm working on this

@k8s-ci-robot k8s-ci-robot reopened this Mar 21, 2023
@k8s-ci-robot
Copy link
Contributor

@lentzi90: Reopened this issue.

In response to this:

/reopen
I'm working on this

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/test-infra repository.

@lentzi90
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 21, 2023
@lentzi90
Copy link
Contributor

After staring at the code for a while and implementing some conversions and tests for this, I'm not convinced it is a good idea to rename securityGroupsFilters to securityGroups. I would instead suggest to keep securityGroupFilters as is and just remove securityGroups. This is easier to handle in the conversions and much easier for users deal with also.

The old securityGroups field would be removed, the new securityGroupFilters would remain as is. Any old securityGroups can easily be converted to securityGroupFilters and they can be used in parallel in v1alpha6. In v1alpha7 securityGroups would be dropped.

The originally suggested change in this issue was to remove the (old) securityGroups and then rename securityGroupFilters to securityGroups. I think this is unnecessary and just makes things more confusing and complicated.

@lentzi90 lentzi90 mentioned this issue Mar 27, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API
Projects
None yet
Milestone
No milestone
7 participants
@mdbooth @apricote @lentzi90 @Xenwar @jichenjc @k8s-ci-robot @k8s-triage-robot