-
Notifications
You must be signed in to change notification settings - Fork 42
Conversation
please hold this PR until #274 , currently under review from @petersutter for urlencoding |
sure, definitely this PR merge will wait for #274 |
find one issue before rebase, will waiting for the fix #273 (comment) |
/lgtm |
/remove reviewed/lgtm |
pkg/cmd/ssh_aws.go
Outdated
if isIP(a.MyPublicIP) { | ||
arguments = fmt.Sprintf("aws ec2 authorize-security-group-ingress --group-id %s --protocol tcp --port 22 --cidr %s/32", a.BastionSecurityGroupID, a.MyPublicIP) | ||
} else { | ||
arguments = fmt.Sprintf("aws ec2 authorize-security-group-ingress --group-id %s --ip-permissions IpProtocol=tcp,FromPort=22,ToPort=22,Ipv6Ranges=[{CidrIpv6=%s/64}]", a.BastionSecurityGroupID, a.MyPublicIP) | ||
} |
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.
isIP
is a custom implemented function to check for ipv4 addresses. Replace the custom implementation by using net.ParseIP
and return netIP != nil && netIP.To4() != nil
to check for ipv4
see also https://github.com/kubernetes/kubernetes/blob/dc025534afb0c3106b3aab166f88b971102135d1/vendor/k8s.io/utils/net/net.go#L116-L125 as example for the ipv6 check
In the end the code should look something like:
if isIPv4String(a.MyPublicIP) {
...
} else if IsIPv6String(a.MyPublicIP) {
...
} else {
// error
}
pkg/cmd/ssh_aws.go
Outdated
@@ -180,10 +181,17 @@ func (a *AwsInstanceAttribute) createBastionHostSecurityGroup() { | |||
a.BastionSecurityGroupID = strings.Trim((capturedOutput), "\n") | |||
arguments = fmt.Sprintf("aws ec2 create-tags --resources %s --tags Key=component,Value=gardenctl", a.BastionSecurityGroupID) | |||
operate("aws", arguments) | |||
arguments = fmt.Sprintf("aws ec2 authorize-security-group-ingress --group-id %s --protocol tcp --port 22 --cidr %s", a.BastionSecurityGroupID, a.MyPublicIP) | |||
|
|||
ip := net.ParseIP(a.MyPublicIP) |
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.
If the string is not a valid textual representation of an IP address, ParseIP returns nil. Hence you also need to check for nil and that's why I suggested initially to move that code into utils/miscellaneous and dump/replace the isIP
function that is already there
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.
update isIP method which return bool to check whether IP is valid for ipv4 or ipv6.
also change to logic in here, add exit after print the "IP not valid:" + a.MyPublicIP.
pkg/cmd/miscellaneous.go
Outdated
func isIP(host string) bool { | ||
return net.ParseIP(host) != nil | ||
} |
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.
Even if the function was named isIP, the old implementation would only return true in case it was an ipv4 address.
Now it will return true if it's an ipv6 and ipv4 address..
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.
should I rename isIP to like isIPv4 something like that then check and return only valid ipv4 address
?
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.
yes I guess this would be better. It's only used in one place currently createBastionHostInstance
(gcp). Not sure if this will work with ipv6 addresses..
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.
fixed, so far GCP createBastionHostInstance
not support ipv6 yet I think, but I will check it later when I work on support GCP host in the user
role 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.
/lgtm, thanks
however I haven't tested it
pkg/cmd/miscellaneous.go
Outdated
@@ -356,5 +342,9 @@ func getPublicIP() string { | |||
defer resp.Body.Close() | |||
ip, err := ioutil.ReadAll(resp.Body) | |||
checkError(err) | |||
if !isIP(string(ip)) { |
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.
if !isIP(string(ip)) { | |
if !isIPv4(string(ip)) { |
shouldn't this be isIPv4?
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.
yes thanks.
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.
aws IPv6 support use net.ParseIP identify ipv4 or ipv6 modify isIP method rename isIP() to isIPv4()
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
however i haven't tested, confirmed with @tedteng he's fully confidence with it
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #275
Special notes for your reviewer:
Release note: