Add code for multiproject GCE Client creation #2725
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
panslava
force-pushed
the
multi-project-gce
branch
4 times, most recently
from
0e8f463
to
3849802
Compare
panslava
force-pushed
the
multi-project-gce
branch
from
3849802
to
49ef5d9
Compare
| @@ -19,6 +19,22 @@ func (p *Project) ProjectName() string { | |||
| return p.ObjectMeta.Labels[flags.F.MultiProjectCRDProjectNameLabel] | |||
| } | |||
|
|
|||
| func (p *Project) ProjectID() string { | |||
There was a problem hiding this comment.
Do we need both ProjectName() and ProjectID()?
There was a problem hiding this comment.
Removed this, not needed for now
pkg/multiproject/gce/gce.go
Outdated
| } | ||
|
|
||
| // convert file struct to bytes | ||
| configBytes, err := json.Marshal(configFile) |
There was a problem hiding this comment.
I suspect that this roundrip of converting the config file back to a gce.conf may not work.
To elaborate, I think json.Marshal(), when converting the structs to JSON would use the default camelCase names for the fields. So something like NetworkProjectID ends up in JSON as networkProjectID.
On the other hand, when gcfg code reads the json, it is likely using the gcfg struct tags to interpret the field names. So it expects the JSON field name to be network-project-id.
There was a problem hiding this comment.
Good catch
I changed the code significantly. Now it treats the file as ini file, and, I also updated unit tests to double check it, looks correct
pkg/multiproject/gce/gce.go
Outdated
| if flags.F.ConfigFilePath == "" { | ||
| return nil | ||
| } | ||
|
|
||
| logger.Info("Reading config from the specified path", "path", flags.F.ConfigFilePath) | ||
| config, err := os.Open(flags.F.ConfigFilePath) | ||
| if err != nil { | ||
| klog.Fatalf("%v", err) | ||
| } | ||
| defer config.Close() | ||
|
|
||
| allConfig, err := io.ReadAll(config) | ||
| if err != nil { | ||
| klog.Fatalf("Error while reading config (%q): %v", flags.F.ConfigFilePath, err) | ||
| } | ||
| logger.V(4).Info("Cloudprovider config file", "config", string(allConfig)) |
There was a problem hiding this comment.
I'm contemplating whether it's helpful to refactor some of the code here that's shared with the NewGCEClient within glbc/app package. On one hand I feel it's maybe not that complex that it needs de-duplication, but on the other hand it maybe feels more natural to just have it in one place (perhaps within the glbc/app package?). Any thoughts?
There was a problem hiding this comment.
I moved a part of it into glbc/app, so we reuse some code, and it just makes it clearer :)
panslava
force-pushed
the
multi-project-gce
branch
from
49ef5d9
to
f36df0a
Compare
k8s-ci-robot
added
size/XXL
panslava
force-pushed
the
multi-project-gce
branch
from
f36df0a
to
140da41
Compare
| } | ||
| } | ||
|
|
||
| func TestGenerateConfigForClusterSlice(t *testing.T) { |
There was a problem hiding this comment.
Should we add a test-case to ensure other random fields which exist in a valid gce.conf are not modified and stay the same?
- Still uses default gce.conf to get most of the info - If project info not specified will use default config values, which corresponds to cluster-project - Overrides TokenURL and TokenBody and Network info if project is specified - Imports new package github.com/go-ini/ini to properly work with gce.conf which is in ini file format
panslava
force-pushed
the
multi-project-gce
branch
from
140da41
to
9fbc1be
Compare
|
Thanks! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal, panslava The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @gauravkghildiyal