[release-4.9] Bug 2050632: UPSTREAM: <drop>: Give warning when ipFamilyPolicy implicitly set#1170
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
bot
added
the
bugzilla/severity-low
|
@andreaskaris: This pull request references Bugzilla bug 2047676, which is invalid:
Comment DetailsIn response to 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. |
openshift-ci
bot
added
the
bugzilla/invalid-bug
|
@andreaskaris: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
Continuation from: #1151 |
|
@andreaskaris: This pull request references Bugzilla bug 2047676, which is invalid:
Comment DetailsIn response to 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. |
|
@andreaskaris: This pull request references Bugzilla bug 2047676, which is invalid:
Comment DetailsIn response to 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. |
andreaskaris
changed the title
openshift-ci
bot
added
bugzilla/valid-bug
|
@andreaskaris: This pull request references Bugzilla bug 2050632, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to 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. |
|
/test k8s-e2e-gcp |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@andreaskaris: This pull request references Bugzilla bug 2050632, which is valid. 6 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to 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. |
1 similar comment
|
@andreaskaris: This pull request references Bugzilla bug 2050632, which is valid. 6 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to 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. |
|
/assign |
There was a problem hiding this comment.
the message should be short
field.NewPath("service","spec", "ipFamilyPolicy").String() + "must be RequireDualStack or PreferDualStack when multiple 'ipFamilies' are specified, this operation will fail starting with Red Hat OpenShift Platform 4.10."
There was a problem hiding this comment.
@sttts the field population happens on the Rest Create and Update hooks that AFAIK run after the admission , so this statement is not true, am I wrong?
|
I would add a test , you can complete this and drop it as a file in /*
Copyright 2022 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package service
import (
"bytes"
"context"
"fmt"
"strings"
"testing"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
utilfeature "k8s.io/apiserver/pkg/util/feature"
clientset "k8s.io/client-go/kubernetes"
restclient "k8s.io/client-go/rest"
featuregatetesting "k8s.io/component-base/featuregate/testing"
kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
"k8s.io/kubernetes/pkg/features"
"k8s.io/kubernetes/test/integration/framework"
)
func Test_ServiceDualStackIPFamilyPolicy(t *testing.T) {
// Create an IPv4IPv6 dual stack control-plane
serviceCIDR := "10.0.0.0/16"
secondaryServiceCIDR := "2001:db8:1::/112"
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, true)()
s := kubeapiservertesting.StartTestServerOrDie(t, nil, []string{"--service-cluster-ip-range", serviceCIDR + "," + secondaryServiceCIDR}, framework.SharedEtcd())
defer s.TearDownFn()
b := &bytes.Buffer{}
warningWriter := restclient.NewWarningWriter(b, restclient.WarningWriterOptions{})
s.ClientConfig.WarningHandler = warningWriter
client := clientset.NewForConfigOrDie(s.ClientConfig)
singleStack := v1.IPFamilyPolicySingleStack
var testcases = []struct {
name string
clusterIPs []string
ipFamilies []v1.IPFamily
ipFamilyPolicy *v1.IPFamilyPolicyType
warnings int
}{
{
name: "Single Stack - IPFamilyPolicy set",
clusterIPs: []string{},
ipFamilies: []v1.IPFamily{v1.IPv4Protocol},
ipFamilyPolicy: &singleStack,
},
{
name: "Single Stack - IPFamilyPolicy nil",
clusterIPs: []string{},
ipFamilies: []v1.IPFamily{v1.IPv4Protocol},
ipFamilyPolicy: nil,
},
{
name: "Dual Stack - IPFamilyPolicy nil",
clusterIPs: []string{},
ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
ipFamilyPolicy: nil,
warnings: 1,
},
{
// COMPLETE HERE WITH MORE VARIATIONS
},
}
for i, tc := range testcases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
svc := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("svc-test-%d", i), // use different services for each test
},
Spec: v1.ServiceSpec{
Type: v1.ServiceTypeClusterIP,
ClusterIPs: tc.clusterIPs,
IPFamilies: tc.ipFamilies,
IPFamilyPolicy: tc.ipFamilyPolicy,
Ports: []v1.ServicePort{
{
Port: 443,
TargetPort: intstr.FromInt(443),
},
},
},
}
// create a service
_, err := client.CoreV1().Services(metav1.NamespaceDefault).Create(context.TODO(), svc, metav1.CreateOptions{})
if err != nil {
t.Fatal(err)
}
if tc.warnings > 0 {
//expectedPatchWarning := field.NewPath("service", "spec", "ipFamilyPolicy").String() + "must be RequireDualStack or PreferDualStack when multiple 'ipFamilies' are specified, this operation will fail starting with Red Hat OpenShift Platform 4.10."
assertWarningCount(t, warningWriter, 1)
//assertWarningMessage(t, b, expectedPatchWarning)
}
})
}
}
type warningCounter interface {
WarningCount() int
}
func assertWarningCount(t *testing.T, counter warningCounter, expected int) {
if counter.WarningCount() != expected {
t.Errorf("unexpected warning count, expected: %v, got: %v", expected, counter.WarningCount())
}
}
func assertWarningMessage(t *testing.T, b *bytes.Buffer, expected string) {
defer b.Reset()
actual := b.String()
if len(expected) == 0 && len(actual) != 0 {
t.Errorf("unexpected warning message, expected no warning, got: %v", actual)
}
if len(expected) == 0 {
return
}
if !strings.Contains(actual, expected) {
t.Errorf("unexpected warning message, expected: %v, got: %v", expected, actual)
}
} |
|
not an expert on the backport process, should this not a |
andreaskaris
force-pushed
the
bz2045576-4.9
branch
from
1bf4a2a to
61fd7f3
Compare
|
@andreaskaris: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
In kube 1.21 and 1.22 (OCP 4.8 and 4.9), the apiserver would default the value of `ipFamilyPolicy` to `RequireDualStack` if you created a Service with two `ipFamilies` or two `clusterIPs` but no explicitly specified `ipFamilyPolicy`. In 1.23/4.10, you must explicitly specify either PreferDualStack or RequireDualStack for DualStack services. Emit a warning in 4.8 and 4.9 to raise awareness about the upcoming API changes. OpenShift 4.8 and 4.9 only, BZ 2045576 Signed-off-by: Andreas Karis <ak.karis@gmail.com>
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
24 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@andreaskaris: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@andreaskaris: All pull requests linked via external trackers have merged: Bugzilla bug 2050632 has been moved to the MODIFIED state. DetailsIn response to 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. |
8 participants
UPSTREAM: : Give warning when ipFamilyPolicy implicitly set
In kube 1.21 and 1.22 (OCP 4.8 and 4.9), the apiserver would default
the value of
ipFamilyPolicytoRequireDualStackif you created aService with two
ipFamiliesor twoclusterIPsbut no explicitlyspecified
ipFamilyPolicy. In 1.23/4.10, you must explicitly specifyeither PreferDualStack or RequireDualStack for DualStack services.
Emit a warning in 4.8 and 4.9 to raise awareness about the upcoming
API changes.
OpenShift 4.8 and 4.9 only, BZ 2047676
What type of PR is this?
What this PR does / why we need it:
Warn users about an upcoming breaking API change.
Which issue(s) this PR fixes:
Fixes BZ 2045576
Special notes for your reviewer:
I am proposing that we introduce a tiny change in openshift/kubernetes downstream and that we generate the warning directly inside the API server.
Does this PR introduce a user-facing change?
Yes, in a DualStack cluster:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: