-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[GCLB] Gracefully handle lack of IAM rights for modifying firewall #1484
Conversation
7b4278e
to
c968e75
Compare
type FirewallSyncError struct { | ||
Internal error | ||
Message string | ||
} |
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.
Let me know if you have any better ideas on how this error should look.
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.
lgtm
} else if utils.IsForbiddenError(err) && fr.cloud.OnXPN() { | ||
gcloudCmd := gce.FirewallToGCloudDeleteCmd(name, fr.cloud.NetworkProjectID()) | ||
glog.V(3).Infof("Could not attempt delete of L7 firewall on XPN cluster. %q needs to be ran.", gcloudCmd) | ||
// FirewallXPN error is not returned here because there is no ingress for attaching. |
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.
It should return an error irrespective? The caller knows that there is no ingress and hence it will just log it
type FirewallSyncError struct { | ||
Internal error | ||
Message string | ||
} |
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.
lgtm
NetworkURL() string | ||
OnXPN() bool |
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.
Add a comment saying this will return true if XPN is enabled in GCP project.
optional: Probably also link to some XPN documentation?
Thanks this looks a lot cleaner now! minor comments. mostly looks good |
c968e75
to
a061b2f
Compare
Changes Unknown when pulling a061b2f on nicksardo:alt-xpn-firewall into ** on kubernetes:master**. |
Moving to new repo. |
No description provided.