Handle ingress domain updates to default cluster ingress #48
+48
−9
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 |
|---|---|---|
|
|
@@ -69,16 +69,56 @@ func (h *Handler) EnsureDefaultClusterIngress() error { | |
| return err | ||
| } | ||
|
|
||
| changed, nci, err := checkClusterIngress(ci) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if changed { | ||
| err = sdk.Update(nci) | ||
| if err != nil { | ||
| return fmt.Errorf("updating default cluster ingress %s/%s: %v", ci.Namespace, ci.Name, err) | ||
| } | ||
| logrus.Infof("updated default cluster ingress %s/%s", ci.Namespace, ci.Name) | ||
| } else if nci == nil { | ||
| err = sdk.Create(ci) | ||
| if err != nil { | ||
| return fmt.Errorf("creating default cluster ingress %s/%s: %v", ci.Namespace, ci.Name, err) | ||
| } | ||
| logrus.Infof("created default cluster ingress %s/%s", ci.Namespace, ci.Name) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func checkClusterIngress(ci *ingressv1alpha1.ClusterIngress) (bool, *ingressv1alpha1.ClusterIngress, error) { | ||
| oldci := &ingressv1alpha1.ClusterIngress{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: ci.Kind, | ||
| APIVersion: ci.APIVersion, | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: ci.Name, | ||
| Namespace: ci.Namespace, | ||
| }, | ||
| } | ||
| err := sdk.Get(oldci) | ||
| if err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return false, nil, fmt.Errorf("failed to fetch existing default cluster ingress %s/%s, %v", ci.Namespace, ci.Name, err) | ||
| } | ||
| return false, nil, nil | ||
| } | ||
|
|
||
| if ci.Spec.IngressDomain == nil { | ||
| return false, nil, fmt.Errorf("invalid ingress domain for default cluster ingress %s/%s", ci.Namespace, ci.Name) | ||
| } | ||
| if oldci.Spec.IngressDomain == nil { | ||
| oldci.Spec.IngressDomain = new(string) | ||
| } | ||
| if *oldci.Spec.IngressDomain != *ci.Spec.IngressDomain { | ||
|
Contributor
There was a problem hiding this comment. If either of this is
Author
There was a problem hiding this comment. Tried this https://play.golang.org/p/i0W49iA8Gjh and I didn't see an issue. |
||
| *oldci.Spec.IngressDomain = *ci.Spec.IngressDomain | ||
| return true, oldci, nil | ||
| } | ||
| return false, oldci, nil | ||
| } | ||
|
|
||
| // Reconcile performs a full reconciliation loop for ingress, including | ||
|
|
||
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.
I think this can use: clusterIngressStatusEqual
from PR: #38
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.
Maybe not
clusterIngressStatusEqual, but the part that compares.loadBalancer.Ingresscould be factored into a function that could be used both bycheckClusterIngressand byclusterIngressStatusEqual. Now that #47 is merged, I'll rebase #38 Tuesday and factor out aLoadBalancerStatusEqualfunction. No need for that to block this PR though; I can changecheckClusterIngressto useLoadBalancerStatusEqualif #48 merges before #38 is ready.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.
Err, actually,
checkClusterIngressis comparing only.spec.ingressDomain(a string) whereasclusterIngressStatusEqualis comparing.status.loadBalancer(a slice of a struct that has two string values), so there is really no opportunity for code re-use here as far as I see.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 confused but if the LoadBalancerIngress struct are different as being checked in your PR [1] it means there's an update it should be enough for this purpouse.
[1] https://github.com/openshift/cluster-ingress-operator/pull/38/files#diff-9ae69f8dcadd8df44e0b10265f3a91a8R107
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're talking about
cluster-ingress-operator/pkg/stub/handler.go
Lines 111 to 117 in 3f8735c
*stringvalues in order to determine whether the old ClusterIngress and new ClusterIngress specify the same ingress domain.clusterIngressStatusEqualcompares[]LoadBalancerIngressvalues, whereLoadBalancerIngressisstruct { IP, Hostname string }, in order to determine whether the old ClusterIngressStatus and new ClusterIngressStatus have the same ingresses.