Skip to content

Short names for staged policies #10280

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

Merged
MichalFupso merged 4 commits into from
Apr 28, 2025
Merged

Short names for staged policies #10280

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
515521e
Short names for staged policies
MichalFupso Apr 23, 2025
e79d543
add short name for stagedkubernetesnetworkpolicy
MichalFupso Apr 23, 2025
0eaeac5
update short names for stagednetworkpolicy
MichalFupso Apr 23, 2025
6716148
update short names for stagednetworkpolicy
MichalFupso Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apiserver/pkg/registry/projectcalico/rest/storage_calico.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (p RESTStorageProvider) NewV3Storage(
},
p.StorageType,
authorizer,
[]string{},
[]string{"sknp"},
)

stagedpolicyRESTOptions, err := restOptionsGetter.GetRESTOptions(calico.Resource("stagednetworkpolicies"), nil)
Expand All @@ -126,7 +126,7 @@ func (p RESTStorageProvider) NewV3Storage(
},
p.StorageType,
authorizer,
[]string{},
[]string{"csnp", "calicostagednetworkpolicy", "calicostagednetworkpolicies"},
Copy link
Member

@matthewdupre matthewdupre Apr 23, 2025

Choose a reason for hiding this comment

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

I think these names are a bit confusing: the resource kind is StagedNetworkPolicy, so these should allude to that. I think it's also preferable to have the s appear at the start. So here I think the good options would be "snp", "stagednetworkpolicy".

Copy link
Member

@matthewdupre matthewdupre Apr 23, 2025

Choose a reason for hiding this comment

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

And then maybe add "scnp", "stagedcaliconetworkpolicy"

Copy link
Contributor Author

@MichalFupso MichalFupso Apr 23, 2025

Choose a reason for hiding this comment

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

I think for this one we might be good with just the "snp", if we would want to add the calico differentiation we should follow the same order as for networkpolicy which has calico at front (caliconetworkpolicy)

Copy link
Member

@matthewdupre matthewdupre Apr 23, 2025

Choose a reason for hiding this comment

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

:( - so much inconsistency. Ah well. I think the names I'd like to see most are the ones that are just word->letter mappings from the kind in the same ordering.

Copy link
Member

@caseydavenport caseydavenport Apr 24, 2025

Choose a reason for hiding this comment

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

Yeah, agreed - the important one here is snp I think

)

networksetRESTOptions, err := restOptionsGetter.GetRESTOptions(calico.Resource("networksets"), nil)
Expand Down Expand Up @@ -214,7 +214,7 @@ func (p RESTStorageProvider) NewV3Storage(
},
p.StorageType,
authorizer,
[]string{},
[]string{"sgnp", "csgnp", "calicostagedglobalnetworkpolicies", "calicostagedglobalnetworkpolicy"},
Copy link
Member

@matthewdupre matthewdupre Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should have the csgnp option (keep staged in front), and I don't think we need the Calico qualifier either since there is no k8s GlobalNetworkPolicy

)

gNetworkSetRESTOptions, err := restOptionsGetter.GetRESTOptions(calico.Resource("globalnetworksets"), nil)
Expand Down
7 changes: 6 additions & 1 deletion apiserver/pkg/registry/projectcalico/stagedglobalnetworkpolicy/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type REST struct {
*genericregistry.Store
rbac.CalicoResourceLister
authorizer authorizer.TierAuthorizer
shortNames []string
}

// EmptyObject returns an empty instance
Expand Down Expand Up @@ -102,7 +103,7 @@ func NewREST(scheme *runtime.Scheme, opts server.Options, calicoResourceLister r
DestroyFunc: dFunc,
}

return &REST{store, calicoResourceLister, authorizer.NewTierAuthorizer(opts.Authorizer)}, nil
return &REST{store, calicoResourceLister, authorizer.NewTierAuthorizer(opts.Authorizer), opts.ShortNames}, nil
}

func (r *REST) List(ctx context.Context, options *metainternalversion.ListOptions) (runtime.Object, error) {
Expand Down Expand Up @@ -166,3 +167,7 @@ func (r *REST) Watch(ctx context.Context, options *metainternalversion.ListOptio

return r.Store.Watch(ctx, options)
}

func (r *REST) ShortNames() []string {
return r.shortNames
}
7 changes: 6 additions & 1 deletion apiserver/pkg/registry/projectcalico/stagedkubernetesnetworkpolicy/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
// rest implements a RESTStorage for API services against etcd
type REST struct {
*genericregistry.Store
shortNames []string
}

// EmptyObject returns an empty instance
Expand Down Expand Up @@ -89,5 +90,9 @@ func NewREST(scheme *runtime.Scheme, opts server.Options) (*REST, error) {
DestroyFunc: dFunc,
}

return &REST{store}, nil
return &REST{store, opts.ShortNames}, nil
}

func (r *REST) ShortNames() []string {
return r.shortNames
}
7 changes: 6 additions & 1 deletion apiserver/pkg/registry/projectcalico/stagednetworkpolicy/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type REST struct {
*genericregistry.Store
rbac.CalicoResourceLister
authorizer authorizer.TierAuthorizer
shortNames []string
}

// EmptyObject returns an empty instance
Expand Down Expand Up @@ -98,7 +99,7 @@ func NewREST(scheme *runtime.Scheme, opts server.Options, calicoResourceLister r
DestroyFunc: dFunc,
}

return &REST{store, calicoResourceLister, authorizer.NewTierAuthorizer(opts.Authorizer)}, nil
return &REST{store, calicoResourceLister, authorizer.NewTierAuthorizer(opts.Authorizer), opts.ShortNames}, nil
}

func (r *REST) List(ctx context.Context, options *metainternalversion.ListOptions) (runtime.Object, error) {
Expand Down Expand Up @@ -162,3 +163,7 @@ func (r *REST) Watch(ctx context.Context, options *metainternalversion.ListOptio

return r.Store.Watch(ctx, options)
}

func (r *REST) ShortNames() []string {
return r.shortNames
}