Skip to content
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

support of kubernetes secrets (service catalog) #25

Merged
merged 16 commits into from
Sep 7, 2021
Merged

Conversation

nenaraab
Copy link
Contributor

@nenaraab nenaraab commented Aug 30, 2021

  • document K8S Support

@nenaraab nenaraab requested a review from a team as a code owner August 30, 2021 16:48
@nenaraab nenaraab added enhancement New feature or request do-not-merge labels Aug 30, 2021
@nenaraab nenaraab requested a review from jkbschmid September 1, 2021 10:41
Copy link
Contributor

@liga-oz liga-oz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few things to check

env/iasConfig.go Outdated Show resolved Hide resolved
env/iasConfig.go Outdated Show resolved Hide resolved
env/iasConfig.go Outdated Show resolved Hide resolved
env/iasConfig.go Outdated Show resolved Hide resolved
env/iasConfig_test.go Show resolved Hide resolved
@nenaraab nenaraab requested review from liga-oz and f-blass September 6, 2021 10:24
env/iasConfig_test.go Show resolved Hide resolved
Comment on lines +141 to +148
err = os.Unsetenv("KUBERNETES_SERVICE_HOST")
if err != nil {
return fmt.Errorf("error cleaning up after test: could not unset env KUBERNETES_SERVICE_HOST: %w", err)
}
err = os.Unsetenv("IAS_CONFIG_PATH")
if err != nil {
return fmt.Errorf("error cleaning up after test: could not unset env IAS_CONFIG_PATH: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

}
instanceCredentialsMap[instanceSecretFile.Name()] = domains
} else if instanceSecretFile.Size() > 0 {
instanceCredentialsMap[instanceSecretFile.Name()] = string(secretContent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using Unmarshal here as well. E.g.

var v interface{} 
err := json.Unmarshal(secretContent, v)
if err != nil {..}
instanceCredentialsMap[instanceSecretFile.Name()] = v

This way, we don't have to have any knowledge about IAS' credential structure and non-string credentials (e.g. []string) are handled properly. This allows us to get rid of the domains special case as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, i already tried it... but it fails with error "invalid character 'c' looking for beginning of value"
reason JSON without double qouted keys it's not JSON at all.

Found a tip to use yaml instead of json parser but this has issues with characters "]" or "," which are valid for secrets. So i still need a fallback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This is a problem with the BTP service operator. There is no way to distinguish, whether ["asdf"] is a string that contains special characters or a list with a single string. Let's follow SAP/sap-btp-service-operator#79 and see whether this will be improved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that you also stumbled upon this. Could you perhaps comment in the issue so that the colleagues can see this is a problem for multiple teams? (also, fyi, this probably also affects nested objects)

Also, are you aware of this issue: SAP/sap-btp-service-operator#78

I guess you must have also noticed that the plan information is unavailable? At least with the java security libraries in version 2.10.5 the CfEnvironment requires a plan. Passing a JSON in there without a plan causes a NPE.

env/iasConfig.go Show resolved Hide resolved
jkbschmid
jkbschmid previously approved these changes Sep 7, 2021
@f-blass
Copy link
Member

f-blass commented Sep 7, 2021

Hi, sorry for commenting so late...
As of Go 1.16 the ioutil package is actually deprecated. Instead the packages io or os should be used which now cover all functionality previously available in ioutil. The specific function documentations will give hints where to find replacements. (https://pkg.go.dev/io/ioutil)
I think we can optimize parsing the credential files by not reading the entire file into a byte slice and then decoding it, but just open an io.Reader with e.g. os.Open and then use json.NewDecoder or yaml.NewDecoder on it. That way the parser can start working while still reading the rest of the file and we save memory as the underlying byte buffer is way smaller than reading the entire file at once.

liga-oz
liga-oz previously approved these changes Sep 7, 2021
Copy link
Contributor

@liga-oz liga-oz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done 👍🏻

@nenaraab nenaraab dismissed stale reviews from liga-oz and jkbschmid via 0619df6 September 7, 2021 11:12
@nenaraab nenaraab requested a review from jkbschmid September 7, 2021 11:13
@nenaraab
Copy link
Contributor Author

nenaraab commented Sep 7, 2021

Hi, sorry for commenting so late...
As of Go 1.16 the ioutil package is actually deprecated. Instead the packages io or os should be used which now cover all functionality previously available in ioutil. The specific function documentations will give hints where to find replacements. (https://pkg.go.dev/io/ioutil)
==> @f-blass fixed

I think we can optimize parsing the credential files by not reading the entire file into a byte slice and then decoding it, but just open an io.Reader with e.g. os.Open and then use json.NewDecoder or yaml.NewDecoder on it. That way the parser can start working while still reading the rest of the file and we save memory as the underlying byte buffer is way smaller than reading the entire file at once.
==> #27

@nenaraab nenaraab merged commit 5ad1903 into master Sep 7, 2021
@nenaraab nenaraab deleted the k8s-support branch September 7, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants