-
Notifications
You must be signed in to change notification settings - Fork 223
NE-199 Phase 1: Add initial Canary Controller and Canary Resources #476
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1ad9866
cmd: Add canary image arg to CLI args
sgreene570 7882f72
manifests: Add ingress canary image ref and RBAC rules
sgreene570 08696f1
assets: Add canary controller assets
sgreene570 ea89c3d
Update bindata
sgreene570 fe5069e
clusteroperator: Add canary namespace
sgreene570 2a52a78
operator: Add initial canary controller
sgreene570 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # Hello Openshift Ingress Canary daemonset | ||
| # Specific values are set at runtime | ||
| kind: DaemonSet | ||
| apiVersion: apps/v1 | ||
| # name and namespace are set at runtime. | ||
| spec: | ||
| progressDeadlineSeconds: 600 | ||
| template: | ||
| spec: | ||
| containers: | ||
| - name: hello-openshift-canary | ||
| # Image is set at runtime | ||
| imagePullPolicy: IfNotPresent | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| - containerPort: 8888 | ||
| protocol: TCP | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 20Mi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| kind: Namespace | ||
| apiVersion: v1 | ||
| metadata: | ||
| name: openshift-ingress-canary |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Hello Openshift Ingress Canary route | ||
| # Specific values are applied at runtime | ||
| kind: Route | ||
| apiVersion: route.openshift.io/v1 | ||
| # name and namespace are set at runtime. | ||
| metadata: | ||
| annotations: | ||
| haproxy.router.openshift.io/balance: "roundrobin" | ||
| spec: | ||
| port: | ||
| targetPort: 8080-tcp | ||
| to: | ||
| kind: Service | ||
| # Service name set at run time | ||
| weight: 100 | ||
| wildcardPolicy: None |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Hello Openshift Ingress Canary service | ||
| # Specific values are applied at runtime | ||
| kind: Service | ||
| apiVersion: v1 | ||
| # name and namespace are set at runtime. | ||
| spec: | ||
| type: ClusterIP | ||
| ports: | ||
| - name: 8080-tcp | ||
| port: 8080 | ||
| protocol: TCP | ||
| targetPort: 8080 | ||
| - name: 8888-tcp | ||
| port: 8888 | ||
| protocol: TCP | ||
| targetPort: 8888 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could define a new role and rolebinding in the
openshift-ingress-canarynamespace to avoid granting such broad permissions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but how would the operator be able to take advantage of a new role + role binding in the
openshift-ingress-canarynamespace? The canary controller (which resides within the ingress operator) needs to be able to perform all (well, most at least, so maybe the "*" isn't necessary) actions on routes.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role would be in the
openshift-ingress-canarynamespace and allow full control over routes, and the role binding would bind that role to the operator's service account. Because it would be a role (not a clusterrole) in theopenshift-ingress-canarynamespace, the role binding should only grant access to routes within that namespace, if I understand correctly. Something like the following should work (but I have not verified that it does):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill include these resources in the manifests in my next push and verify that they work. Thanks!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miciah the CVO does not (and in my opinion, probably should not) create the
openshift-ingress-canarynamespace, so the CVO cannot create a Role in theopenshift-ingress-canarynamespace. Additionally, the operator would need cluster-wide route RBAC permissions to be able to create a role in theopenshift-ingress-canarynamespace for routes.So I see 2 solutions:
openshift-ingress-canarynamespace, and then creates the ingress canary controller role.(This seems like an awkward thing to do especially if the canary controller isnt a mandatory component of the operator)
cluster-role.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, ive reverted this PR to use
*permissions for routes in the operator's cluster roleUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~ @Miciah https://kubernetes.io/docs/reference/access-authn-authz/rbac/#privilege-escalation-prevention-and-bootstrapping is the solution we are looking for! I amended this PR so that the ingress operator is granted
bindandescalatepermissions onrolesandrolebindingsin it's cluster role. I also added therole.yamlandrole-binding.yamlassets for the ingress canary. With these new changes, the ingress operator's route permissions are extended to*for theopenshift-ingress-canarynamespace only. PTAL when you can. ~Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going with route '*' permissions after a discussion with miciah. While these permissions are broad, this is the "least bad" approach. The above mentioned
escalate/bindapproach introduces far more concerning privilege escalation issues to the operator (despite work around efforts to mitigate this).