Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 2 additions & 0 deletions pkg/activator/net/revision_backends.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ type revisionDestsUpdate struct {
const (
probeTimeout time.Duration = 300 * time.Millisecond
probeFrequency time.Duration = 200 * time.Millisecond
probePath = "/_internal/knative/activator/probe"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some random path for activator to probe. It can be changed as @tcnghia has mentioned here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be constant here, which is fine.
It needs to be documented as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Though it seems quite long for a random probe path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can follow up to extract this to a configurable option in the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, please fine an issue, so it's not forgotten

)

// revisionWatcher watches the podIPs and ClusterIP of the service for a revision. It implements the logic
Expand Down Expand Up @@ -135,6 +136,7 @@ func (rw *revisionWatcher) probe(ctx context.Context, dest string) (bool, error)
httpDest := url.URL{
Scheme: "http",
Host: dest,
Path: probePath,
}
// NOTE: changes below may require changes to testing/roundtripper.go to make unit tests passing.
return prober.Do(ctx, rw.transport, httpDest.String(),
Expand Down
7 changes: 7 additions & 0 deletions test/config/100-namespace.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,10 @@ apiVersion: v1
kind: Namespace
metadata:
name: serving-tests-alt
---
apiVersion: v1
kind: Namespace
metadata:
name: serving-tests-sidecar-enabled
labels:
istio-injection: enabled
16 changes: 16 additions & 0 deletions test/config/auth/policy.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
apiVersion: authentication.istio.io/v1alpha1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

kubectl apply -f is not recursive without option, so we need to explicitly apply this policy in e2e-common.sh like

ko apply ${KO_FLAGS} -f test/config/mtls/ || return 1

But could you please consider this https://github.com/knative/serving/pull/6505/files#r365568314 first?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if i add the policy to serving-tests or serving-tests-alt, it would affect the other tests run on these ns as it would require token to call the service end point.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see. I wondered we can set excludedPaths.exact: / and use another path for the test. But as per your report #5918, we want to make sure that / is not necessary to be exluded. Then, it seems that we need to create new namespace for this.

kind: Policy
metadata:
name: default
namespace: serving-tests-sidecar-enabled
spec:
origins:
- jwt:
issuer: testing@secure.istio.io
jwksUri: https://raw.githubusercontent.com/istio/istio/release-1.4/security/tools/jwt/samples/jwks.json
triggerRules:
- excludedPaths:
- prefix: /metrics
- prefix: /_internal/knative/activator/probe
principalBinding: USE_ORIGIN

1 change: 1 addition & 0 deletions test/conformance.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const (
PizzaPlanetText1 = "What a spaceport!"
PizzaPlanetText2 = "Re-energize yourself with a slice of pepperoni!"
HelloWorldText = "Hello World! How about some tasty noodles?"
UnauthorizedText = "Origin authentication failed."

ConcurrentRequests = 50
// We expect to see 100% of requests succeed for traffic sent directly to revisions.
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/e2e.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ func SetupAlternativeNamespace(t *testing.T) *test.Clients {
return SetupWithNamespace(t, test.AlternativeServingNamespace)
}

//set up sidecar enabled ns
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
func SetupSideCarEnabledNamespace(t *testing.T) *test.Clients {
return SetupWithNamespace(t, test.SideCarServingNamespace)
}

// SetupWithNamespace creates the client objects needed in the e2e tests under the specified namespace.
func SetupWithNamespace(t *testing.T, namespace string) *test.Clients {
pkgTest.SetupLoggingFlags()
Expand Down
84 changes: 84 additions & 0 deletions test/e2e/probe_whitelist_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// +build e2e

/*
Copyright 2018 The Knative 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 e2e

import (
"testing"

pkgTest "knative.dev/pkg/test"
"knative.dev/pkg/test/logstream"
"knative.dev/serving/test"
v1a1test "knative.dev/serving/test/v1alpha1"
)

//This test checks if the activator can probe
//the service when istio end user auth policy is
//applied on the service.
//This test needs istio side car injected and
//istio policy check enabled. If both are not
//enabled the test will pass
//policy is present test/config/auth/policy.yaml
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
//apply policy before running this test
func TestProbeWhitelist(t *testing.T) {
t.Parallel()
cancel := logstream.Start(t)
defer cancel()

clients := SetupSideCarEnabledNamespace(t)

names := test.ResourceNames{
Service: test.ObjectNameForTest(t),
Image: "helloworld",
}

test.CleanupOnInterrupt(func() { test.TearDown(clients, names) })
defer test.TearDown(clients, names)

t.Log("Creating a new Service")
resources, httpsTransportOption, err := v1a1test.CreateRunLatestServiceReady(t, clients, &names, test.ServingFlags.Https)
if err != nil {
t.Fatalf("Failed to create initial Service: %v: %v", names.Service, err)
}

url := resources.Route.Status.URL.URL()
var opt interface{}
if test.ServingFlags.Https {
url.Scheme = "https"
if httpsTransportOption == nil {
t.Fatalf("Https transport option is nil")
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
}
opt = *httpsTransportOption
}
if _, err := pkgTest.WaitForEndpointState(
clients.KubeClient,
t.Logf,
url,
v1a1test.RetryingRouteInconsistency(pkgTest.MatchesAllOf(pkgTest.MatchesBody(test.UnauthorizedText))),
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
"HelloWorldServesAuthFailed",
test.ServingFlags.ResolvableDomain,
opt); err != nil {
// check if side car is injected before reporting error
_, err = getContainer(clients.KubeClient, resources.Service.Name, "istio-proxy", resources.Service.Namespace)
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
if err != nil {
t.Log("side car not enabled, skipping test")
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
return
}
t.Fatalf("The endpoint %s for Route %s didn't serve the expected text %q: %v", url, names.Route, test.UnauthorizedText, err)
}
}
2 changes: 2 additions & 0 deletions test/e2e_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ const (
// namespace tests in.
AlternativeServingNamespace = "serving-tests-alt"

// side car injected ns
Comment thread
itsmurugappan marked this conversation as resolved.
Outdated
SideCarServingNamespace = "serving-tests-sidecar-enabled"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

serving-tests and serving-tests-alt namespaces also enabled side car injection on Mesh mode.

serving/test/e2e-common.sh

Lines 433 to 435 in 28f2a24

if (( MESH )); then
kubectl label namespace serving-tests istio-injection=enabled
kubectl label namespace serving-tests-alt istio-injection=enabled

So, it would be better to use either of the namespace. I wonder that we can merge your config into https://github.com/knative/serving/blob/master/test/config/mtls/policy.yaml. Then, we should change the directory name to security or something from /auth, /mtls.

@itsmurugappan itsmurugappan Jan 12, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if i add the policy to serving-tests or serving-tests-alt, it would affect the other tests run on these ns as it would require token to call the service end point.

@itsmurugappan itsmurugappan Jan 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i have moved the files from mtls to security folder and made the changes in e2e-common.sh
@nak3 Please review.

// Environment propagation conformance test objects

// ConformanceConfigMap is the name of the configmap to propagate env variables from
Expand Down