Skip to content

Adds ProviderResource Nil Checks#216

Merged
danehans merged 1 commit intoenvoyproxy:mainfrom
danehans:issue_214
Aug 29, 2022
Merged

Adds ProviderResource Nil Checks#216
danehans merged 1 commit intoenvoyproxy:mainfrom
danehans:issue_214

Conversation

@danehans
Copy link
Copy Markdown
Contributor

Adds nil checks for ProviderResources gatewayclasses, gateways, and httproutes.

Fixes #214

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans danehans requested a review from a team as a code owner August 22, 2022 20:48
@danehans danehans mentioned this pull request Aug 22, 2022
Comment thread internal/gatewayapi/runner/runner.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you probably also want to check if gatewayClasses is nil or not to handle the case when there are no more gateway classes left / the subscribe notification was triggered due to a delete

Comment thread internal/gatewayapi/translator.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

imho this should ideally not be happening, but rather than skip it, we should either

  • log it or
  • return early and empty
    to surface the underlying issue faster

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree we shouldn't be seeing nil gateways, so I'd actually be in favor of doing

if gateway == nil {
  // Log some message here
  panic
}

Comment thread internal/gatewayapi/translator.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same for me, I'd rather see a panic than a continue.

Copy link
Copy Markdown
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Thanks for finding, I think we should be brutal here and just panic if we get nil values, something has gone seriously wrong, and I'd rather find those cases now before v0.2.0.

Comment thread internal/gatewayapi/translator.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree we shouldn't be seeing nil gateways, so I'd actually be in favor of doing

if gateway == nil {
  // Log some message here
  panic
}

Comment thread internal/gatewayapi/translator.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same for me, I'd rather see a panic than a continue.

@danehans
Copy link
Copy Markdown
Contributor Author

@arkodg @youngnick commit b35f07f implements your feedback. However, I am seeing nil gatewayclasses, gateways, and httproutes in my e2e testing so we have an issue elsewhere in the code that must be resolved.

Comment thread internal/gatewayapi/runner/runner.go Outdated
Comment thread internal/gatewayapi/runner/runner.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: this is functionally correct, but I still prefer the if ... else if.... else style here, didgolangci-lint suggest this style :) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I find switch statements easier to read.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I missed this before, but switch statements are generally more idiomatic in Go for more than one if statement.

Comment thread internal/gatewayapi/runner/runner.go Outdated
arkodg
arkodg previously approved these changes Aug 23, 2022
Comment thread internal/gatewayapi/runner/runner.go Outdated
arkodg
arkodg previously approved these changes Aug 26, 2022
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Copy Markdown
Contributor Author

@arkodg I had to rebase. Please retag.

@danehans danehans merged commit 83b6c7e into envoyproxy:main Aug 29, 2022
@danehans danehans deleted the issue_214 branch September 15, 2022 18:03
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.

Runners Require Nil Check

4 participants