knative / pkg Goto Github PK

Blocked on #3

For example creating a Condition for a route requires three package imports

• duckv1alpha1
• servingv1alpha1
• corev1

For example:

duckv1alpha1.Condition{
	Type:    v1alpha1.RouteConditionAllTrafficAssigned,
	Status:  corev1.ConditionUnknown,
	Reason:  "RevisionMissing",
	Message: `Configuration "not-ready" is waiting for a Revision to become ready.`,
}

I'm curious if people would consider type aliasing and potentially some go:generate to automate the aliasing to produce something with one import

For example:

v1alpha1.Condition{
	Type:    v1alpha1.RouteConditionAllTrafficAssigned,
	Status:  v1alpha1.ConditionUnknown,
	Reason:  "RevisionMissing",
	Message: `Configuration "not-ready" is waiting for a Revision to become ready.`,
}

lack of the name of svc, full connectable DNSnames are

1. {svcName}
2. {svcName}.{Namespace}
3. {svcName}.{Namespace}.svc
4. {svcName}.{Namespace}.svc.cluster.local

/assign @zrss

Expected Behavior

I have an array of objects. I want to validate that each of those objects has at least one field. Ideally, I'd like to return a message to the user saying that index 1 and 2 in the array are both missing fields.

Actual Behavior

I can't do that today because there's no concept of a non-fatal validation error. It might be useful for validation methods to take a parameter like testing.T that exposes richer error collection behavior than is possible with a return value.

/cc @n3wscott

and

we have a process that ensures the correct way to handle the mutatingwebhookconfiguration of the webhook server

/assign

Expected Behaviour

Expect running the unit test TestEnqueues to not fail

Actual Behaviour

TestEnqueues did fail here:

I0910 19:38:25.078] --- FAIL: TestEnqueues (0.11s)
I0910 19:38:25.078]     --- PASS: TestEnqueues/do_nothing (0.01s)
I0910 19:38:25.078]     --- PASS: TestEnqueues/enqueue_key (0.01s)
I0910 19:38:25.078]     --- FAIL: TestEnqueues/enqueue_duplicate_key (0.01s)
I0910 19:38:25.079]     	controller_test.go:244: unexpected queue (-want +got): {[]string}:
I0910 19:38:25.079]     			-: []string{"foo/bar"}
I0910 19:38:25.079]     			+: []string(nil)
I0910 19:38:25.079]     --- PASS: TestEnqueues/enqueue_different_keys (0.01s)
I0910 19:38:25.079]     --- PASS: TestEnqueues/enqueue_resource (0.01s)
I0910 19:38:25.079]     --- PASS: TestEnqueues/enqueue_bad_resource (0.01s)
I0910 19:38:25.079]     --- PASS: TestEnqueues/enqueue_controller_of_bad_resource (0.01s)
I0910 19:38:25.079]     --- PASS: TestEnqueues/enqueue_controller_of_resource_without_owner (0.01s)
I0910 19:38:25.079]     --- PASS: TestEnqueues/enqueue_controller_of_resource_with_owner (0.01s)

Steps to Reproduce the Problem

I'm actually unable to reproduce it locally using go version go1.10.3 darwin/amd64

go test -run TestEnqueues -count 1000 > out.txt

Expected Behavior

exit when webhook http server failed to bootstrap

Actual Behavior

Steps to Reproduce the Problem

1. 443 port is lower than 1000, and it is assumed to be a root user port caddy-listen-tcp-443-bind-permission-denied, and webhook server is still runing with the error msg if u run it with other user instead of root

/assign

and kindly ping aprovers for thought (... eh, sorry for a weekend day 😅)

@mattmoor, @grantr, @tcnghia

We should expose a prometheus metric for each configmap with the metadata.resourceVersion that an "informed watcher" has successfully received. This is useful for tracking configuration rollouts.

cc @mdemirhan @dprotaso

Expected Behavior

Expose ClientAuth as an option for webhook

Actual Behavior

/assign

Maybe adding them now is of no use, but at least building them would be useful. The motivation is that #2440 seems to have broken the tests.

/area test-and-release

/kind feature
/kind good-first-issue

cc @jessiezcc @srinivashegde86

It would be good to be able to assess when things enter the controller's workqueue, so that we can surface a metric for queue time.

cc @mdemirhan

There are a class of cases where object Foo is watching object Bar and we want to trigger level-based reconciliation of Foo when Bar changes. There are two cases of this in knative/serving today:

