Skip to content
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.

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

Check for Namespace level config override annotations #3194

Merged
merged 5 commits into from Aug 15, 2019

Conversation

Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Aug 5, 2019

Fixes #2499

This PR makes the proxy-injector check for Namespace Level Override Config annotations after checking the Pod level override config annotations before going with the default values.

I tried with a Cluster and it considers the annotations of the namespace before going with the defaults.

@ihcsim @grampelberg

Signed-off-by: Tarun Pothulapati tarunpothulapati@outlook.com

@alpeb
Copy link
Member

alpeb commented Aug 7, 2019

Thanks @Pothulapati, this looks and works great 👍

Could you add somewhere a test case that verifies this kind of override? Could be for example in in TestAnnotationPermutations in test/inject/inject_test.go.

@Pothulapati Pothulapati force-pushed the namespaceconfig branch 3 times, most recently from 5583ad8 to 9f42ac2 Compare August 10, 2019 18:36
@Pothulapati
Copy link
Contributor Author

@alpeb Added Both Unit and Integration tests :)

@alpeb
Copy link
Member

alpeb commented Aug 12, 2019

Thanks for adding that @Pothulapati
Is the integration test running fine for you? I'm consistently getting the following error:

goroutine 43 [running]:
testing.tRunner.func1(0xc000235400)
        /usr/local/go/src/testing/testing.go:830 +0x392
panic(0x12e9320, 0x219ca20)
        /usr/local/go/src/runtime/panic.go:522 +0x1b5
command-line-arguments.TestNamespaceOverrideAnnotations(0xc000235400)
        /home/alpeb/linkerd2/test/inject/inject_test.go:140 +0xb8b
testing.tRunner(0xc000235400, 0x14fdc78)
        /usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:916 +0x35a
FAIL    command-line-arguments  0.208s
Test script: [get_test.go] Params: [--linkerd-namespace=l5d-integration]
ok      command-line-arguments  11.644s

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
Copy link
Contributor Author

Ah! I was running the test induvidually and it was working!

The problem is that, the test also uses the same namespace name as the other test TestAnnotationPermutations and the namespace annotations are not being added to this test as the namespace is not being created again.

Changed the namespace name and it should work together with other tests.

@alpeb
Copy link
Member

alpeb commented Aug 13, 2019

@Pothulapati Now I'm getting

goroutine 62 [running]:
testing.tRunner.func1(0xc00024f900)
        /usr/local/go/src/testing/testing.go:830 +0x392
panic(0x12e9320, 0x219ca20)
        /usr/local/go/src/runtime/panic.go:522 +0x1b5
command-line-arguments.TestNamespaceOverrideAnnotations(0xc00024f900)
        /home/alpeb/linkerd2/test/inject/inject_test.go:141 +0xb8b
testing.tRunner(0xc00024f900, 0x14fdca0)
        /usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:916 +0x35a
FAIL    command-line-arguments  0.263s
Test script: [get_test.go] Params: [--linkerd-namespace=l5d-integration]
ok      command-line-arguments  15.641s

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Thanks Tarun! Integration tests are passing now for me 🥇

@ihcsim ihcsim merged commit 242566a into linkerd:master Aug 15, 2019
alpeb added a commit that referenced this pull request Aug 15, 2019
Followup to #3194

The namespace was too long for l5d-bot:

```
    inject_test.go:117: failed to create
    l5d-integration-auto-git-9688d9ba-inject-namespace-override-test
    namespace: Namespace
    "l5d-integration-auto-git-9688d9ba-inject-namespace-override-test"
    is invalid: metadata.name: Invalid value:
    "l5d-integration-auto-git-9688d9ba-inject-namespace-override-test":
    must be no more than 63 characters
```

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@alpeb alpeb mentioned this pull request Aug 15, 2019
alpeb added a commit that referenced this pull request Aug 15, 2019
Followup to #3194

The namespace was too long for l5d-bot:

```
    inject_test.go:117: failed to create
    l5d-integration-auto-git-9688d9ba-inject-namespace-override-test
    namespace: Namespace
    "l5d-integration-auto-git-9688d9ba-inject-namespace-override-test"
    is invalid: metadata.name: Invalid value:
    "l5d-integration-auto-git-9688d9ba-inject-namespace-override-test":
    must be no more than 63 characters
```

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespace-level Configs Override Annotations
3 participants