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

Get ingress domain from ingress config #71

Conversation

Miciah
Copy link
Contributor

@Miciah Miciah commented Nov 22, 2018

Import openshift/api/config/v1

Get ingress domain from ingress config

  • manifests/00-cluster-role.yaml: Allow operator to get ingresses.config.openshift.io.
  • pkg/manifests/manifests.go (DefaultClusterIngress): Delete parameter for install config; add parameter for ingress domain.
  • pkg/manifests/manifests_test.go (TestDefaultClusterIngres): Adjust.
  • pkg/stub/handler.go (EnsureDefaultClusterIngress): Get the cluster ingress config and use the domain therefrom to set the domain of the default clusteringress.
    (ensureDNSForLoadBalancer): Don't add an "apps." prefix to the ingress domain; the default cluster ingress config domain already has it.
  • pkg/util/config.go (InstallConfig): Delete Metadata field.
    (InstallConfigMetadata): Delete.

Depends on openshift/api#139 and openshift/installer#717.

Related: openshift/cluster-openshift-apiserver-operator#51.

@ironcladlou

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 22, 2018
@Miciah Miciah force-pushed the get-ingress-domain-from-ingress-config branch 2 times, most recently from 4385819 to 7e2c434 Compare November 26, 2018 20:17
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2018
@Miciah Miciah force-pushed the get-ingress-domain-from-ingress-config branch from 7e2c434 to f82e871 Compare November 26, 2018 21:30
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2018
@Miciah
Copy link
Contributor Author

Miciah commented Nov 26, 2018

Rebased. Added a field DefaultIngressDomain to the new operator.Config object. Changed createHandler to initialize this field, and changed the factory to use it.

@Miciah Miciah force-pushed the get-ingress-domain-from-ingress-config branch from f82e871 to 08dc6a5 Compare November 26, 2018 22:57
@Miciah
Copy link
Contributor Author

Miciah commented Nov 26, 2018

Latest push updates openshift/api to include openshift/api#139.

@ironcladlou
Copy link
Contributor

/retest

2 similar comments
@ironcladlou
Copy link
Contributor

/retest

@ironcladlou
Copy link
Contributor

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 28, 2018

Note: openshift/installer#744 is needed for this to work out of the box.

I tested this PR manually on a cluster that I installed with the changes that are in openshift/installer#744:

% oc -n openshift-ingress-operator delete clusteringresses/default
clusteringress.ingress.openshift.io "default" deleted
% oc -n openshift-ingress-operator scale deployment.apps ingress-operator --replicas=0
deployment.apps/ingress-operator scaled
% oc -n openshift-ingress-operator patch deployment.apps/ingress-operator --type=json --patch='[{"op":"replace","path":"/spec/template/spec/containers/0/image","value":"docker.io/miciah/origin-cluster-ingress-operator:08dc6a5"},{"op":"replace","path":"/spec/template/spec/containers/0/imagePullPolicy","value":"Always"}]'
deployment.apps/ingress-operator patched
% oc -n openshift-ingress-operator scale deployment.apps ingress-operator --replicas=1
deployment.apps/ingress-operator scaled
% oc -n openshift-ingress-operator get clusteringresses/default -o go-template --template=$'{{.spec.ingressDomain}}\n'
apps.mmasters.devcluster.openshift.com
% host foo.apps.mmasters.devcluster.openshift.com
foo.apps.mmasters.devcluster.openshift.com has address 184.169.194.188
foo.apps.mmasters.devcluster.openshift.com has address 52.8.201.142
% oc -n openshift-ingress get services
NAME             TYPE           CLUSTER-IP     EXTERNAL-IP                                                             PORT(S)                      AGE
router-default   LoadBalancer   10.3.178.110   a61f683a9f2a311e8977a068bf0f37b0-85491451.us-west-1.elb.amazonaws.com   80:30481/TCP,443:31043/TCP   10m
% host a61f683a9f2a311e8977a068bf0f37b0-85491451.us-west-1.elb.amazonaws.com
a61f683a9f2a311e8977a068bf0f37b0-85491451.us-west-1.elb.amazonaws.com has address 184.169.194.188
a61f683a9f2a311e8977a068bf0f37b0-85491451.us-west-1.elb.amazonaws.com has address 52.8.201.142

@ironcladlou
Copy link
Contributor

/retest

@ironcladlou
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 28, 2018
@Miciah Miciah mentioned this pull request Nov 28, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@Miciah
Copy link
Contributor Author

Miciah commented Nov 28, 2018

