This was causing tests to pass which ought not be passing. This is not
an API change because we default the value of it when needed. So we
would never see this in the wild, but it makes the tests sloppy.
This scaffolding allows us to assert more on each test case, and more
consistently.
Set input fields from output fields IFF they are expected AND not set on
input. This allows us to verify the "after" state (expected) whether
the test case specified the value or not, and still pass the generic
cmp.Equal.
Use this in a few tests to prove its worth, more to do.
Some of the existing tests that are focused on create and delete can
probably be replaced by these.
This could be used in other test cases that are open-coding a lot of the
same stuff. Later commits.
This is the last layered method. All allocator logic is moved to the
beginUpdate() path. Removing the now-useless layer will happen in a
subsequent commit.
This commit ports the ExternalTrafficPolicy and HealthCheckNodePort
tests from rest_test to storage_test. It's not a direct port, though.
I have added more cases (much more exhaustive) and more assertions.
This commit ports the NodePort test from rest_test to storage_test.
It's not a direct port, though. I have added many more cases (much more
exhaustive) and more assertions.
This includes cases for gate MixedProtocolLBService.
This includes a few cases.
1) TestCreateIgnoresIPFamilyForExternalName: Prove that ExternalName is
ignored for dual-stack. A small set of test cases were chosen to
demonstrate.
2) TestCreateIgnoresIPFamilyWithoutDualStack: Prove that when the
dual-stack gate is off, all services are ignored for dual-stack. A
small set of test cases were chosen to demonstrate
3) TestCreateInitIPFields: Run over a huge array of test cases for
dual-stack. This was generated by this program:
https://gist.github.com/thockin/cccc9c9a580b4830ee0946ddd43eeafe and
then updated by hand.
Gut the "outer" Create() and move it to the inner BeginCreate(). This
uses a "transaction" type to make cleanup functions easy to read.
Background:
Service has an "outer" and "inner" REST handler. This is because of how we do IP and port allocations synchronously, but since we don't have API transactions, we need to roll those back in case of a failure. Both layers use the same `Strategy`, but the outer calls into the inner, which causes a lot of complexity in the code (including an open-coded partial reimplementation of a date-unknown snapshot of the generic REST code) and results in `Prepare` and `Validate` hooks being called twice.
The "normal" REST flow seems to be:
```
mutating webhooks
generic REST store Create {
cleanup = BeginCreate
BeforeCreate {
strategy.PrepareForCreate {
dropDisabledFields
}
strategy.Validate
strategy.Canonicalize
}
createValidation (validating webhooks)
storage Create
cleanup
AfterCreate
Decorator
}
```
Service (before this commit) does:
```
mutating webhooks
svc custom Create {
BeforeCreate {
strategy.PrepareForCreate {
dropDisabledFields
}
strategy.Validate
strategy.Canonicalize
}
Allocations
inner (generic) Create {
cleanup = BeginCreate
BeforeCreate {
strategy.PrepareForCreate {
dropDisabledFields
}
strategy.Validate
strategy.Canonicalize
}
createValidation (validating webhooks)
storage Create
cleanup
AfterCreate
Decorator
}
}
```
After this commit:
```
mutating webhooks
generic REST store Create {
cleanup = BeginCreate
Allocations
BeforeCreate {
strategy.PrepareForCreate {
dropDisabledFields
}
strategy.Validate
strategy.Canonicalize
}
createValidation (validating webhooks)
storage Create
cleanup
AfterCreate
Rollback allocations on error
Decorator
}
```
This same fix pattern will be applied to Delete and Update in subsequent
commits.
All the logic remains unchanged, just reorganized. The functions are
imperfect but emphasize the change being made and can be cleaned up
subsequently.
This makes the following steps easier to comprehend.
Move all allocator-related methods onto the alloc object so it can be
used in either REST layer. There's an INORDINATE amount of test code
here and I am skeptical that it is all useful. That's for later
commits.
Prior to 1.22 a user could change NodePort values within a service
during an update, and the apiserver would allocate values for any that
were not specified.
Consider a YAML like:
```
apiVersion: v1
kind: Service
metadata:
name: foo
spec:
type: NodePort
ports:
- name: p
port: 80
- name: q
port: 81
selector:
app: foo
```
When this is created, nodeport values will be allocated for each port.
Something like:
```
apiVersion: v1
kind: Service
metadata:
name: foo
spec:
clusterIP: 10.0.149.11
type: NodePort
ports:
- name: p
nodePort: 30872
port: 80
protocol: TCP
targetPort: 9376
- name: q
nodePort: 31310
port: 81
protocol: TCP
targetPort: 81
selector:
app: foo
```
If the user PUTs (kubectl replace) the original YAML, we would see that
`.nodePort = 0`, and allocate new ports. This was ugly at best.
In 1.22 we fixed this to not allocate new values if we still had the old
values, but instead re-assign them. Net new ports would still be seen
as `.nodePort = 0` and so new allocations would be made.
This broke a corner case as follows:
Prior to 1.22, the user could PUT this YAML:
```
apiVersion: v1
kind: Service
metadata:
name: foo
spec:
type: NodePort
ports:
- name: p
nodePort: 31310 # note this is the `q` value
port: 80
- name: q
# note this nodePort is not specified
port: 81
selector:
app: foo
```
The `p` port would take the `q` port's value. The `q` port would be
seen as `.nodePort = 0` and a new value allocated. In 1.22 this results
in an error (duplicate value in `p` and `q`).
This is VERY minor but it is an API regression, which we try to avoid,
and the fix is not too horrible.
This commit adds more robust testing of this logic.
Rename `NewCIDRRange()` to `NewInMemory()`
Rename `NewAllocatorCIDRRange()` to `New()`
Rename `NewPortAllocator()` to `NewInMemory()`
Rename `NewPortAllocatorCustom()` to `New()`
If no propagation policy has been set, the pods associated
with the jobs are going to linger because of OrphanDependents
policy set currently. This patch ensures that a warning
will be thrown when the user explicitly doesn't set deletionPolicy.
More context: https://github.com/kubernetes/kubernetes/pull/103449#discussion_r675820335
Add 4 new metrics to the ClusterIP allocators:
- current number of available IPs per Service CIDR
- current number of used IPs per Service CIDR
- total number of allocation per Service CIDR
- total number of allocation errors per ServiceCIDR
For tracking Job Pods that have finished but are not yet counted as failed or succeeded
And feature gate JobTrackingWithFinalizers
Change-Id: I3e080f3ec090922640384b692e88eaf9a544d3b5
It is not uncommon for users to Create a Service and not specify things
like ClusterIP and NodePort, which we then allocate for them. They same
that YAML somewhere and later use it again in an Update, but then it
fails.
That's because we detected them trying to set a ClusterIP from a value
to "", which is not allowed. If it was just NodePort, they would
actually succeed and reallocate a new port.
After this change, we try to "patch" updates where the user did not
specify those values from the old object.
Modify the behavior of the AnyVolumeDataSource alpha feature gate to enable
a new field, DataSourceRef, rather than modifying the behavior of the
existing DataSource field. This allows addition Volume Populators in a way
that doesn't risk breaking backwards compatibility, although it will
result in eventually deprecating the DataSource field.
* pkg/features: promote the ServiceInternalTrafficPolicy field to Beta and on by default
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/api/service/testing: update Service test fixture functions to set internalTrafficPolicy=Cluster by default
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/apis/core/validation: add more Service validation tests for internalTrafficPolicy
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/registry/core/service/storage: fix failing Service REST storage tests to use internalTrafficPolicy: Cluster
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/registry/core/service/storage: add two test cases for Service REST TestServiceRegistryInternalTrafficPolicyClusterThenLocal and TestServiceRegistryInternalTrafficPolicyLocalThenCluster
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/registry/core/service: update strategy unit tests to expect default
internalTrafficPolicy=Cluster
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/proxy/ipvs: fix unit test Test_EndpointSliceReadyAndTerminatingLocal to use internalTrafficPolicy=Cluster
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/apis/core: update fuzzers to set Service internalTrafficPolicy field
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
* pkg/api/service/testing: refactor Service test fixtures to use Tweak funcs
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
1. add AllocateLoadBalancerNodePorts fields in specs for validation test cases
2. update fuzzer
3. in resource quota e2e, allocate node port for loadbalancer type service and
exceed the node port quota
Signed-off-by: Hanlin Shi <shihanlin9@gmail.com>
This change updates the CSR API to add a new, optional field called
expirationSeconds. This field is a request to the signer for the
maximum duration the client wishes the cert to have. The signer is
free to ignore this request based on its own internal policy. The
signers built-in to KCM will honor this field if it is not set to a
value greater than --cluster-signing-duration. The minimum allowed
value for this field is 600 seconds (ten minutes).
This change will help enforce safer durations for certificates in
the Kube ecosystem and will help related projects such as
cert-manager with their migration to the Kube CSR API.
Future enhancements may update the Kubelet to take advantage of this
field when it is configured in a way that can tolerate shorter
certificate lifespans with regular rotation.
Signed-off-by: Monis Khan <mok@vmware.com>
This test was sometimes using the "inner" REST and sometimes using the
"outer" REST. This commit changes all but one test to use the outer.
The remaining test needs rework.
The rest api for services was validating that, on updates, both
the old and new service have the same type. That guarantees that
the type is going to be the same after that, thus we don't need
to validate the service type on the old and the new service.
When the StatefulSetMinReadySeconds feature gate is disabled,
the registry and validation must properly handle dropping the
minReadySeconds and AvailableReplicas fields
Take the following approach:
On a fresh install, all bootstrap configuration objects will
have auto update enabled via the following annotation :
`apf.kubernetes.io/autoupdate: 'true'`
The kube-apiserver periodically checks the bootstrap configuration
objects on the cluster and applies update if necessary.
We enforce an 'always auto-update' policy for the mandatory
configuration object(s).
We update the suggested configuration objects when:
- auto update is enabled (`apf.kubernetes.io/autoupdate: 'true'`) or
- auto update annotation key is missing but `generation` is `1`
If the configuration object is missing the annotation key, we add
it appropriately:
it is set to `true` if `generation` is `1`, `false` otherwise.
The above approach ensures that we don't squash changes made by an
operator. Please note, we can't protect the changes made by the
operator in the following scenario:
- the user changes the spec and then deletes and recreates
the same object. (generation resets to 1)
remove using a marker
Now the following flags have no effect and would be removed in v1.24:
* `--port`
* `--address`
The insecure port flags `--port` may only be set to 0 now.
Signed-off-by: Jian Zeng <zengjian.zj@bytedance.com>
- Test all versions to make sure each resource version is in the
mappings
- Fail when request info contains an unrecognized version. We have tests
that guarantee that all known versions are in the mappings. If we
get a version in request info that is not there we should fail fast to
prevent inconsistent behaviour (e.g. for some reason the mappings is
not up to date).
Ensure all known versions are in mappings
* Use deep copies in `PrepareForUpdate()`
* Preserve select metadata from new pod
* Use patch to add ephemeral container `kubectl debug`
* Distinguish between pod vs /ephemeralcontainers NotFound
This changes the `/ephemeralcontainers` subresource of `/pods` to use
the `Pod` kind rather than `EphemeralContainers`.
When designing this API initially it seemed preferable to create a new
kind containing only the pod's ephemeral containers, similar to how
binding and scaling work.
It later became clear that this made admission control more difficult
because the controller wouldn't be presented with the entire Pod, so we
updated this to operate on the entire Pod, similar to how `/status`
works.
We should not attempt creation of mandatory objects if ensuring
the suggested configuration resulted in an error.
We rely on the presence of the "exempt" priority level configuration
object in the cluster to indicate whether we should ensure
suggested configuration.
Adds and implements ResetFieldsProvder interface in order to ensure that
the fieldmanager no longer owns fields that get reset before the object
is persisted.
Co-authored-by: Kevin Wiesmueller <kwiesmul@redhat.com>
Co-authored-by: Kevin Delgado <kevindelgado@google.com>
* namespace by name default labelling
Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
Co-authored-by: Abhishek Raut <rauta@vmware.com>
* Make some logic improvement into default namespace label
* Fix unit tests
* minor change to trigger the CI
* Correct some tests and validation behaviors
* Add Canonicalize normalization and improve validation
* Remove label validation that should be dealt by strategy
* Update defaults_test.go
add fuzzer
ns spec
* remove the finalizer thingy
* Fix integration test
* Add namespace canonicalize unit test
* Improve validation code and code comments
* move validation of labels to validateupdate
* spacex will save us all
* add comment to testget
* readablility of canonicalize
* Added namespace finalize and status update validation
* comment about ungenerated names
* correcting a missing line on storage_test
* Update the namespace validation unit test
* Add more missing unit test changes
* Let's just blast the value. Also documenting the workflow here
* Remove unnecessary validations
Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
Co-authored-by: Abhishek Raut <rauta@vmware.com>
Co-authored-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
That the object was registered depending on the feature gate was
called out as unusual during the 1.21 review. Previously, all beta
storage APIs were unders such feature gate checks, but its better to
drop that to be consistent with the rest of Kubernetes.
Defaults and validation are such that the field has to be set when
the feature is enabled, just as for the other boolean fields. This
was missing in some tests, which was okay as long as they ran
with the feature disabled. Once it gets enabled, validation will
flag the missing field as error.
Other tests didn't run at all.
1. Add API definitions;
2. Add feature gate and drops the field when feature gate is not on;
3. Set default values for the field;
4. Add API Validation
5. add kube-proxy iptables and ipvs implementations
6. add tests
It's not enough to silently drop the volume type if the feature is
disabled. Instead, the policy should fail validation, just as it would
have if the API server didn't know about the feature at all.
* Removes discovery v1alpha1 API
* Replaces per Endpoint Topology with a read only DeprecatedTopology
in GA API
* Adds per Endpoint Zone field in GA API
EndpointSlice labels can be quite meaningful. They are used to indicate
the controller they are managed by and the Service they are associated
with. Changing these labels can have significant affects on how the
EndpointSlice is consumed so incrementing generation seems appropriate.
Imporved testing turned these up:
1) Headless+Selectorless, on a single-stack cluster, policy=PreferDual
Prior to this commit, the result was a single IPFamiliy (because we
checked that the 2nd allocator was present). This changes that case to
populate both families (we don't care if the allocator exists), which is
the same as RequireDual.
2) ClusterIP, user specifies 2 families but no IPs
Prior to this commit, the policy was inferred to be SingleStack. This
changes that case to correctly default to RequireDual when 2 families
are present but no IPs.
* Fix merge conflict in kube_features
* Add alpha support for EndPort in Network Policy
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
* Add alpha support for EndPort in Network Policy
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
* Add alpha support for EndPort in Network Policy
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
* Correct some nits
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
* Add alpha support for EndPort in Network Policy
* Add alpha support for EndPort in Network Policy
* Add alpha support for EndPort in Network Policy
* Add alpha support for EndPort in Network Policy
Follow the original TODO from back in c86b84c with the errors added
in d3be1ac. Edit the TODO to make clear that a dynamic response would
still be ideal.
Dramatically reduce the time based on suggestion in PR, and remove name from TODO
as not currently active.
When the maxsurge daemonset gate is disabled, the registry and validation
must properly handle stripping the field. In the special case where that
would leave the MaxUnavailable field set to 0, we must set it to 1 which
is the default value.
* Mixed protocol support for Services with type=LoadBalancer
KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20200103-mixed-protocol-lb.md
Add new feature gate to control the support of mixed protocols in Services with type=LoadBalancer
Add new fields to the ServiceStatus
Add Ports to the LoadBalancerIngress, so cloud provider implementations can report the status of the requested load balanc
er ports
Add ServiceCondition to the ServiceStatus so Service controllers can indicate the conditions of the Service
* regenerate conflicting stuff
