Fix: remove hardcoded path to kubeconfig file #96

arnaud-tincelin · 2021-08-07T11:09:09Z

No description provided.

safeermohammed

Tatsinnit

💡 Thank you so much for this 🙏 +1 for This PR as it simplifies the use of containerName and various things it unblocks in future. Issue was there because from day-1 the APIServerName was used as containerName : detailed in #99

This will sit nicely and fix any of the ARC scenario as well.

Few things to stock take please:

( @rzhang628 and @qpetraroia ) Can we please open connecting issues to respective repositories for this work please as communication: https://github.com/Azure/azure-cli-extensions/ and https://github.com/Azure/vscode-aks-tools - Please feel free to assign vscode issue to me. also @sophsoph321 - if you are willing to help this is where you could help in putting and PR for the az-cli fix.
There are couple of removal of small AKS tunnel logs with this PR does either we can remove it completely or add we need to discuss why it existed and make informed decision.

I do know that for any breaking changes we just need plan and some part of above comment is circling around that preparation. (we need to give enough time for other tools to adapt these changes)

Important and might be worth a discussion : I am happy to merge once we catered some of thee recommendations and keep these changes as release v0.5 or something, but we just need to think of scenario if there is something we need to fix for any other consuming tool where we will end with this change which needs work at that consuming tool. to: @rzhang628 , @qpetraroia , @palma21 , @sophsoph321 - as fyi and to please stock take and we can start the work in the consuming tool on the side to avoid any convoluted situation. @JunSun17 - if you have any opinions. Thanks you all.

Rest of PR looks great, but I think a plan is key for all including PMs 👍 thanks heaps

Tatsinnit · 2021-08-10T04:09:31Z

pkg/collector/networkoutbound_collector.go

@@ -57,12 +50,6 @@ func (collector *NetworkOutboundCollector) Collect() error {
 			URL:  "kubernetes.default.svc.cluster.local:443",
 		},
 	)
