Skip to content

Check for Namespace level config override annotations#3194

Merged
ihcsim merged 5 commits intolinkerd:masterfrom
Pothulapati:namespaceconfig
Aug 15, 2019
Merged

Check for Namespace level config override annotations#3194
ihcsim merged 5 commits intolinkerd:masterfrom
Pothulapati:namespaceconfig

Conversation

@Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Aug 5, 2019 •
edited
Loading

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 [email protected]

@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

Pothulapati commented Aug 10, 2019

@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

@Pothulapati
Copy link
Contributor Author

Pothulapati commented Aug 13, 2019

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

alpeb
alpeb approved these changes Aug 13, 2019
View reviewed changes
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
@alpeb
Fix integration test
c681889 
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 <[email protected]>
@alpeb alpeb mentioned this pull request Aug 15, 2019
Merged
alpeb added a commit that referenced this pull request Aug 15, 2019
@alpeb
Fix integration test (#3266)
fd5fc07 
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 <[email protected]>
This was referenced Aug 15, 2019
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alpeb alpeb alpeb approved these changes

+1 more reviewer

@ihcsim ihcsim ihcsim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Namespace-level Configs Override Annotations

3 participants

@Pothulapati @alpeb @ihcsim