USHIFT-2261 USHIFT-2459: Add namespace ownership support for router

[pacevedom]

Which issue(s) this PR addresses:

Closes #

[openshift-ci-robot]

@pacevedom: This pull request references USHIFT-2459 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

Which issue(s) this PR addresses:

Closes #

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[pacevedom]

/hold

[pacevedom]

wait until #3066 is merged

[dhellmann]

It looks like this PR includes the contents of #3066, is that right? If so, that's good, I'd like to land the code change and tests at the same time. Maybe we can close the other PR?

[pacevedom]

It looks like this PR includes the contents of #3066, is that right? If so, that's good, I'd like to land the code change and tests at the same time. Maybe we can close the other PR?

Yes, I was hoping for a quick review of the implementation (as its simple) and then land the tests later (which are more complex). To have them pass the tests I included all the other commits here too and planned to remove them once #3066 merged.

[pacevedom]

/test microshift-metal-tests

[pacevedom]

/jira refresh

[openshift-ci-robot]

@pacevedom: This pull request references USHIFT-2459 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[pacevedom]

/unhold

[ShudiLi]

The function worked well: By default, the ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK in the router deployment was "true", two route with same host names but with different path were admitted and could curl the routes successfully.
Changed it to "false", one route was admitted, the other wasn't.

% oc -n openshift-ingress get deployment -oyaml | grep -A1 ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK
- name: ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK
value: "true"
2.
% oc -n test2 get route
NAME HOST ADMITTED SERVICE TLS
route1 test-route1.apps.example.com True unsec-apach2

% oc -n test get route
NAME HOST ADMITTED SERVICE TLS
route1 test-route1.apps.example.com True unsec-apach2
% oc -n test get route route1 -oyaml | grep -A2 spec:
spec:
host: test-route1.apps.example.com
path: /srv/docs

4. curl the two routes
   sh-4.4# curl http://test-route1.apps.example.com --resolve test-route1.apps.example.com:80:10.42.0.16
   this a test!
   sh-4.4# curl http://test-route1.apps.example.com/srv/docs/book.txt --resolve test-route1.apps.example.com:80:10.42.0.16
   11111ss
   sh-4.4#

5. Changed it to "false", one route was admitted, the other wasn't.
   % oc -n test get route | grep route1
   route1 test-route1.apps.example.com False unsec-apach2
   % oc -n test2 get route | grep route1
   route1 test-route1.apps.example.com True unsec-apach2

6. also checked for edge route and reencrypt route

[ShudiLi]

/label qe-approved
thanks

[openshift-ci-robot]

@pacevedom: This pull request references USHIFT-2459 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

Which issue(s) this PR addresses:

Closes #

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[jerpeter1]

In your EP, you said you intended to expose this via:

router:
  routerAdmissionPolicy:
    namespaceOwnership: <Strict|InterNamespaceAllowed> # Defaults to InterNamespaceAllowed.

here, you're exposing it via ingress.routerAdmissionPolicy.namespaceOwnership - if the implementation is what we want, you should probably go back and fixup the EP.

[pacevedom]

Good catch, will update it right away.

[jerpeter1]

You might consider adding a test case which checks that the default (unconfigured) code path works as expected. Maybe another one for invalid configuration, but I wouldn't consider either test a blocker for merging this.

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerpeter1, pacevedom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jerpeter1,pacevedom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@pacevedom: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[pacevedom]

You might consider adding a test case which checks that the default (unconfigured) code path works as expected. Maybe another one for invalid configuration, but I wouldn't consider either test a blocker for merging this.

/lgtm

Will add them in a separate PR. Thanks!