Skip to content

🌱 improve ip address validation #1249

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

Open
peppi-lotta wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 improve ip address validation #1249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions internal/webhooks/v1alpha1/ipaddress_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,28 @@ func (webhook *IPAddress) ValidateCreate(_ context.Context, obj runtime.Object)
"cannot be empty",
),
)
} else if validateIP(c.Spec.Address) != nil {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("spec", "address"),
c.Spec.Address,
"is not a valid IP address",
),
)
}

// Validate requested IP address if present in annotations (for CAPI claims)
if requestedIP, ok := c.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {
Copy link
Member

@adilGhaffarDev adilGhaffarDev Dec 19, 2025

Choose a reason for hiding this comment

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

Is user allowed to add annotation after creation?
if yes lets add this validation in Update also?

if validateIP(ipamv1.IPAddressStr(requestedIP)) != nil {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("metadata", "annotations", "ipAddress"),
requestedIP,
"is not a valid IP address",
),
)
}
}
Comment on lines +99 to +110
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the new IP address validation in annotations for IPAddress resources. The validation logic added for the "ipAddress" annotation (lines 100-110) should be tested to ensure invalid IP addresses in annotations are properly rejected. Consider adding test cases that verify both valid and invalid IP addresses in the annotation are handled correctly.

Copilot uses AI. Check for mistakes.
if len(allErrs) == 0 {
return nil, nil
}
Expand Down
138 changes: 118 additions & 20 deletions internal/webhooks/v1alpha1/ipaddress_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,31 @@ func TestIPAddressCreateValidation(t *testing.T) {
claim: corev1.ObjectReference{
Name: "abc",
},
address: "abcd",
address: "192.168.1.10",
},
{
name: "should fail with invalid IP address",
expectErr: true,
addressName: "abc-3",
ipPool: corev1.ObjectReference{
Name: "abc",
},
claim: corev1.ObjectReference{
Name: "abc",
},
address: "not-an-ip-address",
},
{
name: "should fail with malformed IP address format",
expectErr: true,
addressName: "abc-4",
ipPool: corev1.ObjectReference{
Name: "abc",
},
claim: corev1.ObjectReference{
Name: "abc",
},
address: "256.256.256.256",
},
{
name: "should fail without address",
Expand All @@ -81,7 +105,7 @@ func TestIPAddressCreateValidation(t *testing.T) {
claim: corev1.ObjectReference{
Name: "abc",
},
address: "abcd",
address: "192.168.1.10",
},
{
name: "should fail without claim name",
Expand All @@ -93,7 +117,7 @@ func TestIPAddressCreateValidation(t *testing.T) {
claim: corev1.ObjectReference{
Namespace: "abc",
},
address: "abcd",
address: "192.168.1.10",
},
}

Expand Down Expand Up @@ -127,6 +151,80 @@ func TestIPAddressCreateValidation(t *testing.T) {
}
}

func TestIPAddressAnnotationValidation(t *testing.T) {
tests := []struct {
name string
expectErr bool
annotations map[string]string
}{
{
name: "should succeed with valid IP in annotation",
expectErr: false,
annotations: map[string]string{
"ipAddress": "192.168.1.100",
},
},
{
name: "should succeed with valid IPv6 in annotation",
expectErr: false,
annotations: map[string]string{
"ipAddress": "2001:db8::1",
},
},
{
name: "should fail with malformed IP in annotation",
expectErr: true,
annotations: map[string]string{
"ipAddress": "not-an-ip",
},
},
{
name: "should fail with invalid IP format in annotation",
expectErr: true,
annotations: map[string]string{
"ipAddress": "256.256.256.256",
},
},
{
name: "should succeed without IP annotation",
expectErr: false,
annotations: map[string]string{},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
webhook := &IPAddress{}

obj := &ipamv1.IPAddress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "test-address",
Annotations: tt.annotations,
},
Spec: ipamv1.IPAddressSpec{
Pool: corev1.ObjectReference{
Name: "test-pool",
},
Claim: corev1.ObjectReference{
Name: "test-claim",
},
Address: "192.168.1.10",
},
}

if tt.expectErr {
_, err := webhook.ValidateCreate(ctx, obj)
g.Expect(err).To(HaveOccurred())
} else {
_, err := webhook.ValidateCreate(ctx, obj)
g.Expect(err).NotTo(HaveOccurred())
}
})
}
}