1. Revision -> Build, which is done via the specialized build tracker (see knative/serving#1267)
2. Route -> Configuration, which is done by relying on the labels we introduced for enforcing 1:N and hope to retain the capacity to relax (see knative/serving#1495).

The former is complicated by our desire to loosen the coupling between our repos.

We effectively want a generalized Tracker that during reconciliation enables us to create edges from corev1.ObjectReference to a key that should be enqueued for reconciliation.

For where we are with Revision -> Build (with spec.buildName) we can use something like:

tracker.Track(corev1.ObjectReference{
  APIVersion: "build.knative.dev/v1alpha1",
  Kind: "Build",
  Namespace: rev.Namespace,
  Name: rev.Spec.BuildName,
}, rev)

For where we want to go (with spec.buildRef) we can use something like:

buildRef := rev.Spec.BuildRef.DeepCopy()
buildRef.Namespace = rev.Namespace 
tracker.Track(buildRef, rev)

For Route -> Configuration and Revision we want something similar to what's outlined for Build where we are today, but needs some facility for aging out records (effectively requiring resyncs to refresh leases).

I'm going to experiment with usable APIs for this, and circle back here to summarize anything I come up with that I like.

cc @tcnghia @imikushin

We just bumped this on the knative/serving controllers, but need the same here for our configmap package.

This should probably be made an argument.

cc @n3wscott @dprotaso

Things like FieldError, Validatable, Defaultable.

Expected Behavior

i can easily add more DNSNames or IP addresses in Subject Alternate Name values of Webhook's Cert

currently, we only support the fixed {serviceName}.{namespace} DNSNames values

Actual Behavior

i expose my webhook server at a IP:PORT address (for fiting the knative to my lab env 😐), and this forces me to change the all func params in the call chain of creating a cert

Additional Info

we will go through this chain when we create a cert,

generateSecret (options) ->

1. CreateCerts (serviceName, namespace)
2. createCA (serviceName, namespace)
3. createCACertTemplate (serviceName, namespace)
4. createCertTemplate (serviceName, namespace)

and we generate the DNSNames at the final step, i.e. createCertTemplate(serviceName, namespace)

we have totally 4 steps to generate the cert, but serviceName and namespace just be paased through in first 3 steps

so i propose refactoring

1. CreateCerts/createCA/createCACertTemplate/createCertTemplate accept a string slice of DNSName and a IP slice of IPAdress (for my case)
2. Generate DNSNames of webhook svc in generateSecret func (as it's the first place that has options)

kindly ping @mattmoor , @grantr , @tcnghia for thoughts

The coverage dashboard for this repo only shows pkg/apis and pkg/configmap, but no other subdirectories.

https://testgrid.knative.dev/knative-pkg#coverage

Expected Behavior

This useful helper function is shared across all repos through pkg.

Actual Behavior

This function is duplicated in several repos, like in https://github.com/knative/build-pipeline/blob/master/test/randstring.go (with test) and https://github.com/knative/serving/blob/02d3c9b657fdefe76690a1d5751ae0b2c4e414ae/test/crd.go#L255

Today we treat ./pkg/apis/duck/v1alpha1 like any other API resources, but because these aren't real resources we likely only need a small subset of the overall codegen (e.g. deepcopy).

This tracks reducing the amount of codegen we do for these types to make sure folks don't inadvertently try to use one of their clients, since these are not real resources and effectively only exist for use with the dynamic client to deserialize partial objects.

cc @vaikas-google

Blocked on knative/serving#1505 (IMO)

Expected Behavior

• knative/pkg/api/istio does not implement ServiceEntry which is part of apis/networking.istio.io/v1alpha3
• Please implement it.

Actual Behavior

• None: It is missing

Steps to Reproduce the Problem

Additional Info

See networking.istio.io/v1alpha3 for the full list.

Prior to knative/serving#1937 there were more options available for running tests. Specifically, those in the ServingEnvironmentFlags struct, but because the flags are now parsed in pkg/test, none of the flags defined in serving/test are available now.

I'm thinking serving/test is where flag.Parse should be called.

This code deals with ResourceVersion. The current use of ResourceVersion stems from the Kubernetes bug where it fails to bump Generation for CRDs (still affects us until we move to 1.11 w/ /status subresource).

For CRDs without status (e.g. WarmImage, BuildTemplate) ResourceVersion is a useful stand-in because it changes at roughly the same pace as Spec.

However, for resources with status (e.g. Revision, or K8s resources) the methods we have are of limited use because they are too high-churn; this is because ResourceVersion will change every time /status changes as well, making the current methods unsuitable for child-resource instantiation for these parent resources.

These Generation variants will only be useful for our own CRDs once we're on 1.11, but once we are they should become the standard way of achieving this.

We're using duck.VerifyType to enforce that serialization formats match. These statements are being placed in packages as follows:

This technique means verify is invoked at runtime during package initialization.

As we add more potential types that need to be verified this package initialization technique has an affect on a program's startup.

To avoid this startup time cost, we can simply move these assertions to unit tests that invoke VerifyType

func TestTargetImplementsTargetable(t *testing.T) {
  duck.VerifyType(&Target{}, &Targetable{}) 
}

Serving currently contains CRD types and client for Istio. This is also used by eventing and should be moved here to further decouple eventing from serving.

https://github.com/knative/serving/tree/master/pkg/apis/istio

Just saw this in the client-go CHANGELOG.md for v9.0.0

[k8s.io/apimachinery] Unstructured metadata accessors now respect omitempty semantics i.e. a field having zero value will now be removed from the unstructured metadata map. (#67635)

If we're using an older version this bug might affect duck typing - unsure how and when this would be an issue

This repo houses shared Knative libraries. The original intent was to share common controller libraries and whatnot, but I think that there's a clear and growing need to share common test and release logic as well.

Two clear candidates off the top of my head (the common bits):

1. github.com/knative/serving/pkg/controller/testing
2. github.com/knative/serving/test

cc @bobcatfish (emeritus)

Would it make sense to cache the http.Client in the Client object? Might allow for easier testing if you wanted to replace it.

Originally posted by @evankanderson in https://github.com/knative/pkg/diffs

/area test-and-release
/kind cleanup

We should have flag parsing being performed in pkg, because that initializes the common logging infrastructure, and also avoids replicated boilerplate code (plus bug fixes, unification, etc).

We should have a simple and documented process of adding extra flags defined in a repo/test/etc.

See knative/serving#2372 for an example of this. Sanitizing inputs would have caught this.

Expected Behavior

Applying this YAML via kubectl

apiVersion: eventing.knative.dev/v1alpha1
kind: Channel
metadata:
  name: empty

Should have a spec added with the correct default values (such as spec.generation) and the resource should be created in the cluster.

Actual Behavior

The YAML is rejected because the mutating webhook generates the following JSON patch:

[{"op":"add","path":"/spec/generation","value":1}]

The API server rejects the patch and thus the entire object is not created:

$ kubectl apply -f channel-empty.yaml
Error from server (InternalError): error when creating "channel-empty.yaml": Internal error occurred: Internal error occurred: jsonpatch add operation does not apply: doc is missing path: /spec/generation

Steps to Reproduce the Problem

1. Find or create a GenericCRD that has a spec.generation but nothing is validated.

2. Make sure the Webhook is registered to mutate that GenericCRD.

3. Apply a yaml that does not include the spec.

4. kubectl should give an error saying: doc is missing path: /spec/generation.

5. Look in the Webhook's logs to find the exact JSON patch instructions given. Here is a log from my webhook:

   {
     "caller": "webhook/webhook.go:442",
     "knative.dev/controller": "webhook",
     "knative.dev/kind": "eventing.knative.dev/v1alpha1, Kind=Channel",
     "knative.dev/name": "",
     "knative.dev/namespace": "default",
     "knative.dev/operation": "CREATE",
     "knative.dev/resource": "{eventing.knative.dev v1alpha1 channels}",
     "knative.dev/subresource": "",
     "knative.dev/userinfo": "{[email protected] [system:authenticated] map[user-assertion.cloud.google.com:[...]]}",
     "level": "info",
     "logger": "webhook",
     "msg": "AdmissionReview for eventing.knative.dev/v1alpha1, Kind=Channel: default/ response=&AdmissionResponse{UID:fa015085-e12c-11e8-9bb9-42010a8000e5,Allowed:true,Result:nil,Patch:*[91 123 34 111 112 34 58 34 97 100 100 34 44 34 112 97 116 104 34 58 34 47 115 112 101 99 47 103 101 110 101 114 97 116 105 111 110 34 44 34 118 97 108 117 101 34 58 49 125 93],PatchType:*JSONPatch,}",
     "ts": "2018-11-05T18:59:52.402Z"
   }

   • Note that the byte array is the JSON patch as a series of characters. I used the following python to translate back to a readable string:

     def p(s):
      o = ""
      t = s.replace(" ", ", ")
      t2 = ast.literal_eval(t)
      for c in t2:
        o += unichr(c)
      return o

Additional Info

The API server is correct to reject the request because the RFC specifically states:

For example, an "add" with a target location of "/a/b" ...
It is an error in this document:

   { "q": { "bar": 2 } }

   because "a" does not exist.

Everything works correctly if spec is defined as an object.

apiVersion: eventing.knative.dev/v1alpha1
kind: Channel
metadata:
  name: empty
spec: {}

Expected Problem

We believe the problem exists because we create the JSON patch by comparing deserialized Go objects. As the Go object uses a struct, rather than a pointer to a struct, as its field, Go automatically 'fills in' what is missing. A possible solution is to seed the list of patches with the differences between the original bytes and the round-tripped through Go bytes here. Note that because patch operations are ordered, this would need to happen before setting the generation here

roundTrippedNewObj, err := json.Marshall(newObj)
if err != nil {
  return nil, err
}
patches = append(patches, jsonpatch.CreatePatch(newObj, roundTrippedNewObj))

Expected Behavior

New, required fields must have a default value to avoid breaking upgrades.

Actual Behavior

metrics.backend-destination is required but not defaulted.

Expected Behavior

I should be able to pass a patch created from our apis.duck.patch to a fake client.

Actual Behavior

This fails due to https://github.com/knative/pkg/blob/master/vendor/k8s.io/client-go/testing/fixture.go#L140 - k8s fake clients only support StrategicMergePatch's (https://github.com/kubernetes/community/blob/master/contributors/devel/strategic-merge-patch.md) and we produce a JSONPatch.

Expected Behavior

If we don't find the correct key in the configmap for webhook registrationg, we display the correct value:

Actual Behavior

prints ca.crt instead of the key we're missing (in this case:)
requestheader-client-ca-file

Steps to Reproduce the Problem

1. Delete that key and watch the webhook pods be unhappy (crashloop backoff)
2. log messages display incorrect error message

Additional Info

Today, pkg/apis/duck is almost exclusively useful for reading duck-typed data. However, it is also possible for us to leverage duck typing for read-modify-write operations on duck-typed data.

The basic idea is that we:

1. Define a partial schema for spec,
2. Parse the opaque resource we want to mutate as our Duck type,
3. DeepCopy the original resource,
4. Modify the copy via the duck type's spec,
5. Compute a JSON Patch of the delta,
6. Issue a PATCH against the API via the dynamic client.

A great example to help us drive this is the way we adjust Generations in our mutating webhook, which does all but the last of the above (the JSON Patch is returned from the mutating webhook instead).

/assign @mattmoor
cc @vaikas-google @n3wscott

Kudos to @imjasonh for the inspiration.

Expected Behavior

In the CloudEvent Spec the ContentType attribute is used to describe the type of the data payload of the CloudEvent. This should be able to be set to any MIME type.

Actual Behavior

The CloudEvents SDK gives the following error

Failed to create http request: Cannot encode content type "text/plain"

Steps to Reproduce the Problem

ctx := cloudevents.EventContext{
CloudEventsVersion: cloudevents.CloudEventsVersion,
EventType:          "foo.
EventID:            "foo",
Source:             "foo",
ContentType:        "text/plain",
}
...
req, err := cloudevents.Binary.NewRequest("http://localhost:8080/foo", "foobar", *ctx)

//error

Expected Behavior

CNCF will announce 0.2 of the spec

Actual Behavior

Knative's impl is compliant. Question: how much do we care about backwards comp. ? Or just move on ?

Expected Behavior

When adding a ChannelSubscriberSpec it's not possible to currently have duplicate subscriptions and it is probably better to include either an ObjectRef or something like that as part of the entry being added to the list so that Subscription Controller can keep track of Subscription -> Entry in the list of subscribers.

// ChannelSubscriberSpec defines a single subscriber to a Channel.
// CallableDomain is the endpoint for the call
// SinkableDomain is the endpoint for the result
// One of them must be present
type ChannelSubscriberSpec struct {
// +optional
CallableDomain string json:"callableDomain,omitempty"
// +optional
SinkableDomain string json:"sinkableDomain,omitempty"
}

Right now for example it's not possible to have duplicate Subscriptions because of this. Well, at least not correctly without complicating things (like refcounting).

@grantr @n3wscott

Actual Behavior

Steps to Reproduce the Problem

Additional Info

/area test-and-release
/kind cleanup
/kind good-first-issue

Flagset is used to define a set of related flags. We can use a flagset to define the different set of flags used in our e2e tests.

More info on flagset:
https://golang.org/pkg/flag/#FlagSet

Example usage:
http://blog.ralch.com/tutorial/golang-subcommands/

Expected Behavior

./pkg/controller tests to always pass

Actual Behavior

it failed. Then passed. Running it multiple times, saw failures. This test might be flaky.
--- FAIL: TestEnqueues (0.11s)
--- FAIL: TestEnqueues/enqueue_controller_of_resource_with_owner (0.01s)
controller_test.go:244: unexpected queue (-want +got): {[]string}:
-: []string{"bar/baz"}
+: []string(nil)

Steps to Reproduce the Problem

1. checkout the repo
2. run controller tests multiple times: go test ./controller/... -count 1000 2>&1 | tee /tmp/testlogs
3. fails

Additional Info

/area API
/area test-and-release

/kind cleanup

Expected Behavior

In general libraries should make logging optional or not log at all, so that the user of the library can control when the libraries log.

Specifically, the logging library in serving should allow users to control whether or not it outputs logs, or not log at all.

Actual Behavior

If a new logger is instantiated with knative/serving/blob/master/pkg/logging/config.go, logs are output at Info and Error levels. This can be seen when running the e2e tests (see below), the first line of output is:

info	logging/config.go:90	Successfully created the logger.	{"knative.dev/jsonconfig": "{\n\t  \"level\": \"info\",\n\t  \"encoding\": \"console\",\n\t  \"outputPaths\": [\"stdout\"],\n\t  \"errorOutputPaths\": [\"stderr\"],\n\t  \"encoderConfig\": {\n\t    \"messageKey\": \"message\",\n\t\t\t\"levelKey\": \"level\",\n\t\t\t\"nameKey\": \"logger\",\n\t\t\t\"callerKey\": \"caller\",\n\t\t\t\"messageKey\": \"msg\",\n      \"stacktraceKey\": \"stacktrace\",\n      \"lineEnding\": \"\",\n      \"levelEncoder\": \"\",\n      \"timeEncoder\": \"\",\n      \"durationEncoder\": \"\",\n      \"callerEncoder\": \"\"\n\t  }\n\t}"}

This should be optional so the tests can suppress it. (Bonus, if we DO want output like this, it could be human readable.)

Steps to Reproduce the Problem

You can reproduce this by running the tests with go test -v -tags=e2e -count=1 ./test/conformance:

➜  serving git:(master) ✗ go test -v -tags=e2e -count=1 ./test/conformance
info	logging/config.go:90	Successfully created the logger.	{"knative.dev/jsonconfig": "{\n\t  \"level\": \"info\",\n\t  \"encoding\": \"console\",\n\t  \"outputPaths\": [\"stdout\"],\n\t  \"errorOutputPaths\": [\"stderr\"],\n\t  \"encoderConfig\": {\n\t    \"messageKey\": \"message\",\n\t\t\t\"levelKey\": \"level\",\n\t\t\t\"nameKey\": \"logger\",\n\t\t\t\"callerKey\": \"caller\",\n\t\t\t\"messageKey\": \"msg\",\n      \"stacktraceKey\": \"stacktrace\",\n      \"lineEnding\": \"\",\n      \"levelEncoder\": \"\",\n      \"timeEncoder\": \"\",\n      \"durationEncoder\": \"\",\n      \"callerEncoder\": \"\"\n\t  }\n\t}"}
info	logging/config.go:91	Logging level set to info
=== RUN   TestBlueGreenRoute
info	TestBlueGreenRoute	test/crd.go:162	Seeding rand.Rand with 1532043361426063011
info	TestBlueGreenRoute	conformance/blue_green_test.go:166	Creating a Configuration
info	TestBlueGreenRoute	conformance/blue_green_test.go:173	The Configuration will be updated with the name of the Revision once it is created

Additional Info

n/a

Expected Behavior

Log messages to display what resources they are working with

Actual Behavior

They are not:
{"level":"info","ts":1547068546.5497584,"logger":"fallback","caller":"sdk/reconciler.go:52","msg":"Reconciling &TypeMeta{Kind:,APIVersion:,} default/laconia-source"}
{"level":"warn","ts":1547068546.687146,"logger":"fallback","caller":"sdk/reconciler.go:74","msg":"Failed to reconcile &TypeMeta{Kind:,APIVersion:,}: services "message-dumper" is forbidden: User "system:serviceaccount:knative-sources:gcppubsub" cannot get services in the namespace "default""}
{"level":"info","ts":1547068567.1675546,"logger":"fallback","caller":"sdk/reconciler.go:52","msg":"Reconciling &TypeMeta{Kind:,APIVersion:,} default/laconia-source"}

Steps to Reproduce the Problem

1. Run any eventing-source examples, say pubsub
2. look at the logs and you see that the type information is not properly displayed in the logs.

Additional Info

Makes things less bad wrt API versioning.