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

fix: check nils in ingressClass.Parameters #2970

Merged
merged 1 commit into from
Sep 23, 2022
Merged

fix: check nils in ingressClass.Parameters #2970

merged 1 commit into from
Sep 23, 2022

Conversation

randmonkey
Copy link
Contributor

What this PR does / why we need it:

fix panics when some field is nil in ingressClass.spec.parameters

Which issue this PR fixes:

fixes #2958.

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@randmonkey randmonkey requested a review from a team as a code owner September 23, 2022 09:57
@randmonkey randmonkey temporarily deployed to Configure ci September 23, 2022 09:57 Inactive
@randmonkey randmonkey temporarily deployed to Configure ci September 23, 2022 10:06 Inactive
@randmonkey randmonkey temporarily deployed to Configure ci September 23, 2022 16:43 Inactive
@rainest
Copy link
Contributor

rainest commented Sep 23, 2022

Initial failures in integration-tests-dbless were for UDPRoute and TLSRoute:

UDPRoute failed because the listener had no valid Kong port to associate with. Somehow this port was present on the deployment, but was TCP instead of UDP. Not sure how KTF would have that temporarily, but the code there does look at least partially incorrect. UDP stream listens should live in a different config section because of LoadBalancer inability to handle mixed TCP/UDP services:

09:51:05-0700 esenin $ grep -r 9999
pkg/clusters/addons/kong/vars.go:	DefaultUDPServicePort = 9999
09:51:08-0700 esenin $ grep -r DefaultUDPServicePort
pkg/clusters/addons/kong/vars.go:	// DefaultUDPServicePort indicates the default open port to be found on the Kong proxy's UDP service.
pkg/clusters/addons/kong/vars.go:	DefaultUDPServicePort = 9999
pkg/clusters/addons/kong/addon.go:	return urlForService(ctx, cluster, types.NamespacedName{Namespace: a.namespace, Name: DefaultUDPServiceName}, DefaultUDPServicePort)
pkg/clusters/addons/kong/addon.go:		"--set", fmt.Sprintf("proxy.stream[1].containerPort=%d", DefaultUDPServicePort),
pkg/clusters/addons/kong/addon.go:		"--set", fmt.Sprintf("proxy.stream[1].servicePort=%d", DefaultUDPServicePort),
pkg/clusters/addons/kong/addon.go:		Port:     DefaultUDPServicePort,

However, elsewhere it tries to set the udp parameter manually, so that should satisfy what Gateway wants to check:

 562 func exposePortsDefault() []string {                                                                                    
  563     return []string{                                                                                                    
  564         "--set", fmt.Sprintf("proxy.stream[0].containerPort=%d", DefaultTCPServicePort),                                
  565         "--set", fmt.Sprintf("proxy.stream[0].servicePort=%d", DefaultTCPServicePort),                                  
  566         "--set", fmt.Sprintf("proxy.stream[1].containerPort=%d", DefaultUDPServicePort),                                                
  567         "--set", fmt.Sprintf("proxy.stream[1].servicePort=%d", DefaultUDPServicePort),                                  
  568         "--set", "proxy.stream[1].parameters[0]=udp",                                                                   
  569         "--set", "proxy.stream[1].parameters[1]=reuseport",                                                             
  570         "--set", fmt.Sprintf("proxy.stream[2].containerPort=%d", DefaultTLSServicePort),                                
  571         "--set", fmt.Sprintf("proxy.stream[2].servicePort=%d", DefaultTLSServicePort),                                  
  572         "--set", "proxy.stream[2].parameters[0]=ssl",                                                                   
  573         "--set", "proxy.stream[2].parameters[1]=reuseport",                                                             
  574     }                                                                                                                   
  575 }

TLSRoute unsure. Its listener is fine, but the route itself has no status and was apparently never acknowledged. Controller logs appear to simply not see it.

@randmonkey randmonkey temporarily deployed to Configure ci September 23, 2022 17:02 Inactive
@rainest rainest enabled auto-merge (rebase) September 23, 2022 17:34
@rainest rainest merged commit d3488b5 into main Sep 23, 2022
@rainest rainest deleted the fix/ic_param_nil branch September 23, 2022 17:34
},
}
cacheStores, err := store.NewCacheStoresFromObjs()
assert.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those would better use require instead to prevent continuing execution, so that when an error occurs we don't continue the test as it would be pointless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly integration tests failing - GKE & Kubernetes v1.21.0 (test-previous-kubernetes-gke)
5 participants