func TestIPAddressUpdateValidation(t *testing.T) {
tests := []struct {
name string
Expand All @@ -144,7 +242,7 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Claim: corev1.ObjectReference{
Name: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Pool: corev1.ObjectReference{
Expand All @@ -153,7 +251,7 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Claim: corev1.ObjectReference{
Name: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
{
Expand All @@ -163,7 +261,7 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Pool: corev1.ObjectReference{
Name: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: nil,
},
Expand All @@ -174,13 +272,13 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Pool: corev1.ObjectReference{
Name: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Pool: corev1.ObjectReference{
Name: "abc",
},
Address: "abcde",
Address: "192.168.1.11",
},
},
{
Expand All @@ -190,13 +288,13 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Pool: corev1.ObjectReference{
Name: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Pool: corev1.ObjectReference{
Name: "abcd",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
{
Expand All @@ -207,14 +305,14 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Name: "abc",
Namespace: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Pool: corev1.ObjectReference{
Name: "abc",
Namespace: "abcd",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
{
Expand All @@ -225,14 +323,14 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Name: "abc",
Kind: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Pool: corev1.ObjectReference{
Name: "abc",
Kind: "abcd",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
{
Expand All @@ -242,13 +340,13 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Claim: corev1.ObjectReference{
Name: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Claim: corev1.ObjectReference{
Name: "abcd",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
{
Expand All @@ -259,14 +357,14 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Name: "abc",
Namespace: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Claim: corev1.ObjectReference{
Name: "abc",
Namespace: "abcd",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
{
Expand All @@ -277,14 +375,14 @@ func TestIPAddressUpdateValidation(t *testing.T) {
Name: "abc",
Kind: "abc",
},
Address: "abcd",
Address: "192.168.1.10",
},
old: &ipamv1.IPAddressSpec{
Claim: corev1.ObjectReference{
Name: "abc",
Kind: "abcd",
},
Address: "abcd",
Address: "192.168.1.10",
},
},
}
Expand Down
26 changes: 26 additions & 0 deletions internal/webhooks/v1alpha1/ipclaim_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,19 @@ func (webhook *IPClaim) ValidateCreate(_ context.Context, obj runtime.Object) (a
)
}

// Validate requested IP address if present in annotations
if requestedIP, ok := c.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {
if err := validateIP(ipamv1.IPAddressStr(requestedIP)); err != nil {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("metadata", "annotations", "ipAddress"),
requestedIP,
"is not a valid IP address",
),
)
}
}
Comment on lines +71 to +82
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the new IP address validation in annotations. The validation logic added for the "ipAddress" annotation (lines 72-82) should be tested to ensure invalid IP addresses in annotations are properly rejected. Consider adding test cases that verify both valid and invalid IP addresses in the annotation are handled correctly.

Copilot uses AI. Check for mistakes.

if len(allErrs) == 0 {
return nil, nil
}
Expand Down Expand Up @@ -113,6 +126,19 @@ func (webhook *IPClaim) ValidateUpdate(_ context.Context, oldObj, newObj runtime
)
}

// Validate requested IP address if present in annotations
if requestedIP, ok := newIPClaim.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {
if validateIP(ipamv1.IPAddressStr(requestedIP)) != nil {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("metadata", "annotations", "ipAddress"),
requestedIP,
"is not a valid IP address",
),
)
}
}
Comment on lines +129 to +140
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the new IP address validation in annotations during updates. The validation logic added for the "ipAddress" annotation (lines 130-140) should be tested to ensure invalid IP addresses in annotations are properly rejected during update operations. Consider adding test cases that verify both valid and invalid IP addresses in the annotation are handled correctly.

Copilot uses AI. Check for mistakes.

if len(allErrs) == 0 {
return nil, nil
}
Expand Down
Loading