Don't require a compartment #82
Conversation
pkg/oci/util/instance_metadata.go
Outdated
| return metadata, nil | ||
| } | ||
|
|
||
| // A concrete InstanceMetadataService implementation. |
There was a problem hiding this comment.
Minor lint error which failed build
comment on exported function InstanceMetadataFromAPI should be of the form "InstanceMetadataFromAPI ...
// InstanceMetadataFromAPI returns a ...
alapidas
force-pushed
the
task/dontRequireCompartment
branch
from
b9ef69a
to
7553688
Compare
pkg/oci/util/instance_metadata.go
Outdated
| } | ||
| defer rs.Body.Close() | ||
|
|
||
| body, err := ioutil.ReadAll(rs.Body) |
There was a problem hiding this comment.
this should just be
md := &InstanceMetadata{}
err = json.NewDecoder(rs.Body).Decode(md)
if err != nil {
return nil, err
}
return md, nil
and you can delete the other method
pkg/oci/util/instance_metadata.go
Outdated
|
|
||
| const ( | ||
| instanceMetaDataAPI = "http://169.254.169.254" | ||
| instanceMetaDataEndpoint = instanceMetaDataAPI + "/opc/v1/instance/" |
| t.Fatalf("Got unexpected error %s", err) | ||
| } | ||
| if metadata.InstanceOCID != expected.InstanceOCID { | ||
| t.Fatalf("%v != %v", metadata.InstanceOCID, expected.InstanceOCID) |
There was a problem hiding this comment.
Should use Errorf for these checks.
pkg/oci/util/instance_metadata.go
Outdated
| } | ||
| defer rs.Body.Close() | ||
|
|
||
| body, err := ioutil.ReadAll(rs.Body) |
There was a problem hiding this comment.
Should make sure the status code is OK.
pkg/oci/client/client.go
Outdated
| @@ -148,9 +148,19 @@ func New(cfg *Config) (Interface, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| compartmentOCID := cfg.Auth.CompartmentOCID | |||
| if compartmentOCID == "" { | |||
| glog.Info("Compartment not supplied in config - attempting to infer from instance metadata") | |||
There was a problem hiding this comment.
s/Compartment/compartment since it's not capitalized in the config.
pkg/oci/util/instance_metadata.go
Outdated
| ) | ||
|
|
||
| // InstanceMetadata holds OCI API details about the node the driver is executing | ||
| // on. |
There was a problem hiding this comment.
Should include the link to the metadata docs: https://docs.us-phoenix-1.oraclecloud.com/Content/Compute/Tasks/gettingmetadata.htm
pkg/oci/util/instance_metadata.go
Outdated
| } | ||
|
|
||
| // InstanceMetadataService defines a capability to obtain the OCI InstanceMetadata. | ||
| type InstanceMetadataService func() (*InstanceMetadata, error) |
There was a problem hiding this comment.
Is this used anywhere? If not I recommend removing it.
pkg/oci/util/instance_metadata.go
Outdated
|
|
||
| body, err := ioutil.ReadAll(rs.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read node api metadata: %s", err) |
There was a problem hiding this comment.
s/node/instance for these errors since it's the instance metadata endpoint & struct.
|
So ... the instance metadata code was copied wholesale from https://github.com/oracle/oci-volume-provisioner/blob/master/cmd/oci-volume-provisioner/instance_metadata.go in an attempt to quickly get this working. Would it make more sense to just import from |
|
We can just land it as is and I'll clean it up later. That's ok with me. I don't see anything that's a show stopper. I'd prefer to not share code that way since that adds dependencies across projects (e.g. one may be public before the other and depending on layout you could be forced to share dependencies). IMO, it'd be nicest to have a metadata part added to the go sdk 👼 . |
|
I recently rewrote the instance metadata code in the Flexvolume driver (https://github.com/oracle/oci-flexvolume-driver/blob/master/pkg/oci/instancemeta/instance_metadata.go). Some of your comments still apply @jhorwit2 so I'll apply them to the Flexvolume repos and then open a PR here with the updated code if everyone's happy w/ that. |
|
That sounds fine w/me. I am having some trouble testing this fix out in an OKE cluster, but will update the MR once I can test it end2end. |
prydie
force-pushed
the
task/dontRequireCompartment
branch
from
7553688
to
27fc5ec
Compare
prydie
force-pushed
the
task/dontRequireCompartment
branch
from
27fc5ec
to
bc1d32c
Compare
| // Get either returns the cached metadata for the current instance or queries | ||
| // the instance metadata API, populates the cache, and returns the result. | ||
| func (m *metadataGetter) Get() (*InstanceMetadata, error) { | ||
| req, err := http.NewRequest("GET", m.baseURL+metadataEndpoint, nil) |
There was a problem hiding this comment.
nit: should use path.Join(m.baseURL, metadataEndpoint)
| "testing" | ||
| ) | ||
|
|
||
| const exampleResponse = `{ |
There was a problem hiding this comment.
nit: test cases should live in the test method itself or be the return of a helper func.
Fixes #81