/hold
We'll try to get openshift/installer#754 merged first.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2018
* cmd/cluster-ingress-operator/main.go (createHandler): Get the cluster
ingress config and use the domain therefrom to set the operator config
object's DefaultIngressDomain field for the ingress domain of the default
clusteringress.
* manifests/00-cluster-role.yaml: Allow operator to get
ingresses.config.openshift.io.
* pkg/manifests/manifests.go (DefaultClusterIngress): Delete parameter for
install config.  Use the operator config object's DefaultClusterIngress
field to get the ingress domain.
* pkg/manifests/manifests_test.go (TestDefaultClusterIngress): Specify
DefaultIngressDomain in operator config object.  Delete argument for
install config in call to DefaultClusterIngress.
* pkg/operator/config.go (Config): Add DefaultIngressDomain field.
* pkg/stub/handler.go (EnsureDefaultClusterIngress): Delete argument for
install config in call to DefaultClusterIngress.
(ensureDNSForLoadBalancer): Don't add an "apps." prefix to the ingress
domain; the default cluster ingress config domain already has it.
* pkg/util/config.go (InstallConfig): Delete Metadata field.
(InstallConfigMetadata): Delete.
@Miciah Miciah force-pushed the get-ingress-domain-from-ingress-config branch from 08dc6a5 to 6cc7d30 Compare November 28, 2018 19:23
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2018
@Miciah
Copy link
Contributor Author

Miciah commented Nov 28, 2018

Amended in anticipation of openshift/installer#754. Once that PR merges, I'll test the changes in this PR again, then cancel the hold.

@Miciah
Copy link
Contributor Author

Miciah commented Nov 28, 2018

Tested as follows.

  1. Delete the old default clusteringress:
% oc -n openshift-ingress-operator delete clusteringresses/default
clusteringress.ingress.openshift.io "default" deleted
  1. Scale down the old operator:
% oc -n openshift-ingress-operator scale deployment.apps ingress-operator --replicas=0
deployment.apps/ingress-operator scaled
  1. Patch in an image with the most recent changes in this PR:
% oc -n openshift-ingress-operator patch deployment.apps/ingress-operator --type=json --patch='[{"op":"replace","path":"/spec/template/spec/containers/0/image","value":"docker.io/miciah/origin-cluster-ingress-operator:6cc7d30"},"op":"replace","path":"/spec/template/spec/containers/0/imagePullPolicy","value":"Always"}]'
deployment.apps/ingress-operator patched
  1. Give the operator permission to read ingresses.config.openshift.io:
% oc apply -f ~/src/github.com/openshift/cluster-ingress-operator/manifests/00-cluster-role.yaml
Warning: oc apply should be used on resource created by either oc create --save-config or oc apply
clusterrole.rbac.authorization.k8s.io/openshift-ingress-operator configured
  1. Verify we have the ingress under the new name, not the old:
% oc get ingresses.config.openshift.io                                                                                                                                                          NAME      AGE
cluster   4h
  1. Scale up the new operator:
% oc -n openshift-ingress-operator scale deployment.apps ingress-operator --replicas=1
deployment.apps/ingress-operator scaled
  1. Check the new default clusteringress:
% oc -n openshift-ingress-operator get clusteringresses/default -o $'jsonpath={.spec.ingressDomain}\n'
apps.mmasters.devcluster.openshift.com
  1. Check the corresponding service:
% oc -n openshift-ingress get svc/router-default
NAME             TYPE           CLUSTER-IP    EXTERNAL-IP                                                               PORT(S)                      AGE
router-default   LoadBalancer   10.3.60.244   a32827412f36511e8a413062a456765a-1574451749.us-west-1.elb.amazonaws.com   80:32385/TCP,443:30527/TCP   17s
  1. Verify the wildcard DNS record resolves to the IP addresses of the LB service:
% host foo.apps.mmasters.devcluster.openshift.com 8.8.8.8
Using domain server:
Name: 8.8.8.8
Address: 8.8.8.8#53
Aliases:

foo.apps.mmasters.devcluster.openshift.com has address 52.9.211.221
foo.apps.mmasters.devcluster.openshift.com has address 13.57.69.99
% host a32827412f36511e8a413062a456765a-1574451749.us-west-1.elb.amazonaws.com 8.8.8.8
Using domain server:
Name: 8.8.8.8
Address: 8.8.8.8#53
Aliases:

a32827412f36511e8a413062a456765a-1574451749.us-west-1.elb.amazonaws.com has address 52.9.211.221
a32827412f36511e8a413062a456765a-1574451749.us-west-1.elb.amazonaws.com has address 13.57.69.99

@ironcladlou
Copy link
Contributor

/lgtm
/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

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:

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

@ironcladlou
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2018
@Miciah
Copy link
Contributor Author

Miciah commented Nov 29, 2018

/retest

6 similar comments
@Miciah
Copy link
Contributor Author

Miciah commented Nov 29, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

Only three flakes this last time:

openshift-tests [Feature:DeploymentConfig] deploymentconfigs with multiple image change triggers [Conformance] should run a successful deployment with a trigger used by different containers [Suite:openshift/conformance/parallel/minimal] [Suite:openshift/smoke-4] 6m8s
openshift-tests [Feature:DeploymentConfig] deploymentconfigs with multiple image change triggers [Conformance] should run a successful deployment with multiple triggers [Suite:openshift/conformance/parallel/minimal] [Suite:openshift/smoke-4] 6m11s
operator Instantiate the template e2e-aws into the operator namespace and wait for any pods to complete 52m25s

@ironcladlou
Copy link
Contributor

/retest

2 similar comments
@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Nov 30, 2018

/retest
Just one flake this time:

operator Instantiate the template e2e-aws into the operator namespace and wait for any pods to complete 8m15s

@Miciah
Copy link
Contributor Author

Miciah commented Dec 1, 2018

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants