cilium: Make CLI more graceful on environments with IPv6 disabled #16168
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
Maddy007-maha
changed the title
Maddy007-maha
force-pushed
the
fix_13834_cli_ipv6
branch
6 times, most recently
from
34dd200
to
e7bbafb
Compare
twpayne
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
cilium/cmd/helpers.go
Outdated
| } else { // else read the EnableIPv6 status from the file-system | ||
| agentConfigFile := filepath.Join(defaults.RuntimePath, defaults.StateDir, | ||
| "agent-runtime-config.json") | ||
| var agentConfig interface{} |
There was a problem hiding this comment.
Better to use the appropriate concrete type here, rather than an empty interface and lots of unchecked type assertions below.
cilium/cmd/helpers.go
Outdated
| "agent-runtime-config.json") | ||
| var agentConfig interface{} | ||
|
|
||
| if _, err := os.Stat(agentConfigFile); !os.IsNotExist(err) { |
There was a problem hiding this comment.
The call to os.Stat is unnecessary - an error will be returned by ReadFile anyway. Note also that the condition is wrong: any other error than IsNotExist will cause the condition to be true.
cilium/cmd/helpers.go
Outdated
| var agentConfig interface{} | ||
|
|
||
| if _, err := os.Stat(agentConfigFile); !os.IsNotExist(err) { | ||
| if byteValue, err := ioutil.ReadFile(agentConfigFile); err == nil { |
There was a problem hiding this comment.
ioutil.ReadFile is deprecated. Please use os.ReadFile instead.
There was a problem hiding this comment.
Thanks for the update, incorporated your all comments
cilium/cmd/helpers.go
Outdated
| } | ||
| } | ||
| // returning the EnableIPv6 default status | ||
| return true |
There was a problem hiding this comment.
This should probably be defaults.EnableIPv6.
Maddy007-maha
force-pushed
the
fix_13834_cli_ipv6
branch
from
e7bbafb
to
af64411
Compare
cilium/cmd/helpers.go
Outdated
|
|
||
| if byteValue, err := os.ReadFile(agentConfigFile); err == nil { | ||
| if err = json.Unmarshal(byteValue, &agentConfig); err == nil { | ||
| return agentConfig["EnableIPv6"].(bool) |
There was a problem hiding this comment.
This is still doing an unchecked type assertion and using a map with string keys where better safety can be used by defining agentConfig to be a github.com/cilium/cilium/pkg/option.DaemonConfig.
There was a problem hiding this comment.
The change isn't what was requested. The file should be unmarshalled into a DaemonConfig struct, not add a new field to DaemonConfig.
There was a problem hiding this comment.
Thanks for the input, Have incorporated your comment.
Maddy007-maha
force-pushed
the
fix_13834_cli_ipv6
branch
from
af64411
to
5136c99
Compare
Description:
Issue: Some CLI's are complaining to stderr even on environments with IPV6 disabled
Root cause: As part of CLI implemenation, it is always retrieving the GlobalMaps using true flag irrespective of IPV6 is enabled/disabled
Suggested fix: Now, GlobalMaps are based on correct IPV6 status from the cilium-agent else read from the system config (absence of agent)
Affected CLIs:
$ cilium bpf ct ...
$ cilium bpf nat ...
$ cilium bpf recorder ...
Fixes: cilium#13834
Signed-off-by: Mahadev Panchal <mahadev.panchal@accuknox.com>
Maddy007-maha
force-pushed
the
fix_13834_cli_ipv6
branch
from
5136c99
to
aead81c
Compare
|
test-me-please |
cilium: Make CLI more graceful on environments with IPv6 disabled
Description:
Issue: Some CLI's are complaining to stderr even on environments with IPV6 disabled
Root cause: As part of CLI implementation, it is always retrieving the GlobalMaps using true flag irrespective of IPV6 is enabled/disabled
Suggested fix: Now, GlobalMaps are based on correct IPV6 status from the cilium-agent else read from the system config (absence of agent)
Affected CLIs:
Fixes: #13834
Signed-off-by: Mahadev Panchal mahadev.panchal@accuknox.com