Add `AliasPrefix` and `AliasPrefixRouting` types to networking API #372

afritzler · 2022-04-26T16:59:08Z

Proposed Changes

Add AliasPrefix and AliasPrefixRouting types to networking API
Add sample for AliasPrefix
Add validation and validation test
Fixed samples

Fixes #366 #367

- Add validation and validation test - Fixed samples

afritzler · 2022-04-28T21:57:12Z

Fixed the tests by increasing the runner cores to 4 + increasing the apiserver timeout to 5 mins.

adracus · 2022-04-29T05:01:18Z

apis/networking/common.go

+	"github.com/onmetal/onmetal-api/apis/ipam"
+)
+
+type EphemeralPrefixSource struct {


Why was the signature of the EphemeralPrefixSource changed? Wouldn't this also alter the NetworkInterface type?
In k8s, for templated types, the structure is

type ...Source struct { ...Template ...Template }

as well

That was the problem with the inline + pointer type. The api-server panics if I use something like this:

type EphemeralPrefixSource struct { PrefixTemplate *ipamv1alpha1.PrefixTemplateSpec `json:",inline"` }

adracus · 2022-04-29T05:02:19Z

apis/networking/v1alpha1/common.go

+	ipamv1alpha1 "github.com/onmetal/onmetal-api/apis/ipam/v1alpha1"
+)
+
+type EphemeralPrefixSource struct {


Same as above.

adracus · 2022-04-29T05:04:13Z

apis/networking/validation/aliasprefix.go

+	for _, msg := range apivalidation.NameIsDNSLabel(spec.NetworkRef.Name, false) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("networkRef").Child("name"), spec.NetworkRef.Name, msg))
+	}


Please move this into the else block of the

if spec.NetworkRef ==

part. The apivalidation.NameIsDNSLabel already errors on empty names and error messages might be misleading.

adracus · 2022-04-29T05:06:59Z

apis/networking/validation/aliasprefix.go

+	return allErrs
+}
+
+func validateAliasPrefixSources(prefixSource networking.PrefixSource, fldPath *field.Path) field.ErrorList {


Please add some validation in case somebody specifies both Value and EphemeralPrefix - this should be disallowed.

adracus · 2022-04-29T05:09:03Z

apis/networking/validation/aliasprefixrouting.go

+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+type aliasPrefixRoutingSubsetKey struct {


Why introduce this key type if it only contains commonv1alpha1.LocalUIDReference?
Also, the identity should be the name only, as we also don't want two entries that are equal in name but non-equal in UID.

adracus · 2022-04-29T05:12:28Z

registry/networking/aliasprefix/storage/tableconvertor.go

+		aliasPrefix := obj.(*networking.AliasPrefix)
+
+		cells = append(cells, name)
+		cells = append(cells, aliasPrefix.Status.Prefix)


Please add a check if this is nil - then we should print something like <none> / None / <undefined> (have a look at the other table convertors.

adracus · 2022-04-29T05:13:14Z

registry/networking/aliasprefix/strategy.go

+}
+
+func (aliasPrefixStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
+	return field.ErrorList{}


The update validation hook has to be added.

adracus · 2022-04-29T05:14:16Z

registry/networking/aliasprefixrouting/storage/storage.go

+}
+
+func (REST) ShortNames() []string {
+	return []string{"apr"}


please also add aprouting

adracus · 2022-04-29T05:14:47Z

registry/networking/aliasprefixrouting/storage/storage.go

+	}, nil
+}
+
+type StatusREST struct {


The StatusREST should be removed.

adracus · 2022-04-29T05:15:22Z

registry/networking/aliasprefixrouting/storage/tableconvertor.go

+	objectMetaSwaggerDoc = metav1.ObjectMeta{}.SwaggerDoc()
+
+	headers = []metav1.TableColumnDefinition{
+		{Name: "Name", Type: "string", Format: "name", Description: objectMetaSwaggerDoc["name"]},


Please add a column of some sort that prints the targets / subsets.

github-actions bot added the size/XXL label Apr 26, 2022

afritzler changed the title ~~Add AliasPrefix and AliasPrefixRouting types to networking API~~ WIP: Add AliasPrefix and AliasPrefixRouting types to networking API Apr 26, 2022

github-actions bot added api-change bug Something isn't working compute ipam networking storage labels Apr 26, 2022

afritzler force-pushed the enh/aliasprefix-type branch 8 times, most recently from 1c02e46 to a6fe24a Compare April 27, 2022 14:16

afritzler changed the title ~~WIP: Add AliasPrefix and AliasPrefixRouting types to networking API~~ Add AliasPrefix and AliasPrefixRouting types to networking API Apr 27, 2022

This comment was marked as outdated.

Sign in to view

afritzler force-pushed the enh/aliasprefix-type branch from a6fe24a to f1d2ebc Compare April 27, 2022 14:59

afritzler added 2 commits April 28, 2022 14:30

Add AliasPrefix and AliasPrefixRouting types to networking API

226b956

- Add validation and validation test - Fixed samples

Fix aliasprefix server panic

dc92177

afritzler force-pushed the enh/aliasprefix-type branch from f1d2ebc to dc92177 Compare April 28, 2022 12:58

afritzler requested a review from adracus April 28, 2022 13:06

Increase apiserver timeout to 5 mins

825458b

afritzler force-pushed the enh/aliasprefix-type branch from b81f263 to 825458b Compare April 28, 2022 21:22

Add api-reference docs

fd5d508

github-actions bot added the documentation Improvements or additions to documentation label Apr 28, 2022

afritzler added the ok-to-🐳 label Apr 28, 2022

Increased test coverage

cb28190

adracus suggested changes Apr 29, 2022

View reviewed changes

Add PR review fixes

1ab546c

afritzler force-pushed the enh/aliasprefix-type branch from 6e19465 to 1ab546c Compare April 29, 2022 08:56

Update api reference docs

4c478a3

adracus approved these changes May 2, 2022

View reviewed changes

adracus merged commit 8755cfb into main May 2, 2022

adracus deleted the enh/aliasprefix-type branch May 2, 2022 14:40

afritzler mentioned this pull request May 2, 2022

Add AliasPrefixRouting type to networking API group #367

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `AliasPrefix` and `AliasPrefixRouting` types to networking API #372

Add `AliasPrefix` and `AliasPrefixRouting` types to networking API #372

afritzler commented Apr 26, 2022 •

edited

Loading

This comment was marked as outdated.

afritzler commented Apr 28, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

adracus Apr 29, 2022

afritzler Apr 29, 2022

Add AliasPrefix and AliasPrefixRouting types to networking API #372

Add AliasPrefix and AliasPrefixRouting types to networking API #372

Conversation

afritzler commented Apr 26, 2022 • edited Loading

Proposed Changes

This comment was marked as outdated.

afritzler commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `AliasPrefix` and `AliasPrefixRouting` types to networking API #372

Add `AliasPrefix` and `AliasPrefixRouting` types to networking API #372

afritzler commented Apr 26, 2022 •

edited

Loading