-	outboundTypes = append(outboundTypes,


🐛 Can we not make this removal part of this PR please, we don't know why it existed, @rzhang628 , @qpetraroia or @palma21 - If anyone of you have any insight for this please? removing this means taking small functionality out of periscope and since its in Azure org I would prefer wider eyes on this please. Thanks

what should we set instead?

Tatsinnit · 2021-08-10T04:11:57Z

pkg/exporter/azureblob_exporter.go


-	containerName := strings.Replace(APIServerFQDN, ".", "-", -1)
-	containerLen := strings.Index(containerName, "-hcp-")


💡 This logic needs to be extracted in some kind of git issue, (cc: @sophsoph321 - because az-cli will now do this before supplying the container name as APIServerName ) also fyi: @rzhang628 and @qpetraroia - vscode also need similar tracking workitem in its queue and I can work on that change.

yes, this needs to be documented in the release 0.5. Not sure what would be the benefit of an issue

Cool, so the reason we need this is because periscope as a tool from beginning has working contract with “az-cli” via kollect command and latter vscode started using so anything effecting long term to these tools or any change of contract needs to be highlighted.

with tracking workitem we need to inform those consuming tools: (give enough information for this change)

refrence:

https://github.com/Azure/aks-periscope#dependent-consuming-tools-and-working-contract

https://github.com/Azure/aks-periscope#user-guide

thanks 🙏

I think these 2 paragraphs have to be removed from our README:

first one: we are not developping aks-periscope only for vscode & az cli; some others might use it and anyway noone can change this file unless a PR is approved. + we already documented this file as deprecated

2nd one: the detail of how to use an extension of az-cli shall be in the extension repo. We are not going to watch the extension repo so that we can align our README. However we can indicate that az-cli can be used to deploy aks-periscope and add a link to our repo to the extension

see #102. We should add a PR to az cli extension with the README about kollect command

Tatsinnit · 2021-08-10T04:15:59Z

pkg/exporter/azureblob_exporter.go

-	containerLen := strings.Index(containerName, "-hcp-")
-	if containerLen == -1 {
-		containerLen = maxContainerNameLength
+	if accountName == "" || sasKey == "" || containerName == "" {


🐛 Please add check for containerName length. 3 to 63 is the min and max - otherwise it will fail and in that case how will we handle that scenario? https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#container-names

There are bunch of stuff removed and will sit under consuming tool which looks cool.

hmmm sure but in the same logic we could also check the accountname format, the saskey format etc ... I'd rather not do checks here. Anyway the user will eventually have an error from the service

💡This logic (containerName specific) was already pre-existing in this tool, if we keep that length as it is it and do not remove we take pain from other consuming tool managing their own version of this?

Tatsinnit · 2021-08-10T04:18:01Z

pkg/utils/helper.go

@@ -132,28 +131,6 @@ func GetHostName() (string, error) {
 	return hostName.HostName, nil
 }

-// GetAPIServerFQDN gets the API Server FQDN from the kubeconfig file
-func GetAPIServerFQDN() (string, error) {


💡 Same as above comment, that we could move this to netwrokbound_collector.go but completely removing which include taking small chunk of log out of the logs which gets dumb - looks ok to my eyes but need folks like @JunSun17 or @rzhang628, @qpetraroia to verify what are the key reasons it was there at first place.

Tatsinnit · 2021-08-10T04:19:46Z

docs/kustomize.md

@@ -1,50 +0,0 @@
-# Deploy with Kustomize


💡 Can we please keep this as a document, will be good to update the instructions for newbieusers who want to see how kustomize file will fit together and how they can consume this in their tool or their purpose. Thanks

cc: @rzhang628 and @qpetraroia

documentation is in the example file we added. This brings no value so we should remove it

deployment/examples/kustomization_storage_account.yaml

Tatsinnit · 2021-08-11T22:04:15Z

Thanks Arnaud, @sophsoph321 can you please test this as a test image? cc: @safeermohammed Thank you.

can you publish your local image or you need hand for that please? (please don't hesitate) thank you

arnaud-tincelin · 2021-08-11T22:13:36Z

Thanks Arnaud, @sophsoph321 can you please test this as a test image? cc: @safeermohammed Thank you.

can you publish your local image or you need hand for that please? (Don't hesitate?) thank you

Feel free to use ghcr.io/arnaud-tincelin/aks-periscope:0.5

sophsoph321 · 2021-08-12T01:53:15Z

Thank you @arnaud-tincelin for helping us get unblocked. I have tested using the image you provided on the different distros for our MVP and those changes look good.
If we can add the env variable AZURE_BLOB_CONTAINER_NAME in the aks-periscope.yaml file, we should be good to merge this PR.

Tatsinnit · 2021-08-12T02:08:46Z

Thank you @arnaud-tincelin for helping us get unblocked. I have tested using the image you provided on the different distros for our MVP and those changes look good.
If we can add the env variable AZURE_BLOB_CONTAINER_NAME in the aks-periscope.yaml file, we should be good to merge this PR.

Thanks @sophsoph321 for checking, that is good that image is good for your scenarios. - To expedite this, I would recommend - share your *.yaml you are using and we could keep it under examples folder under deployment which will be only short term solution until you start to do whole kubectl kustomize at your functionality level. Just as a thought: We cannot change aks-periscope.yaml because it is used by because it is consumed az-cli and vscode - Please read this tracking item - #99

And, If I may suggest, lets add a sample way how to use kubectl kustomize command, and to build it with storage_details (Which is one of the key sceanrio).

Thanks.

sophsoph321 · 2021-08-12T06:01:14Z

Thanks @sophsoph321 for checking, that is good that image is good for your scenarios. - To expedite this, I would recommend - share your *.yaml you are using and we could keep it under examples folder under deployment which will be only short term solution until you start to do whole kubectl kustomize at your functionality level. Just as a thought: We cannot change aks-periscope.yaml because it is used by because it is consumed az-cli and vscode - Please read this tracking item - #99

And, If I may suggest, lets add a sample way how to use kubectl kustomize command, and to build it with storage_details.

Thanks.

@arnaud-tincelin Spoke to @Tatsinnit offline. We used the "kubectl kustomize" to construct a single yaml file for the kustomize template. The aks-periscope pods have a ContainerCreationError with the following error message: "Error: secret "azureblob-secret" not found".
For the time being, I used this yaml file to test your code changes, the contents of this file can reside in the examples directory for kustomize (git will not allow me to attach yaml files, so it's in txt format)
azure-k8s-periscope.txt

arnaud-tincelin · 2021-08-12T08:23:50Z

Hi @sophsoph321 apologies, there were 2 mistakes in the example file:

namespace was not specified so the secret was not created (add namespace: aks-periscope)
value of the secret should not be base64 encoded (value of AZURE_BLOB_SAS_KEY)

Let me know if it works better this way

Here is the fixed kustomization.yaml file. You may deploy it with kubectl apply <(kustomize build):

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- https://github.com/Azure/aks-periscope/deployment//

namespace: aks-periscope

secretGenerator:
- name: azureblob-secret
  literals:
  - AZURE_BLOB_SAS_KEY=?sv=2020-08-04&ss=bfqt&srt=sco&sp=rwdlacuptfx&se=2021-08-15T16:02:03Z&st=2021-08-10T08:02:03Z&spr=https&sig=REDACTED
       
patches:
- target:
    group: apps
    kind: DaemonSet
    name: aks-periscope
    version: v1
  patch: |-
    - op: add
      path: '/spec/template/spec/containers/0/env'
      value:
        - name: AZURE_BLOB_ACCOUNT_NAME
          value: mystorageaccount
        - name: AZURE_BLOB_CONTAINER_NAME
          value: mycontainer
- target:
    group: apps
    kind: DaemonSet
    name: aks-periscope
    version: v1
  patch: |-
    - op: add
      path: '/spec/template/spec/containers/0/envFrom/-'
      value:
        secretRef:
          name: azureblob-secret

images:
  - name: aksrepos.azurecr.io/staging/aks-periscope
    newName: ghcr.io/arnaud-tincelin/aks-periscope
    newTag: '0.5'

sophsoph321

Approving this PR. I added a workaround on my side for the CLI changes have tested regarding the AZURE_BLOB_CONTAINER_NAME field and the changes look good. So adding the yaml file I shared last night to the example folder is completely optional.

sophsoph321 · 2021-08-12T17:52:18Z

Hi @sophsoph321 apologies, there were 2 mistakes in the example file:

namespace was not specified so the secret was not created (add namespace: aks-periscope)
value of the secret should not be base64 encoded (value of AZURE_BLOB_SAS_KEY)

Let me know if it works better this way

Here is the fixed kustomization.yaml file. You may deploy it with kubectl apply <(kustomize build):

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- https://github.com/Azure/aks-periscope/deployment//

namespace: aks-periscope

secretGenerator:
- name: azureblob-secret
  literals:
  - AZURE_BLOB_SAS_KEY=?sv=2020-08-04&ss=bfqt&srt=sco&sp=rwdlacuptfx&se=2021-08-15T16:02:03Z&st=2021-08-10T08:02:03Z&spr=https&sig=REDACTED
       
patches:
- target:
    group: apps
    kind: DaemonSet
    name: aks-periscope
    version: v1
  patch: |-
    - op: add
      path: '/spec/template/spec/containers/0/env'
      value:
        - name: AZURE_BLOB_ACCOUNT_NAME
          value: mystorageaccount
        - name: AZURE_BLOB_CONTAINER_NAME
          value: mycontainer
- target:
    group: apps
    kind: DaemonSet
    name: aks-periscope
    version: v1
  patch: |-
    - op: add
      path: '/spec/template/spec/containers/0/envFrom/-'
      value:
        secretRef:
          name: azureblob-secret

images:
  - name: aksrepos.azurecr.io/staging/aks-periscope
    newName: ghcr.io/arnaud-tincelin/aks-periscope
    newTag: '0.5'

@arnaud-tincelin thanks for the fix. I have tested these new fixes and generated a single yaml file based on your changes and it looks good.

Tatsinnit

+1 Since @sophsoph321 approves it with scenarios which ARC needs to fit in, moving forward keeping this issue in mind: #99 - we can approve this and move forward.

Thanks you so much guys 🙏☕️❤️

arnaud-tincelin force-pushed the fix/remove-hardcoded-kubeconfig branch 2 times, most recently from 52d1cd4 to dc918a1 Compare August 7, 2021 14:06

Tatsinnit requested review from Tatsinnit and JunSun17 August 8, 2021 05:45

Tatsinnit mentioned this pull request Aug 8, 2021

Fix APIServerNaming issue - handle Ripple effect in AZ-CLI and VsCode. #99

Closed

Tatsinnit linked an issue Aug 8, 2021 that may be closed by this pull request

Fix APIServerNaming issue - handle Ripple effect in AZ-CLI and VsCode. #99

Closed

safeermohammed approved these changes Aug 9, 2021

View reviewed changes

arnaud-tincelin force-pushed the fix/remove-hardcoded-kubeconfig branch 2 times, most recently from 757f41d to 9211b6a Compare August 9, 2021 22:11

Tatsinnit requested a review from sophsoph321 August 10, 2021 04:06

Tatsinnit requested changes Aug 10, 2021

View reviewed changes

Tatsinnit assigned arnaud-tincelin Aug 10, 2021

Tatsinnit reviewed Aug 10, 2021

View reviewed changes

deployment/examples/kustomization_storage_account.yaml Show resolved Hide resolved

arnaud-tincelin force-pushed the fix/remove-hardcoded-kubeconfig branch from 9211b6a to 5959c22 Compare August 10, 2021 21:10

arnaud-tincelin force-pushed the fix/remove-hardcoded-kubeconfig branch from 5959c22 to fbe1bf0 Compare August 11, 2021 22:06

arnaud-tincelin force-pushed the fix/remove-hardcoded-kubeconfig branch from fbe1bf0 to d41386f Compare August 12, 2021 08:27

Fix: remove hardcoded path to kubeconfig file

463e94b

arnaud-tincelin force-pushed the fix/remove-hardcoded-kubeconfig branch from d41386f to 463e94b Compare August 12, 2021 09:16

sophsoph321 approved these changes Aug 12, 2021

View reviewed changes

Tatsinnit approved these changes Aug 12, 2021

View reviewed changes

Tatsinnit added the breaking change 🚨 This label indicates that the work item contains breaking change. label Aug 12, 2021

Tatsinnit merged commit 41d83b0 into Azure:master Aug 12, 2021

arnaud-tincelin deleted the fix/remove-hardcoded-kubeconfig branch August 12, 2021 21:17

peterbom mentioned this pull request Mar 6, 2022

Return error from createContainerURL if storage settings are not configured #156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: remove hardcoded path to kubeconfig file #96

Fix: remove hardcoded path to kubeconfig file #96

arnaud-tincelin commented Aug 7, 2021

safeermohammed left a comment

Tatsinnit left a comment •

edited

Loading

Tatsinnit Aug 10, 2021

arnaud-tincelin Aug 10, 2021

Tatsinnit Aug 10, 2021

arnaud-tincelin Aug 10, 2021

Tatsinnit Aug 11, 2021

arnaud-tincelin Aug 11, 2021 •

edited

Loading

Tatsinnit Aug 10, 2021

arnaud-tincelin Aug 10, 2021

Tatsinnit Aug 11, 2021 •

edited

Loading

Tatsinnit Aug 10, 2021

Tatsinnit Aug 10, 2021

arnaud-tincelin Aug 10, 2021

Tatsinnit commented Aug 11, 2021 •

edited

Loading

arnaud-tincelin commented Aug 11, 2021

sophsoph321 commented Aug 12, 2021

Tatsinnit commented Aug 12, 2021 •

edited

Loading

sophsoph321 commented Aug 12, 2021

arnaud-tincelin commented Aug 12, 2021 •

edited

Loading

sophsoph321 left a comment

sophsoph321 commented Aug 12, 2021

Tatsinnit left a comment


		containerName := strings.Replace(APIServerFQDN, ".", "-", -1)
		containerLen := strings.Index(containerName, "-hcp-")

Fix: remove hardcoded path to kubeconfig file #96

Fix: remove hardcoded path to kubeconfig file #96

Conversation

arnaud-tincelin commented Aug 7, 2021

safeermohammed left a comment

Choose a reason for hiding this comment

Tatsinnit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaud-tincelin Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tatsinnit Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tatsinnit commented Aug 11, 2021 • edited Loading

arnaud-tincelin commented Aug 11, 2021

sophsoph321 commented Aug 12, 2021

Tatsinnit commented Aug 12, 2021 • edited Loading

sophsoph321 commented Aug 12, 2021

arnaud-tincelin commented Aug 12, 2021 • edited Loading

sophsoph321 left a comment

Choose a reason for hiding this comment

sophsoph321 commented Aug 12, 2021

Tatsinnit left a comment

Choose a reason for hiding this comment

Tatsinnit left a comment •

edited

Loading

arnaud-tincelin Aug 11, 2021 •

edited

Loading

Tatsinnit Aug 11, 2021 •

edited

Loading

Tatsinnit commented Aug 11, 2021 •

edited

Loading

Tatsinnit commented Aug 12, 2021 •

edited

Loading

arnaud-tincelin commented Aug 12, 2021 •

edited

Loading